[KYUUBI #7568][AUTHZ] Eliminate TypeOfPlaceHolder across the whole plan#7569
Closed
j1wonpark wants to merge 2 commits into
Closed
[KYUUBI #7568][AUTHZ] Eliminate TypeOfPlaceHolder across the whole plan#7569j1wonpark wants to merge 2 commits into
j1wonpark wants to merge 2 commits into
Conversation
…ole plan RuleApplyTypeOfMarker replaces every TypeOf in the plan with a TypeOfPlaceHolder using transformAllExpressions, but RuleEliminateTypeOf reverts them with transformExpressionsUp, which only visits the root node's expressions. Any placeholder living below the root -- under Sort, Union, Aggregate, or a subquery -- therefore survives into execution and fails, because TypeOfPlaceHolder implements doGenCode but no eval: [CLASS_NOT_OVERRIDE_EXPECTED_METHOD] TypeOfPlaceHolder must override either eval or nullSafeEval SELECT typeof(id) AS t FROM t1 ORDER BY t -- fails SELECT typeof(id) FROM t1 UNION ALL SELECT ... -- fails SELECT typeof(id) FROM t1 -- works (root is Project) The whole-plan walk was dropped in KYUUBI apache#5997, which removed the outer transformUp as an unnecessary loop. It was not: transformExpressionsUp does not descend into children. Use transformAllExpressions instead, mirroring RuleApplyTypeOfMarker and keeping the single-pass traversal that apache#5997 aimed for. The existing test only covers queries whose root node is a Project, which is why the regression went unnoticed. Add coverage for Sort, Union, Aggregate, and subquery roots. Signed-off-by: Jiwon Park <jpark92@outlook.kr>
pan3793
approved these changes
Jul 15, 2026
…hile keeping Union as the root node Signed-off-by: Jiwon Park <jpark92@outlook.kr>
pan3793
approved these changes
Jul 15, 2026
pan3793
pushed a commit
that referenced
this pull request
Jul 16, 2026
### Why are the changes needed? Fixes #7568. `RuleApplyTypeOfMarker` marks every `TypeOf` in the whole plan (`transformAllExpressions`), but since #5997 `RuleEliminateTypeOf` reverts them with `transformExpressionsUp`, which only visits the root node's expressions. A `TypeOfPlaceHolder` below the root — under `Sort`, `Union`, `Aggregate`, or a subquery — survives into execution and fails with `CLASS_NOT_OVERRIDE_EXPECTED_METHOD`, since it implements `doGenCode` but no `eval`. This change uses `transformAllExpressions` on the eliminating side as well, so both rules cover the same scope in a single pass. Affects 1.9.0 onwards, including master. ### How was this patch tested? Added a test to `RangerSparkExtensionSuite` covering the root shapes the existing `Project`-root-only test misses: `Sort`, `Union`, `Aggregate`, and a subquery. It fails on master and passes with this change. Full `extensions/spark/kyuubi-spark-authz` suite: 657 passed, 0 failed. ### Was this patch authored or co-authored using generative AI tooling? Assisted-by: Claude:claude-opus-4-8 Closes #7569 from j1wonpark/fix/authz-eliminate-typeof-whole-plan. Closes #7568 38464f1 [Jiwon Park] Sort collected rows in the UNION ALL case to avoid order dependency while keeping Union as the root node 9ffea2d [Jiwon Park] [KYUUBI #7568][AUTHZ] Eliminate TypeOfPlaceHolder across the whole plan Authored-by: Jiwon Park <jpark92@outlook.kr> Signed-off-by: Cheng Pan <chengpan@apache.org> (cherry picked from commit 8fe59c5) Signed-off-by: Cheng Pan <chengpan@apache.org>
pan3793
pushed a commit
that referenced
this pull request
Jul 16, 2026
### Why are the changes needed? Fixes #7568. `RuleApplyTypeOfMarker` marks every `TypeOf` in the whole plan (`transformAllExpressions`), but since #5997 `RuleEliminateTypeOf` reverts them with `transformExpressionsUp`, which only visits the root node's expressions. A `TypeOfPlaceHolder` below the root — under `Sort`, `Union`, `Aggregate`, or a subquery — survives into execution and fails with `CLASS_NOT_OVERRIDE_EXPECTED_METHOD`, since it implements `doGenCode` but no `eval`. This change uses `transformAllExpressions` on the eliminating side as well, so both rules cover the same scope in a single pass. Affects 1.9.0 onwards, including master. ### How was this patch tested? Added a test to `RangerSparkExtensionSuite` covering the root shapes the existing `Project`-root-only test misses: `Sort`, `Union`, `Aggregate`, and a subquery. It fails on master and passes with this change. Full `extensions/spark/kyuubi-spark-authz` suite: 657 passed, 0 failed. ### Was this patch authored or co-authored using generative AI tooling? Assisted-by: Claude:claude-opus-4-8 Closes #7569 from j1wonpark/fix/authz-eliminate-typeof-whole-plan. Closes #7568 38464f1 [Jiwon Park] Sort collected rows in the UNION ALL case to avoid order dependency while keeping Union as the root node 9ffea2d [Jiwon Park] [KYUUBI #7568][AUTHZ] Eliminate TypeOfPlaceHolder across the whole plan Authored-by: Jiwon Park <jpark92@outlook.kr> Signed-off-by: Cheng Pan <chengpan@apache.org> (cherry picked from commit 8fe59c5) Signed-off-by: Cheng Pan <chengpan@apache.org>
Member
|
thanks, merged to master/1.11/1.10 |
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
2 participants
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.
Why are the changes needed?
Fixes #7568.
RuleApplyTypeOfMarkermarks everyTypeOfin the whole plan (transformAllExpressions), but since #5997RuleEliminateTypeOfreverts them withtransformExpressionsUp, which only visits the root node's expressions. ATypeOfPlaceHolderbelow the root — underSort,Union,Aggregate, or a subquery — survives into execution and fails withCLASS_NOT_OVERRIDE_EXPECTED_METHOD, since it implementsdoGenCodebut noeval.This change uses
transformAllExpressionson the eliminating side as well, so both rules cover the same scope in a single pass.Affects 1.9.0 onwards, including master.
How was this patch tested?
Added a test to
RangerSparkExtensionSuitecovering the root shapes the existingProject-root-only test misses:Sort,Union,Aggregate, and a subquery. It fails on master and passes with this change. Fullextensions/spark/kyuubi-spark-authzsuite: 657 passed, 0 failed.Was this patch authored or co-authored using generative AI tooling?
Assisted-by: Claude:claude-opus-4-8