branch-4.1: [fix](iceberg) Resolve dropped equality-delete fields in manifest-cache planning - #68433
Open
hubgeter wants to merge 1 commit into
Conversation
…manifest-cache planning An equality delete can reference a field that a later schema change dropped. Iceberg 1.11.0 resolves such field IDs against every historical schema, and apache#67479 synced Doris's DeleteFileIndex with it, but the manifest-cache planner builds the index from specsById alone. Those specs are bound to the current schema, so planning fails with IllegalArgumentException: Cannot find field for ID 1 and falls back to the SDK scan. Results stay correct, but every query logs a WARN with a stack trace and the manifest cache is bypassed for the table. Pass the table's schemas to the index, as DataTableScan does and as master's connector already does.
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review completed on exact head 76375a30e5a009a6b6af2707ec83aa64d2a22d31 against base 12a53cd879c68c2fa6eeffb0dd5713c1b514cee0.
No new valuable findings.
- Correctness and lifecycle: reviewed all three changed files and traced manifest-cache planning through the frozen Iceberg table generation, SDK fallback, cached file copies, delete sequence/spec/partition applicability, and dropped-field schema resolution. The new schema map matches Iceberg's normal planner input for the retained-history case addressed by this patch.
- Compatibility and boundaries: the builder visibility widening is compatible with the existing vendored API and is required by the cross-package caller. The patch changes no persisted format, configuration/session behavior, FE/BE protocol, memory ownership, or nullability contract. No distinct performance or observability issue was substantiated.
- Tests and validation: the added regression is a meaningful parent-code negative and checks that the equality delete attaches only to the older data file. Validation was static only as required; no build or test was run.
git diff --checkpassed. Author and CI claims were not treated as independent validation. - Scope and completeness: there was no additional user focus and no pre-existing inline thread to address. Two complete review rounds plus a final changed-file and unresolved-candidate sweep covered the production, test, API, cache, fallback, schema-evolution, and cleanup paths. A broader cleanup-pruned-schema lifecycle was investigated and dismissed as an inline finding because it fails identically at the parent revision; it should be tracked separately rather than attributed to this backport.
The review is complete for the supplied exact-head bundle, with zero new inline comments by design.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Related PR: #67479, #67687
Problem Summary:
#67479 lets
DeleteFileIndexresolve equality-delete fields that were later dropped, by looking them up in historical schemas. It covers the default planner but misses the manifest-cache planner inIcebergScanNode, which still passes onlyspecsById. When the manifest cache is enabled, planning fails withCannot find field for ID Nand falls back to the SDK scan: results are correct, but each query logs a WARN and skips the cache.The master port (#67687) already covers this path. This PR applies the same one-line change to branch-4.1: pass
icebergTable.schemas()to the index.Release note
None
Check List (For Author)
Test
Manual test: with the manifest cache enabled,
EXPLAIN VERBOSEshowsfailures=0instead of1, and the fallback WARN is gone.Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)