feat: Correctly set fields on Remove action when AMT is enabled - #3318
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3318 +/- ##
=======================================
Coverage 90.50% 90.51%
=======================================
Files 255 255
Lines 91199 91213 +14
Branches 91199 91213 +14
=======================================
+ Hits 82543 82558 +15
Misses 5642 5642
+ Partials 3014 3013 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
This PR wires adaptive_metadata_enabled into the Remove projection and correctly nulls deletionTimestamp under adaptiveMetadata (AMT), which is the one obligation it fully implements. Feature detection via is_feature_enabled(AdaptiveMetadataPreview) is sound, and both the parameterized unit test and the new integration test cover the deletionTimestamp branch well. The gap is that the PR adds two more "must" invariants to the Remove doc comments that the code does not enforce, and public write paths can produce Remove actions that violate them under AMT.
Summary
The deletionTimestamp change is correct and well tested, but the PR documents extendedFileMetadata and stats as hard AMT invariants that the code does not enforce, and public scan/remove paths can emit Remove actions that violate them. Either enforce those invariants or soften the doc claims to match actual behavior before merge.
Automated review - workflow run
| }; | ||
| // Note: The Delta protocol requires `partitionValues`, `size`, and `tags` when | ||
| // `extendedFileMetadata` is true. We require only `partitionValues` and `size` to match Spark. | ||
| // Under adaptiveMetadata both are guaranteed present |
There was a problem hiding this comment.
Blocker1 extendedFileMetadata is not forced true under adaptiveMetadata. The new doc on Remove::extended_file_metadata says "Must be true when adaptiveMetadata is enabled", but build_remove_struct_patch computes it as the same presence predicate (size is_not_null AND partitionValues is_not_null) for both paths; adaptive_metadata_enabled only affects deletionTimestamp. Commit validation requires size but not partitionValues, so an AMT remove with null partitionValues emits extendedFileMetadata=false, silently violating the RFC. The comment "Under adaptiveMetadata both are guaranteed present" is an unchecked assumption (and lacks a trailing period). Raised by: delta-protocol-reviewer, maintainer-claude-reviewer, architecture-reviewer, docs-reviewer, test-coverage-reviewer, maintainer-codex-reviewer. Suggested fix: under adaptive_metadata_enabled, validate size and partitionValues are present and emit lit(true) for extendedFileMetadata (mirroring the deletionTimestamp handling), or reword the doc/comment to describe the presence-predicate behavior instead of asserting a hard invariant.
There was a problem hiding this comment.
This is determined by the writer:
delta-kernel-rs/kernel/src/transaction/mod.rs
Lines 1605 to 1610 in f030ff6
| /// Contains [statistics] (e.g., count, min/max values for columns) about the data in this | ||
| /// logical file encoded as a JSON string. | ||
| /// | ||
| /// Must be set when adaptiveMetadata is enabled on the table. |
There was a problem hiding this comment.
Blocker2 stats "Must be set when adaptiveMetadata is enabled" is documented but not enforced. build_remove_struct_patch only passes stats through or coalesces with stats_parsed, so a null input stays null. requires_stats_num_records covers only IcebergCompatV3, which AMT does not pull in, and remove validation checks only path and size. A caller can scan with StatsOptions::none() and feed that to remove_files, producing an AMT Remove with null stats that violates the documented invariant; the new integration test never asserts stats. Raised by: delta-protocol-reviewer, maintainer-claude-reviewer, docs-reviewer, test-coverage-reviewer. Suggested fix: enforce non-null stats for AMT removes (e.g. extend the remove validation path) and add a negative test, or soften the doc claim to match actual behavior.
| // adaptiveMetadata requires column mapping in `id` mode plus RowTracking, DomainMetadata, | ||
| // DeletionVectors, and InCommitTimestamp. Auto-enable them (mirroring the ReaderWriter features | ||
| // into both lists) so callers can pass just `adaptiveMetadata-preview` and get a loadable | ||
| // table. |
There was a problem hiding this comment.
Nit1 create_table is accreting per-feature dependency blocks: this is the second hand-written "feature implies features + config" block plus a special-case columnMapping.mode/nested-id branch on enable_adaptive_metadata. Each new gated feature adds another if-block and OR term, and it duplicates the kernel's feature-dependency graph in a test helper where it can drift. Test-only, non-blocking. Raised by: architecture-reviewer. Suggested fix: replace the if-blocks with a data-driven table mapping each feature to its required features and config entries so adding a feature is a data entry.
Benchmark results: ✅ PassSummary: 🚀 0 · ✅ 8 · ☑️ 6 · 🚧 1 · ❌ 0 Per-benchmark results (15 rows)
Legend: 🚀 ≥1.15x faster · ✅ faster or unchanged · ☑️ ≤1.03x slower · 🚧 1.03x-1.15x slower · ❌ ≥1.15x slower |
| pub(crate) data_change: bool, | ||
|
|
||
| /// When true the fields `partition_values`, `size`, and `tags` are present | ||
| /// When true, the fields `partition_values`, `size`, and `tags` are present |
There was a problem hiding this comment.
I think tags was removed from the RFC
There was a problem hiding this comment.
You're right! Thanks!
| let engine = Arc::new(engine); | ||
| let schema = schema_ref! { nullable "number": INTEGER }; | ||
|
|
||
| // `test_utils::create_table` auto-enables the adaptiveMetadata dependency features and column |
There was a problem hiding this comment.
nit: seems like needless detail?
There was a problem hiding this comment.
Yes, I agree. I've cleaned it up
| /// `test_remove_files_adds_expected_entries` covers the timestamped, conditionally-extended shape. | ||
| #[cfg(feature = "adaptive-metadata-in-dev")] | ||
| #[tokio::test] | ||
| async fn remove_on_adaptive_metadata_table_nulls_deletion_timestamp_and_forces_extended_metadata( |
There was a problem hiding this comment.
nit: this method is 90 lines long, I wonder if there are sensible places that we could add helpers for to make the core test more compact?
There was a problem hiding this comment.
I'm afraid the end-to-end tests are a bit longer by nature. The test above this one is over 160 lines long. We repeat a pattern multiple times where we create a temp dir, which we can move to a helper. Are you okay with doing that in a separate PR so we can keep the PRs small? I can follow up directly after this one goes in
There was a problem hiding this comment.
yeah separate PR is fine if we can clean it up.
| // DeletionVectors, and InCommitTimestamp. Auto-enable them (mirroring the ReaderWriter features | ||
| // into both lists) so callers can pass just `adaptiveMetadata-preview` and get a loadable | ||
| // table. | ||
| let enable_adaptive_metadata = reader_features.contains(&"adaptiveMetadata-preview") |
There was a problem hiding this comment.
should this be a helper?
There was a problem hiding this comment.
I think this mimmics the pattern above, but happy to pull it into a private helper.
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
The deletionTimestamp change is correct: build_remove_struct_patch emits a null literal under adaptiveMetadata and a commit-timestamp literal otherwise, and both the parameterized unit test and the new integration test cover it. The problem in this PR is an unrelated content-tree schema change that breaks an existing contract test and diverges from the RFC.
The three AMT Remove-invariant items (extendedFileMetadata and stats documented as "must" but not enforced, and the test-utils feature-dependency accretion) were already raised against this same head SHA by the previous review, and the code behavior is unchanged, so they are not re-filed here. They remain unresolved and should still be addressed before merge.
Summary
The deletionTimestamp handling under adaptiveMetadata is correct and well tested. The blocking problem is the spec_id field-id change to 502, which breaks the existing content-entry schema-contract test and diverges from the RFC's assignment of 141; revert it or justify it against an authoritative revision and update the test. The new TrackingInfo field id 160 is unverifiable against the spec and has no user yet. Both content-tree edits are unrelated to this PR's Remove-action scope. The previously reported AMT extendedFileMetadata/stats doc-vs-code gaps are unchanged at this head SHA and still need resolution.
Automated review - workflow run
|
|
||
| /// ID of partition spec used to write manifest or data/delete files. | ||
| #[field_id = 141] | ||
| #[field_id = 502] |
There was a problem hiding this comment.
Blocker1 spec_id field id is changed from 141 to 502, but the schema-contract test content_tree_node_entry_schema_field_contract (line 368) still asserts (PARTITION_SPEC_ID, Some(141), false) and compares the generated Parquet field id, so it fails with field id mismatch for specId. The module is gated by adaptive-metadata-in-dev, which cargo test --all-features (the pre-push gate) enables. Separately, the Iceberg-v4-metadata RFC assigns spec_id field id 141; 502 does not appear there (it is Iceberg's manifest-list partition_spec_id), so since readers resolve by field id this breaks interop. This change is also unrelated to the Remove-action subject of the PR. Raised by: maintainer-claude-reviewer, maintainer-codex-reviewer, delta-protocol-reviewer, test-coverage-reviewer, architecture-reviewer. Suggested fix: revert to #[field_id = 141]; if a newer authoritative RFC reassigns it to 502, cite that revision and update the contract test to Some(502) in the same change.
There was a problem hiding this comment.
This was actually a discrapency in the draft V4 spec
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
No blocking issues. The core change is correct: build_remove_struct_patch emits a null deletionTimestamp literal under adaptiveMetadata and a commit-timestamp literal otherwise, which matches the iceberg-v4-metadata RFC Remove-file requirement. The parameterized unit test and the new end-to-end integration test both cover the null and timestamped branches, and the test-utils AMT dependency auto-enable (columnMapping id mode plus rowTracking, domainMetadata, deletionVectors, inCommitTimestamp, with the correct reader/writer split) matches the RFC and the kernel's enforced feature requirements.
Summary
The adaptiveMetadata deletionTimestamp handling is correct and well covered by both unit and integration tests, and the test-utils feature wiring is accurate. The only new item is a minor test-assertion tightening. Note that the previously reported extendedFileMetadata and stats doc-vs-enforcement gaps are unchanged at this head SHA and remain unresolved; they are not re-filed here.
Automated review - workflow run
| let removes = read_actions_from_commit(&table_url, version, "remove")?; | ||
| assert_eq!(removes.len(), 1, "expected exactly one remove action"); | ||
| let remove = &removes[0]; | ||
| assert!( |
There was a problem hiding this comment.
Nit1 The deletionTimestamp assertion uses remove.get("deletionTimestamp").is_none_or(|v| v.is_null()), which also passes if the field is absent entirely. Since the invariant is that an explicit null is emitted, a regression that dropped the field rather than serializing null would slip through. Raised by: test-coverage-reviewer. Suggested fix: assert the key is present and null, e.g. assert!(remove["deletionTimestamp"].is_null(), ...), after confirming the committed action serializes null_lit as an explicit null.
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
No new blocking issues at this head SHA.
The core change is correct. Under adaptiveMetadata, build_remove_struct_patch emits null_lit(DataType::LONG) for deletionTimestamp and a commit-timestamp literal otherwise, which matches the iceberg-v4-metadata RFC Remove-file requirement. Feature detection via is_feature_enabled(&TableFeature::AdaptiveMetadataPreview) is sound, the make_eval closure's explicit DeltaResult return type is a no-op clarity change, and the test-utils AMT dependency auto-enable (columnMapping id mode plus rowTracking, domainMetadata, deletionVectors, inCommitTimestamp with the correct reader/writer split) matches the RFC and the kernel's feature_requirements. The parameterized unit test and the new integration test both exercise the null and timestamped branches.
The reviewers reconfirmed the earlier doc-vs-enforcement gaps: the newly documented extendedFileMetadata "must be true" and stats "must be set" AMT invariants are not enforced by build_remove_struct_patch or the commit path, the inline comment "Under adaptiveMetadata both are guaranteed present" states an unchecked assumption, and the integration test's deletionTimestamp assertion (is_none_or(|v| v.is_null())) also passes if the field is dropped entirely. These match findings already raised against this same head SHA in the supplied review history, and the code behavior is unchanged, so they are not re-filed here. They remain unresolved and should still be addressed before merge, either by enforcing the invariants under adaptive_metadata_enabled or by softening the doc and comment wording to match the presence-predicate behavior.
Summary
The deletionTimestamp handling under adaptiveMetadata is correct and covered by both unit and integration tests, and the test-utils feature wiring is accurate. No new blocking issues at this head SHA. The previously reported extendedFileMetadata/stats doc-vs-enforcement gaps and the weak deletionTimestamp test assertion are unchanged and not re-filed, but they still need resolution before merge. All six reviewers completed; no reduced coverage.
Automated review - workflow run
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
No new blocking issues at this head SHA.
The core change is correct and RFC-conformant. Under adaptiveMetadata, build_remove_struct_patch emits null_lit(DataType::LONG) for deletionTimestamp and a commit-timestamp literal otherwise, which matches the iceberg-v4-metadata RFC Remove-file requirement. Feature detection via is_feature_enabled(&TableFeature::AdaptiveMetadataPreview) is sound, the make_eval explicit DeltaResult<_> return type is a clarity-only change, and the test-utils AMT dependency auto-enable (columnMapping id mode plus rowTracking, domainMetadata, deletionVectors, inCommitTimestamp with the correct reader/writer split) mirrors the kernel's feature_requirements. The parameterized unit test and the new integration test both exercise the null and timestamped branches.
The reviewers reconfirmed the previously reported gaps at this same head SHA: the documented extendedFileMetadata "must be true" and stats "must be set" AMT invariants are not enforced by build_remove_struct_patch or the commit path (both remain the presence predicate / pass-through for both paths), the inline comment "Under adaptiveMetadata both are guaranteed present" states an unchecked assumption, and the integration test's deletionTimestamp assertion (is_none_or(|v| v.is_null())) also passes if the field is dropped entirely. These match findings already raised against this head SHA in the supplied review history and the code behavior is unchanged, so they are not re-filed here. They remain unresolved and should be closed out before merge, either by enforcing the invariants under adaptive_metadata_enabled or by softening the doc and comment wording to match the presence-predicate behavior.
Summary
The deletionTimestamp handling under adaptiveMetadata is correct, RFC-compliant, and covered by both unit and integration tests, and the test-utils feature wiring is accurate. No new blocking issues at this head SHA. The previously reported extendedFileMetadata/stats doc-vs-enforcement gaps, the unchecked inline comment, and the weak deletionTimestamp test assertion are unchanged and not re-filed, but they still need resolution before merge. All six reviewers completed; no reduced coverage.
Automated review - workflow run
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
No blocking issues.
The core change is correct and matches the iceberg-v4-metadata RFC Remove-file requirement. Under adaptiveMetadata, build_remove_struct_patch emits null_lit(DataType::LONG) for deletionTimestamp and lit(commit_timestamp) otherwise. Feature detection via is_feature_enabled(&TableFeature::AdaptiveMetadataPreview) is sound, the flag is computed once in generate_remove_actions and threaded as a plain bool into a config-free builder, and the literal type matches the Option<i64> field. The test-utils enable_adaptive_metadata_dependencies wiring mirrors the kernel's feature_requirements: the ReaderWriter deps (columnMapping, deletionVectors) plus adaptiveMetadata-preview go into both lists, and the writer-only deps (rowTracking, domainMetadata, inCommitTimestamp) into the writer list only. The parameterized unit test and the new integration test exercise both the null and timestamped branches.
The reviewers reconfirmed several pre-existing gaps at this same head SHA, all with unchanged code behavior: the documented extendedFileMetadata "must be true" and stats "must be set" AMT invariants are not enforced by build_remove_struct_patch (both stay the presence predicate / pass-through on both paths), the inline comment "Under adaptiveMetadata both are guaranteed present" states an unchecked assumption, the integration test's is_none_or(|v| v.is_null()) assertion also passes if the field is dropped entirely, and the test-utils helper hand-transcribes the kernel's feature-dependency graph rather than deriving it. These match findings already raised against this head SHA in the supplied review history, so they are not re-filed. They remain unresolved and should be closed before merge, either by enforcing the invariants under adaptive_metadata_enabled or by softening the doc and comment wording to match the presence-predicate behavior.
Automated review - workflow run
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
Summary
The deletionTimestamp logic is correct and RFC-conformant: build_remove_struct_patch emits null_lit(DataType::LONG) under adaptiveMetadata and lit(commit_timestamp) otherwise, feature detection via is_feature_enabled(&TableFeature::AdaptiveMetadataPreview) is sound, and both the parameterized unit test and the new integration test cover the two branches. The test-utils dependency wiring matches the kernel feature requirements. The one blocker is a duplicate null_lit import that fails to compile under adaptive-metadata-in-dev/--all-features; fixing the import is a one-line change. The previously reported doc-vs-enforcement gaps (extendedFileMetadata/stats documented as required but not enforced), the "guaranteed present" inline comment, and the permissive is_none_or deletionTimestamp assertion are unchanged at this head SHA and are not re-filed, but they remain open. All six reviewers and the disprove gate completed; no reduced coverage.
Automated review - workflow run
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
No blocking issues.
The core change is correct and matches the iceberg-v4-metadata RFC Remove-file requirement. Under adaptiveMetadata, build_remove_struct_patch emits null_lit(DataType::LONG) for deletionTimestamp and lit(commit_timestamp) otherwise; the literal type matches the Option<i64> field. Feature detection via is_feature_enabled(&TableFeature::AdaptiveMetadataPreview) is sound, computed once in generate_remove_actions and threaded as a plain bool into the config-free builder. The previously reported duplicate null_lit import is resolved at this head SHA: null_lit now appears only in the unconditional grouped import and is used on the always-compiled path, so both the default and adaptive-metadata-in-dev / --all-features builds compile cleanly. The test-utils enable_adaptive_metadata_dependencies helper mirrors the kernel's real ADAPTIVE_METADATA_PREVIEW_INFO.feature_requirements (ReaderWriter deps in both lists, writer-only deps in the writer list). The parameterized unit test and the new integration test exercise the null and timestamped branches.
The reviewers reconfirmed several pre-existing items at this unchanged head SHA: the documented extendedFileMetadata "must be true" and stats "must be set" invariants are not enforced by build_remove_struct_patch (both remain the presence predicate / pass-through on both paths), the inline comment "Under adaptiveMetadata both are guaranteed present" states an unchecked assumption, and the integration test's deletionTimestamp assertion also passes if the field is absent. These match findings already raised against this same head SHA in the supplied review history, so they are not re-filed. The test-coverage reviewer also noted that, because the kernel omits null Remove fields from commit JSON (as the sibling backReference test documents), the current is_none_or(|v| v.is_null()) assertion is appropriate and should not be tightened to require an explicit present-and-null key. All six reviewers completed; no reduced coverage.
Automated review - workflow run
3 participants
What changes are proposed in this pull request?
As described in the Delta RFC:
https://github.com/delta-io/delta/blob/master/protocol_rfcs/iceberg-v4-metadata.md#remove-file
How was this change tested?