raghav-reglobe opened a new pull request, #70554:
URL: https://github.com/apache/doris/pull/70554
### What problem does this PR solve?
Issue Number: close #70553
Related PR: none
Problem Summary:
A statement with an aggregate that has no GROUP BY and an argument like
`CASE WHEN a IS NOT NULL THEN f(a, b) END` (or the `IF` form) never finishes
planning. The FE thread spins in the rewrite stage, the plan grows by one
`LogicalFilter` on every pass until the heap is full, and neither
`query_timeout` nor a kill ends it. The issue has the reproduction.
Two things meet:
1. `EliminateAggCaseWhen` turns `avg(if(a IS NOT NULL, f(a, b), NULL))` into
`avg(f(a, b))` over a new filter `a IS NOT NULL` directly under the aggregate.
2. `InferAggNotNull` infers `a IS NOT NULL` and `b IS NOT NULL` from the
argument, looks for them only in the conjuncts of the aggregate's child filter,
and put what was missing into a new filter on top. With `a` already there, the
new filter held only `b`; on the next pass the child filter held only `b`, so
`a` looked missing and got a filter of its own, then `b` again, and so on.
`TopDownVisitorRewriteJob.doRewrite` applies a node's rules in a `while
(true)` until the node stops changing, with the planning timeout checked
outside that loop, so nothing ended it.
This PR:
- `InferAggNotNull`: when the aggregate's child is a `LogicalFilter`, the
generated not-nulls are added to that filter's conjuncts, as
`InferFilterNotNull` does. No second filter is put on top.
- `TopDownVisitorRewriteJob.doRewrite`: checks the planning timeout on every
pass, as the bottom-up job does, and fails the statement with the rule's name
once one node has been rewritten 10000 times.
### Release note
Fix a planner loop: a statement with an ungrouped aggregate over `CASE WHEN
x IS NOT NULL THEN f(x, y) END` never finished planning and exhausted the FE's
heap.
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] Regression test
- [x] Unit Test
- [x] 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 <!-- Add your reason? -->
- Behavior changed:
- [ ] No.
- [x] Yes. A rewrite that does not converge now fails the statement
("The rewrite does not converge: rule ... still changes ...") after 10000
rewrites of one node, and the planning timeout applies inside that loop.
Before, such a statement ran until the FE's heap was full.
- Does this need documentation?
- [x] No.
- [ ] Yes. <!-- Add document PR link here. eg:
https://github.com/apache/doris-website/pull/1214 -->
Unit tests:
- `InferAggNotNullTest.testInferJoinsTheFilterThatAlreadyHoldsOneOfThem`: an
aggregate over a filter that already holds one of the two not-nulls ends with
one filter holding both.
- `TopDownVisitorRewriteJobTest`: a rule that changes its node on every pass
is stopped with the rule's name in the error.
- On this branch (master at ccfd4378d42 plus this commit): the FE builds,
`checkstyle:check` on fe-core passes, and 26 test classes pass with 238 tests,
0 failures, 0 errors: the two above with `EliminateAggCaseWhenTest`,
`InferFilterNotNullTest`, `InferJoinNotNullTest` and `RewriteTopDownJobTest`,
and 20 classes that run the whole rewrite stage (`InferPredicatesTest`,
`NormalizeAggregateTest`, `EagerAggRewriterTest`, `ReorderJoinTest`,
`RewriteRuleSuiteTest` and others).
Manual test, on a build of this same commit on a master snapshot of early
November (not on a build of this branch), with `INFER_AGG_NOT_NULL` enabled and
on another table than the issue's `repro.t`, with the same statement shapes:
- These hung before the change and return in under a second with it: `AVG`
and `SUM` of `CASE WHEN x IS NOT NULL THEN f(x, y) END` with `DATEDIFF` on two
DATE columns, the same with `x + y` on two INT columns, the `IF` form, the
statement with a WHERE added, and a branch that uses three columns.
- Over five rows with NULL combinations the results equal the hand-computed
values and are the same with the rule enabled and with it disabled.
- `EXPLAIN REWRITTEN PLAN PROCESS` shows the rule firing once and leaving
one filter that holds both not-nulls under the aggregate.
There is no regression suite in this PR. I can add one if you want it.
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR should
merge into -->
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
