From 56cdee1fd8b7d8102c06fc40415d272cabb5c7d8 Mon Sep 17 00:00:00 2001 From: Ian Manske Date: Sat, 13 Apr 2024 14:58:54 +0000 Subject: [PATCH] Refactor `first` and `last` (#12478) # Description - Refactors `first` and `last` using `Vec::truncate` and `Vec::drain`. - `std::mem::take` was also used to eliminate a few `Value` clones. - The `NeedsPositiveValue` error now uses the span of the `rows` argument instead of the call head span. - `last` now errors on an empty stream to match `first` which does error. - Made metadata preservation more consistent. # User-Facing Changes Breaking change: `last` now errors on an empty stream to match `first` which does error. --- crates/nu-command/src/filters/first.rs | 56 ++++++++-------- crates/nu-command/src/filters/last.rs | 71 ++++++++++----------- crates/nu-command/tests/commands/default.rs | 2 +- crates/nu-command/tests/commands/first.rs | 2 +- crates/nu-command/tests/commands/last.rs | 2 +- 5 files changed, 65 insertions(+), 68 deletions(-) diff --git a/crates/nu-command/src/filters/first.rs b/crates/nu-command/src/filters/first.rs index 13408c00bb..eabd370858 100644 --- a/crates/nu-command/src/filters/first.rs +++ b/crates/nu-command/src/filters/first.rs @@ -79,60 +79,60 @@ fn first_helper( input: PipelineData, ) -> Result { let head = call.head; - let rows: Option = call.opt(engine_state, stack, 0)?; + let rows: Option> = call.opt(engine_state, stack, 0)?; + // FIXME: for backwards compatibility reasons, if `rows` is not specified we // return a single element and otherwise we return a single list. We should probably // remove `rows` so that `first` always returns a single element; getting a list of // the first N elements is covered by `take` let return_single_element = rows.is_none(); - let rows_desired: usize = match rows { - Some(i) if i < 0 => return Err(ShellError::NeedsPositiveValue { span: head }), - Some(x) => x as usize, - None => 1, + let rows = if let Some(rows) = rows { + if rows.item < 0 { + return Err(ShellError::NeedsPositiveValue { span: rows.span }); + } else { + rows.item as usize + } + } else { + 1 }; - let ctrlc = engine_state.ctrlc.clone(); let metadata = input.metadata(); // early exit for `first 0` - if rows_desired == 0 { - return Ok(Vec::::new().into_pipeline_data_with_metadata(metadata, ctrlc)); + if rows == 0 { + return Ok(Value::list(Vec::new(), head).into_pipeline_data_with_metadata(metadata)); } match input { PipelineData::Value(val, _) => { let span = val.span(); match val { - Value::List { vals, .. } => { + Value::List { mut vals, .. } => { if return_single_element { - if vals.is_empty() { - Err(ShellError::AccessEmptyContent { span: head }) + if let Some(val) = vals.first_mut() { + Ok(std::mem::take(val).into_pipeline_data()) } else { - Ok(vals[0].clone().into_pipeline_data()) + Err(ShellError::AccessEmptyContent { span: head }) } } else { - Ok(vals - .into_iter() - .take(rows_desired) - .into_pipeline_data_with_metadata(metadata, ctrlc)) + vals.truncate(rows); + Ok(Value::list(vals, span).into_pipeline_data_with_metadata(metadata)) } } - Value::Binary { val, .. } => { + Value::Binary { mut val, .. } => { if return_single_element { - if val.is_empty() { - Err(ShellError::AccessEmptyContent { span: head }) + if let Some(&val) = val.first() { + Ok(Value::int(val.into(), span).into_pipeline_data()) } else { - Ok(PipelineData::Value( - Value::int(val[0] as i64, span), - metadata, - )) + Err(ShellError::AccessEmptyContent { span: head }) } } else { - let slice: Vec = val.into_iter().take(rows_desired).collect(); - Ok(PipelineData::Value(Value::binary(slice, span), metadata)) + val.truncate(rows); + Ok(Value::binary(val, span).into_pipeline_data_with_metadata(metadata)) } } Value::Range { val, .. } => { + let ctrlc = engine_state.ctrlc.clone(); let mut iter = val.into_range_iter(span, ctrlc.clone()); if return_single_element { if let Some(v) = iter.next() { @@ -142,7 +142,7 @@ fn first_helper( } } else { Ok(iter - .take(rows_desired) + .take(rows) .into_pipeline_data_with_metadata(metadata, ctrlc)) } } @@ -165,8 +165,8 @@ fn first_helper( } } else { Ok(ls - .take(rows_desired) - .into_pipeline_data_with_metadata(metadata, ctrlc)) + .take(rows) + .into_pipeline_data_with_metadata(metadata, engine_state.ctrlc.clone())) } } PipelineData::ExternalStream { span, .. } => Err(ShellError::OnlySupportsThisInputType { diff --git a/crates/nu-command/src/filters/last.rs b/crates/nu-command/src/filters/last.rs index 92d83d1595..f41b7c7e4d 100644 --- a/crates/nu-command/src/filters/last.rs +++ b/crates/nu-command/src/filters/last.rs @@ -70,34 +70,41 @@ impl Command for Last { input: PipelineData, ) -> Result { let head = call.head; - let rows: Option = call.opt(engine_state, stack, 0)?; + let rows: Option> = call.opt(engine_state, stack, 0)?; // FIXME: Please read the FIXME message in `first.rs`'s `first_helper` implementation. // It has the same issue. let return_single_element = rows.is_none(); - let rows_desired: usize = match rows { - Some(i) if i < 0 => return Err(ShellError::NeedsPositiveValue { span: head }), - Some(x) => x as usize, - None => 1, + let rows = if let Some(rows) = rows { + if rows.item < 0 { + return Err(ShellError::NeedsPositiveValue { span: rows.span }); + } else { + rows.item as usize + } + } else { + 1 }; - let ctrlc = engine_state.ctrlc.clone(); let metadata = input.metadata(); // early exit for `last 0` - if rows_desired == 0 { - return Ok(Vec::::new().into_pipeline_data_with_metadata(metadata, ctrlc)); + if rows == 0 { + return Ok(Value::list(Vec::new(), head).into_pipeline_data_with_metadata(metadata)); } match input { PipelineData::ListStream(_, _) | PipelineData::Value(Value::Range { .. }, _) => { let iterator = input.into_iter_strict(head)?; - // only keep last `rows_desired` rows in memory - let mut buf = VecDeque::<_>::new(); + // only keep the last `rows` in memory + let mut buf = VecDeque::new(); for row in iterator { - if buf.len() == rows_desired { + if nu_utils::ctrl_c::was_pressed(&engine_state.ctrlc) { + return Err(ShellError::InterruptedByUser { span: Some(head) }); + } + + if buf.len() == rows { buf.pop_front(); } @@ -106,51 +113,41 @@ impl Command for Last { if return_single_element { if let Some(last) = buf.pop_back() { - Ok(last.into_pipeline_data_with_metadata(metadata)) + Ok(last.into_pipeline_data()) } else { - Ok(PipelineData::empty().set_metadata(metadata)) + Err(ShellError::AccessEmptyContent { span: head }) } } else { - Ok(buf.into_pipeline_data_with_metadata(metadata, ctrlc)) + Ok(Value::list(buf.into(), head).into_pipeline_data_with_metadata(metadata)) } } PipelineData::Value(val, _) => { - let val_span = val.span(); - + let span = val.span(); match val { - Value::List { vals, .. } => { + Value::List { mut vals, .. } => { if return_single_element { - if let Some(v) = vals.last() { - Ok(v.clone().into_pipeline_data()) + if let Some(v) = vals.pop() { + Ok(v.into_pipeline_data()) } else { Err(ShellError::AccessEmptyContent { span: head }) } } else { - Ok(vals - .into_iter() - .rev() - .take(rows_desired) - .rev() - .into_pipeline_data_with_metadata(metadata, ctrlc)) + let i = vals.len().saturating_sub(rows); + vals.drain(..i); + Ok(Value::list(vals, span).into_pipeline_data_with_metadata(metadata)) } } - Value::Binary { val, .. } => { + Value::Binary { mut val, .. } => { if return_single_element { - if let Some(b) = val.last() { - Ok(PipelineData::Value( - Value::int(*b as i64, val_span), - metadata, - )) + if let Some(val) = val.pop() { + Ok(Value::int(val.into(), span).into_pipeline_data()) } else { Err(ShellError::AccessEmptyContent { span: head }) } } else { - let slice: Vec = - val.into_iter().rev().take(rows_desired).rev().collect(); - Ok(PipelineData::Value( - Value::binary(slice, val_span), - metadata, - )) + let i = val.len().saturating_sub(rows); + val.drain(..i); + Ok(Value::binary(val, span).into_pipeline_data()) } } // Propagate errors by explicitly matching them before the final case. diff --git a/crates/nu-command/tests/commands/default.rs b/crates/nu-command/tests/commands/default.rs index 87426fb1ea..4fd41d7ec0 100644 --- a/crates/nu-command/tests/commands/default.rs +++ b/crates/nu-command/tests/commands/default.rs @@ -28,7 +28,7 @@ fn adds_row_data_if_column_missing() { #[test] fn default_after_empty_filter() { - let actual = nu!("[a b] | where $it == 'c' | last | default 'd'"); + let actual = nu!("[a b] | where $it == 'c' | get -i 0 | default 'd'"); assert_eq!(actual.out, "d"); } diff --git a/crates/nu-command/tests/commands/first.rs b/crates/nu-command/tests/commands/first.rs index b8fe816e1b..b034274db0 100644 --- a/crates/nu-command/tests/commands/first.rs +++ b/crates/nu-command/tests/commands/first.rs @@ -51,7 +51,7 @@ fn gets_first_row_when_no_amount_given() { fn gets_first_row_as_list_when_amount_given() { let actual = nu!("[1, 2, 3] | first 1 | describe"); - assert_eq!(actual.out, "list (stream)"); + assert_eq!(actual.out, "list"); } #[test] diff --git a/crates/nu-command/tests/commands/last.rs b/crates/nu-command/tests/commands/last.rs index 013c1e7391..f842f26520 100644 --- a/crates/nu-command/tests/commands/last.rs +++ b/crates/nu-command/tests/commands/last.rs @@ -51,7 +51,7 @@ fn requests_more_rows_than_table_has() { fn gets_last_row_as_list_when_amount_given() { let actual = nu!("[1, 2, 3] | last 1 | describe"); - assert_eq!(actual.out, "list (stream)"); + assert_eq!(actual.out, "list"); } #[test]