Repository navigation
fix: validate substrait filters that need no field pruning - #8484
Merged
Xuanwo merged 3 commits intoAug 23, 2026
Merged
Conversation
Contributor
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The conditional-validation defect and its panic regressions are now covered: unsupported operators are checked for unpruned schemas, omitted roots remain compatible, and malformed non-leading IfThen clauses return an input error.
westonpace
approved these changes
Aug 11, 2026
remap_expr_references also rejects operators that cannot be pushed down, but it only ran when remove_extension_types actually dropped a field. A filter over a schema with no extension types therefore skipped the checks and reached the DataFusion consumer. Run it unconditionally. When nothing is dropped the index mapping is the identity, so the remap is a no-op and only the validation is added. Signed-off-by: 1fanwang <1fannnw@gmail.com>
Running the walker on every filter exposed its unwraps on optional proto fields. An omitted root_type means a root reference, and a leading if-clause without then supplies the case expression, so both are valid input that panicked. Handle those, and report the remaining absent fields as invalid input rather than panicking on user-supplied bytes. Signed-off-by: 1fanwang <1fannnw@gmail.com>
DataFusion only reads a clause's condition as the case expression for ifs[0]; it unwraps then for every later clause. Skipping an absent then on all of them let that panic through instead of reporting invalid input. Signed-off-by: 1fanwang <1fannnw@gmail.com>
1fanwang
force-pushed
the
fix/7805-substrait-validate-unpruned
branch
from
August 11, 2026 19:36
9278c30 to
58d3461
Compare
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.
Which issue does this PR close?
Rationale for this change
remap_expr_referencesdoubles as the validation pass that rejects operators we cannot push down (window functions, subqueries, lambdas), butparse_substraitonly calls it whenremove_extension_typesactually drops a field. A filter whose base schema has no extension types skips the checks entirely, and the unsupported expression reaches the DataFusion consumer instead.The user sees a consumer-level error rather than ours. A subquery over an unpruned schema reports:
instead of
Window functions or subqueries not allowed in filter expression.What changes are included in this PR?
remap_expr_referencesnow runs unconditionally.The guard it replaces is not load-bearing for the remapping itself:
remove_extension_typesbuilds the index mapping by walking every field and only advancing the destination counter for fields it keeps, so when nothing is dropped the mapping is the identity and the remap is a no-op. Dropping the guard adds the validation without changing any index.This keeps validation and remapping as one traversal rather than two that have to be kept in step.
Are these changes tested?
Yes.
test_unsupported_operator_rejected_without_pruningparses a subquery and a lambda against a single-Int32base schema, which needs no pruning, and asserts each is rejected with its own message.On the current code the subquery case reaches the consumer instead:
Before
After
The roundtrip tests above cover the pruning and nested-schema paths, including
List<Struct>and extension-type removal, so they would catch a remap regression. The wholelance-datafusionsuite passes (85 tests).Are there any user-facing changes?
Unsupported filter operators are now rejected with the descriptive error in the case that previously slipped through. No API change.