Repository navigation
fix(core): keep unmatched rows' struct values when merge widens a struct - #4889
Conversation
…idens a struct When merge_schema=True changed a column's type (e.g. a struct gained a field), the per-column CASE defaulted to the source column, so rows in rewritten files that no clause set (copied unmatched rows, or matched rows whose update didn't set the column) got the source value, which is null for unmatched rows. Default to the target column instead and cast every branch to the merged type; DataFusion's struct cast fills the new nested fields with null, including inside lists. Closes delta-io#4887 Signed-off-by: ahlag <[email protected]>
3a5cf3c to
cfcd1b9
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4889 +/- ##
==========================================
+ Coverage 81.92% 81.94% +0.01%
==========================================
Files 200 200
Lines 75656 75740 +84
Branches 75656 75740 +84
==========================================
+ Hits 61982 62063 +81
- Misses 10040 10044 +4
+ Partials 3634 3633 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@ahlag found one more edge case (AI assisted find with Claude Code). With Same happens for The cause is in DataFusion: Since the merged type is a superset of the target and source types, a strict cast shouldn't lose anything here. So I think we can just use let op = if field_changed {
- merge_cast(op, cast_type.clone(), safe_cast)
+ cast(op, cast_type.clone())
} else {
op
};
@@
merge_cast(
Expr::Column(Column::new(qualifier.clone(), delta_field.name())),
cast_type.clone(),
- safe_cast,
+ safe_cast && !field_changed,
)
});Tested the repro and this change locally, it fixes all four cases and the merge tests still pass. Could you add this together with a Could you address this in a follow-up? |
|
Yes, sure. Taking a look now. |
|
Thanks for catching this! Follow-up is in #4917. It uses |
# Description Follow-up to #4889, from @ion-elgreco's [comment](#4889 (comment)). With `safe_cast` enabled (`error_on_type_mismatch=False` in Python), a merge with `merge_schema=True` that widens a struct fails during planning: ``` Unsupported TRY_CAST from Struct("x": Int64) to Struct("x": Int64, "y": Utf8) ``` DataFusion's `cast` checks struct casts by field name, but `try_cast` still plans with arrow's `can_cast_types`, which needs the same fields (`datafusion-physical-expr` 55.2 `try_cast.rs`, also on DataFusion `main`). The merged type is a superset of the target and source types, so this PR uses a strict `cast` for changed fields, in the CASE branches and in the CDF target preimage. This is the change Ion proposed. Tests: the struct-widening merge tests now run with and without `with_safe_cast(true)` (`rstest` `#[values(false, true)]`): - `test_merge_with_schema_merge_struct_added_field_keeps_copied_rows` - `test_merge_with_schema_merge_struct_added_field_keeps_unset_column` - `test_merge_with_schema_merge_and_pre_existing_struct_added_column` (the source struct only has the new field) - new `test_merge_with_schema_merge_struct_added_field_cdf`: CDF enabled, checks the preimage and postimage of the updated row. The four `safe_cast = true` cases fail on `main` with the error above. Testing (macOS arm64): - `cargo test -p deltalake-core --features datafusion --lib`: 1362 passed - `cargo fmt --check`: clean. `cargo clippy -p deltalake-core --features datafusion --tests --no-deps` reports nothing in `merge/mod.rs`. - Python bindings built from this branch: `tests/test_merge.py` 104 passed. With `error_on_type_mismatch=False`, a struct, a `list<struct>`, a source struct with only the new field, and a struct with CDF enabled all merge, and the unmatched row keeps `{x: 2, y: None}`. On `deltalake` 1.6.6, the first three drop the unmatched row's value to null and the CDF case fails with the same error. AI use: I used Claude Code to apply the change, write the tests, and run the test suites. I reviewed the diff and checked the DataFusion planning check and the test results myself. # Related Issue(s) - follow-up to #4889 / #4887 - upstream: apache/datafusion#26528 (`try_cast` rejects struct casts that `cast` accepts) # Documentation - DataFusion `try_cast` planning: https://github.com/apache/datafusion/blob/main/datafusion/physical-expr/src/expressions/try_cast.rs --------- Signed-off-by: ahlag <[email protected]>
…uct (delta-io#4889) When a merge with `merge_schema=True` widens a column (e.g. a struct gains a field), unmatched target rows in the files it rewrites are written with that column as `null`. The original values are lost in the new Parquet files. Cause: in the per-column CASE in `merge/mod.rs`, a field whose type changed used the source column as the default for rows that no clause sets. Unmatched target rows have no source row, so the copied value was null. The same column was also used as the lookup key for the clause expressions, but those are keyed by the target column, so the lookup never matched. As a result, a matched row whose update didn't set the column also got the source value. The old comment says this was done because casting to the new nested type breaks. With DataFusion 55, `cast`/`try_cast` go through `nested_struct::cast_column`, which matches struct fields by name and fills missing nullable fields with null, including inside lists. So this PR: - uses the target column as the default and lookup key for a changed field, like for unchanged fields; - casts every CASE branch for that field to the merged type from the new schema. Casting to the source's type fails when the source struct has no field in common with the target (`test_merge_with_schema_merge_and_pre_existing_struct_added_column`), and mixed struct types in one CASE can't be coerced. Tests: - `test_merge_with_schema_merge_struct_added_field_keeps_copied_rows`: unmatched rows A/C/D keep `nested`. Fails on `main` (they read as null). - `test_merge_with_schema_merge_struct_added_field_keeps_unset_column`: the update sets only `value` and an insert clause widens `nested`. Matched row B keeps its own `nested`. Fails on `main` (B gets the source struct and A/C/D are null). Testing (macOS arm64): - `cargo test -p deltalake-core --features datafusion --lib`: 1335 passed - `cargo fmt --check`: clean. `cargo clippy -p deltalake-core --features datafusion --tests --no-deps -- -D warnings` reports only lints in other files with my local toolchain, none in `merge/mod.rs`. - Python bindings built from this branch: `tests/test_merge.py` 104 passed. The issue's repro plus two variants (`when_matched_update({"s": "s.s"})` and `list<struct>`) all lose the row on `deltalake` 1.6.6 and keep `{x: 2, y: None}` with this change, both when read and in the Parquet file. AI use: I used Claude Code to trace the cause, draft the change and tests, and run the test suites. I reviewed the diff and checked the DataFusion cast behavior and the test results myself. - closes delta-io#4887 - DataFusion nested struct casting: https://docs.rs/datafusion-common/55.2.0/datafusion_common/nested_struct/index.html Signed-off-by: ahlag <[email protected]> (cherry picked from commit bfd03f3) Co-Authored-By: Devstral 2 <[email protected]> Signed-off-by: R. Tyler Croy <[email protected]> Co-Authored-By: R. Tyler Croy <[email protected]> Signed-off-by: R. Tyler Croy <[email protected]>
Follow-up to delta-io#4889, from @ion-elgreco's [comment](delta-io#4889 (comment)). With `safe_cast` enabled (`error_on_type_mismatch=False` in Python), a merge with `merge_schema=True` that widens a struct fails during planning: ``` Unsupported TRY_CAST from Struct("x": Int64) to Struct("x": Int64, "y": Utf8) ``` DataFusion's `cast` checks struct casts by field name, but `try_cast` still plans with arrow's `can_cast_types`, which needs the same fields (`datafusion-physical-expr` 55.2 `try_cast.rs`, also on DataFusion `main`). The merged type is a superset of the target and source types, so this PR uses a strict `cast` for changed fields, in the CASE branches and in the CDF target preimage. This is the change Ion proposed. Tests: the struct-widening merge tests now run with and without `with_safe_cast(true)` (`rstest` `#[values(false, true)]`): - `test_merge_with_schema_merge_struct_added_field_keeps_copied_rows` - `test_merge_with_schema_merge_struct_added_field_keeps_unset_column` - `test_merge_with_schema_merge_and_pre_existing_struct_added_column` (the source struct only has the new field) - new `test_merge_with_schema_merge_struct_added_field_cdf`: CDF enabled, checks the preimage and postimage of the updated row. The four `safe_cast = true` cases fail on `main` with the error above. Testing (macOS arm64): - `cargo test -p deltalake-core --features datafusion --lib`: 1362 passed - `cargo fmt --check`: clean. `cargo clippy -p deltalake-core --features datafusion --tests --no-deps` reports nothing in `merge/mod.rs`. - Python bindings built from this branch: `tests/test_merge.py` 104 passed. With `error_on_type_mismatch=False`, a struct, a `list<struct>`, a source struct with only the new field, and a struct with CDF enabled all merge, and the unmatched row keeps `{x: 2, y: None}`. On `deltalake` 1.6.6, the first three drop the unmatched row's value to null and the CDF case fails with the same error. AI use: I used Claude Code to apply the change, write the tests, and run the test suites. I reviewed the diff and checked the DataFusion planning check and the test results myself. - follow-up to delta-io#4889 / delta-io#4887 - upstream: apache/datafusion#26528 (`try_cast` rejects struct casts that `cast` accepts) - DataFusion `try_cast` planning: https://github.com/apache/datafusion/blob/main/datafusion/physical-expr/src/expressions/try_cast.rs --------- Signed-off-by: ahlag <[email protected]> (cherry picked from commit 0a14212) Co-Authored-By: Devstral 2 <[email protected]> Signed-off-by: R. Tyler Croy <[email protected]> Co-Authored-By: R. Tyler Croy <[email protected]> Signed-off-by: R. Tyler Croy <[email protected]>
…uct (delta-io#4889) When a merge with `merge_schema=True` widens a column (e.g. a struct gains a field), unmatched target rows in the files it rewrites are written with that column as `null`. The original values are lost in the new Parquet files. Cause: in the per-column CASE in `merge/mod.rs`, a field whose type changed used the source column as the default for rows that no clause sets. Unmatched target rows have no source row, so the copied value was null. The same column was also used as the lookup key for the clause expressions, but those are keyed by the target column, so the lookup never matched. As a result, a matched row whose update didn't set the column also got the source value. The old comment says this was done because casting to the new nested type breaks. With DataFusion 55, `cast`/`try_cast` go through `nested_struct::cast_column`, which matches struct fields by name and fills missing nullable fields with null, including inside lists. So this PR: - uses the target column as the default and lookup key for a changed field, like for unchanged fields; - casts every CASE branch for that field to the merged type from the new schema. Casting to the source's type fails when the source struct has no field in common with the target (`test_merge_with_schema_merge_and_pre_existing_struct_added_column`), and mixed struct types in one CASE can't be coerced. Tests: - `test_merge_with_schema_merge_struct_added_field_keeps_copied_rows`: unmatched rows A/C/D keep `nested`. Fails on `main` (they read as null). - `test_merge_with_schema_merge_struct_added_field_keeps_unset_column`: the update sets only `value` and an insert clause widens `nested`. Matched row B keeps its own `nested`. Fails on `main` (B gets the source struct and A/C/D are null). Testing (macOS arm64): - `cargo test -p deltalake-core --features datafusion --lib`: 1335 passed - `cargo fmt --check`: clean. `cargo clippy -p deltalake-core --features datafusion --tests --no-deps -- -D warnings` reports only lints in other files with my local toolchain, none in `merge/mod.rs`. - Python bindings built from this branch: `tests/test_merge.py` 104 passed. The issue's repro plus two variants (`when_matched_update({"s": "s.s"})` and `list<struct>`) all lose the row on `deltalake` 1.6.6 and keep `{x: 2, y: None}` with this change, both when read and in the Parquet file. AI use: I used Claude Code to trace the cause, draft the change and tests, and run the test suites. I reviewed the diff and checked the DataFusion cast behavior and the test results myself. - closes delta-io#4887 - DataFusion nested struct casting: https://docs.rs/datafusion-common/55.2.0/datafusion_common/nested_struct/index.html Signed-off-by: ahlag <[email protected]> (cherry picked from commit bfd03f3) Signed-off-by: R. Tyler Croy <[email protected]> Co-Authored-By: R. Tyler Croy <[email protected]> Co-Authored-By: Claude Haiku 5.5 <[email protected]>
Follow-up to delta-io#4889, from @ion-elgreco's [comment](delta-io#4889 (comment)). With `safe_cast` enabled (`error_on_type_mismatch=False` in Python), a merge with `merge_schema=True` that widens a struct fails during planning: ``` Unsupported TRY_CAST from Struct("x": Int64) to Struct("x": Int64, "y": Utf8) ``` DataFusion's `cast` checks struct casts by field name, but `try_cast` still plans with arrow's `can_cast_types`, which needs the same fields (`datafusion-physical-expr` 55.2 `try_cast.rs`, also on DataFusion `main`). The merged type is a superset of the target and source types, so this PR uses a strict `cast` for changed fields, in the CASE branches and in the CDF target preimage. This is the change Ion proposed. Tests: the struct-widening merge tests now run with and without `with_safe_cast(true)` (`rstest` `#[values(false, true)]`): - `test_merge_with_schema_merge_struct_added_field_keeps_copied_rows` - `test_merge_with_schema_merge_struct_added_field_keeps_unset_column` - `test_merge_with_schema_merge_and_pre_existing_struct_added_column` (the source struct only has the new field) - new `test_merge_with_schema_merge_struct_added_field_cdf`: CDF enabled, checks the preimage and postimage of the updated row. The four `safe_cast = true` cases fail on `main` with the error above. Testing (macOS arm64): - `cargo test -p deltalake-core --features datafusion --lib`: 1362 passed - `cargo fmt --check`: clean. `cargo clippy -p deltalake-core --features datafusion --tests --no-deps` reports nothing in `merge/mod.rs`. - Python bindings built from this branch: `tests/test_merge.py` 104 passed. With `error_on_type_mismatch=False`, a struct, a `list<struct>`, a source struct with only the new field, and a struct with CDF enabled all merge, and the unmatched row keeps `{x: 2, y: None}`. On `deltalake` 1.6.6, the first three drop the unmatched row's value to null and the CDF case fails with the same error. AI use: I used Claude Code to apply the change, write the tests, and run the test suites. I reviewed the diff and checked the DataFusion planning check and the test results myself. - follow-up to delta-io#4889 / delta-io#4887 - upstream: apache/datafusion#26528 (`try_cast` rejects struct casts that `cast` accepts) - DataFusion `try_cast` planning: https://github.com/apache/datafusion/blob/main/datafusion/physical-expr/src/expressions/try_cast.rs --------- Signed-off-by: ahlag <[email protected]> (cherry picked from commit 0a14212) Signed-off-by: R. Tyler Croy <[email protected]> Co-Authored-By: R. Tyler Croy <[email protected]> Co-Authored-By: Claude Haiku 5.5 <[email protected]>
…uct (#4889) When a merge with `merge_schema=True` widens a column (e.g. a struct gains a field), unmatched target rows in the files it rewrites are written with that column as `null`. The original values are lost in the new Parquet files. Cause: in the per-column CASE in `merge/mod.rs`, a field whose type changed used the source column as the default for rows that no clause sets. Unmatched target rows have no source row, so the copied value was null. The same column was also used as the lookup key for the clause expressions, but those are keyed by the target column, so the lookup never matched. As a result, a matched row whose update didn't set the column also got the source value. The old comment says this was done because casting to the new nested type breaks. With DataFusion 55, `cast`/`try_cast` go through `nested_struct::cast_column`, which matches struct fields by name and fills missing nullable fields with null, including inside lists. So this PR: - uses the target column as the default and lookup key for a changed field, like for unchanged fields; - casts every CASE branch for that field to the merged type from the new schema. Casting to the source's type fails when the source struct has no field in common with the target (`test_merge_with_schema_merge_and_pre_existing_struct_added_column`), and mixed struct types in one CASE can't be coerced. Tests: - `test_merge_with_schema_merge_struct_added_field_keeps_copied_rows`: unmatched rows A/C/D keep `nested`. Fails on `main` (they read as null). - `test_merge_with_schema_merge_struct_added_field_keeps_unset_column`: the update sets only `value` and an insert clause widens `nested`. Matched row B keeps its own `nested`. Fails on `main` (B gets the source struct and A/C/D are null). Testing (macOS arm64): - `cargo test -p deltalake-core --features datafusion --lib`: 1335 passed - `cargo fmt --check`: clean. `cargo clippy -p deltalake-core --features datafusion --tests --no-deps -- -D warnings` reports only lints in other files with my local toolchain, none in `merge/mod.rs`. - Python bindings built from this branch: `tests/test_merge.py` 104 passed. The issue's repro plus two variants (`when_matched_update({"s": "s.s"})` and `list<struct>`) all lose the row on `deltalake` 1.6.6 and keep `{x: 2, y: None}` with this change, both when read and in the Parquet file. AI use: I used Claude Code to trace the cause, draft the change and tests, and run the test suites. I reviewed the diff and checked the DataFusion cast behavior and the test results myself. - closes #4887 - DataFusion nested struct casting: https://docs.rs/datafusion-common/55.2.0/datafusion_common/nested_struct/index.html Signed-off-by: ahlag <[email protected]> (cherry picked from commit bfd03f3) Signed-off-by: R. Tyler Croy <[email protected]> Co-Authored-By: R. Tyler Croy <[email protected]> Co-Authored-By: Claude Haiku 5.5 <[email protected]>
Follow-up to #4889, from @ion-elgreco's [comment](#4889 (comment)). With `safe_cast` enabled (`error_on_type_mismatch=False` in Python), a merge with `merge_schema=True` that widens a struct fails during planning: ``` Unsupported TRY_CAST from Struct("x": Int64) to Struct("x": Int64, "y": Utf8) ``` DataFusion's `cast` checks struct casts by field name, but `try_cast` still plans with arrow's `can_cast_types`, which needs the same fields (`datafusion-physical-expr` 55.2 `try_cast.rs`, also on DataFusion `main`). The merged type is a superset of the target and source types, so this PR uses a strict `cast` for changed fields, in the CASE branches and in the CDF target preimage. This is the change Ion proposed. Tests: the struct-widening merge tests now run with and without `with_safe_cast(true)` (`rstest` `#[values(false, true)]`): - `test_merge_with_schema_merge_struct_added_field_keeps_copied_rows` - `test_merge_with_schema_merge_struct_added_field_keeps_unset_column` - `test_merge_with_schema_merge_and_pre_existing_struct_added_column` (the source struct only has the new field) - new `test_merge_with_schema_merge_struct_added_field_cdf`: CDF enabled, checks the preimage and postimage of the updated row. The four `safe_cast = true` cases fail on `main` with the error above. Testing (macOS arm64): - `cargo test -p deltalake-core --features datafusion --lib`: 1362 passed - `cargo fmt --check`: clean. `cargo clippy -p deltalake-core --features datafusion --tests --no-deps` reports nothing in `merge/mod.rs`. - Python bindings built from this branch: `tests/test_merge.py` 104 passed. With `error_on_type_mismatch=False`, a struct, a `list<struct>`, a source struct with only the new field, and a struct with CDF enabled all merge, and the unmatched row keeps `{x: 2, y: None}`. On `deltalake` 1.6.6, the first three drop the unmatched row's value to null and the CDF case fails with the same error. AI use: I used Claude Code to apply the change, write the tests, and run the test suites. I reviewed the diff and checked the DataFusion planning check and the test results myself. - follow-up to #4889 / #4887 - upstream: apache/datafusion#26528 (`try_cast` rejects struct casts that `cast` accepts) - DataFusion `try_cast` planning: https://github.com/apache/datafusion/blob/main/datafusion/physical-expr/src/expressions/try_cast.rs --------- Signed-off-by: ahlag <[email protected]> (cherry picked from commit 0a14212) Signed-off-by: R. Tyler Croy <[email protected]> Co-Authored-By: R. Tyler Croy <[email protected]> Co-Authored-By: Claude Haiku 5.5 <[email protected]>
Description
When a merge with
merge_schema=Truewidens a column (e.g. a struct gains a field), unmatched target rows in the files it rewrites are written with that column asnull. The original values are lost in the new Parquet files.Cause: in the per-column CASE in
merge/mod.rs, a field whose type changed used the source column as the default for rows that no clause sets. Unmatched target rows have no source row, so the copied value was null. The same column was also used as the lookup key for the clause expressions, but those are keyed by the target column, so the lookup never matched. As a result, a matched row whose update didn't set the column also got the source value.The old comment says this was done because casting to the new nested type breaks. With DataFusion 55,
cast/try_castgo throughnested_struct::cast_column, which matches struct fields by name and fills missing nullable fields with null, including inside lists. So this PR:test_merge_with_schema_merge_and_pre_existing_struct_added_column), and mixed struct types in one CASE can't be coerced.Tests:
test_merge_with_schema_merge_struct_added_field_keeps_copied_rows: unmatched rows A/C/D keepnested. Fails onmain(they read as null).test_merge_with_schema_merge_struct_added_field_keeps_unset_column: the update sets onlyvalueand an insert clause widensnested. Matched row B keeps its ownnested. Fails onmain(B gets the source struct and A/C/D are null).Testing (macOS arm64):
cargo test -p deltalake-core --features datafusion --lib: 1335 passedcargo fmt --check: clean.cargo clippy -p deltalake-core --features datafusion --tests --no-deps -- -D warningsreports only lints in other files with my local toolchain, none inmerge/mod.rs.tests/test_merge.py104 passed. The issue's repro plus two variants (when_matched_update({"s": "s.s"})andlist<struct>) all lose the row ondeltalake1.6.6 and keep{x: 2, y: None}with this change, both when read and in the Parquet file.AI use: I used Claude Code to trace the cause, draft the change and tests, and run the test suites. I reviewed the diff and checked the DataFusion cast behavior and the test results myself.
Related Issue(s)
Documentation