From eca812f25f41dcd2ee8cba5c5bb2ad71157e168f Mon Sep 17 00:00:00 2001 From: Firegem Date: Sun, 27 Apr 2025 11:33:17 -0400 Subject: [PATCH 01/11] implement lazy eval for basic values Still need to work on porting old mapping code to support lists and records --- crates/nu-command/src/filters/default.rs | 62 +++++++++++++++++++++++- 1 file changed, 60 insertions(+), 2 deletions(-) diff --git a/crates/nu-command/src/filters/default.rs b/crates/nu-command/src/filters/default.rs index f0dc641d5f..1a47c145fc 100644 --- a/crates/nu-command/src/filters/default.rs +++ b/crates/nu-command/src/filters/default.rs @@ -1,4 +1,4 @@ -use nu_engine::command_prelude::*; +use nu_engine::{command_prelude::*, ClosureEvalOnce}; use nu_protocol::{ListStream, Signals}; #[derive(Clone)] @@ -29,6 +29,11 @@ impl Command for Default { "also replace empty items like \"\", {}, and []", Some('e'), ) + .switch( + "lazy", + "if default value is a closure, evaluate it", + Some('l'), + ) .category(Category::Filters) } @@ -44,7 +49,8 @@ impl Command for Default { input: PipelineData, ) -> Result { let empty = call.has_flag(engine_state, stack, "empty")?; - default(engine_state, stack, call, input, empty) + let lazy = call.has_flag(engine_state, stack, "lazy")?; + default2(engine_state, stack, call, input, empty, lazy) } fn examples(&self) -> Vec { @@ -105,6 +111,58 @@ impl Command for Default { } } +fn default2( + engine_state: &EngineState, + stack: &mut Stack, + call: &Call, + input: PipelineData, + empty: bool, + lazy: bool, +) -> Result { + let default_value: Value = call.req(engine_state, stack, 0)?; + let _column: Option> = call.opt(engine_state, stack, 1)?; + + if input.is_nothing() + || (empty && matches!(input, PipelineData::Value(ref value, _) if value.is_empty())) + { + // If input is empty, use default value + default_value_or_eval_once(engine_state, stack, input, default_value, lazy) + } else if let PipelineData::ByteStream(stream, ..) = input { + // Else if input is a bytestream, collect the stream into value and check if it's empty + let value = stream.into_value()?; + if value.is_nothing() || (empty && value.is_empty()) { + default_value_or_eval_once( + engine_state, + stack, + PipelineData::Empty, + default_value, + lazy, + ) + } else { + Ok(value.into_pipeline_data()) + } + } else { + // Else input should be liststream or single value, so map over it like previous implementation + default(engine_state, stack, call, input, empty) // TODO: copy logic and handle lazy evaluation + } +} + +fn default_value_or_eval_once( + engine_state: &EngineState, + stack: &mut Stack, + input: PipelineData, + default_value: Value, + lazy: bool, +) -> Result { + match (&default_value, lazy) { + (Value::Closure { val, .. }, true) => { + let closure = ClosureEvalOnce::new(engine_state, stack, *val.clone()); + closure.run_with_input(input) + } + _ => Ok(default_value.into_pipeline_data()), + } +} + fn default( engine_state: &EngineState, stack: &mut Stack, From fbc71ec3e174e8b9bfe51c6c246702c50e6f62ef Mon Sep 17 00:00:00 2001 From: Firegem Date: Sun, 27 Apr 2025 20:38:28 -0400 Subject: [PATCH 02/11] revert previous commit; modify existing function --- crates/nu-command/src/filters/default.rs | 172 ++++++++++++++--------- 1 file changed, 109 insertions(+), 63 deletions(-) diff --git a/crates/nu-command/src/filters/default.rs b/crates/nu-command/src/filters/default.rs index 1a47c145fc..3fc9887e5b 100644 --- a/crates/nu-command/src/filters/default.rs +++ b/crates/nu-command/src/filters/default.rs @@ -1,4 +1,4 @@ -use nu_engine::{command_prelude::*, ClosureEvalOnce}; +use nu_engine::{command_prelude::*, ClosureEval, ClosureEvalOnce}; use nu_protocol::{ListStream, Signals}; #[derive(Clone)] @@ -34,6 +34,11 @@ impl Command for Default { "if default value is a closure, evaluate it", Some('l'), ) + .switch( + "lazy-once", + "evaluate the closure only once, even for lists (no input)", + Some('L'), + ) .category(Category::Filters) } @@ -50,7 +55,8 @@ impl Command for Default { ) -> Result { let empty = call.has_flag(engine_state, stack, "empty")?; let lazy = call.has_flag(engine_state, stack, "lazy")?; - default2(engine_state, stack, call, input, empty, lazy) + let lazy_once = call.has_flag(engine_state, stack, "lazy-once")?; + default(engine_state, stack, call, input, empty, lazy, lazy_once) } fn examples(&self) -> Vec { @@ -111,42 +117,6 @@ impl Command for Default { } } -fn default2( - engine_state: &EngineState, - stack: &mut Stack, - call: &Call, - input: PipelineData, - empty: bool, - lazy: bool, -) -> Result { - let default_value: Value = call.req(engine_state, stack, 0)?; - let _column: Option> = call.opt(engine_state, stack, 1)?; - - if input.is_nothing() - || (empty && matches!(input, PipelineData::Value(ref value, _) if value.is_empty())) - { - // If input is empty, use default value - default_value_or_eval_once(engine_state, stack, input, default_value, lazy) - } else if let PipelineData::ByteStream(stream, ..) = input { - // Else if input is a bytestream, collect the stream into value and check if it's empty - let value = stream.into_value()?; - if value.is_nothing() || (empty && value.is_empty()) { - default_value_or_eval_once( - engine_state, - stack, - PipelineData::Empty, - default_value, - lazy, - ) - } else { - Ok(value.into_pipeline_data()) - } - } else { - // Else input should be liststream or single value, so map over it like previous implementation - default(engine_state, stack, call, input, empty) // TODO: copy logic and handle lazy evaluation - } -} - fn default_value_or_eval_once( engine_state: &EngineState, stack: &mut Stack, @@ -169,42 +139,112 @@ fn default( call: &Call, input: PipelineData, default_when_empty: bool, + lazy_eval: bool, + lazy_eval_once: bool, ) -> Result { let metadata = input.metadata(); let value: Value = call.req(engine_state, stack, 0)?; let column: Option> = call.opt(engine_state, stack, 1)?; if let Some(column) = column { - input - .map( - move |mut item| match item { - Value::Record { - val: ref mut record, - .. - } => { - let record = record.to_mut(); - if let Some(val) = record.get_mut(&column.item) { - if matches!(val, Value::Nothing { .. }) - || (default_when_empty && val.is_empty()) - { - *val = value.clone(); + if lazy_eval && !lazy_eval_once && matches!(value, Value::Closure { .. }) { + let Value::Closure { + val: ref closure, + internal_span: closure_span, + } = value + else { + unreachable!() + }; + let mut closure = ClosureEval::new(engine_state, stack, *closure.clone()); + input + .map( + move |mut item| match item { + Value::Record { + val: ref mut record, + internal_span: record_span, + } => { + let closure_input = record.clone().into_owned(); + let record = record.to_mut(); + if let Some(val) = record.get_mut(&column.item) { + if matches!(val, Value::Nothing { .. }) + || (default_when_empty && val.is_empty()) + { + *val = match closure + .run_with_value(Value::record(closure_input, record_span)) + { + Ok(value) => value + .into_value(closure_span) + .unwrap_or_else(|err| Value::error(err, closure_span)), + Err(err) => Value::error(err, closure_span), + }; + } + } else { + let new_value = match closure + .run_with_value(Value::record(closure_input, record_span)) + { + Ok(value) => value + .into_value(closure_span) + .unwrap_or_else(|err| Value::error(err, closure_span)), + Err(err) => Value::error(err, closure_span), + }; + record.push(column.item.clone(), new_value); } - } else { - record.push(column.item.clone(), value.clone()); - } - item - } - _ => item, - }, - engine_state.signals(), - ) - .map(|x| x.set_metadata(metadata)) + item + } + _ => item, + }, + engine_state.signals(), + ) + .map(|x| x.set_metadata(metadata)) + } else { + let value_span = value.span(); + let value = default_value_or_eval_once( + engine_state, + stack, + PipelineData::Empty, + value, + lazy_eval_once, + )? + .into_value(value_span)?; + + input + .map( + move |mut item| match item { + Value::Record { + val: ref mut record, + .. + } => { + let record = record.to_mut(); + if let Some(val) = record.get_mut(&column.item) { + if matches!(val, Value::Nothing { .. }) + || (default_when_empty && val.is_empty()) + { + *val = value.clone(); + } + } else { + record.push(column.item.clone(), value.clone()); + } + + item + } + _ => item, + }, + engine_state.signals(), + ) + .map(|x| x.set_metadata(metadata)) + } } else if input.is_nothing() || (default_when_empty && matches!(input, PipelineData::Value(ref value, _) if value.is_empty())) { - Ok(value.into_pipeline_data()) + default_value_or_eval_once( + engine_state, + stack, + input, + value, + lazy_eval || lazy_eval_once, + ) } else if default_when_empty && matches!(input, PipelineData::ListStream(..)) { let PipelineData::ListStream(ls, metadata) = input else { unreachable!() @@ -212,7 +252,13 @@ fn default( let span = ls.span(); let mut stream = ls.into_inner().peekable(); if stream.peek().is_none() { - return Ok(value.into_pipeline_data()); + return default_value_or_eval_once( + engine_state, + stack, + PipelineData::Empty, + value, + lazy_eval || lazy_eval_once, + ); } // stream's internal state already preserves the original signals config, so if this From 55337845f3583e155bdc7d8e2c7e30a83f59c8dc Mon Sep 17 00:00:00 2001 From: Firegem Date: Sun, 27 Apr 2025 21:20:26 -0400 Subject: [PATCH 03/11] Add examples for evaluating closures with `--lazy` --- crates/nu-command/src/filters/default.rs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/crates/nu-command/src/filters/default.rs b/crates/nu-command/src/filters/default.rs index 3fc9887e5b..dbe0c3323e 100644 --- a/crates/nu-command/src/filters/default.rs +++ b/crates/nu-command/src/filters/default.rs @@ -113,6 +113,25 @@ impl Command for Default { }), ])), }, + Example { + description: r#"Generate a default value from a closure"#, + example: "null | default --lazy { 1 + 2 }", + result: Some(Value::test_int(3)), + }, + Example { + description: r#"Generate missing values in a column from a closure"#, + example: "[{a:1 b:2} {b:1}] | default -l { $in.b + 1 } a", + result: Some(Value::test_list(vec![ + Value::test_record(record! { + "a" => Value::test_int(1), + "b" => Value::test_int(2), + }), + Value::test_record(record! { + "a" => Value::test_int(2), + "b" => Value::test_int(1), + }), + ])), + }, ] } } From 82d429d20407b244d05182f499dc26b0e1bb70fb Mon Sep 17 00:00:00 2001 From: Firegem Date: Sun, 27 Apr 2025 23:05:18 -0400 Subject: [PATCH 04/11] Clarify `--lazy-once` description --- crates/nu-command/src/filters/default.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/nu-command/src/filters/default.rs b/crates/nu-command/src/filters/default.rs index dbe0c3323e..a1ce21148a 100644 --- a/crates/nu-command/src/filters/default.rs +++ b/crates/nu-command/src/filters/default.rs @@ -36,7 +36,7 @@ impl Command for Default { ) .switch( "lazy-once", - "evaluate the closure only once, even for lists (no input)", + "evaluate the closure only once, even for lists (no closure input)", Some('L'), ) .category(Category::Filters) From 3046341643801e455a3315c8b04416129e1c1e89 Mon Sep 17 00:00:00 2001 From: Firegem Date: Tue, 29 Apr 2025 17:07:10 -0400 Subject: [PATCH 05/11] enable lazy closure evaluation by default --- crates/nu-command/src/filters/default.rs | 36 ++++++++++++++++-------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/crates/nu-command/src/filters/default.rs b/crates/nu-command/src/filters/default.rs index a1ce21148a..94e46a5aa2 100644 --- a/crates/nu-command/src/filters/default.rs +++ b/crates/nu-command/src/filters/default.rs @@ -30,14 +30,14 @@ impl Command for Default { Some('e'), ) .switch( - "lazy", - "if default value is a closure, evaluate it", - Some('l'), + "no-eval", + "if default value is a closure, pass it as-is", + Some('n'), ) .switch( - "lazy-once", - "evaluate the closure only once, even for lists (no closure input)", - Some('L'), + "eval-once", + "evaluate default value closure only once, even for lists (no closure input)", + Some('o'), ) .category(Category::Filters) } @@ -54,9 +54,23 @@ impl Command for Default { input: PipelineData, ) -> Result { let empty = call.has_flag(engine_state, stack, "empty")?; - let lazy = call.has_flag(engine_state, stack, "lazy")?; - let lazy_once = call.has_flag(engine_state, stack, "lazy-once")?; - default(engine_state, stack, call, input, empty, lazy, lazy_once) + let no_eval = call.has_flag(engine_state, stack, "no-eval")?; + let eval_once = call.has_flag(engine_state, stack, "eval-once")?; + + if no_eval && eval_once { + Err(ShellError::IncompatibleParameters { + left_message: String::from("incompatible flag"), + left_span: call + .get_flag_span(stack, "no-eval") + .unwrap_or_else(Span::unknown), + right_message: String::from("cannot be used with --no-eval"), + right_span: call + .get_flag_span(stack, "eval-once") + .unwrap_or_else(Span::unknown), + }) + } else { + default(engine_state, stack, call, input, empty, !no_eval, eval_once) + } } fn examples(&self) -> Vec { @@ -115,12 +129,12 @@ impl Command for Default { }, Example { description: r#"Generate a default value from a closure"#, - example: "null | default --lazy { 1 + 2 }", + example: "null | default { 1 + 2 }", result: Some(Value::test_int(3)), }, Example { description: r#"Generate missing values in a column from a closure"#, - example: "[{a:1 b:2} {b:1}] | default -l { $in.b + 1 } a", + example: "[{a:1 b:2} {b:1}] | default { $in.b + 1 } a", result: Some(Value::test_list(vec![ Value::test_record(record! { "a" => Value::test_int(1), From d285c9115106828e067215892c26944e5dbad7d5 Mon Sep 17 00:00:00 2001 From: Firegem Date: Tue, 29 Apr 2025 21:05:02 -0400 Subject: [PATCH 06/11] when given a ListStream, pass it to closure input --- crates/nu-command/src/filters/default.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/nu-command/src/filters/default.rs b/crates/nu-command/src/filters/default.rs index 94e46a5aa2..da62ac821c 100644 --- a/crates/nu-command/src/filters/default.rs +++ b/crates/nu-command/src/filters/default.rs @@ -288,7 +288,7 @@ fn default( return default_value_or_eval_once( engine_state, stack, - PipelineData::Empty, + PipelineData::ListStream(ListStream::new(stream, span, Signals::empty()), metadata), value, lazy_eval || lazy_eval_once, ); From 563430b90e92fc2dfd565206506c1a8f97b86939 Mon Sep 17 00:00:00 2001 From: Firegem Date: Sun, 4 May 2025 17:08:44 -0400 Subject: [PATCH 07/11] Follow suggestions from @Bahex Value is now always evaluated once if a closure, so there's no need for either `--no-eval` or `--eval-once`. The closure is also always run without any input. Because of that, the column code can be deduplicated again, and the default value can be calculated once at the start of the function, so cells won't store `Value::Error` values anymore. --- crates/nu-command/src/filters/default.rs | 172 +++++------------------ 1 file changed, 35 insertions(+), 137 deletions(-) diff --git a/crates/nu-command/src/filters/default.rs b/crates/nu-command/src/filters/default.rs index da62ac821c..c58838b32c 100644 --- a/crates/nu-command/src/filters/default.rs +++ b/crates/nu-command/src/filters/default.rs @@ -1,4 +1,4 @@ -use nu_engine::{command_prelude::*, ClosureEval, ClosureEvalOnce}; +use nu_engine::{command_prelude::*, ClosureEvalOnce}; use nu_protocol::{ListStream, Signals}; #[derive(Clone)] @@ -29,16 +29,6 @@ impl Command for Default { "also replace empty items like \"\", {}, and []", Some('e'), ) - .switch( - "no-eval", - "if default value is a closure, pass it as-is", - Some('n'), - ) - .switch( - "eval-once", - "evaluate default value closure only once, even for lists (no closure input)", - Some('o'), - ) .category(Category::Filters) } @@ -54,23 +44,7 @@ impl Command for Default { input: PipelineData, ) -> Result { let empty = call.has_flag(engine_state, stack, "empty")?; - let no_eval = call.has_flag(engine_state, stack, "no-eval")?; - let eval_once = call.has_flag(engine_state, stack, "eval-once")?; - - if no_eval && eval_once { - Err(ShellError::IncompatibleParameters { - left_message: String::from("incompatible flag"), - left_span: call - .get_flag_span(stack, "no-eval") - .unwrap_or_else(Span::unknown), - right_message: String::from("cannot be used with --no-eval"), - right_span: call - .get_flag_span(stack, "eval-once") - .unwrap_or_else(Span::unknown), - }) - } else { - default(engine_state, stack, call, input, empty, !no_eval, eval_once) - } + default(engine_state, stack, call, input, empty) } fn examples(&self) -> Vec { @@ -133,8 +107,8 @@ impl Command for Default { result: Some(Value::test_int(3)), }, Example { - description: r#"Generate missing values in a column from a closure"#, - example: "[{a:1 b:2} {b:1}] | default { $in.b + 1 } a", + description: r#"Fill missing column values based on other columns"#, + example: "[{a:1 b:2} {b:1}] | upsert a {|rc| default { $rc.b + 1 } }", result: Some(Value::test_list(vec![ Value::test_record(record! { "a" => Value::test_int(1), @@ -150,17 +124,15 @@ impl Command for Default { } } -fn default_value_or_eval_once( +fn eval_default( engine_state: &EngineState, stack: &mut Stack, - input: PipelineData, default_value: Value, - lazy: bool, ) -> Result { - match (&default_value, lazy) { - (Value::Closure { val, .. }, true) => { + match &default_value { + Value::Closure { val, .. } => { let closure = ClosureEvalOnce::new(engine_state, stack, *val.clone()); - closure.run_with_input(input) + closure.run_with_input(PipelineData::Empty) } _ => Ok(default_value.into_pipeline_data()), } @@ -172,112 +144,44 @@ fn default( call: &Call, input: PipelineData, default_when_empty: bool, - lazy_eval: bool, - lazy_eval_once: bool, ) -> Result { let metadata = input.metadata(); - let value: Value = call.req(engine_state, stack, 0)?; + let dv: Spanned = call.req(engine_state, stack, 0)?; + let default = eval_default(engine_state, stack, dv.item)?; let column: Option> = call.opt(engine_state, stack, 1)?; if let Some(column) = column { - if lazy_eval && !lazy_eval_once && matches!(value, Value::Closure { .. }) { - let Value::Closure { - val: ref closure, - internal_span: closure_span, - } = value - else { - unreachable!() - }; - let mut closure = ClosureEval::new(engine_state, stack, *closure.clone()); - input - .map( - move |mut item| match item { - Value::Record { - val: ref mut record, - internal_span: record_span, - } => { - let closure_input = record.clone().into_owned(); - let record = record.to_mut(); - if let Some(val) = record.get_mut(&column.item) { - if matches!(val, Value::Nothing { .. }) - || (default_when_empty && val.is_empty()) - { - *val = match closure - .run_with_value(Value::record(closure_input, record_span)) - { - Ok(value) => value - .into_value(closure_span) - .unwrap_or_else(|err| Value::error(err, closure_span)), - Err(err) => Value::error(err, closure_span), - }; - } - } else { - let new_value = match closure - .run_with_value(Value::record(closure_input, record_span)) - { - Ok(value) => value - .into_value(closure_span) - .unwrap_or_else(|err| Value::error(err, closure_span)), - Err(err) => Value::error(err, closure_span), - }; - record.push(column.item.clone(), new_value); + let default = default.into_value(dv.span)?; + input + .map( + move |mut item| match item { + Value::Record { + val: ref mut record, + .. + } => { + let record = record.to_mut(); + if let Some(val) = record.get_mut(&column.item) { + if matches!(val, Value::Nothing { .. }) + || (default_when_empty && val.is_empty()) + { + *val = default.clone(); } - - item + } else { + record.push(column.item.clone(), default.clone()); } - _ => item, - }, - engine_state.signals(), - ) - .map(|x| x.set_metadata(metadata)) - } else { - let value_span = value.span(); - let value = default_value_or_eval_once( - engine_state, - stack, - PipelineData::Empty, - value, - lazy_eval_once, - )? - .into_value(value_span)?; - input - .map( - move |mut item| match item { - Value::Record { - val: ref mut record, - .. - } => { - let record = record.to_mut(); - if let Some(val) = record.get_mut(&column.item) { - if matches!(val, Value::Nothing { .. }) - || (default_when_empty && val.is_empty()) - { - *val = value.clone(); - } - } else { - record.push(column.item.clone(), value.clone()); - } - - item - } - _ => item, - }, - engine_state.signals(), - ) - .map(|x| x.set_metadata(metadata)) - } + item + } + _ => item, + }, + engine_state.signals(), + ) + .map(|x| x.set_metadata(metadata)) } else if input.is_nothing() || (default_when_empty && matches!(input, PipelineData::Value(ref value, _) if value.is_empty())) { - default_value_or_eval_once( - engine_state, - stack, - input, - value, - lazy_eval || lazy_eval_once, - ) + Ok(default) } else if default_when_empty && matches!(input, PipelineData::ListStream(..)) { let PipelineData::ListStream(ls, metadata) = input else { unreachable!() @@ -285,13 +189,7 @@ fn default( let span = ls.span(); let mut stream = ls.into_inner().peekable(); if stream.peek().is_none() { - return default_value_or_eval_once( - engine_state, - stack, - PipelineData::ListStream(ListStream::new(stream, span, Signals::empty()), metadata), - value, - lazy_eval || lazy_eval_once, - ); + return Ok(default); } // stream's internal state already preserves the original signals config, so if this From 2557a99e87c9db5d517cd41108d74dd1b6744316 Mon Sep 17 00:00:00 2001 From: Firegem Date: Sun, 4 May 2025 17:36:47 -0400 Subject: [PATCH 08/11] fix example tests to include `upsert` command --- crates/nu-command/src/filters/default.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/crates/nu-command/src/filters/default.rs b/crates/nu-command/src/filters/default.rs index c58838b32c..c17a2571ed 100644 --- a/crates/nu-command/src/filters/default.rs +++ b/crates/nu-command/src/filters/default.rs @@ -108,7 +108,7 @@ impl Command for Default { }, Example { description: r#"Fill missing column values based on other columns"#, - example: "[{a:1 b:2} {b:1}] | upsert a {|rc| default { $rc.b + 1 } }", + example: r#"[{a:1 b:2} {b:1}] | upsert a {|rc| default { $rc.b + 1 } }"#, result: Some(Value::test_list(vec![ Value::test_record(record! { "a" => Value::test_int(1), @@ -203,12 +203,14 @@ fn default( #[cfg(test)] mod test { + use crate::Upsert; + use super::*; #[test] fn test_examples() { - use crate::test_examples; + use crate::test_examples_with_commands; - test_examples(Default {}) + test_examples_with_commands(Default {}, &[&Upsert]); } } From 122d28afacd652f6a9f47478167e7256c2908477 Mon Sep 17 00:00:00 2001 From: Firegem Date: Sun, 4 May 2025 18:32:53 -0400 Subject: [PATCH 09/11] partially fix lazy evaluation --- crates/nu-command/src/filters/default.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/nu-command/src/filters/default.rs b/crates/nu-command/src/filters/default.rs index c17a2571ed..ecd53a4e43 100644 --- a/crates/nu-command/src/filters/default.rs +++ b/crates/nu-command/src/filters/default.rs @@ -146,12 +146,12 @@ fn default( default_when_empty: bool, ) -> Result { let metadata = input.metadata(); - let dv: Spanned = call.req(engine_state, stack, 0)?; - let default = eval_default(engine_state, stack, dv.item)?; + let default_value: Spanned = call.req(engine_state, stack, 0)?; let column: Option> = call.opt(engine_state, stack, 1)?; if let Some(column) = column { - let default = default.into_value(dv.span)?; + let default = eval_default(engine_state, stack, default_value.item)? + .into_value(default_value.span)?; input .map( move |mut item| match item { @@ -181,7 +181,7 @@ fn default( || (default_when_empty && matches!(input, PipelineData::Value(ref value, _) if value.is_empty())) { - Ok(default) + eval_default(engine_state, stack, default_value.item) } else if default_when_empty && matches!(input, PipelineData::ListStream(..)) { let PipelineData::ListStream(ls, metadata) = input else { unreachable!() @@ -189,7 +189,7 @@ fn default( let span = ls.span(); let mut stream = ls.into_inner().peekable(); if stream.peek().is_none() { - return Ok(default); + return eval_default(engine_state, stack, default_value.item); } // stream's internal state already preserves the original signals config, so if this From f636e54af800fd1e411383e288a6c73c975645ad Mon Sep 17 00:00:00 2001 From: Firegem Date: Wed, 7 May 2025 15:23:48 -0400 Subject: [PATCH 10/11] Add lazy closure evaluation to records and list, with cached values This will only ever run the closure one time, when needed, then cache the values, so long operations are only ever run once, or not at all if not necessary. Another change is allowing multiple column names, and erroring when the user gives a column name, whereas previously the command would always return the pipeline input. Also changed is the function signature, since `default` never supported ByteStream input, and this doesn't add it either. This change and the added error can be reverted if desired. --- crates/nu-command/src/filters/default.rs | 160 ++++++++++++++++++----- 1 file changed, 130 insertions(+), 30 deletions(-) diff --git a/crates/nu-command/src/filters/default.rs b/crates/nu-command/src/filters/default.rs index ecd53a4e43..9c40e480de 100644 --- a/crates/nu-command/src/filters/default.rs +++ b/crates/nu-command/src/filters/default.rs @@ -1,4 +1,4 @@ -use nu_engine::{command_prelude::*, ClosureEvalOnce}; +use nu_engine::{command_prelude::*, ClosureEval, ClosureEvalOnce}; use nu_protocol::{ListStream, Signals}; #[derive(Clone)] @@ -13,13 +13,25 @@ impl Command for Default { Signature::build("default") // TODO: Give more specific type signature? // TODO: Declare usage of cell paths in signature? (It seems to behave as if it uses cell paths) - .input_output_types(vec![(Type::Any, Type::Any)]) + .input_output_types(vec![ + (Type::Nothing, Type::Any), + (Type::String, Type::Any), + (Type::record(), Type::Any), + (Type::list(Type::Any), Type::Any), + (Type::Number, Type::Number), + (Type::Closure, Type::Closure), + (Type::Filesize, Type::Filesize), + (Type::Bool, Type::Bool), + (Type::Date, Type::Date), + (Type::Duration, Type::Duration), + (Type::Range, Type::Range), + ]) .required( "default value", SyntaxShape::Any, "The value to use as a default.", ) - .optional( + .rest( "column name", SyntaxShape::String, "The name of the column.", @@ -138,6 +150,55 @@ fn eval_default( } } +fn default_record_columns( + record: &mut Record, + default_value: Spanned, + columns: &[String], + empty: bool, + engine_state: &EngineState, + stack: &mut Stack, + calculated_value: &mut Option, +) -> Result { + if let Value::Closure { val: closure, .. } = &default_value.item { + // Cache the value of the closure to avoid running it multiple times + let mut closure = ClosureEval::new(engine_state, stack, *closure.clone()); + for col in columns { + if let Some(val) = record.get_mut(col) { + if matches!(val, Value::Nothing { .. }) || (empty && val.is_empty()) { + if let Some(ref new_value) = calculated_value { + *val = new_value.clone(); + } else { + let new_value = closure + .run_with_input(PipelineData::Empty)? + .into_value(default_value.span)?; + *calculated_value = Some(new_value.clone()); + *val = new_value; + } + } + } else if let Some(ref new_value) = calculated_value { + record.push(col.clone(), new_value.clone()); + } else { + let new_value = closure + .run_with_input(PipelineData::Empty)? + .into_value(default_value.span)?; + *calculated_value = Some(new_value.clone()); + record.push(col.clone(), new_value); + } + } + } else { + for col in columns { + if let Some(val) = record.get_mut(col) { + if matches!(val, Value::Nothing { .. }) || (empty && val.is_empty()) { + *val = default_value.item.clone(); + } + } else { + record.push(col.clone(), default_value.item.clone()); + } + } + } + Ok(Value::record(record.clone(), Span::unknown()).into_pipeline_data()) +} + fn default( engine_state: &EngineState, stack: &mut Stack, @@ -145,38 +206,76 @@ fn default( input: PipelineData, default_when_empty: bool, ) -> Result { + let input_span = input.span().unwrap_or_else(Span::unknown); let metadata = input.metadata(); let default_value: Spanned = call.req(engine_state, stack, 0)?; - let column: Option> = call.opt(engine_state, stack, 1)?; + let columns: Vec = call.rest(engine_state, stack, 1)?; - if let Some(column) = column { - let default = eval_default(engine_state, stack, default_value.item)? - .into_value(default_value.span)?; - input - .map( - move |mut item| match item { - Value::Record { - val: ref mut record, - .. - } => { - let record = record.to_mut(); - if let Some(val) = record.get_mut(&column.item) { - if matches!(val, Value::Nothing { .. }) - || (default_when_empty && val.is_empty()) - { - *val = default.clone(); - } - } else { - record.push(column.item.clone(), default.clone()); - } - - item - } - _ => item, - }, - engine_state.signals(), + // If user supplies columns, check if input is a record or list of records + // and set the default value for the specified record columns + if !columns.is_empty() { + // Single record arm + if matches!(input, PipelineData::Value(Value::Record { .. }, _)) { + let Value::Record { + val: ref mut record, + .. + } = input.into_value(input_span)? + else { + unreachable!() + }; + let record = record.to_mut(); + default_record_columns( + record, + default_value, + columns.as_slice(), + default_when_empty, + engine_state, + stack, + &mut None, ) .map(|x| x.set_metadata(metadata)) + // ListStream arm + } else if matches!(input, PipelineData::ListStream(..)) + || matches!(input, PipelineData::Value(Value::List { .. }, _)) + { + let mut calculated_value: Option = None; + let mut output_list: Vec = vec![]; + for mut item in input { + if let Value::Record { + val: ref mut record, + internal_span, + } = item + { + let item = default_record_columns( + record.to_mut(), + default_value.clone(), + columns.as_slice(), + default_when_empty, + engine_state, + stack, + &mut calculated_value, + )?; + output_list.push(item.into_value(internal_span)?); + } else { + output_list.push(item); + } + } + let ls = ListStream::new( + output_list.into_iter(), + call.head, + engine_state.signals().clone(), + ); + Ok(PipelineData::ListStream(ls, metadata)) + // If columns are given, but input does not use columns, return an error + } else { + Err(ShellError::PipelineMismatch { + exp_input_type: "record, table".to_string(), + dst_span: input_span, + src_span: input_span, + }) + } + // Otherwise, if no column name is given, check if value is null + // or an empty string, list, or record when --empty is passed } else if input.is_nothing() || (default_when_empty && matches!(input, PipelineData::Value(ref value, _) if value.is_empty())) @@ -196,6 +295,7 @@ fn default( // Signals::empty list stream gets interrupted it will be caught by the underlying iterator let ls = ListStream::new(stream, span, Signals::empty()); Ok(PipelineData::ListStream(ls, metadata)) + // Otherwise, return the input as is } else { Ok(input) } From 9d510ff66ce4d6f41a2684ff8b7f73a5e7d89f7f Mon Sep 17 00:00:00 2001 From: Firegem Date: Wed, 7 May 2025 15:33:39 -0400 Subject: [PATCH 11/11] revert command type signature change --- crates/nu-command/src/filters/default.rs | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/crates/nu-command/src/filters/default.rs b/crates/nu-command/src/filters/default.rs index 9c40e480de..36d4f0e5b6 100644 --- a/crates/nu-command/src/filters/default.rs +++ b/crates/nu-command/src/filters/default.rs @@ -13,19 +13,7 @@ impl Command for Default { Signature::build("default") // TODO: Give more specific type signature? // TODO: Declare usage of cell paths in signature? (It seems to behave as if it uses cell paths) - .input_output_types(vec![ - (Type::Nothing, Type::Any), - (Type::String, Type::Any), - (Type::record(), Type::Any), - (Type::list(Type::Any), Type::Any), - (Type::Number, Type::Number), - (Type::Closure, Type::Closure), - (Type::Filesize, Type::Filesize), - (Type::Bool, Type::Bool), - (Type::Date, Type::Date), - (Type::Duration, Type::Duration), - (Type::Range, Type::Range), - ]) + .input_output_types(vec![(Type::Any, Type::Any)]) .required( "default value", SyntaxShape::Any,