[fix](iceberg) Avoid unsafe historical predicate pushdown - #67465
[fix](iceberg) Avoid unsafe historical predicate pushdown#67465Gabriel39 wants to merge 1 commit into
Conversation
Validate time-travel pruning predicates against both the selected scan schema and the current Iceberg schema. Skip metadata pushdown when a column name resolves to a different field ID, while retaining Doris execution filtering and safe predicate pruning.
|
run buildall |
|
/review |
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review status: complete after two convergence rounds; requesting changes.
I found one blocking correctness issue in the historical predicate pruning guard. The intended null fallback is conservative and preserves the original Doris conjunct for BE row filtering, but aggregate field-ID set equality does not prove equivalent leaf bindings or valid exact names under the current schema; see the inline comment.
Critical checkpoints:
- Correctness: blocking. Legal name swaps can prune matching historical files, and case-only renames can still fail current-spec binding.
- Planning/residual/lifecycle: ordinary scans, manifest-cache planning, and position-delete planning are conservative when the helper returns null; no separate residual-loss or lifecycle issue found.
- Compatibility/performance: the Iceberg 1.10.1 current-spec behavior was traced; no additional compatibility or material planning-cost issue found.
- Tests: simple rename, drop, and same-name replacement are covered, but the accepted compound/case-only production paths are not. No local build was run, as required by the review runner instructions.
- Release note/docs: the release note matches the intended fix; no documentation change appears necessary.
- User focus: no additional user-provided focus was supplied.
No existing live inline comment or review duplicated this finding. Reviewed head: 4f34782c2d8749dbf3ea64fe39019ef7fa1a3147.
| currentSchema.asStruct(), Collections.singletonList(currentExpression), true); | ||
| // Iceberg 1.10 plans manifests with current partition specs, so only push an | ||
| // historical predicate when its names still resolve to the same field IDs. | ||
| return scanFieldIds.equals(currentFieldIds) ? scanExpression : null; |
There was a problem hiding this comment.
[P1] Validate each predicate leaf, not the aggregate ID set
boundReferences returns a set, so equality here does not prove that the returned unbound expression has the same binding under both schemas. For example, historical 1:a, 2:b can legally evolve via a -> tmp, b -> a, tmp -> b to current 1:b, 2:a; a = 1 OR b = 2 yields {1,2} on both sides, passes this guard, and Iceberg 1.10.1 then rebinds the historical names against current partition specs, allowing a matching historical file to be pruned. A case-only rename (Foo -> foo) also passes {7} == {7}, but the returned Foo fails case-sensitive binding against the current spec. Please verify each converted leaf's name-to-ID correspondence (including exact spelling), or suppress pushdown, and add partitioned/case-only regressions that actually attach the predicate to planFiles().
FE UT Coverage ReportIncrement line coverage |
2 participants
What problem does this PR solve?
Issue Number: None
Related PR: apache/iceberg#13301
Problem Summary:
Iceberg 1.10 plans manifest filters against current partition specs. A time-travel predicate using a column that was later renamed, dropped, or replaced by a same-named field can therefore fail to bind or bind to the wrong field ID.
This change validates pruning predicates against both the selected scan schema and the current table schema. It skips only predicates whose names no longer resolve to the same field IDs, leaving the original Doris conjunct for execution filtering while preserving safe Iceberg metadata pruning.
Release note
Fix Iceberg time-travel queries that filter on columns renamed or dropped after the selected snapshot.
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)