Skip to content

[fix](nereids) eager aggregation: never substitute a literal inside the If condition - #69582

Open
raghav-reglobe wants to merge 1 commit into
apache:masterfrom
raghav-reglobe:fix-eager-agg-if-literal
Open

raghav-reglobe wants to merge 1 commit into
apache:masterfrom
raghav-reglobe:fix-eager-agg-if-literal

Conversation

@raghav-reglobe

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: close #xxx

Related PR: #xxx

Problem Summary:

With eager_aggregation_on_broadcast_join = true, PushDownAggregation decomposes Sum/Max/Min(If(cond, a, b)) into func(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.replace swaps every equal expression in the output, so for sum(if(flag > 0, 1, 0)) the literal 0 inside the condition is replaced by the pushed BIGINT sum slot. The BE then fails the query:

[RUNTIME_ERROR] not same type in function gt, left: TINYINT, right: BIGINT

Reproduce on master (any plan big enough for the rewrite to fire — e.g. a broadcast join under the aggregate):

set eager_aggregation_on_broadcast_join = true;
create table t4 (id4 int, flag boolean, qty bigint) duplicate key(id4) distributed by hash(id4) buckets 1 properties("replication_num"="1");
create table t2 (id2 int) duplicate key(id2) distributed by hash(id2) buckets 1 properties("replication_num"="1");
select sum(if(t4.flag > 0, 1, 0)), t2.id2 from t4 join t2 on t4.id4 = t2.id2 group by t2.id2;

EXPLAIN VERBOSE shows cast(flag as TINYINT) > sum[#n] where sum[#n] is the pushed sum of the literal 0. 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, or flag = TRUE, works.

Fix: key the replacement on the WHOLE aggregate function (ExpressionUtils.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.

Release note

Fix eager aggregation (eager_aggregation_on_broadcast_join) replacing a literal inside an IF condition 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

    • Regression test
    • Unit Test
    • Manual test (add detailed scripts or steps below)
    • No need to test or manual test. Explain why:
      • This is a refactor/code format and no logic has been changed.
      • Previous test can cover this change.
      • No code files have been changed.
      • Other reason

    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 VERBOSE keeps the cnt$c multipliers (eager aggregation still applies) and shows cast(is_delivered as TINYINT) > 0 with the literal intact.

  • Behavior changed:

    • No.
    • Yes.
  • Does this need documentation?

    • No.
    • Yes.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

…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).
@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants