From a5fefaf78bcf9663f67ff25300d22bbf86b9340a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9s=20N=2E=20Robalino?= Date: Fri, 5 Feb 2021 19:34:26 -0500 Subject: [PATCH] Ensure selection of columns are done once per column (#3012) --- crates/nu-command/src/commands.rs | 3 +- crates/nu-command/src/commands/benchmark.rs | 2 +- crates/nu-command/src/commands/do_.rs | 2 +- .../nu-command/src/commands/each/command.rs | 2 +- crates/nu-command/src/commands/each/group.rs | 2 +- crates/nu-command/src/commands/each/window.rs | 2 +- crates/nu-command/src/commands/empty.rs | 42 +----- crates/nu-command/src/commands/group_by.rs | 2 +- crates/nu-command/src/commands/if_.rs | 2 +- crates/nu-command/src/commands/insert.rs | 2 +- crates/nu-command/src/commands/merge.rs | 2 +- crates/nu-command/src/commands/reduce.rs | 2 +- crates/nu-command/src/commands/select.rs | 48 ++----- crates/nu-command/src/commands/update.rs | 2 +- crates/nu-command/src/commands/where_.rs | 2 +- crates/nu-command/src/commands/with_env.rs | 2 +- crates/nu-command/src/utils.rs | 1 + crates/nu-command/src/utils/arguments.rs | 37 ++++++ crates/nu-command/tests/commands/select.rs | 120 +++++++++--------- crates/nu-engine/src/evaluation_context.rs | 12 -- 20 files changed, 131 insertions(+), 158 deletions(-) create mode 100644 crates/nu-command/src/utils/arguments.rs diff --git a/crates/nu-command/src/commands.rs b/crates/nu-command/src/commands.rs index 11580efd8a..a5ac12a949 100644 --- a/crates/nu-command/src/commands.rs +++ b/crates/nu-command/src/commands.rs @@ -245,7 +245,7 @@ pub(crate) use reverse::Reverse; pub(crate) use rm::Remove; pub(crate) use run_external::RunExternalCommand; pub(crate) use save::Save; -pub(crate) use select::Select; +pub(crate) use select::Command as Select; pub(crate) use seq::Seq; pub(crate) use seq_dates::SeqDates; pub(crate) use shells::Shells; @@ -299,6 +299,7 @@ mod tests { whole_stream_command(Move), whole_stream_command(Update), whole_stream_command(Empty), + //whole_stream_command(Select), // Str Command Suite whole_stream_command(Str), whole_stream_command(StrToDecimal), diff --git a/crates/nu-command/src/commands/benchmark.rs b/crates/nu-command/src/commands/benchmark.rs index 38e0ced8f2..bfbb63b8e6 100644 --- a/crates/nu-command/src/commands/benchmark.rs +++ b/crates/nu-command/src/commands/benchmark.rs @@ -70,7 +70,7 @@ impl WholeStreamCommand for Benchmark { async fn benchmark(raw_args: CommandArgs) -> Result { let tag = raw_args.call_info.args.span; - let mut context = EvaluationContext::from_raw(&raw_args); + let mut context = EvaluationContext::from_args(&raw_args); let scope = raw_args.scope.clone(); let (BenchmarkArgs { block, passthrough }, input) = raw_args.process().await?; diff --git a/crates/nu-command/src/commands/do_.rs b/crates/nu-command/src/commands/do_.rs index 4040a9d96f..c1c08cbfb5 100644 --- a/crates/nu-command/src/commands/do_.rs +++ b/crates/nu-command/src/commands/do_.rs @@ -55,7 +55,7 @@ impl WholeStreamCommand for Do { async fn do_(raw_args: CommandArgs) -> Result { let external_redirection = raw_args.call_info.args.external_redirection; - let context = EvaluationContext::from_raw(&raw_args); + let context = EvaluationContext::from_args(&raw_args); let ( DoArgs { ignore_errors, diff --git a/crates/nu-command/src/commands/each/command.rs b/crates/nu-command/src/commands/each/command.rs index ff5aca94c9..dfb5dbae9f 100644 --- a/crates/nu-command/src/commands/each/command.rs +++ b/crates/nu-command/src/commands/each/command.rs @@ -111,7 +111,7 @@ pub(crate) fn make_indexed_item(index: usize, item: Value) -> Value { } async fn each(raw_args: CommandArgs) -> Result { - let context = Arc::new(EvaluationContext::from_raw(&raw_args)); + let context = Arc::new(EvaluationContext::from_args(&raw_args)); let (each_args, input): (EachArgs, _) = raw_args.process().await?; let block = Arc::new(Box::new(each_args.block)); diff --git a/crates/nu-command/src/commands/each/group.rs b/crates/nu-command/src/commands/each/group.rs index 2953a33de7..b7fbcabd36 100644 --- a/crates/nu-command/src/commands/each/group.rs +++ b/crates/nu-command/src/commands/each/group.rs @@ -46,7 +46,7 @@ impl WholeStreamCommand for EachGroup { } async fn run(&self, raw_args: CommandArgs) -> Result { - let context = Arc::new(EvaluationContext::from_raw(&raw_args)); + let context = Arc::new(EvaluationContext::from_args(&raw_args)); let (each_args, input): (EachGroupArgs, _) = raw_args.process().await?; let block = Arc::new(Box::new(each_args.block)); diff --git a/crates/nu-command/src/commands/each/window.rs b/crates/nu-command/src/commands/each/window.rs index 2cb91c4b18..9343d3e29a 100644 --- a/crates/nu-command/src/commands/each/window.rs +++ b/crates/nu-command/src/commands/each/window.rs @@ -51,7 +51,7 @@ impl WholeStreamCommand for EachWindow { } async fn run(&self, raw_args: CommandArgs) -> Result { - let context = Arc::new(EvaluationContext::from_raw(&raw_args)); + let context = Arc::new(EvaluationContext::from_args(&raw_args)); let (each_args, mut input): (EachWindowArgs, _) = raw_args.process().await?; let block = Arc::new(Box::new(each_args.block)); diff --git a/crates/nu-command/src/commands/empty.rs b/crates/nu-command/src/commands/empty.rs index 8b0b7bb8ab..491573847e 100644 --- a/crates/nu-command/src/commands/empty.rs +++ b/crates/nu-command/src/commands/empty.rs @@ -6,10 +6,10 @@ use nu_protocol::{ hir::CapturedBlock, ColumnPath, Primitive, ReturnSuccess, Signature, SyntaxShape, UntaggedValue, Value, }; -use nu_source::Tagged; -use nu_value_ext::{as_string, ValueExt}; +use crate::utils::arguments::arguments; use futures::stream::once; +use nu_value_ext::{as_string, ValueExt}; #[derive(Deserialize)] pub struct Arguments { @@ -84,9 +84,10 @@ impl WholeStreamCommand for Command { async fn is_empty(args: CommandArgs) -> Result { let tag = args.call_info.name_tag.clone(); let name_tag = Arc::new(args.call_info.name_tag.clone()); - let context = Arc::new(EvaluationContext::from_raw(&args)); - let (Arguments { rest }, input) = args.process().await?; - let (columns, default_block): (Vec, Option>) = arguments(rest)?; + let context = Arc::new(EvaluationContext::from_args(&args)); + let (Arguments { mut rest }, input) = args.process().await?; + let (columns, default_block): (Vec, Option>) = + arguments(&mut rest)?; let default_block = Arc::new(default_block); if input.is_empty() { @@ -130,37 +131,6 @@ async fn is_empty(args: CommandArgs) -> Result { .to_output_stream()) } -fn arguments( - rest: Vec, -) -> Result<(Vec, Option>), ShellError> { - let mut rest = rest; - let mut columns = vec![]; - let mut default = None; - - let last_argument = rest.pop(); - - match last_argument { - Some(Value { - value: UntaggedValue::Block(call), - .. - }) => default = Some(call), - Some(other) => { - let Tagged { item: path, .. } = other.as_column_path()?; - - columns = vec![path]; - } - None => {} - }; - - for argument in rest { - let Tagged { item: path, .. } = argument.as_column_path()?; - - columns.push(path); - } - - Ok((columns, default)) -} - async fn process_row( context: Arc, input: Value, diff --git a/crates/nu-command/src/commands/group_by.rs b/crates/nu-command/src/commands/group_by.rs index d5dd8336e1..422d3d136c 100644 --- a/crates/nu-command/src/commands/group_by.rs +++ b/crates/nu-command/src/commands/group_by.rs @@ -130,7 +130,7 @@ enum Grouper { pub async fn group_by(args: CommandArgs) -> Result { let name = args.call_info.name_tag.clone(); - let context = Arc::new(EvaluationContext::from_raw(&args)); + let context = Arc::new(EvaluationContext::from_args(&args)); let (Arguments { grouper }, input) = args.process().await?; let values: Vec = input.collect().await; diff --git a/crates/nu-command/src/commands/if_.rs b/crates/nu-command/src/commands/if_.rs index c9ddbf728b..cd5b1fff4b 100644 --- a/crates/nu-command/src/commands/if_.rs +++ b/crates/nu-command/src/commands/if_.rs @@ -66,7 +66,7 @@ impl WholeStreamCommand for If { } async fn if_command(raw_args: CommandArgs) -> Result { let tag = raw_args.call_info.name_tag.clone(); - let context = Arc::new(EvaluationContext::from_raw(&raw_args)); + let context = Arc::new(EvaluationContext::from_args(&raw_args)); let ( IfArgs { diff --git a/crates/nu-command/src/commands/insert.rs b/crates/nu-command/src/commands/insert.rs index 5530d41640..281b6f5b06 100644 --- a/crates/nu-command/src/commands/insert.rs +++ b/crates/nu-command/src/commands/insert.rs @@ -154,7 +154,7 @@ async fn process_row( } async fn insert(raw_args: CommandArgs) -> Result { - let context = Arc::new(EvaluationContext::from_raw(&raw_args)); + let context = Arc::new(EvaluationContext::from_args(&raw_args)); let (Arguments { column, value }, input) = raw_args.process().await?; let value = Arc::new(value); let column = Arc::new(column); diff --git a/crates/nu-command/src/commands/merge.rs b/crates/nu-command/src/commands/merge.rs index 69b8ec1a9c..be3914e99f 100644 --- a/crates/nu-command/src/commands/merge.rs +++ b/crates/nu-command/src/commands/merge.rs @@ -47,7 +47,7 @@ impl WholeStreamCommand for Merge { } async fn merge(raw_args: CommandArgs) -> Result { - let context = EvaluationContext::from_raw(&raw_args); + let context = EvaluationContext::from_args(&raw_args); let name_tag = raw_args.call_info.name_tag.clone(); let (merge_args, input): (MergeArgs, _) = raw_args.process().await?; let block = merge_args.block; diff --git a/crates/nu-command/src/commands/reduce.rs b/crates/nu-command/src/commands/reduce.rs index 39929feb07..2519355762 100644 --- a/crates/nu-command/src/commands/reduce.rs +++ b/crates/nu-command/src/commands/reduce.rs @@ -95,7 +95,7 @@ async fn process_row( async fn reduce(raw_args: CommandArgs) -> Result { let span = raw_args.call_info.name_tag.span; - let context = Arc::new(EvaluationContext::from_raw(&raw_args)); + let context = Arc::new(EvaluationContext::from_args(&raw_args)); let (reduce_args, mut input): (ReduceArgs, _) = raw_args.process().await?; let block = Arc::new(reduce_args.block); let (ioffset, start) = if !input.is_empty() { diff --git a/crates/nu-command/src/commands/select.rs b/crates/nu-command/src/commands/select.rs index 6b89e4494e..b461f3007f 100644 --- a/crates/nu-command/src/commands/select.rs +++ b/crates/nu-command/src/commands/select.rs @@ -1,30 +1,27 @@ use crate::prelude::*; +use crate::utils::arguments::arguments; use nu_engine::WholeStreamCommand; use nu_errors::ShellError; use nu_protocol::{ - ColumnPath, PathMember, Primitive, ReturnSuccess, Signature, SyntaxShape, TaggedDictBuilder, - UnspannedPathMember, UntaggedValue, Value, + hir::CapturedBlock, ColumnPath, PathMember, Primitive, ReturnSuccess, Signature, SyntaxShape, + TaggedDictBuilder, UnspannedPathMember, UntaggedValue, Value, }; use nu_value_ext::{as_string, get_data_by_column_path}; - #[derive(Deserialize)] -struct SelectArgs { - rest: Vec, +struct Arguments { + rest: Vec, } -pub struct Select; +pub struct Command; #[async_trait] -impl WholeStreamCommand for Select { +impl WholeStreamCommand for Command { fn name(&self) -> &str { "select" } fn signature(&self) -> Signature { - Signature::build("select").rest( - SyntaxShape::ColumnPath, - "the columns to select from the table", - ) + Signature::build("select").rest(SyntaxShape::Any, "the columns to select from the table") } fn usage(&self) -> &str { @@ -53,8 +50,10 @@ impl WholeStreamCommand for Select { async fn select(args: CommandArgs) -> Result { let name = args.call_info.name_tag.clone(); - let (SelectArgs { rest: mut fields }, mut input) = args.process().await?; - if fields.is_empty() { + let (Arguments { mut rest }, mut input) = args.process().await?; + let (columns, _): (Vec, Option>) = arguments(&mut rest)?; + + if columns.is_empty() { return Err(ShellError::labeled_error( "Select requires columns to select", "needs parameter", @@ -62,18 +61,10 @@ async fn select(args: CommandArgs) -> Result { )); } - let member = fields.remove(0); - let member = vec![member]; - - let column_paths = vec![&member, &fields] - .into_iter() - .flatten() - .cloned() - .collect::>(); let mut bring_back: indexmap::IndexMap> = indexmap::IndexMap::new(); while let Some(value) = input.next().await { - for path in &column_paths { + for path in &columns { let fetcher = get_data_by_column_path( &value, &path, @@ -165,16 +156,3 @@ async fn select(args: CommandArgs) -> Result { })) .to_output_stream()) } - -#[cfg(test)] -mod tests { - use super::Select; - use super::ShellError; - - #[test] - fn examples_work_as_expected() -> Result<(), ShellError> { - use crate::examples::test as test_examples; - - Ok(test_examples(Select {})?) - } -} diff --git a/crates/nu-command/src/commands/update.rs b/crates/nu-command/src/commands/update.rs index ad0f68bf6b..434742181d 100644 --- a/crates/nu-command/src/commands/update.rs +++ b/crates/nu-command/src/commands/update.rs @@ -174,7 +174,7 @@ async fn process_row( async fn update(raw_args: CommandArgs) -> Result { let name_tag = Arc::new(raw_args.call_info.name_tag.clone()); - let context = Arc::new(EvaluationContext::from_raw(&raw_args)); + let context = Arc::new(EvaluationContext::from_args(&raw_args)); let (Arguments { field, replacement }, input) = raw_args.process().await?; let replacement = Arc::new(replacement); let field = Arc::new(field); diff --git a/crates/nu-command/src/commands/where_.rs b/crates/nu-command/src/commands/where_.rs index 08525f84e8..1a40ba2b2f 100644 --- a/crates/nu-command/src/commands/where_.rs +++ b/crates/nu-command/src/commands/where_.rs @@ -61,7 +61,7 @@ impl WholeStreamCommand for Where { } } async fn where_command(raw_args: CommandArgs) -> Result { - let ctx = Arc::new(EvaluationContext::from_raw(&raw_args)); + let ctx = Arc::new(EvaluationContext::from_args(&raw_args)); let tag = raw_args.call_info.name_tag.clone(); let (WhereArgs { block }, input) = raw_args.process().await?; let condition = { diff --git a/crates/nu-command/src/commands/with_env.rs b/crates/nu-command/src/commands/with_env.rs index 427b3f76f1..32abbadec1 100644 --- a/crates/nu-command/src/commands/with_env.rs +++ b/crates/nu-command/src/commands/with_env.rs @@ -69,7 +69,7 @@ impl WholeStreamCommand for WithEnv { } async fn with_env(raw_args: CommandArgs) -> Result { - let context = EvaluationContext::from_raw(&raw_args); + let context = EvaluationContext::from_args(&raw_args); let (WithEnvArgs { variable, block }, input) = raw_args.process().await?; let mut env = IndexMap::new(); diff --git a/crates/nu-command/src/utils.rs b/crates/nu-command/src/utils.rs index ed6dc62fcf..e96612410d 100644 --- a/crates/nu-command/src/utils.rs +++ b/crates/nu-command/src/utils.rs @@ -1,2 +1,3 @@ +pub mod arguments; pub mod suggestions; pub mod test_bins; diff --git a/crates/nu-command/src/utils/arguments.rs b/crates/nu-command/src/utils/arguments.rs new file mode 100644 index 0000000000..498c65e94e --- /dev/null +++ b/crates/nu-command/src/utils/arguments.rs @@ -0,0 +1,37 @@ +use indexmap::IndexSet; +use nu_errors::ShellError; +use nu_protocol::{hir::CapturedBlock, ColumnPath, UntaggedValue, Value}; +use nu_source::Tagged; +use nu_value_ext::ValueExt; + +pub fn arguments( + rest: &mut Vec, +) -> Result<(Vec, Option>), ShellError> { + let last_argument = rest.pop(); + + let mut columns: IndexSet<_> = rest.iter().collect(); + let mut column_paths = vec![]; + + let mut default = None; + + for argument in columns.drain(..) { + let Tagged { item: path, .. } = argument.as_column_path()?; + + column_paths.push(path); + } + + match last_argument { + Some(Value { + value: UntaggedValue::Block(call), + .. + }) => default = Some(call), + Some(other) => { + let Tagged { item: path, .. } = other.as_column_path()?; + + column_paths.push(path); + } + None => {} + }; + + Ok((column_paths, default)) +} diff --git a/crates/nu-command/tests/commands/select.rs b/crates/nu-command/tests/commands/select.rs index 3c3b15e964..87b1708be9 100644 --- a/crates/nu-command/tests/commands/select.rs +++ b/crates/nu-command/tests/commands/select.rs @@ -4,29 +4,22 @@ use nu_test_support::{nu, pipeline}; #[test] fn regular_columns() { - Playground::setup("select_test_1", |dirs, sandbox| { - sandbox.with_files(vec![FileWithContentToBeTrimmed( - "los_tres_caballeros.csv", - r#" - first_name,last_name,rusty_at,type - Andrés,Robalino,10/11/2013,A - Jonathan,Turner,10/12/2013,B - Yehuda,Katz,10/11/2013,A - "#, - )]); + let actual = nu!(cwd: ".", pipeline( + r#" + echo [ + [first_name, last_name, rusty_at, type]; - let actual = nu!( - cwd: dirs.test(), pipeline( - r#" - open los_tres_caballeros.csv - | select rusty_at last_name - | nth 0 - | get last_name - "# - )); + [Andrés Robalino 10/11/2013 A] + [Jonathan Turner 10/12/2013 B] + [Yehuda Katz 10/11/2013 A] + ] + | select rusty_at last_name + | nth 0 + | get last_name + "# + )); - assert_eq!(actual.out, "Robalino"); - }) + assert_eq!(actual.out, "Robalino"); } #[test] @@ -73,52 +66,57 @@ fn complex_nested_columns() { #[test] fn allows_if_given_unknown_column_name_is_missing() { - Playground::setup("select_test_3", |dirs, sandbox| { - sandbox.with_files(vec![FileWithContentToBeTrimmed( - "los_tres_caballeros.csv", - r#" - first_name,last_name,rusty_at,type - Andrés,Robalino,10/11/2013,A - Jonathan,Turner,10/12/2013,B - Yehuda,Katz,10/11/2013,A - "#, - )]); + let actual = nu!(cwd: ".", pipeline( + r#" + echo [ + [first_name, last_name, rusty_at, type]; - let actual = nu!( - cwd: dirs.test(), pipeline( - r#" - open los_tres_caballeros.csv - | select rrusty_at first_name - | count - "# - )); + [Andrés Robalino 10/11/2013 A] + [Jonathan Turner 10/12/2013 B] + [Yehuda Katz 10/11/2013 A] + ] + | select rrusty_at first_name + | count + "# + )); - assert_eq!(actual.out, "3"); - }) + assert_eq!(actual.out, "3"); } #[test] fn column_names_with_spaces() { - Playground::setup("select_test_4", |dirs, sandbox| { - sandbox.with_files(vec![FileWithContentToBeTrimmed( - "test_data.csv", - r#" - first name,last name - Jonathan,Turner - Jonathan,Arns - "#, - )]); + let actual = nu!(cwd: ".", pipeline( + r#" + echo [ + ["first name", "last name"]; - let actual = nu!( - cwd: dirs.test(), pipeline( - r#" - open test_data.csv - | select "last name" - | to json - "# - )); + [Andrés Robalino] + [Andrés Jnth] + ] + | select "last name" + | get "last name" + | str collect " " + "# + )); - let expected_output = r#"[{"last name":"Turner"},{"last name":"Arns"}]"#; - assert_eq!(actual.out, expected_output); - }) + assert_eq!(actual.out, "Robalino Jnth"); +} + +#[test] +fn ignores_duplicate_columns_selected() { + let actual = nu!(cwd: ".", pipeline( + r#" + echo [ + ["first name", "last name"]; + + [Andrés Robalino] + [Andrés Jnth] + ] + | select "first name" "last name" "first name" + | get + | str collect " " + "# + )); + + assert_eq!(actual.out, "first name last name"); } diff --git a/crates/nu-engine/src/evaluation_context.rs b/crates/nu-engine/src/evaluation_context.rs index 6c24f6da5e..12f2454711 100644 --- a/crates/nu-engine/src/evaluation_context.rs +++ b/crates/nu-engine/src/evaluation_context.rs @@ -27,18 +27,6 @@ pub struct EvaluationContext { } impl EvaluationContext { - pub fn from_raw(raw_args: &CommandArgs) -> EvaluationContext { - EvaluationContext { - scope: raw_args.scope.clone(), - host: raw_args.host.clone(), - current_errors: raw_args.current_errors.clone(), - ctrl_c: raw_args.ctrl_c.clone(), - shell_manager: raw_args.shell_manager.clone(), - user_recently_used_autoenv_untrust: Arc::new(AtomicBool::new(false)), - windows_drives_previous_cwd: Arc::new(Mutex::new(std::collections::HashMap::new())), - } - } - pub fn from_args(args: &CommandArgs) -> EvaluationContext { EvaluationContext { scope: args.scope.clone(),