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
This commit is contained in:
Stefan Holderbach
2023-11-22 23:48:48 +01:00
committed by GitHub
parent 823e578c46
commit b2734db015
10 changed files with 90 additions and 107 deletions

View File

@ -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);

View File

@ -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()

View File

@ -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 {

View File

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

View File

@ -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

View File

@ -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,24 +254,17 @@ 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);
@ -280,14 +272,14 @@ fn merge_record(record: &Record, head: Span, span: Span) -> Result<PathBuf, Shel
}
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('.');