mirror of
https://github.com/nushell/nushell.git
synced 2026-10-28 17:05:03 +08:00
fix(join): error when a table has no rows with the join column (#19219)
## Description
`join` accepted a join column that its tables don't have. Depending on
the join type, it returned an empty list or rows padded with `null`:
```nushell
> [[a b]; [1 2] [3 4]] | join [[a c]; [1 3] [4 5]] d | to nuon
[]
> [[a b]; [1 2] [3 4]] | join --left [[a c]; [1 3] [4 5]] a z | to nuon
[[a, b, a_, c]; [1, 2, null, null], [3, 4, null, null]]
```
`join_rows` returned early when the table it was iterating had no row
with the key, and the lookup table built from the other side came out
empty when that side lacked the key. Neither path reported anything.
## User-facing changes (Release notes)
`join` now raises a `column_not_found` error when the input table or the
right table has rows but none of them contain the join column. Before, a
misspelled column name silently produced an empty result or rows full of
`null`. Empty tables and tables where only some rows lack the column
behave as before.
```nushell
Error: nu::shell::column_not_found
× Cannot find column 'd'
╭─[source:1:1]
1 │ [[a b]; [1 2] [3 4]] | join [[a c]; [1 3] [4 5]] d
· ──────────┬───────── ┬
· │ ╰── column 'd' is missing in one or more values
· ╰── value originates here
╰────
help: If some rows have this column, try using 'd?' for optional access, or
pre-fill using the `default` command
```
## Additional notes
closes https://github.com/nushell/nushell/issues/19215
This commit is contained in:
1 parent
fab6af9c2b
commit
af58d1426b
3 files changed
+156
-87
No files matched your search
@@ -1,11 +1,10 @@
|
||||
use itertools::Itertools;
|
||||
use std::str::FromStr;
|
||||
|
||||
use fancy_regex::Regex;
|
||||
use itertools::Itertools;
|
||||
use nu_cmd_base::input_handler::{CmdArgument, operate};
|
||||
use nu_engine::command_prelude::*;
|
||||
use nu_parser::{DURATION_UNIT_GROUPS, parse_unit_value};
|
||||
use nu_protocol::{SUPPORTED_DURATION_UNITS, Unit, ast::Expr};
|
||||
use std::str::FromStr;
|
||||
use std::sync::LazyLock;
|
||||
|
||||
const NS_PER_MS: i64 = 1_000_000;
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
use nu_engine::command_prelude::*;
|
||||
use nu_protocol::Config;
|
||||
use nu_protocol::{Config, shell_error::generic::GenericError};
|
||||
use std::{
|
||||
cmp::max,
|
||||
collections::{HashMap, HashSet},
|
||||
@@ -16,9 +16,22 @@ enum JoinType {
|
||||
Anti,
|
||||
}
|
||||
|
||||
enum IncludeInner {
|
||||
No,
|
||||
Yes,
|
||||
// The input table that `join_rows` iterates over (`this`). The other table is
|
||||
// its lookup table. Merged rows always put the left table's columns first.
|
||||
enum Side {
|
||||
Left,
|
||||
Right,
|
||||
}
|
||||
|
||||
// Which rows of `this` table `join_rows` emits.
|
||||
enum IncludeRows {
|
||||
// Rows with a match in the lookup table (inner join).
|
||||
Matched,
|
||||
// Rows without a match, padded with nulls (anti join, and the second pass
|
||||
// of an outer join).
|
||||
Unmatched,
|
||||
// Both (left and right joins, and the first pass of an outer join).
|
||||
All,
|
||||
}
|
||||
|
||||
#[derive(Debug, Default)]
|
||||
@@ -74,6 +87,10 @@ impl Command for Join {
|
||||
"Join two tables."
|
||||
}
|
||||
|
||||
fn extra_description(&self) -> &str {
|
||||
"If the input table or the right table has rows but none of them has its join column (`left-on` for the input, `right-on` for the right table), the join fails with a column-not-found error. This applies to every join type, including --left, --right, --outer and --anti. An empty table passes this check. In a table where only some rows have the join column, the other rows join as non-matching rows. To join a table whose rows all lack the column, add it first, for example with `default null <column>`."
|
||||
}
|
||||
|
||||
fn search_terms(&self) -> Vec<&str> {
|
||||
vec!["sql"]
|
||||
}
|
||||
@@ -122,10 +139,12 @@ impl Command for Join {
|
||||
(
|
||||
Value::List { vals: rows_1, .. },
|
||||
Value::List { vals: rows_2, .. },
|
||||
Value::String { val: l_on, .. },
|
||||
Value::String { val: r_on, .. },
|
||||
Value::String { val: l_key, .. },
|
||||
Value::String { val: r_key, .. },
|
||||
) => {
|
||||
let result = join(rows_1, rows_2, l_on, r_on, join_type, &rename, span);
|
||||
check_join_column(rows_1, l_key, l_on.span(), "input")?;
|
||||
check_join_column(rows_2, r_key, r_on.span(), "right")?;
|
||||
let result = join(rows_1, rows_2, l_key, r_key, join_type, &rename, span);
|
||||
Ok(PipelineData::value(result, metadata))
|
||||
}
|
||||
_ => Err(ShellError::UnsupportedInput {
|
||||
@@ -179,6 +198,15 @@ impl Command for Join {
|
||||
"x" => Value::test_int(9),
|
||||
})])),
|
||||
},
|
||||
Example {
|
||||
description: "Left join a table whose rows all lack the join column by adding the column first",
|
||||
example: "[{id: 1 x: 10}] | join --left ([{y: 20}] | default null id) id",
|
||||
result: Some(Value::test_list(vec![Value::test_record(record! {
|
||||
"id" => Value::test_int(1),
|
||||
"x" => Value::test_int(10),
|
||||
"y" => Value::test_nothing(),
|
||||
})])),
|
||||
},
|
||||
]
|
||||
}
|
||||
}
|
||||
@@ -263,7 +291,12 @@ fn join(
|
||||
// rows having that value.
|
||||
let mut result: Vec<Value> = Vec::new();
|
||||
let is_outer = matches!(join_type, JoinType::Outer);
|
||||
let (this, this_join_key, other, other_keys, join_type, include_inner) = match join_type {
|
||||
let include_rows = match join_type {
|
||||
JoinType::Inner => IncludeRows::Matched,
|
||||
JoinType::Left | JoinType::Right | JoinType::Outer => IncludeRows::All,
|
||||
JoinType::Anti => IncludeRows::Unmatched,
|
||||
};
|
||||
let (this, this_join_key, other, other_keys, this_side) = match join_type {
|
||||
JoinType::Left | JoinType::Outer => (
|
||||
left,
|
||||
left_join_key,
|
||||
@@ -271,24 +304,23 @@ fn join(
|
||||
column_names(right),
|
||||
// For Outer we do a Left pass and a Right pass; this is the Left
|
||||
// pass.
|
||||
JoinType::Left,
|
||||
IncludeInner::Yes,
|
||||
Side::Left,
|
||||
),
|
||||
JoinType::Anti => (
|
||||
left,
|
||||
left_join_key,
|
||||
lookup_table(right, right_join_key, sep, cap, &config),
|
||||
column_names(right),
|
||||
JoinType::Anti,
|
||||
IncludeInner::No,
|
||||
// An anti join adds no columns from the right table, so the
|
||||
// unmatched left rows are emitted as they are.
|
||||
Vec::new(),
|
||||
Side::Left,
|
||||
),
|
||||
JoinType::Inner | JoinType::Right => (
|
||||
right,
|
||||
right_join_key,
|
||||
lookup_table(left, left_join_key, sep, cap, &config),
|
||||
column_names(left),
|
||||
join_type,
|
||||
IncludeInner::Yes,
|
||||
Side::Right,
|
||||
),
|
||||
};
|
||||
join_rows(
|
||||
@@ -298,20 +330,19 @@ fn join(
|
||||
other,
|
||||
other_keys,
|
||||
shared_join_key,
|
||||
&join_type,
|
||||
include_inner,
|
||||
this_side,
|
||||
include_rows,
|
||||
sep,
|
||||
&config,
|
||||
rename,
|
||||
span,
|
||||
);
|
||||
if is_outer {
|
||||
let (this, this_join_key, other, other_names, join_type) = (
|
||||
let (this, this_join_key, other, other_names) = (
|
||||
right,
|
||||
right_join_key,
|
||||
lookup_table(left, left_join_key, sep, cap, &config),
|
||||
column_names(left),
|
||||
JoinType::Right,
|
||||
);
|
||||
join_rows(
|
||||
&mut result,
|
||||
@@ -320,8 +351,8 @@ fn join(
|
||||
other,
|
||||
other_names,
|
||||
shared_join_key,
|
||||
&join_type,
|
||||
IncludeInner::No,
|
||||
Side::Right,
|
||||
IncludeRows::Unmatched,
|
||||
sep,
|
||||
&config,
|
||||
rename,
|
||||
@@ -341,23 +372,18 @@ fn join_rows(
|
||||
other: HashMap<String, Vec<&Record>>,
|
||||
other_keys: Vec<&String>,
|
||||
shared_join_key: Option<&str>,
|
||||
join_type: &JoinType,
|
||||
include_inner: IncludeInner,
|
||||
this_side: Side,
|
||||
include_rows: IncludeRows,
|
||||
sep: &str,
|
||||
config: &Config,
|
||||
rename: &RightColumnRename,
|
||||
span: Span,
|
||||
) {
|
||||
if !this
|
||||
.iter()
|
||||
.any(|this_record| match this_record.as_record() {
|
||||
Ok(record) => record.contains(this_join_key),
|
||||
Err(_) => false,
|
||||
})
|
||||
{
|
||||
// `this` table does not contain the join column; do nothing
|
||||
return;
|
||||
}
|
||||
// Merge a row of `this` with a row of `other`, left input table first.
|
||||
let merge = |this_record: &Record, other_record: &Record| match this_side {
|
||||
Side::Left => merge_records(this_record, other_record, shared_join_key, rename),
|
||||
Side::Right => merge_records(other_record, this_record, shared_join_key, rename),
|
||||
};
|
||||
for this_row in this {
|
||||
if let Value::Record {
|
||||
val: this_record, ..
|
||||
@@ -366,71 +392,73 @@ fn join_rows(
|
||||
if let Some(this_valkey) = this_record.get(this_join_key)
|
||||
&& let Some(other_rows) = other.get(&this_valkey.to_expanded_string(sep, config))
|
||||
{
|
||||
if let IncludeInner::Yes = include_inner {
|
||||
if matches!(include_rows, IncludeRows::Matched | IncludeRows::All) {
|
||||
for other_record in other_rows {
|
||||
// `other` table contains rows matching `this` row on the join column
|
||||
let record = match join_type {
|
||||
JoinType::Inner | JoinType::Right => merge_records(
|
||||
other_record, // `other` (lookup) is the left input table
|
||||
this_record,
|
||||
shared_join_key,
|
||||
rename,
|
||||
),
|
||||
JoinType::Left => merge_records(
|
||||
this_record, // `this` is the left input table
|
||||
other_record,
|
||||
shared_join_key,
|
||||
rename,
|
||||
),
|
||||
JoinType::Outer | JoinType::Anti => panic!("not implemented"),
|
||||
};
|
||||
result.push(Value::record(record, span))
|
||||
result.push(Value::record(merge(this_record, other_record), span))
|
||||
}
|
||||
}
|
||||
continue;
|
||||
}
|
||||
if !matches!(join_type, JoinType::Inner) {
|
||||
if matches!(include_rows, IncludeRows::Unmatched | IncludeRows::All) {
|
||||
// Either `this` row is missing a value for the join column or
|
||||
// `other` table did not contain any rows matching
|
||||
// `this` row on the join column; emit a single joined
|
||||
// row with null values for columns not present
|
||||
let other_record = || -> Record {
|
||||
other_keys
|
||||
.iter()
|
||||
.map(|&key| {
|
||||
let val = if Some(key.as_ref()) == shared_join_key {
|
||||
this_record
|
||||
.get(key)
|
||||
.cloned()
|
||||
.unwrap_or_else(|| Value::nothing(span))
|
||||
} else {
|
||||
Value::nothing(span)
|
||||
};
|
||||
let other_record = other_keys
|
||||
.iter()
|
||||
.map(|&key| {
|
||||
let val = if Some(key.as_ref()) == shared_join_key {
|
||||
this_record
|
||||
.get(key)
|
||||
.cloned()
|
||||
.unwrap_or_else(|| Value::nothing(span))
|
||||
} else {
|
||||
Value::nothing(span)
|
||||
};
|
||||
|
||||
(key.clone(), val)
|
||||
})
|
||||
.collect()
|
||||
};
|
||||
(key.clone(), val)
|
||||
})
|
||||
.collect();
|
||||
|
||||
let value = match join_type {
|
||||
JoinType::Inner | JoinType::Right => Value::record(
|
||||
merge_records(&other_record(), this_record, shared_join_key, rename),
|
||||
span,
|
||||
),
|
||||
JoinType::Left => Value::record(
|
||||
merge_records(this_record, &other_record(), shared_join_key, rename),
|
||||
span,
|
||||
),
|
||||
JoinType::Anti => this_row.clone(),
|
||||
JoinType::Outer => panic!("not implemented"),
|
||||
};
|
||||
|
||||
result.push(value)
|
||||
result.push(Value::record(merge(this_record, &other_record), span))
|
||||
}
|
||||
};
|
||||
}
|
||||
}
|
||||
|
||||
// Fail when a non-empty table has no row with the join column, so a misspelled
|
||||
// column name is reported instead of producing an empty or all-null join.
|
||||
// Empty tables pass because they have no rows to show which columns they
|
||||
// hold. Rows that lack the column in a table where other rows have it are
|
||||
// still joined as non-matching rows. `table` names the table ("input" or
|
||||
// "right") in the error, which points at the column argument. The error keeps
|
||||
// the `column_not_found` code of `ShellError::CantFindColumn`, but not its help,
|
||||
// which suggests `column?` optional access that `join` does not support.
|
||||
fn check_join_column(
|
||||
rows: &[Value],
|
||||
column: &str,
|
||||
column_span: Span,
|
||||
table: &str,
|
||||
) -> Result<(), ShellError> {
|
||||
let has_column = |row: &Value| row.as_record().is_ok_and(|record| record.contains(column));
|
||||
if rows.is_empty() || rows.iter().any(has_column) {
|
||||
Ok(())
|
||||
} else {
|
||||
Err(ShellError::Generic(
|
||||
GenericError::new(
|
||||
format!("Cannot find column '{column}'"),
|
||||
format!("no row in the {table} table has this column"),
|
||||
column_span,
|
||||
)
|
||||
.with_code("nu::shell::column_not_found")
|
||||
.with_help(
|
||||
"Check the column name. To join a table whose rows lack the column, add it first, for example with `default null <column>`",
|
||||
),
|
||||
))
|
||||
}
|
||||
}
|
||||
|
||||
// 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]) -> Vec<&String> {
|
||||
|
||||
@@ -1,5 +1,7 @@
|
||||
use nu_protocol::{ShellError, shell_error::ErrorSite};
|
||||
use nu_test_support::prelude::*;
|
||||
use pretty_assertions::assert_matches;
|
||||
use rstest::rstest;
|
||||
|
||||
#[test]
|
||||
fn cases_where_result_is_same_between_join_types_inner() -> Result {
|
||||
@@ -70,10 +72,9 @@ fn do_cases_where_result_is_same_between_join_types(join_type: &str) -> Result {
|
||||
// .mode column
|
||||
// .headers on
|
||||
for ((left, right, on), expected) in [
|
||||
// Empty tables have no rows that could hold the join column, so any
|
||||
// column name is accepted for them.
|
||||
(("[]", "[]", "_"), "[]"),
|
||||
(("[]", "[{a: 1}]", "_"), "[]"),
|
||||
(("[{a: 1}]", "[]", "_"), "[]"),
|
||||
(("[{a: 1}]", "[{a: 1}]", "_"), "[]"),
|
||||
(("[{a: 1}]", "[{a: 1}]", "a"), "[[a]; [1]]"),
|
||||
(("[{a: 1} {a: 1}]", "[{a: 1}]", "a"), "[[a]; [1], [1]]"),
|
||||
(("[{a: 1}]", "[{a: 1} {a: 1}]", "a"), "[[a]; [1], [1]]"),
|
||||
@@ -467,6 +468,47 @@ fn test_alternative_table_syntax() -> Result {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
// A non-empty table where no row has the join column is an error, for every
|
||||
// join type and for streamed input. `right_and_keys` is the `join` arguments
|
||||
// after the flag: the right table, `left-on` and the optional `right-on`. The
|
||||
// error names `table` and points at the `missing` column argument, so the
|
||||
// checks of the two tables cannot swap their key or table without failing.
|
||||
#[rstest]
|
||||
#[case::neither_table_has_key("[[a b]; [1 2] [3 4]]", "[[a c]; [1 3] [4 5]] d", "input", "d")]
|
||||
#[case::left_table_lacks_key("[{b: 1}]", "[{a: 1}] a", "input", "a")]
|
||||
#[case::right_table_lacks_key("[{a: 1}]", "[{b: 1}] a", "right", "a")]
|
||||
#[case::left_table_lacks_left_on("[{a: 1}]", "[{z: 1}] b z", "input", "b")]
|
||||
#[case::right_table_lacks_right_on("[{a: 1}]", "[{z: 1}] a y", "right", "y")]
|
||||
#[case::left_empty_right_lacks_key("[]", "[{b: 1}] a", "right", "a")]
|
||||
#[case::right_empty_left_lacks_key("[{b: 1}]", "[] a", "input", "a")]
|
||||
fn join_errors_when_table_has_no_join_column(
|
||||
#[case] left: &str,
|
||||
#[case] right_and_keys: &str,
|
||||
#[case] table: &str,
|
||||
#[case] missing: &str,
|
||||
#[values("--inner", "--left", "--right", "--outer", "--anti")] join_type: &str,
|
||||
#[values("", "each { |i| $i } | ")] to_list_stream: &str,
|
||||
) -> Result {
|
||||
let code = format!("{left} | {to_list_stream}join {join_type} {right_and_keys}");
|
||||
let mut tester = test();
|
||||
let err = match tester.run(code).expect_shell_error()? {
|
||||
ShellError::Generic(err) => err,
|
||||
err => panic!("expected a generic error, got {err:?}"),
|
||||
};
|
||||
assert_eq!(err.code, "nu::shell::column_not_found");
|
||||
assert_eq!(err.error, format!("Cannot find column '{missing}'"));
|
||||
assert_eq!(
|
||||
err.msg,
|
||||
format!("no row in the {table} table has this column")
|
||||
);
|
||||
let ErrorSite::Span(span) = err.site else {
|
||||
panic!("expected the error to point at the column argument");
|
||||
};
|
||||
let span_text = String::from_utf8_lossy(tester.engine_state.get_span_contents(span));
|
||||
assert_eq!(span_text, missing);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_incompatible_anti_join_args() -> Result {
|
||||
let err = test()
|
||||
|
||||
Reference in new issue
Block a user