Repository navigation
[fix](nereids) eager aggregation: never substitute a literal inside the If condition - #69582
Open
raghav-reglobe wants to merge 1 commit into
Open
raghav-reglobe wants to merge 1 commit into
raghav-reglobe wants to merge 1 commit into
Conversation
…he If condition PushDownAggregation decomposes Sum/Max/Min(If(cond, a, b)) into func(a) + func(b) pushed below the join and rebuilt the upper output with a replace map keyed on the branch VALUE expressions (func.child(0)). ExpressionUtils.replace swaps every equal expression in the output, so for sum(if(flag > 0, 1, 0)) the literal 0 inside the condition became the pushed BIGINT sum slot: the BE fails with "not same type in function gt, left: TINYINT, right: BIGINT" on BOOLEAN/TINYINT columns whenever the plan is big enough for the rewrite to fire (prod: a 9-join statement failed 111 times in a day while a 2-table shape and '= TRUE' worked). Key the map on the whole aggregate function instead (replace is top-down and short-circuits): a decomposed func(If(cond, a, b)) is rebuilt as func(If(cond, pushedA, pushedB)), a plain func(x) as func(pushedX), count(x) as sum0(pushedX) as before. Nothing inside the condition or beside the function is touched. This also stops two functions over the same child in one output expression from overwriting each other's child-keyed entry. Test: EagerAggRewriterTest.testDecomposedIfKeepsConditionLiteral - sum(if(bool > 0, 1, 0)), max(if(tinyint = 1, 1, 0)), sum(if(bigint > 1, 1, 0)) under a join: the rewrite still fires, both branch values are pushed slots, every condition keeps its literal (the first two assertions fail before this change).
raghav-reglobe
requested review from
924060929,
englefly,
morrySnow and
starocean999
as code owners
October 25, 2026 07:05
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
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: close #xxx
Related PR: #xxx
Problem Summary:
With
eager_aggregation_on_broadcast_join = true,PushDownAggregationdecomposesSum/Max/Min(If(cond, a, b))intofunc(a)+func(b)pushed below the join, then rebuilds the upper output with a replace map keyed on the branch VALUE expressions (func.child(0)).ExpressionUtils.replaceswaps every equal expression in the output, so forsum(if(flag > 0, 1, 0))the literal0inside the condition is replaced by the pushed BIGINT sum slot. The BE then fails the query:Reproduce on master (any plan big enough for the rewrite to fire — e.g. a broadcast join under the aggregate):
EXPLAIN VERBOSEshowscast(flag as TINYINT) > sum[#n]wheresum[#n]is the pushed sum of the literal0. Exposure: comparison literals that stay equal to a branch literal after type coercion (BOOLEAN/TINYINT columns → BE type error; for max/min the pushed slot is the constant itself, numerically harmless). A 2-table shape where the rewrite does not fire, orflag = TRUE, works.Fix: key the replacement on the WHOLE aggregate function (
ExpressionUtils.replaceis top-down and short-circuits): a decomposedfunc(If(cond, a, b))is rebuilt asfunc(If(cond, pushedA, pushedB)), a plainfunc(x)asfunc(pushedX),count(x)assum0(pushedX)as before. Nothing inside the condition or beside the function is touched. This also stops two functions over the same child in one output expression from overwriting each other's child-keyed entry.Release note
Fix eager aggregation (
eager_aggregation_on_broadcast_join) replacing a literal inside anIFcondition with the pushed aggregate slot when it equals a decomposed branch value (sum(if(col > 0, 1, 0))failed with "not same type in function gt").Check List (For Author)
Test
Unit test:
EagerAggRewriterTest.testDecomposedIfKeepsConditionLiteral—sum(if(bool > 0, 1, 0)),max(if(tinyint = 1, 1, 0)),sum(if(bigint > 1, 1, 0))under a join: the rewrite still fires, both branch values are pushed slots, every condition keeps its literal (the first two assertions fail without this change).Manual: the repro above returns the right row instead of the BE type error; the production statement that failed 111 times a day (a 9-join aggregate over BOOLEAN columns) has run clean for 4+ hours on a build with this patch with the rewrite enabled globally;
EXPLAIN VERBOSEkeeps thecnt$cmultipliers (eager aggregation still applies) and showscast(is_delivered as TINYINT) > 0with the literal intact.Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)