From d409171ba830111658c8ee3eb2321d8b676ce9c6 Mon Sep 17 00:00:00 2001 From: Reilly Wood <26268125+rgwood@users.noreply.github.com> Date: Sat, 25 Mar 2023 16:10:09 -0700 Subject: [PATCH] Change comparison operators to allow nulls (#8617) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prior to this PR, the less/greater than operators (`<`, `>`, `<=`, `>=`) would throw an error if either side was null. After this PR, these operators return null if either side (or both) is null. ### Examples ```bash 1 < 3 # true 1 < null # null null < 3 # null null < null # null ``` ### Motivation JT [asked the C# folks](https://discord.com/channels/601130461678272522/615329862395101194/1086137515053957140) and this is apparently the approach they would choose for comparison operators if they could start from scratch. This PR makes `where` more convenient to use on jagged/missing data. For example, we can now filter on columns that may not be present in every row: ``` > [{foo: 123} {}] | where foo? > 10 ╭───┬─────╮ │ # │ foo │ ├───┼─────┤ │ 0 │ 123 │ ╰───┴─────╯ ``` --- crates/nu-command/tests/commands/where_.rs | 7 +++ crates/nu-parser/src/type_check.rs | 12 +++++ crates/nu-protocol/src/value/mod.rs | 16 ++++++ src/tests/test_math.rs | 59 ++++++++++++++++++++++ 4 files changed, 94 insertions(+) diff --git a/crates/nu-command/tests/commands/where_.rs b/crates/nu-command/tests/commands/where_.rs index cd57f0b5ac..ada5ad147f 100644 --- a/crates/nu-command/tests/commands/where_.rs +++ b/crates/nu-command/tests/commands/where_.rs @@ -194,3 +194,10 @@ fn fail_on_non_iterator() { assert!(actual.err.contains("only_supports_this_input_type")); } + +// Test that filtering on columns that might be missing/null works +#[test] +fn where_gt_null() { + let actual = nu!("[{foo: 123} {}] | where foo? > 10 | to nuon"); + assert_eq!(actual.out, "[[foo]; [123]]"); +} diff --git a/crates/nu-parser/src/type_check.rs b/crates/nu-parser/src/type_check.rs index fb8fada23f..5b51f8d636 100644 --- a/crates/nu-parser/src/type_check.rs +++ b/crates/nu-parser/src/type_check.rs @@ -295,6 +295,9 @@ pub fn math_result_type( (Type::Custom(a), Type::Custom(b)) if a == b => (Type::Custom(a.to_string()), None), (Type::Custom(a), _) => (Type::Custom(a.to_string()), None), + (Type::Nothing, _) => (Type::Nothing, None), + (_, Type::Nothing) => (Type::Nothing, None), + (Type::Any, _) => (Type::Bool, None), (_, Type::Any) => (Type::Bool, None), _ => { @@ -322,6 +325,9 @@ pub fn math_result_type( (Type::Custom(a), Type::Custom(b)) if a == b => (Type::Custom(a.to_string()), None), (Type::Custom(a), _) => (Type::Custom(a.to_string()), None), + (Type::Nothing, _) => (Type::Nothing, None), + (_, Type::Nothing) => (Type::Nothing, None), + (Type::Any, _) => (Type::Bool, None), (_, Type::Any) => (Type::Bool, None), _ => { @@ -351,6 +357,9 @@ pub fn math_result_type( (Type::Any, _) => (Type::Bool, None), (_, Type::Any) => (Type::Bool, None), + + (Type::Nothing, _) => (Type::Nothing, None), + (_, Type::Nothing) => (Type::Nothing, None), _ => { *op = Expression::garbage(op.span); ( @@ -378,6 +387,9 @@ pub fn math_result_type( (Type::Any, _) => (Type::Bool, None), (_, Type::Any) => (Type::Bool, None), + + (Type::Nothing, _) => (Type::Nothing, None), + (_, Type::Nothing) => (Type::Nothing, None), _ => { *op = Expression::garbage(op.span); ( diff --git a/crates/nu-protocol/src/value/mod.rs b/crates/nu-protocol/src/value/mod.rs index 76ff6073ee..87501f3f94 100644 --- a/crates/nu-protocol/src/value/mod.rs +++ b/crates/nu-protocol/src/value/mod.rs @@ -2658,6 +2658,10 @@ impl Value { return lhs.operation(*span, Operator::Comparison(Comparison::LessThan), op, rhs); } + if matches!(self, Value::Nothing { .. }) || matches!(rhs, Value::Nothing { .. }) { + return Ok(Value::nothing(span)); + } + if !type_compatible(self.get_type(), rhs.get_type()) && (self.get_type() != Type::Any) && (rhs.get_type() != Type::Any) @@ -2697,6 +2701,10 @@ impl Value { ); } + if matches!(self, Value::Nothing { .. }) || matches!(rhs, Value::Nothing { .. }) { + return Ok(Value::nothing(span)); + } + if !type_compatible(self.get_type(), rhs.get_type()) && (self.get_type() != Type::Any) && (rhs.get_type() != Type::Any) @@ -2734,6 +2742,10 @@ impl Value { ); } + if matches!(self, Value::Nothing { .. }) || matches!(rhs, Value::Nothing { .. }) { + return Ok(Value::nothing(span)); + } + if !type_compatible(self.get_type(), rhs.get_type()) && (self.get_type() != Type::Any) && (rhs.get_type() != Type::Any) @@ -2771,6 +2783,10 @@ impl Value { ); } + if matches!(self, Value::Nothing { .. }) || matches!(rhs, Value::Nothing { .. }) { + return Ok(Value::nothing(span)); + } + if !type_compatible(self.get_type(), rhs.get_type()) && (self.get_type() != Type::Any) && (rhs.get_type() != Type::Any) diff --git a/src/tests/test_math.rs b/src/tests/test_math.rs index 094be2e7c6..ee4d139e39 100644 --- a/src/tests/test_math.rs +++ b/src/tests/test_math.rs @@ -119,3 +119,62 @@ fn precedence_of_or_groups() -> TestResult { fn test_filesize_op() -> TestResult { run_test("-5kb + 4.5kb", "-500 B") } + +#[test] +fn lt() -> TestResult { + run_test("1 < 3", "true").unwrap(); + run_test("3 < 3", "false").unwrap(); + run_test("3 < 1", "false") +} + +// Comparison operators return null if 1 side or both side is null. +// The motivation for this behaviour: JT asked the C# devs and they said this is +// the behaviour they would choose if they were starting from scratch. +#[test] +fn lt_null() -> TestResult { + run_test("3 < null | to nuon", "null").unwrap(); + run_test("null < 3 | to nuon", "null").unwrap(); + run_test("null < null | to nuon", "null") +} + +#[test] +fn lte() -> TestResult { + run_test("1 <= 3", "true").unwrap(); + run_test("3 <= 3", "true").unwrap(); + run_test("3 <= 1", "false") +} + +#[test] +fn lte_null() -> TestResult { + run_test("3 <= null | to nuon", "null").unwrap(); + run_test("null <= 3 | to nuon", "null").unwrap(); + run_test("null <= null | to nuon", "null") +} + +#[test] +fn gt() -> TestResult { + run_test("1 > 3", "false").unwrap(); + run_test("3 > 3", "false").unwrap(); + run_test("3 > 1", "true") +} + +#[test] +fn gt_null() -> TestResult { + run_test("3 > null | to nuon", "null").unwrap(); + run_test("null > 3 | to nuon", "null").unwrap(); + run_test("null > null | to nuon", "null") +} + +#[test] +fn gte() -> TestResult { + run_test("1 >= 3", "false").unwrap(); + run_test("3 >= 3", "true").unwrap(); + run_test("3 >= 1", "true") +} + +#[test] +fn gte_null() -> TestResult { + run_test("3 >= null | to nuon", "null").unwrap(); + run_test("null >= 3 | to nuon", "null").unwrap(); + run_test("null >= null | to nuon", "null") +}