holm/nushell
1
1
Fork 0
forked from extern/nushell
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Optional members in cell paths: Attempt 2 (#8379)

Browse Source 
This is a follow up from https://github.com/nushell/nushell/pull/7540.
Please provide feedback if you have the time!

## Summary

This PR lets you use `?` to indicate that a member in a cell path is
optional and Nushell should return `null` if that member cannot be
accessed.

Unlike the previous PR, `?` is now a _postfix_ modifier for cell path
members. A cell path of `.foo?.bar` means that `foo` is optional and
`bar` is not.

`?` does _not_ suppress all errors; it is intended to help in situations
where data has "holes", i.e. the data types are correct but something is
missing. Type mismatches (like trying to do a string path access on a
date) will still fail.

### Record Examples

```bash

{ foo: 123 }.foo # returns 123

{ foo: 123 }.bar # errors
{ foo: 123 }.bar? # returns null

{ foo: 123 } | get bar # errors
{ foo: 123 } | get bar? # returns null

{ foo: 123 }.bar.baz # errors
{ foo: 123 }.bar?.baz # errors because `baz` is not present on the result from `bar?`
{ foo: 123 }.bar.baz? # errors
{ foo: 123 }.bar?.baz? # returns null
```

### List Examples
```
〉[{foo: 1} {foo: 2} {}].foo
Error: nu:🐚:column_not_found

  × Cannot find column
   ╭─[entry #30:1:1]
 1 │ [{foo: 1} {foo: 2} {}].foo
   ·                    ─┬  ─┬─
   ·                     │   ╰── cannot find column 'foo'
   ·                     ╰── value originates here
   ╰────
〉[{foo: 1} {foo: 2} {}].foo?
╭───┬───╮
│ 0 │ 1 │
│ 1 │ 2 │
│ 2 │   │
╰───┴───╯
〉[{foo: 1} {foo: 2} {}].foo?.2 | describe
nothing

〉[a b c].4? | describe
nothing

〉[{foo: 1} {foo: 2} {}] | where foo? == 1
╭───┬─────╮
│ # │ foo │
├───┼─────┤
│ 0 │   1 │
╰───┴─────╯
```

# Breaking changes

1. Column names with `?` in them now need to be quoted.
2. The `-i`/`--ignore-errors` flag has been removed from `get` and
`select`
1. After this PR, most `get` error handling can be done with `?` and/or
`try`/`catch`.
4. Cell path accesses like this no longer work without a `?`:
```bash
〉[{a:1 b:2} {a:3}].b.0
2
```
We had some clever code that was able to recognize that since we only
want row `0`, it's OK if other rows are missing column `b`. I removed
that because it's tricky to maintain, and now that query needs to be
written like:


```bash
〉[{a:1 b:2} {a:3}].b?.0
2
```

I think the regression is acceptable for now. I plan to do more work in
the future to enable streaming of cell path accesses, and when that
happens I'll be able to make `.b.0` work again.
This commit is contained in:
Reilly Wood
2023-03-15 20:50:58 -07:00
committed by GitHub
parent d3be5ec750
commit 21b84a6d65
32 changed files with 510 additions and 277 deletions
Download Patch File Download Diff File Expand all files Collapse all files

51
crates/nu-protocol/src/ast/cell_path.rs
View File

@ -5,15 +5,45 @@ use std::fmt::Write;
#[derive(Debug, Clone, PartialOrd, Serialize, Deserialize)]
pub enum PathMember {
String { val: String, span: Span },
Int { val: usize, span: Span },
String {
val: String,
span: Span,
optional: bool,
},
Int {
val: usize,
span: Span,
optional: bool,
},
}
impl PartialEq for PathMember {
fn eq(&self, other: &Self) -> bool {
match (self, other) {
(Self::String { val: l_val, .. }, Self::String { val: r_val, .. }) => l_val == r_val,
(Self::Int { val: l_val, .. }, Self::Int { val: r_val, .. }) => l_val == r_val,
(
Self::String {
val: l_val,
optional: l_opt,
..
},
Self::String {
val: r_val,
optional: r_opt,
..
},
) => l_val == r_val && l_opt == r_opt,
(
Self::Int {
val: l_val,
optional: l_opt,
..
},
Self::Int {
val: r_val,
optional: r_opt,
..
},
) => l_val == r_val && l_opt == r_opt,
_ => false,
}
}
@ -42,6 +72,19 @@ impl CellPath {
output
}
pub fn make_optional(&mut self) {
for member in &mut self.members {
match member {
PathMember::String {
ref mut optional, ..
} => *optional = true,
PathMember::Int {
ref mut optional, ..
} => *optional = true,
}
}
}
}
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]

5
crates/nu-protocol/src/pipeline_data.rs
View File

@ -322,7 +322,6 @@ impl PipelineData {
cell_path: &[PathMember],
head: Span,
insensitive: bool,
ignore_errors: bool,
) -> Result<Value, ShellError> {
match self {
// FIXME: there are probably better ways of doing this
@ -330,8 +329,8 @@ impl PipelineData {
vals: stream.collect(),
span: head,
}
.follow_cell_path(cell_path, insensitive, ignore_errors),
PipelineData::Value(v, ..) => v.follow_cell_path(cell_path, insensitive, ignore_errors),
.follow_cell_path(cell_path, insensitive),
PipelineData::Value(v, ..) => v.follow_cell_path(cell_path, insensitive),
_ => Err(ShellError::IOError("can't follow stream paths".into())),
}
}

2
crates/nu-protocol/src/value/from_value.rs
View File

@ -302,6 +302,7 @@ impl FromValue for CellPath {
members: vec![PathMember::String {
val: val.clone(),
span,
optional: false,
}],
}),
Value::Int { val, span } => {
@ -312,6 +313,7 @@ impl FromValue for CellPath {
members: vec![PathMember::Int {
val: *val as usize,
span: *span,
optional: false,
}],
})
}

138
crates/nu-protocol/src/value/mod.rs
View File

@ -681,9 +681,8 @@ impl Value {
self,
cell_path: &[PathMember],
insensitive: bool,
nullify_errors: bool,
) -> Result<Value, ShellError> {
self.follow_cell_path_helper(cell_path, insensitive, nullify_errors, true)
self.follow_cell_path_helper(cell_path, insensitive, true)
}
pub fn follow_cell_path_not_from_user_input(
@ -691,24 +690,20 @@ impl Value {
cell_path: &[PathMember],
insensitive: bool,
) -> Result<Value, ShellError> {
self.follow_cell_path_helper(cell_path, insensitive, false, false)
self.follow_cell_path_helper(cell_path, insensitive, false)
}
fn follow_cell_path_helper(
self,
cell_path: &[PathMember],
insensitive: bool,
nullify_errors: bool, // Turn all errors into Value::Nothing
from_user_input: bool,
) -> Result<Value, ShellError> {
let mut current = self;
// TODO remove this
macro_rules! err_or_null {
($e:expr, $span:expr) => {
return if nullify_errors {
Ok(Value::nothing($span))
} else {
Err($e)
}
return Err($e)
};
}
for member in cell_path {
@ -718,12 +713,15 @@ impl Value {
PathMember::Int {
val: count,
span: origin_span,
optional,
} => {
// Treat a numeric path member as `select <val>`
match &mut current {
Value::List { vals: val, .. } => {
if let Some(item) = val.get(*count) {
current = item.clone();
} else if *optional {
current = Value::nothing(*origin_span);
} else if val.is_empty() {
err_or_null!(
ShellError::AccessEmptyContent { span: *origin_span },
@ -742,6 +740,8 @@ impl Value {
Value::Binary { val, .. } => {
if let Some(item) = val.get(*count) {
current = Value::int(*item as i64, *origin_span);
} else if *optional {
current = Value::nothing(*origin_span);
} else if val.is_empty() {
err_or_null!(
ShellError::AccessEmptyContent { span: *origin_span },
@ -760,6 +760,8 @@ impl Value {
Value::Range { val, .. } => {
if let Some(item) = val.clone().into_range_iter(None)?.nth(*count) {
current = item.clone();
} else if *optional {
current = Value::nothing(*origin_span);
} else {
err_or_null!(
ShellError::AccessBeyondEndOfStream { span: *origin_span },
@ -768,7 +770,19 @@ impl Value {
}
}
Value::CustomValue { val, .. } => {
current = val.follow_path_int(*count, *origin_span)?;
current = match val.follow_path_int(*count, *origin_span) {
Ok(val) => val,
Err(err) => {
if *optional {
Value::nothing(*origin_span)
} else {
return Err(err);
}
}
};
}
Value::Nothing { .. } if *optional => {
current = Value::nothing(*origin_span);
}
// Records (and tables) are the only built-in which support column names,
// so only use this message for them.
@ -792,6 +806,7 @@ impl Value {
PathMember::String {
val: column_name,
span: origin_span,
optional,
} => match &mut current {
Value::Record { cols, vals, span } => {
let cols = cols.clone();
@ -806,6 +821,8 @@ impl Value {
}
}) {
current = found.1.clone();
} else if *optional {
current = Value::nothing(*origin_span);
} else {
if from_user_input {
if let Some(suggestion) = did_you_mean(&cols, column_name) {
@ -830,6 +847,8 @@ impl Value {
if columns.contains(&column_name.as_str()) {
current = val.get_column_value(column_name)?;
} else if *optional {
current = Value::nothing(*origin_span);
} else {
if from_user_input {
if let Some(suggestion) = did_you_mean(&columns, column_name) {
@ -852,10 +871,9 @@ impl Value {
// String access of Lists always means Table access.
// Create a List which contains each matching value for contained
// records in the source list.
// If nullify_errors is true, table holes are converted to null.
Value::List { vals, span } => {
// TODO: this should stream instead of collecting
let mut output = vec![];
let mut found_at_least_1_value = false;
for val in vals {
// only look in records; this avoids unintentionally recursing into deeply nested tables
if matches!(val, Value::Record { .. }) {
@ -863,69 +881,40 @@ impl Value {
&[PathMember::String {
val: column_name.clone(),
span: *origin_span,
optional: *optional,
}],
insensitive,
nullify_errors,
) {
found_at_least_1_value = true;
output.push(result);
} else {
// Consider [{a:1 b:2} {a:3}]:
// [{a:1 b:2} {a:3}].b should error.
// [{a:1 b:2} {a:3}].b.1 should error.
// [{a:1 b:2} {a:3}].b.0 should NOT error because the path can find a proper value (2)
// but if this returns an error immediately, it will.
//
// Solution: push a Value::Error into this result list instead of returning it.
// This also means that `[{a:1 b:2} {a:2}].b | reject 1` also doesn't error.
// Anything that needs to use every value inside the list should propagate
// the error outward, though.
output.push(if nullify_errors {
Value::nothing(*origin_span)
} else {
Value::Error {
error: Box::new(ShellError::CantFindColumn {
col_name: column_name.to_string(),
span: *origin_span,
src_span: val.span().unwrap_or(*span),
}),
}
return Err(ShellError::CantFindColumn {
col_name: column_name.to_string(),
span: *origin_span,
src_span: val.span().unwrap_or(*span),
});
}
} else if *optional && matches!(val, Value::Nothing { .. }) {
output.push(Value::nothing(*origin_span));
} else {
// See comment above.
output.push(if nullify_errors {
Value::nothing(*origin_span)
} else {
Value::Error {
error: Box::new(ShellError::CantFindColumn {
col_name: column_name.to_string(),
span: *origin_span,
src_span: val.span().unwrap_or(*span),
}),
}
return Err(ShellError::CantFindColumn {
col_name: column_name.to_string(),
span: *origin_span,
src_span: val.span().unwrap_or(*span),
});
}
}
if found_at_least_1_value {
current = Value::List {
vals: output,
span: *span,
};
} else {
err_or_null!(
ShellError::CantFindColumn {
col_name: column_name.to_string(),
span: *origin_span,
src_span: *span
},
*origin_span
);
}
current = Value::List {
vals: output,
span: *span,
};
}
Value::CustomValue { val, .. } => {
current = val.follow_path_string(column_name.clone(), *origin_span)?;
}
Value::Nothing { .. } if *optional => {
current = Value::nothing(*origin_span);
}
Value::Error { error } => err_or_null!(*error.to_owned(), *origin_span),
x => {
err_or_null!(
@ -956,7 +945,7 @@ impl Value {
) -> Result<(), ShellError> {
let orig = self.clone();
let new_val = callback(&orig.follow_cell_path(cell_path, false, false)?);
let new_val = callback(&orig.follow_cell_path(cell_path, false)?);
match new_val {
Value::Error { error } => Err(*error),
@ -974,6 +963,7 @@ impl Value {
PathMember::String {
val: col_name,
span,
..
} => match self {
Value::List { vals, .. } => {
for val in vals.iter_mut() {
@ -1055,7 +1045,9 @@ impl Value {
})
}
},
PathMember::Int { val: row_num, span } => match self {
PathMember::Int {
val: row_num, span, ..
} => match self {
Value::List { vals, .. } => {
if let Some(v) = vals.get_mut(*row_num) {
v.upsert_data_at_cell_path(&cell_path[1..], new_val)?
@ -1094,7 +1086,7 @@ impl Value {
) -> Result<(), ShellError> {
let orig = self.clone();
let new_val = callback(&orig.follow_cell_path(cell_path, false, false)?);
let new_val = callback(&orig.follow_cell_path(cell_path, false)?);
match new_val {
Value::Error { error } => Err(*error),
@ -1113,6 +1105,7 @@ impl Value {
PathMember::String {
val: col_name,
span,
..
} => match self {
Value::List { vals, .. } => {
for val in vals.iter_mut() {
@ -1183,7 +1176,9 @@ impl Value {
})
}
},
PathMember::Int { val: row_num, span } => match self {
PathMember::Int {
val: row_num, span, ..
} => match self {
Value::List { vals, .. } => {
if let Some(v) = vals.get_mut(*row_num) {
v.update_data_at_cell_path(&cell_path[1..], new_val)?
@ -1221,6 +1216,7 @@ impl Value {
PathMember::String {
val: col_name,
span,
..
} => match self {
Value::List { vals, .. } => {
for val in vals.iter_mut() {
@ -1289,7 +1285,9 @@ impl Value {
src_span: v.span()?,
}),
},
PathMember::Int { val: row_num, span } => match self {
PathMember::Int {
val: row_num, span, ..
} => match self {
Value::List { vals, .. } => {
if vals.get_mut(*row_num).is_some() {
vals.remove(*row_num);
@ -1316,6 +1314,7 @@ impl Value {
PathMember::String {
val: col_name,
span,
..
} => match self {
Value::List { vals, .. } => {
for val in vals.iter_mut() {
@ -1380,7 +1379,9 @@ impl Value {
src_span: v.span()?,
}),
},
PathMember::Int { val: row_num, span } => match self {
PathMember::Int {
val: row_num, span, ..
} => match self {
Value::List { vals, .. } => {
if let Some(v) = vals.get_mut(*row_num) {
v.remove_data_at_cell_path(&cell_path[1..])
@ -1414,6 +1415,7 @@ impl Value {
PathMember::String {
val: col_name,
span,
..
} => match self {
Value::List { vals, .. } => {
for val in vals.iter_mut() {
@ -1492,7 +1494,9 @@ impl Value {
))
}
},
PathMember::Int { val: row_num, span } => match self {
PathMember::Int {
val: row_num, span, ..
} => match self {
Value::List { vals, .. } => {
if let Some(v) = vals.get_mut(*row_num) {
v.insert_data_at_cell_path(&cell_path[1..], new_val, head_span)?