From b0ab78a76705550dbeb5a2226ccd986977d80de3 Mon Sep 17 00:00:00 2001 From: JT Date: Tue, 7 Sep 2021 19:07:11 +1200 Subject: [PATCH 1/2] Switch tables to list/streams of records --- TODO.md | 2 +- crates/nu-cli/src/errors.rs | 4 +- crates/nu-command/src/each.rs | 4 +- crates/nu-command/src/for_.rs | 4 +- crates/nu-command/src/length.rs | 18 +- crates/nu-engine/src/eval.rs | 17 +- crates/nu-parser/src/parser.rs | 6 +- .../src/engine/evaluation_context.rs | 11 +- crates/nu-protocol/src/syntax_shape.rs | 2 +- crates/nu-protocol/src/ty.rs | 6 +- crates/nu-protocol/src/value.rs | 166 ++++++------------ 11 files changed, 75 insertions(+), 165 deletions(-) diff --git a/TODO.md b/TODO.md index 36c294ace7..6f2b6e3626 100644 --- a/TODO.md +++ b/TODO.md @@ -23,7 +23,7 @@ - [ ] Source - [ ] Autoenv - [ ] Externals -- [ ] let [first, rest] = [1, 2, 3] +- [ ] let [first, rest] = [1, 2, 3] (design question: how do you pattern match a table?) ## Maybe: - [ ] default param values? diff --git a/crates/nu-cli/src/errors.rs b/crates/nu-cli/src/errors.rs index 81cce51b5c..a05a6cb238 100644 --- a/crates/nu-cli/src/errors.rs +++ b/crates/nu-cli/src/errors.rs @@ -348,9 +348,9 @@ pub fn report_shell_error( let (diag_file_id, diag_range) = convert_span_to_diag(working_set, span)?; Diagnostic::error() - .with_message("Data cannot be accessed with a column path") + .with_message("Data cannot be accessed with a cell path") .with_labels(vec![Label::primary(diag_file_id, diag_range) - .with_message(format!("{} doesn't support column paths", name))]) + .with_message(format!("{} doesn't support cell paths", name))]) } ShellError::CantFindColumn(span) => { let (diag_file_id, diag_range) = convert_span_to_diag(working_set, span)?; diff --git a/crates/nu-command/src/each.rs b/crates/nu-command/src/each.rs index 10d04e3c46..24733b1490 100644 --- a/crates/nu-command/src/each.rs +++ b/crates/nu-command/src/each.rs @@ -52,7 +52,7 @@ impl Command for Each { .into_value_stream(), span: call.head, }), - Value::List { val, .. } => Ok(Value::ValueStream { + Value::List { vals: val, .. } => Ok(Value::ValueStream { stream: val .into_iter() .map(move |x| { @@ -95,8 +95,6 @@ impl Command for Each { .into_value_stream(), span: call.head, }), - Value::RowStream { .. } => panic!("iterating row streams is not yet supported"), - Value::Table { .. } => panic!("table iteration not yet supported"), x => { //TODO: we need to watch to make sure this is okay let engine_state = context.engine_state.borrow(); diff --git a/crates/nu-command/src/for_.rs b/crates/nu-command/src/for_.rs index 7965c1fb5d..5ba8bb8df4 100644 --- a/crates/nu-command/src/for_.rs +++ b/crates/nu-command/src/for_.rs @@ -68,8 +68,8 @@ impl Command for For { .into_value_stream(), span: call.head, }), - Value::List { val, .. } => Ok(Value::List { - val: val + Value::List { vals: val, .. } => Ok(Value::List { + vals: val .into_iter() .map(move |x| { let engine_state = context.engine_state.borrow(); diff --git a/crates/nu-command/src/length.rs b/crates/nu-command/src/length.rs index 00247287ce..854a5c4af9 100644 --- a/crates/nu-command/src/length.rs +++ b/crates/nu-command/src/length.rs @@ -24,15 +24,7 @@ impl Command for Length { input: Value, ) -> Result { match input { - Value::List { val, .. } => { - let length = val.len(); - - Ok(Value::Int { - val: length as i64, - span: call.head, - }) - } - Value::Table { val, .. } => { + Value::List { vals: val, .. } => { let length = val.len(); Ok(Value::Int { @@ -48,14 +40,6 @@ impl Command for Length { span: call.head, }) } - Value::RowStream { stream, .. } => { - let length = stream.count(); - - Ok(Value::Int { - val: length as i64, - span: call.head, - }) - } Value::Nothing { .. } => Ok(Value::Int { val: 0, span: call.head, diff --git a/crates/nu-engine/src/eval.rs b/crates/nu-engine/src/eval.rs index ba7ac8593b..d04c393484 100644 --- a/crates/nu-engine/src/eval.rs +++ b/crates/nu-engine/src/eval.rs @@ -55,7 +55,7 @@ fn eval_call(context: &EvaluationContext, call: &Call, input: Value) -> Result { let value = eval_expression(context, &column_path.head)?; - value.follow_column_path(&column_path.tail) + value.follow_cell_path(&column_path.tail) } Expr::Call(call) => eval_call(context, call, Value::nothing()), Expr::ExternalCall(_, _) => Err(ShellError::ExternalNotSupported(expr.span)), @@ -176,7 +176,7 @@ pub fn eval_expression( output.push(eval_expression(context, expr)?); } Ok(Value::List { - val: output, + vals: output, span: expr.span, }) } @@ -192,11 +192,14 @@ pub fn eval_expression( for expr in val { row.push(eval_expression(context, expr)?); } - output_rows.push(row); + output_rows.push(Value::Record { + cols: output_headers.clone(), + vals: row, + span: expr.span, + }); } - Ok(Value::Table { - headers: output_headers, - val: output_rows, + Ok(Value::List { + vals: output_rows, span: expr.span, }) } diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index 45cddb5d21..c3c05e2d57 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -1781,7 +1781,7 @@ pub fn parse_table_expression( Expression { expr: Expr::List(vec![]), span, - ty: Type::Table, + ty: Type::List(Box::new(Type::Unknown)), }, None, ), @@ -1828,7 +1828,7 @@ pub fn parse_table_expression( Expression { expr: Expr::Table(table_headers, rows), span, - ty: Type::Table, + ty: Type::List(Box::new(Type::Unknown)), }, error, ) @@ -1965,7 +1965,7 @@ pub fn parse_value( // First, check the special-cases. These will likely represent specific values as expressions // and may fit a variety of shapes. // - // We check variable first because immediately following we check for variables with column paths + // We check variable first because immediately following we check for variables with cell paths // which might result in a value that fits other shapes (and require the variable to already be // declared) if shape == &SyntaxShape::Variable { diff --git a/crates/nu-protocol/src/engine/evaluation_context.rs b/crates/nu-protocol/src/engine/evaluation_context.rs index 8eaa68752e..01dd9ea56d 100644 --- a/crates/nu-protocol/src/engine/evaluation_context.rs +++ b/crates/nu-protocol/src/engine/evaluation_context.rs @@ -29,17 +29,8 @@ impl EvaluationContext { // TODO: add ctrl-c support let value = match value { - Value::RowStream { - headers, - stream, - span, - } => Value::Table { - headers, - val: stream.collect(), - span, - }, Value::ValueStream { stream, span } => Value::List { - val: stream.collect(), + vals: stream.collect(), span, }, x => x, diff --git a/crates/nu-protocol/src/syntax_shape.rs b/crates/nu-protocol/src/syntax_shape.rs index 8e73ef22f0..3e3337cd0f 100644 --- a/crates/nu-protocol/src/syntax_shape.rs +++ b/crates/nu-protocol/src/syntax_shape.rs @@ -96,7 +96,7 @@ impl SyntaxShape { SyntaxShape::RowCondition => Type::Bool, SyntaxShape::Signature => Type::Unknown, SyntaxShape::String => Type::String, - SyntaxShape::Table => Type::Table, + SyntaxShape::Table => Type::List(Box::new(Type::Unknown)), // FIXME SyntaxShape::VarWithOptType => Type::Unknown, SyntaxShape::Variable => Type::Unknown, } diff --git a/crates/nu-protocol/src/ty.rs b/crates/nu-protocol/src/ty.rs index 6056b641dd..bd441c3409 100644 --- a/crates/nu-protocol/src/ty.rs +++ b/crates/nu-protocol/src/ty.rs @@ -15,8 +15,7 @@ pub enum Type { List(Box), Number, Nothing, - Table, - RowStream, + Record(Vec, Vec), ValueStream, Unknown, Error, @@ -34,13 +33,12 @@ impl Display for Type { Type::Float => write!(f, "float"), Type::Int => write!(f, "int"), Type::Range => write!(f, "range"), + Type::Record(cols, vals) => write!(f, "record<{}, {:?}>", cols.join(", "), vals), Type::List(l) => write!(f, "list<{}>", l), Type::Nothing => write!(f, "nothing"), Type::Number => write!(f, "number"), Type::String => write!(f, "string"), - Type::Table => write!(f, "table"), Type::ValueStream => write!(f, "value stream"), - Type::RowStream => write!(f, "row stream"), Type::Unknown => write!(f, "unknown"), Type::Error => write!(f, "error"), } diff --git a/crates/nu-protocol/src/value.rs b/crates/nu-protocol/src/value.rs index 53ca35c281..0fc760406e 100644 --- a/crates/nu-protocol/src/value.rs +++ b/crates/nu-protocol/src/value.rs @@ -247,22 +247,17 @@ pub enum Value { val: String, span: Span, }, + Record { + cols: Vec, + vals: Vec, + span: Span, + }, ValueStream { stream: ValueStream, span: Span, }, - RowStream { - headers: Vec, - stream: RowStream, - span: Span, - }, List { - val: Vec, - span: Span, - }, - Table { - headers: Vec, - val: Vec>, + vals: Vec, span: Span, }, Block { @@ -292,10 +287,9 @@ impl Value { Value::Float { span, .. } => *span, Value::Range { span, .. } => *span, Value::String { span, .. } => *span, + Value::Record { span, .. } => *span, Value::List { span, .. } => *span, - Value::Table { span, .. } => *span, Value::Block { span, .. } => *span, - Value::RowStream { span, .. } => *span, Value::ValueStream { span, .. } => *span, Value::Nothing { span, .. } => *span, Value::Error { .. } => Span::unknown(), @@ -309,10 +303,9 @@ impl Value { Value::Float { span, .. } => *span = new_span, Value::Range { span, .. } => *span = new_span, Value::String { span, .. } => *span = new_span, - Value::RowStream { span, .. } => *span = new_span, + Value::Record { span, .. } => *span = new_span, Value::ValueStream { span, .. } => *span = new_span, Value::List { span, .. } => *span = new_span, - Value::Table { span, .. } => *span = new_span, Value::Block { span, .. } => *span = new_span, Value::Nothing { span, .. } => *span = new_span, Value::Error { .. } => {} @@ -328,12 +321,13 @@ impl Value { Value::Float { .. } => Type::Float, Value::Range { .. } => Type::Range, Value::String { .. } => Type::String, + Value::Record { cols, vals, .. } => { + Type::Record(cols.clone(), vals.iter().map(|x| x.get_type()).collect()) + } Value::List { .. } => Type::List(Box::new(Type::Unknown)), // FIXME - Value::Table { .. } => Type::Table, // FIXME Value::Nothing { .. } => Type::Nothing, Value::Block { .. } => Type::Block, Value::ValueStream { .. } => Type::ValueStream, - Value::RowStream { .. } => Type::RowStream, Value::Error { .. } => Type::Error, } } @@ -364,29 +358,21 @@ impl Value { } Value::String { val, .. } => val, Value::ValueStream { stream, .. } => stream.into_string(), - Value::List { val, .. } => format!( + Value::List { vals: val, .. } => format!( "[{}]", val.into_iter() .map(|x| x.into_string()) .collect::>() .join(", ") ), - Value::Table { val, headers, .. } => format!( - "[= {} =\n {}]", - headers.join(", "), - val.into_iter() - .map(|x| { - x.into_iter() - .map(|x| x.into_string()) - .collect::>() - .join(", ") - }) + Value::Record { cols, vals, .. } => format!( + "{{{}}}", + cols.iter() + .zip(vals.iter()) + .map(|(x, y)| format!("{}: {}", x, y.clone().into_string())) .collect::>() - .join("\n") + .join(", ") ), - Value::RowStream { - headers, stream, .. - } => stream.into_string(headers), Value::Block { val, .. } => format!("", val), Value::Nothing { .. } => String::new(), Value::Error { error } => format!("{:?}", error), @@ -399,7 +385,7 @@ impl Value { } } - pub fn follow_column_path(self, column_path: &[PathMember]) -> Result { + pub fn follow_cell_path(self, column_path: &[PathMember]) -> Result { let mut current = self; for member in column_path { // FIXME: this uses a few extra clones for simplicity, but there may be a way @@ -411,7 +397,7 @@ impl Value { } => { // Treat a numeric path member as `nth ` match &mut current { - Value::List { val, .. } => { + Value::List { vals: val, .. } => { if let Some(item) = val.get(*count) { current = item.clone(); } else { @@ -425,32 +411,6 @@ impl Value { return Err(ShellError::AccessBeyondEndOfStream(*origin_span)); } } - Value::Table { headers, val, span } => { - if let Some(row) = val.get(*count) { - current = Value::Table { - headers: headers.clone(), - val: vec![row.clone()], - span: *span, - } - } else { - return Err(ShellError::AccessBeyondEnd(val.len(), *origin_span)); - } - } - Value::RowStream { - headers, - stream, - span, - } => { - if let Some(row) = stream.nth(*count) { - current = Value::Table { - headers: headers.clone(), - val: vec![row.clone()], - span: *span, - } - } else { - return Err(ShellError::AccessBeyondEndOfStream(*origin_span)); - } - } x => { return Err(ShellError::IncompatiblePathAccess( format!("{}", x.get_type()), @@ -460,28 +420,15 @@ impl Value { } } PathMember::String { - val, + val: column_name, span: origin_span, } => match &mut current { - Value::Table { - headers, - val: cells, - span, - } => { + Value::Record { cols, vals, .. } => { let mut found = false; - for header in headers.iter().enumerate() { - if header.1 == val { + for col in cols.iter().zip(vals.iter()) { + if col.0 == column_name { + current = col.1.clone(); found = true; - - let mut column = vec![]; - for row in cells { - column.push(row[header.0].clone()) - } - - current = Value::List { - val: column, - span: *span, - }; break; } } @@ -490,33 +437,22 @@ impl Value { return Err(ShellError::CantFindColumn(*origin_span)); } } - Value::RowStream { - headers, - stream, - span, - } => { - let mut found = false; - for header in headers.iter().enumerate() { - if header.1 == val { - found = true; - - let mut column = vec![]; - for row in stream { - column.push(row[header.0].clone()) + Value::List { vals, span } => { + let mut output = vec![]; + for val in vals { + if let Value::Record { cols, vals, .. } = val { + for col in cols.iter().enumerate() { + if col.1 == column_name { + output.push(vals[col.0].clone()); + } } - - current = Value::List { - val: column, - span: *span, - }; - break; } } - if !found { - //FIXME: add "did you mean" - return Err(ShellError::CantFindColumn(*origin_span)); - } + current = Value::List { + vals: output, + span: *span, + }; } x => { return Err(ShellError::IncompatiblePathAccess( @@ -844,19 +780,19 @@ impl Value { val: lhs == rhs, span, }), - (Value::List { val: lhs, .. }, Value::List { val: rhs, .. }) => Ok(Value::Bool { + (Value::List { vals: lhs, .. }, Value::List { vals: rhs, .. }) => Ok(Value::Bool { val: lhs == rhs, span, }), ( - Value::Table { - val: lhs, - headers: lhs_headers, + Value::Record { + vals: lhs, + cols: lhs_headers, .. }, - Value::Table { - val: rhs, - headers: rhs_headers, + Value::Record { + vals: rhs, + cols: rhs_headers, .. }, ) => Ok(Value::Bool { @@ -899,19 +835,19 @@ impl Value { val: lhs != rhs, span, }), - (Value::List { val: lhs, .. }, Value::List { val: rhs, .. }) => Ok(Value::Bool { + (Value::List { vals: lhs, .. }, Value::List { vals: rhs, .. }) => Ok(Value::Bool { val: lhs != rhs, span, }), ( - Value::Table { - val: lhs, - headers: lhs_headers, + Value::Record { + vals: lhs, + cols: lhs_headers, .. }, - Value::Table { - val: rhs, - headers: rhs_headers, + Value::Record { + vals: rhs, + cols: rhs_headers, .. }, ) => Ok(Value::Bool { From 6af3affee2f4c3cb61c7d8609d970c7cf5b82862 Mon Sep 17 00:00:00 2001 From: JT Date: Tue, 7 Sep 2021 19:09:49 +1200 Subject: [PATCH 2/2] add a test and update TODO --- TODO.md | 2 +- src/tests.rs | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/TODO.md b/TODO.md index 6f2b6e3626..5a2cb05062 100644 --- a/TODO.md +++ b/TODO.md @@ -16,7 +16,7 @@ - [x] Ranges - [x] Column path - [x] ...rest without calling it rest -- [ ] Iteration (`each`) over tables +- [x] Iteration (`each`) over tables - [ ] ctrl-c support - [ ] operator overflow - [ ] finish operator type-checking diff --git a/src/tests.rs b/src/tests.rs index 1d8e9c80a2..403d9d2b30 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -279,3 +279,11 @@ fn cell_path_var2() -> TestResult { fn custom_rest_var() -> TestResult { run_test("def foo [...x] { $x.0 + $x.1 }; foo 10 80", "90") } + +#[test] +fn row_iteration() -> TestResult { + run_test( + "[[name, size]; [tj, 100], [rl, 200]] | each { $it.size * 8 }", + "[800, 1600]", + ) +}