HIVE-29729: AIBOE in HiveFilterProjectTransposeRule during CBO planni…#6601
HIVE-29729: AIBOE in HiveFilterProjectTransposeRule during CBO planni…#6601rtrivedi12 wants to merge 2 commits into
Conversation
…ng for filters over windowing Projects
asf-ci-hive
added
tests pending
tests failed
tests passed
and removed
tests pending
tests failed
labels
| ) ranked_once | ||
| ) ranked_twice | ||
| ) cohorted | ||
| where cg_tg_cohort is not null; |
There was a problem hiding this comment.
Could you please replace the q file test with a minimal one that reproduces the exception? How about the following q file test?
-- HIVE-29729: an IS NOT NULL predicate above nested windowing (OVER) Projects must
-- not trip the RedundancyChecker in HiveFilterProjectTransposeRule.
set hive.cbo.fallback.strategy=NEVER;
create table tab1 (id string);
select rnk2
from (
select
row_number() over(order by rnk) as rnk2
from (
select count(*) over() as rnk
from (select count(*) from tab1) t
) t2
) t3
where rnk2 is not null;
There was a problem hiding this comment.
Thank you so much for the review @thomasrebele and short repro!
| filterCondition, (Project) node, bloat); | ||
| filterCondition, project, bloat); | ||
| if (condition != null) { | ||
| filterCondition = condition; |
There was a problem hiding this comment.
I did a bit of debugging, and the problem might be around here. pushPastProjectUnlessBloat checks for OVER and returns null. The filterCondition is not updated, so it continues to hold the filter condition of the parent.
For the q test file, the filterCondition corresponding to the parent is IS NOT NULL(row_number() OVER (PARTITION BY 0 ORDER BY $1 NULLS LAST RANGE BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING)). This will be also applied to the child, which is HiveProject(_o__c0=[$0]). So a project with just one field, making the $1 RexInputRef illegal.
There was a problem hiding this comment.
I see .. The real defect was the condition == null path: when pushPastProjectUnlessBloat couldn't push the predicate, the old code kept the un-translated condition and still descended.
|
| Project project = (Project) node; | ||
| RexNode condition = HiveRelOptUtil.pushPastProjectUnlessBloat( | ||
| filterCondition, (Project) node, bloat); | ||
| if (condition != null) { | ||
| filterCondition = condition; | ||
| filterCondition, project, bloat); | ||
| if (condition == null) { | ||
| // the condition could not be pushed, so bail out | ||
| return; | ||
| } | ||
| filterCondition = condition; |
There was a problem hiding this comment.
nit: This can be rewritten as
node instanceof Project project
and also I think filterCondition can be directly assigned HiveRelOptUtil.pushPastProjectUnlessBloat( filterCondition, project, bloat); without any side effects?
Neither of these are needed for the PR btw so I will leave it up to you.
| @@ -0,0 +1,16 @@ | |||
| -- HIVE-29729: an IS NOT NULL predicate above nested windowing (OVER) Projects must | |||
| -- not trip the RedundancyChecker in HiveFilterProjectTransposeRule. | |||
| set hive.cbo.fallback.strategy=NEVER; | |||
There was a problem hiding this comment.
nit: Is this needed? I think NEVER is the default value.
soumyakanti3578
left a comment
soumyakanti3578
left a comment
There was a problem hiding this comment.
LGTM as is. There are nits but they are not strictly needed for the PR.
I will merge this sometime tomorrow.
4 participants
…ng for filters over windowing Projects
What changes were proposed in this pull request?
RedundancyChecker.visit(...) in HiveFilterProjectTransposeRule now skips (returns) when it reaches a Project that contains window functions (RexOver), instead of calling pushPastProjectUnlessBloat on it. This mirrors the guard that already exists in onMatch(...) and the visitor's existing "unsupported node → return" behaviour
Why are the changes needed?
isRedundantIsNotNull(...) (added in HIVE-25275) walks down the plan and pushes the predicate past each Project. For a Project holding RexOver expressions, Calcite's pushPastProjectUnlessBloat rewrites the RexInputRefs inside the window PARTITION BY / ORDER BY specs; those refs index into the input row type and can exceed the projection list size, throwing ArrayIndexOutOfBoundsException during CBO planning
Does this PR introduce any user-facing change?
No
How was this patch tested?
mvn test -pl itests/qtest -Pitests -Dtest=TestMiniLlapLocalCliDriver -Dqfile=cbo_filter_pushdown_windowing_notnull.q