From b2734db015ff538fedb00ad846868b7d87c0ad34 Mon Sep 17 00:00:00 2001 From: Stefan Holderbach Date: Wed, 22 Nov 2023 23:48:48 +0100 Subject: [PATCH] Move more commands to opaque `Record` type (#11122) # Description Further work towards the goal that we can make `Record`'s field private and experiment with different internal representations ## Details - Use inplace record iter in `nu-command/math/utils` - Guarantee that existing allocation can be reused - Use proper record iterators in `path join` - Remove unnecesary hashmap in `path join` - Should minimally reduce the overhead - Unzip records in `nu-command` - Refactor `query web` plugin to use record APIs - Use `Record::into_values` for `values` command - Use `Record::columns()` in `join` instead. - Potential minor pessimisation - Not the hot value path - Use sane `Record` iters in example `Debug` impl - Avoid layout assumption in `nu-cmd-extra/roll/mod` - Potential minor pessimisation - relegated to `extra`, changing the representation may otherwise break this op. - Use record api in `rotate` - Minor risk that this surfaces some existing invalid behavior as panics as we now validate column/value lengths - `extra` so things are unstable - Remove unnecessary references in `rotate` - Bonus cleanup # User-Facing Changes None functional, minor potential differences in runtime. You win some, you lose some. # Tests + Formatting Relying on existing tests --- .../src/extra/filters/roll/mod.rs | 17 +++++----- .../nu-cmd-extra/src/extra/filters/rotate.rs | 29 +++++++--------- crates/nu-cmd-lang/src/example_support.rs | 9 +++-- crates/nu-command/src/debug/inspect_table.rs | 18 +++++----- crates/nu-command/src/filters/join.rs | 8 ++--- crates/nu-command/src/filters/values.rs | 24 +++++++++----- crates/nu-command/src/formats/from/yaml.rs | 14 ++++---- crates/nu-command/src/math/utils.rs | 27 +++++++-------- crates/nu-command/src/path/join.rs | 18 +++------- crates/nu_plugin_query/src/query_web.rs | 33 ++++++++----------- 10 files changed, 90 insertions(+), 107 deletions(-) diff --git a/crates/nu-cmd-extra/src/extra/filters/roll/mod.rs b/crates/nu-cmd-extra/src/extra/filters/roll/mod.rs index 70f029b65..f1a46e31d 100644 --- a/crates/nu-cmd-extra/src/extra/filters/roll/mod.rs +++ b/crates/nu-cmd-extra/src/extra/filters/roll/mod.rs @@ -4,7 +4,7 @@ mod roll_left; mod roll_right; mod roll_up; -use nu_protocol::{ShellError, Value}; +use nu_protocol::{Record, ShellError, Value}; pub use roll_::Roll; pub use roll_down::RollDown; pub use roll_left::RollLeft; @@ -54,24 +54,23 @@ fn horizontal_rotate_value( ) -> Result { let span = value.span(); match value { - Value::Record { - val: mut record, .. - } => { + Value::Record { val: record, .. } => { let rotations = by.map(|n| n % record.len()).unwrap_or(1); + let (mut cols, mut vals): (Vec<_>, Vec<_>) = record.into_iter().unzip(); if !cells_only { match direction { - HorizontalDirection::Right => record.cols.rotate_right(rotations), - HorizontalDirection::Left => record.cols.rotate_left(rotations), + HorizontalDirection::Right => cols.rotate_right(rotations), + HorizontalDirection::Left => cols.rotate_left(rotations), } }; match direction { - HorizontalDirection::Right => record.vals.rotate_right(rotations), - HorizontalDirection::Left => record.vals.rotate_left(rotations), + HorizontalDirection::Right => vals.rotate_right(rotations), + HorizontalDirection::Left => vals.rotate_left(rotations), } - Ok(Value::record(record, span)) + Ok(Value::record(Record::from_raw_cols_vals(cols, vals), span)) } Value::List { vals, .. } => { let values = vals diff --git a/crates/nu-cmd-extra/src/extra/filters/rotate.rs b/crates/nu-cmd-extra/src/extra/filters/rotate.rs index 2340d71b1..29c5c3968 100644 --- a/crates/nu-cmd-extra/src/extra/filters/rotate.rs +++ b/crates/nu-cmd-extra/src/extra/filters/rotate.rs @@ -166,7 +166,7 @@ pub fn rotate( let mut old_column_names = vec![]; let mut new_values = vec![]; let mut not_a_record = false; - let total_rows = &mut values.len(); + let mut total_rows = values.len(); let ccw: bool = call.has_flag("ccw"); if !ccw { @@ -178,10 +178,9 @@ pub fn rotate( let span = val.span(); match val { Value::Record { val: record, .. } => { - old_column_names = record.cols; - for v in record.vals { - new_values.push(v) - } + let (cols, vals): (Vec<_>, Vec<_>) = record.into_iter().unzip(); + old_column_names = cols; + new_values.extend_from_slice(&vals); } Value::List { vals, .. } => { not_a_record = true; @@ -208,17 +207,17 @@ pub fn rotate( }); } - let total_columns = &old_column_names.len(); + let total_columns = old_column_names.len(); // we use this for building columns names, but for non-records we get an extra row so we remove it - if *total_columns == 0 { - *total_rows -= 1; + if total_columns == 0 { + total_rows -= 1; } // holder for the new column names, particularly if none are provided by the user we create names as column0, column1, etc. let mut new_column_names = { let mut res = vec![]; - for idx in 0..(*total_rows + 1) { + for idx in 0..(total_rows + 1) { res.push(format!("column{idx}")); } res.to_vec() @@ -237,10 +236,7 @@ pub fn rotate( if not_a_record { return Ok(Value::list( vec![Value::record( - Record { - cols: new_column_names, - vals: new_values, - }, + Record::from_raw_cols_vals(new_column_names, new_values), call.head, )], call.head, @@ -276,7 +272,7 @@ pub fn rotate( let new_vals = { // move through the array with a step, which is every new_values size / total rows, starting from our old column's index // so if initial data was like this [[a b]; [1 2] [3 4]] - we basically iterate on this [3 4 1 2] array, so we pick 3, then 1, and then when idx increases, we pick 4 and 2 - for i in (idx..new_values.len()).step_by(new_values.len() / *total_rows) { + for i in (idx..new_values.len()).step_by(new_values.len() / total_rows) { res.push(new_values[i].clone()); } // when rotating clockwise, the old column names become the last column's values @@ -286,10 +282,7 @@ pub fn rotate( res.to_vec() }; final_values.push(Value::record( - Record { - cols: new_column_names.clone(), - vals: new_vals, - }, + Record::from_raw_cols_vals(new_column_names.clone(), new_vals), call.head, )) } diff --git a/crates/nu-cmd-lang/src/example_support.rs b/crates/nu-cmd-lang/src/example_support.rs index e713db3f5..f7b66ca95 100644 --- a/crates/nu-cmd-lang/src/example_support.rs +++ b/crates/nu-cmd-lang/src/example_support.rs @@ -233,13 +233,12 @@ impl<'a> std::fmt::Debug for DebuggableValue<'a> { } Value::Record { val, .. } => { write!(f, "{{")?; - for i in 0..val.len() { - let col = &val.cols[i]; - let value = &val.vals[i]; - - if i > 0 { + let mut first = true; + for (col, value) in val.into_iter() { + if !first { write!(f, ", ")?; } + first = false; write!(f, "{:?}: {:?}", col, DebuggableValue(value))?; } write!(f, "}}") diff --git a/crates/nu-command/src/debug/inspect_table.rs b/crates/nu-command/src/debug/inspect_table.rs index a79201152..0a83a5829 100644 --- a/crates/nu-command/src/debug/inspect_table.rs +++ b/crates/nu-command/src/debug/inspect_table.rs @@ -200,14 +200,16 @@ mod util { pub fn collect_input(value: Value) -> (Vec, Vec>) { let span = value.span(); match value { - Value::Record { val: record, .. } => ( - record.cols, - vec![record - .vals - .into_iter() - .map(|s| debug_string_without_formatting(&s)) - .collect()], - ), + Value::Record { val: record, .. } => { + let (cols, vals): (Vec<_>, Vec<_>) = record.into_iter().unzip(); + ( + cols, + vec![vals + .into_iter() + .map(|s| debug_string_without_formatting(&s)) + .collect()], + ) + } Value::List { vals, .. } => { let mut columns = get_columns(&vals); let data = convert_records_to_dataset(&columns, vals); diff --git a/crates/nu-command/src/filters/join.rs b/crates/nu-command/src/filters/join.rs index 24549f179..7e02070b3 100644 --- a/crates/nu-command/src/filters/join.rs +++ b/crates/nu-command/src/filters/join.rs @@ -245,7 +245,7 @@ fn join_rows( this: &[Value], this_join_key: &str, other: HashMap>, - other_keys: &[String], + other_keys: Vec<&String>, shared_join_key: Option<&str>, join_type: &JoinType, include_inner: IncludeInner, @@ -285,7 +285,7 @@ fn join_rows( // row with null values for columns not present, let other_record = other_keys .iter() - .map(|key| { + .map(|&key| { let val = if Some(key.as_ref()) == shared_join_key { this_record .get(key) @@ -318,11 +318,11 @@ fn join_rows( // Return column names (i.e. ordered keys from the first row; we assume that // these are the same for all rows). -fn column_names(table: &[Value]) -> &[String] { +fn column_names(table: &[Value]) -> Vec<&String> { table .iter() .find_map(|val| match val { - Value::Record { val, .. } => Some(&*val.cols), + Value::Record { val, .. } => Some(val.columns().collect()), _ => None, }) .unwrap_or_default() diff --git a/crates/nu-command/src/filters/values.rs b/crates/nu-command/src/filters/values.rs index 5a6be33b7..e74c8a65f 100644 --- a/crates/nu-command/src/filters/values.rs +++ b/crates/nu-command/src/filters/values.rs @@ -163,16 +163,24 @@ fn values( Err(err) => Err(err), } } - Value::Record { val, .. } => { - Ok(val.vals.into_pipeline_data(ctrlc).set_metadata(metadata)) - } - Value::LazyRecord { val, .. } => Ok(val - .collect()? - .as_record()? - .vals - .clone() + Value::Record { val, .. } => Ok(val + .into_values() .into_pipeline_data(ctrlc) .set_metadata(metadata)), + Value::LazyRecord { val, .. } => { + let record = match val.collect()? { + Value::Record { val, .. } => val, + _ => Err(ShellError::NushellFailedSpanned { + msg: "`LazyRecord::collect()` promises `Value::Record`".into(), + label: "Violating lazy record found here".into(), + span, + })?, + }; + Ok(record + .into_values() + .into_pipeline_data(ctrlc) + .set_metadata(metadata)) + } // Propagate errors Value::Error { error, .. } => Err(*error), other => Err(ShellError::OnlySupportsThisInputType { diff --git a/crates/nu-command/src/formats/from/yaml.rs b/crates/nu-command/src/formats/from/yaml.rs index 113433658..adc98ec88 100644 --- a/crates/nu-command/src/formats/from/yaml.rs +++ b/crates/nu-command/src/formats/from/yaml.rs @@ -350,16 +350,16 @@ mod test { let actual_record = actual_vals[jj].as_record().unwrap(); let expected_record = expected_vals[jj].as_record().unwrap(); - let actual_columns = &actual_record.cols; - let expected_columns = &expected_record.cols; - assert_eq!( - expected_columns, actual_columns, + let actual_columns = actual_record.columns(); + let expected_columns = expected_record.columns(); + assert!( + expected_columns.eq(actual_columns), "record {jj}, iteration {ii}" ); - let actual_vals = &actual_record.vals; - let expected_vals = &expected_record.vals; - assert_eq!(expected_vals, actual_vals, "record {jj}, iteration {ii}") + let actual_vals = actual_record.values(); + let expected_vals = expected_record.values(); + assert!(expected_vals.eq(actual_vals), "record {jj}, iteration {ii}") } } } diff --git a/crates/nu-command/src/math/utils.rs b/crates/nu-command/src/math/utils.rs index ee1dc8228..d10e88d22 100644 --- a/crates/nu-command/src/math/utils.rs +++ b/crates/nu-command/src/math/utils.rs @@ -1,6 +1,8 @@ +use core::slice; + use indexmap::map::IndexMap; use nu_protocol::ast::Call; -use nu_protocol::{IntoPipelineData, PipelineData, Record, ShellError, Span, Value}; +use nu_protocol::{IntoPipelineData, PipelineData, ShellError, Span, Value}; pub fn run_with_function( call: &Call, @@ -81,21 +83,14 @@ pub fn calculate( _ => mf(vals, span, name), }, PipelineData::Value(Value::Record { val: record, .. }, ..) => { - let new_vals: Result, ShellError> = record - .vals - .into_iter() - .map(|val| mf(&[val], span, name)) - .collect(); - match new_vals { - Ok(vec) => Ok(Value::record( - Record { - cols: record.cols, - vals: vec, - }, - span, - )), - Err(err) => Err(err), - } + let mut record = record; + record + .iter_mut() + .try_for_each(|(_, val)| -> Result<(), ShellError> { + *val = mf(slice::from_ref(val), span, name)?; + Ok(()) + })?; + Ok(Value::record(record, span)) } PipelineData::Value(Value::Range { val, .. }, ..) => { let new_vals: Result, ShellError> = val diff --git a/crates/nu-command/src/path/join.rs b/crates/nu-command/src/path/join.rs index 5196fe674..bfe618c77 100644 --- a/crates/nu-command/src/path/join.rs +++ b/crates/nu-command/src/path/join.rs @@ -1,4 +1,3 @@ -use std::collections::HashMap; use std::path::{Path, PathBuf}; use nu_engine::CallExt; @@ -246,7 +245,7 @@ fn join_record(record: &Record, head: Span, span: Span, args: &Arguments) -> Val } fn merge_record(record: &Record, head: Span, span: Span) -> Result { - for key in &record.cols { + for key in record.columns() { if !super::ALLOWED_COLUMNS.contains(&key.as_str()) { let allowed_cols = super::ALLOWED_COLUMNS.join(", "); return Err(ShellError::UnsupportedInput { msg: format!( @@ -255,24 +254,17 @@ fn merge_record(record: &Record, head: Span, span: Span) -> Result = record - .cols - .iter() - .map(String::as_str) - .zip(&record.vals) - .collect(); - let mut result = PathBuf::new(); #[cfg(windows)] - if let Some(val) = entries.get("prefix") { + if let Some(val) = record.get("prefix") { let p = val.as_string()?; if !p.is_empty() { result.push(p); } } - if let Some(val) = entries.get("parent") { + if let Some(val) = record.get("parent") { let p = val.as_string()?; if !p.is_empty() { result.push(p); @@ -280,14 +272,14 @@ fn merge_record(record: &Record, head: Span, span: Span) -> Result Value { table_out.push(Value::record(record, span)) } else { for row in &table { - let mut vals = vec![]; - let record_cols = &cols; - for col in &cols { - let val = row - .get(col) - .unwrap_or(&format!("Missing column: '{}'", &col)) - .to_string(); + let record = cols + .iter() + .map(|col| { + let val = row + .get(col) + .unwrap_or(&format!("Missing column: '{}'", &col)) + .to_string(); - if !at_least_one_row_filled && val != format!("Missing column: '{}'", &col) { - at_least_one_row_filled = true; - } - vals.push(Value::string(val, span)); - } - table_out.push(Value::record( - Record { - cols: record_cols.to_vec(), - vals, - }, - span, - )) + if !at_least_one_row_filled && val != format!("Missing column: '{}'", &col) { + at_least_one_row_filled = true; + } + (col.clone(), Value::string(val, span)) + }) + .collect(); + table_out.push(Value::record(record, span)) } } if !at_least_one_row_filled {