starocean999 opened a new pull request, #63763:
URL: https://github.com/apache/doris/pull/63763
Problem Summary:
The `AggScalarSubQueryToWindowFunction` rewrite rule (WinMagic) rewrites a
correlated scalar subquery like:
```sql
SELECT d.did, f.id, f.k, f.v
FROM fact f, dim d
WHERE f.k = d.k
AND f.v * 2 > (
SELECT SUM(f2.v)
FROM fact f2
WHERE f2.k = d.k
)
```
into a window-function plan:
```
Filter(f.v * 2 > SUM(v) OVER (PARTITION BY d.k))
Window(SUM(v) OVER (PARTITION BY d.k))
CrossJoin(Scan(fact f), Scan(dim d))
```
This rewrite had three correctness issues:
---
**Bug 1 — Join amplification when the correlated key is not unique**
```sql
-- dim has DUPLICATE KEY(did), so k is NOT unique and can be NULL
CREATE TABLE dim (did INT, k INT, tag INT)
ENGINE=OLAP DUPLICATE KEY(did) ...;
-- data: two rows with the same k=1
INSERT INTO dim VALUES (10, 1, 1), (11, 1, 1);
-- fact and dim are joined on f.k = d.k
SELECT d.did, f.id, f.k, f.v
FROM fact f, dim d
WHERE f.k = d.k
AND f.v * 2 > (
SELECT SUM(f2.v) FROM fact f2 WHERE f2.k = d.k
);
```
Before the fix, the rule **did not check** whether the correlated outer-only
table's key (`d.k`) is unique. Since `dim` is a DUPLICATE KEY table, `k=1` has
two rows. After the CrossJoin, each fact row with `k=1` joins with both dim
rows, producing 2× more output rows than the original correlated scalar
subquery (which is evaluated per outer row). The window function then sees this
inflated row set, producing wrong aggregate values.
**Fix:** Added `checkUniqueCorrelatedTable()` which verifies the correlated
outer-only table's key is unique & non-null via
`DataTrait.isUniqueAndNotNull()`. This covers OLAP UNIQUE_KEYS metadata,
explicit `UNIQUE` constraints, and rejects nullable keys. Queries against
non-unique-keyed tables are left as original scalar subqueries.
---
**Bug 2 — Inner filter split by PushDownFilterThroughProject causes rewrite
to fail**
```sql
-- inner subquery has both a correlated predicate and a non-correlated
predicate
SELECT d.did, f.id, f.k, f.v
FROM fact f, dim_unique d
WHERE f.k = d.k
AND f.v < 10
AND f.v * 2 > (
SELECT SUM(f2.v)
FROM fact f2
WHERE f2.k = d.k -- correlated, cannot push through project
AND f2.v < 10 -- non-correlated, can push through project
);
```
The optimizer's `PushDownFilterThroughProject` splits the inner `WHERE f2.k
= d.k AND f2.v < 10` into two separate `LogicalFilter` nodes:
- `Filter(k = d.k)` stays above the project (references correlated slot
`d.k`, not in project output)
- `Filter(v < 10)` is pushed below the project (only references `v`, which
is in the project output)
The old `checkFilter()` required **exactly one** inner `LogicalFilter` and
returned false when seeing two, causing the rule to miss this valid rewrite.
**Fix:** `checkFilter()` now collects conjuncts from **all** inner
`LogicalFilter` nodes rather than requiring exactly one.
---
**Bug 3 — Matched inner-filter predicates incorrectly placed above the
window**
```sql
-- f.v < 10 appears both in the outer WHERE and the inner WHERE
SELECT d.did, f.id, f.k, f.v
FROM fact f, dim_unique d
WHERE f.k = d.k
AND f.v < 10
AND f.v * 2 > (
SELECT SUM(f2.v)
FROM fact f2
WHERE f2.k = d.k
AND f2.v < 10
);
```
The predicate `f.v < 10` is **semantically part of the inner aggregate's
filter** — the original scalar subquery only sums `f2.v` where `v < 10`.
However, the old rewrite placed this conjunct **above** the window:
```
-- Wrong: f.v < 10 above the window lets the window see ALL rows
Filter(f.v < 10 AND f.v * 2 > SUM(v) OVER (...))
Window(SUM(v) OVER (PARTITION BY d.k)) -- aggregates all rows, not just
v<10
CrossJoin(Scan(fact f), Scan(dim_unique d))
```
This means the window function computes `SUM(v)` over **all** fact rows per
key (e.g., `v=5+7+6+4+10+6+8`), instead of only rows where `v < 10` (e.g.,
`v=5+7+6+4+6+8`). The comparison `f.v * 2 > sum` then uses the wrong aggregate,
potentially returning incorrect rows.
**Fix:** `checkFilter()` now tracks which outer conjuncts were matched
against inner subquery filter conjuncts. `rewrite()` places these matched
conjuncts **below** the window, preserving the inner aggregate's filter
semantic:
```
-- Correct: f.v < 10 below the window restricts the window's input
Filter(f.v * 2 > SUM(v) OVER (...))
Window(SUM(v) OVER (PARTITION BY d.k))
Filter(f.v < 10) -- restricts rows seen by the window
CrossJoin(Scan(fact f), Scan(dim_unique d))
```
---
### Changes
- `checkUniqueCorrelatedTable()` — validates correlated key uniqueness via
`DataTrait.isUniqueAndNotNull()`
- Fix `checkFilter()` — collect conjuncts from ALL inner filters, track
matched conjuncts
- Enhance `rewrite()` — classify conjuncts: matched inner-filter → below
window, shared-table → above, volatile → above, outer-only → below
- Add `stripOuterFilters()` and `ensureProjectOutput()` helpers
- 13 unit tests + 6 regression cases with result and plan-shape verification
### Release note
Scalar correlated subquery to window function rewrite is now correct:
non-unique keys are rejected, split inner filters are handled, and inner-filter
predicates are placed below the window.
### Check List (For Author)
- Test:
- Unit Test: 13 tests, all passing
- Regression Test: 6 cases, all passing with verified plan shapes
- Behavior changed: Yes — previously the rule could produce incorrect
results for non-unique keys and misplaced predicates
- Does this need documentation: No
```
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] 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 <!-- Add your reason? -->
- Behavior changed:
- [ ] No.
- [ ] Yes. <!-- Explain the behavior change -->
- Does this need documentation?
- [ ] No.
- [ ] Yes. <!-- Add document PR link here. eg:
https://github.com/apache/doris-website/pull/1214 -->
### 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 -->
--
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]
