Move more commands to opaque Record type (nushell#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

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<Value, ShellError> {
     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

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,
         ))
     }

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, "}}")

crates/nu-command/src/debug/inspect_table.rs

@@ -200,14 +200,16 @@ mod util {
     pub fn collect_input(value: Value) -> (Vec<String>, Vec<Vec<String>>) {
         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);

crates/nu-command/src/filters/join.rs

@@ -245,7 +245,7 @@ fn join_rows(
     this: &[Value],
     this_join_key: &str,
     other: HashMap<String, Vec<&Record>>,
-    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()

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 {

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}")
             }
         }
     }

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<Vec<Value>, 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<Vec<Value>, ShellError> = val

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<PathBuf, ShellError> {
-    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,39 +254,32 @@ fn merge_record(record: &Record, head: Span, span: Span) -> Result<PathBuf, Shel
         }
     }
 
-    let entries: HashMap<&str, &Value> = 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);
         }
     }
 
     let mut basename = String::new();
-    if let Some(val) = entries.get("stem") {
+    if let Some(val) = record.get("stem") {
         let p = val.as_string()?;
         if !p.is_empty() {
             basename.push_str(&p);
         }
     }
 
-    if let Some(val) = entries.get("extension") {
+    if let Some(val) = record.get("extension") {
         let p = val.as_string()?;
         if !p.is_empty() {
             basename.push('.');

crates/nu_plugin_query/src/query_web.rs

@@ -180,26 +180,21 @@ fn retrieve_table(mut table: WebTable, columns: &Value, span: Span) -> 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 {