github-actions[bot] commented on code in PR #67152:
URL: https://github.com/apache/doris/pull/67152#discussion_r3868919984
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/FillUpQualifyMissingSlot.java:
##########
@@ -158,23 +216,50 @@ public List<Rule> buildRules() {
qualify -> having -> project(distinct)
*/
RuleType.FILL_UP_QUALIFY_HAVING_PROJECT.build(
- logicalQualify(logicalHaving(logicalProject())).then(qualify
-> {
+ logicalQualify(logicalHaving(logicalProject())).thenApply(ctx
-> {
+ LogicalQualify<LogicalHaving<LogicalProject<Plan>>>
qualify = ctx.root;
checkWindow(qualify);
+ Optional<Scope> outerScope =
ctx.cascadesContext.getOuterScope();
LogicalHaving<LogicalProject<Plan>> having =
qualify.child();
LogicalProject<Plan> project = qualify.child().child();
- return createPlan(project, qualify.getConjuncts(),
(newConjuncts, projects) -> {
+ return createPlan(project, qualify.getConjuncts(),
outerScope, (newConjuncts, projects) -> {
ImmutableList<NamedExpression> copyOutput =
ImmutableList.copyOf(project.getOutput());
if (project.isDistinct()) {
+ // Keep correlated predicates that only depend on
outer slots together with the
+ // having's own correlated predicates, on the same
decorrelatable side of the
+ // distinct barrier, so subquery unnesting can
collect and decorrelate them
+ // together (otherwise one of them is left
dangling in the apply's right side).
+ // A predicate that is constant per outer row is
equivalent before/after
+ // distinct, so moving it above the distinct
project preserves semantics.
+ Set<Expression> newHavingConjuncts = new
LinkedHashSet<>(having.getConjuncts());
+ Set<Expression> distinctQualifyConjuncts = new
LinkedHashSet<>();
+ if (outerScope.isPresent()) {
+ Set<Slot> correlatedSlots =
outerScope.get().getCorrelatedSlots();
+ for (Expression conjunct : newConjuncts) {
+ Set<Slot> inputSlots =
conjunct.getInputSlots();
+ if (!inputSlots.isEmpty() &&
correlatedSlots.containsAll(inputSlots)) {
Review Comment:
[P1] Keep volatile QUALIFY predicates on their original side of DISTINCT
Visible outer slots do not make a predicate constant. For example, `(o.flag
<> 1 OR random() < 0.5)` has input slots `{o.flag}`, so this branch moves it
from QUALIFY into HAVING:
```text
intended: ProjectDistinct[k] -> Qualify[window_ok, P(random)]
patched: Having[P(random)] -> ProjectDistinct[k] -> Qualify[window_ok]
```
With two duplicate `k` rows and `o.flag=1`, QUALIFY evaluates `P` twice
before DISTINCT (the key survives with probability 3/4), while the patched plan
evaluates it once after DISTINCT (probability 1/2). This is separate from the
existing deterministic lost-correlation thread: the new repair changes a
volatile predicate's evaluation domain. Require determinism/movability before
this motion, or expose correlation without relocating the predicate, and add
duplicate-key volatile/non-movable coverage.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/FillUpQualifyMissingSlot.java:
##########
@@ -189,10 +274,49 @@ interface PlanGenerator {
Plan apply(Set<Expression> newConjuncts, List<NamedExpression>
projects);
}
- private Plan createPlan(LogicalProject<Plan> project, Set<Expression>
conjuncts, PlanGenerator planGenerator) {
+ private Plan createPlan(LogicalProject<Plan> project, Set<Expression>
conjuncts,
+ Optional<Scope> outerScope, PlanGenerator planGenerator) {
Set<Slot> projectOutputSet = project.getOutputSet();
List<NamedExpression> newOutputSlots = Lists.newArrayList();
Set<Expression> newConjuncts = new LinkedHashSet<>();
+
+ // A correlated column referenced in qualify may be hidden behind a
project alias, e.g.
+ // `QUALIFY f = 1` where f is aliased as an outer column o.flag. If
the project also
+ // contains a window expression, filter pushdown cannot rewrite f back
to its producer
+ // before apply decorrelation, so the alias-producer dependency would
be lost and the
+ // correlation slot would never be collected into the apply. Resolve
such aliases whose
+ // producers reference only outer correlated slots, so the correlation
stays visible to
+ // subquery unnesting.
+ Map<Slot, Expression> correlatedAliasToProducer = Maps.newHashMap();
+ // If any qualify conjunct contains a subquery (e.g. an IN/NOT IN
predicate), the alias
+ // replacement below would descend into the subquery and break the
apply decorrelation
+ // (the substituted slot would be owned by neither side of the apply).
Skip the rewrite
+ // in that case.
+ boolean conjunctsHaveSubquery = conjuncts.stream().anyMatch(c ->
c.containsType(SubqueryExpr.class));
+ if (outerScope.isPresent() && !conjunctsHaveSubquery) {
+ Set<Slot> correlatedSlots = outerScope.get().getCorrelatedSlots();
+ for (Map.Entry<Slot, Expression> entry :
project.getAliasToProducer().entrySet()) {
+ Expression producer = entry.getValue();
+ if (!producer.getInputSlots().isEmpty()
+ &&
correlatedSlots.containsAll(producer.getInputSlots())
+ // a window producer would be re-extracted into a
fresh alias, which would be
+ // rewritten again, causing an infinite rewrite loop
before a fixed point.
+ && !producer.containsType(WindowExpression.class)
+ // a producer containing a subquery would be copied
into both the project and
+ // the rewritten qualify, and only one copy would be
unnested, leaving a
+ // dangling slot in the other.
+ && !producer.containsType(SubqueryExpr.class)) {
Review Comment:
[P1] Keep volatile alias producers single-evaluated
These exclusions still admit an alias such as `random() + o.flag AS f`. The
replacement then creates this reduced tree:
```text
Project[f]
Qualify[random2()+o.flag > 1.5, rn=1]
Project[random1()+o.flag AS f, window AS rn]
```
In a correlated scalar subquery, the copied predicate can admit the row
using `random2()` while the returned `f` from `random1()` does not satisfy the
QUALIFY condition. `PushDownFilterThroughProject` already blocks alias
expansion when a producer `containsVolatileExpression()` for exactly this
identity reason; none-movable/error expressions need the same audit.
Materialize the producer once (or reject the unsupported class) and add a
full-pipeline scalar regression.
##########
regression-test/suites/query_p0/sql_functions/window_functions/test_qualify_query.groovy:
##########
@@ -112,6 +112,288 @@ suite("test_qualify_query") {
qt_select_35 "select year + 1, country from sales having profit >= 100
qualify row_number() over (order by profit) = 6;"
qt_select_36 "select year + 1, country, row_number() over (order by
profit) rk from sales having profit >= 100 qualify rk = 6;"
+
+ // correlated subquery: an outer column referenced in qualify after an
explicit group by
+ // should not be treated as a missing inner group-by column under
ONLY_FULL_GROUP_BY.
+ qt_select_37 """
+ SELECT o.k
+ FROM (
+ SELECT CAST(10 AS INT) AS k, CAST(1 AS INT) AS flag
+ UNION ALL
+ SELECT CAST(20 AS INT) AS k, CAST(0 AS INT) AS flag
+ ) AS o
+ WHERE EXISTS (
+ SELECT i.k
+ FROM (
+ SELECT CAST(1 AS INT) AS k
+ UNION ALL
+ SELECT CAST(2 AS INT) AS k
+ ) AS i
+ GROUP BY i.k
+ QUALIFY row_number() OVER (ORDER BY i.k) = 1
+ AND o.flag = 1
+ )
+ ORDER BY o.k;
+ """
+
+ // an inner non-grouped column referenced in qualify should still be
rejected
+ // under ONLY_FULL_GROUP_BY.
+ test {
+ sql """
+ SELECT o.k
+ FROM (
+ SELECT CAST(10 AS INT) AS k, CAST(1 AS INT) AS flag
+ UNION ALL
+ SELECT CAST(20 AS INT) AS k, CAST(0 AS INT) AS flag
+ ) AS o
+ WHERE EXISTS (
+ SELECT i.k
+ FROM (
+ SELECT CAST(1 AS INT) AS k, CAST(1 AS INT) AS not_grouped
+ UNION ALL
+ SELECT CAST(2 AS INT) AS k, CAST(2 AS INT) AS not_grouped
+ ) AS i
+ GROUP BY i.k
+ QUALIFY row_number() OVER (ORDER BY i.k) = 1
+ AND i.not_grouped = 1
+ )
+ """
+ exception "must appear in the GROUP BY clause"
+ }
+
+ // correlated subquery over a plain project (no group by): the outer
column in
+ // qualify must not be pushed into the inner project's output.
+ qt_select_38 """
+ SELECT o.k
+ FROM (
+ SELECT CAST(10 AS INT) AS k, CAST(1 AS INT) AS flag
+ UNION ALL
+ SELECT CAST(20 AS INT) AS k, CAST(0 AS INT) AS flag
+ ) AS o
+ WHERE EXISTS (
+ SELECT i.k
+ FROM (
+ SELECT CAST(1 AS INT) AS k
+ UNION ALL
+ SELECT CAST(2 AS INT) AS k
+ ) AS i
+ QUALIFY row_number() OVER (ORDER BY i.k) = 1
+ AND o.flag = 1
+ )
+ ORDER BY o.k;
+ """
+
+ // correlated subquery over qualify -> having -> project: the outer column
in
+ // qualify must not be pushed into the inner project's output.
+ qt_select_39 """
+ SELECT o.k
+ FROM (
+ SELECT CAST(10 AS INT) AS k, CAST(1 AS INT) AS flag
+ UNION ALL
+ SELECT CAST(20 AS INT) AS k, CAST(0 AS INT) AS flag
+ ) AS o
+ WHERE EXISTS (
+ SELECT i.k
+ FROM (
+ SELECT CAST(1 AS INT) AS k
+ UNION ALL
+ SELECT CAST(2 AS INT) AS k
+ ) AS i
+ HAVING i.k >= 1
+ QUALIFY row_number() OVER (ORDER BY i.k) = 1
+ AND o.flag = 1
+ )
+ ORDER BY o.k;
+ """
+
+ // qualify -> having -> agg where both the having and the qualify
reference correlated outer
+ // columns. The window in qualify is extracted into a project above the
having, so the having's
+ // correlated predicate must be conjoined into the qualify to be
decorrelated together.
+ // o.h = 0 for k = 10 while o.flag = 1: the having predicate must still
filter it out.
+ qt_select_40 """
+ SELECT o.k
+ FROM (
+ SELECT CAST(10 AS INT) AS k, CAST(1 AS INT) AS flag, CAST(0 AS INT)
AS h
+ UNION ALL
+ SELECT CAST(20 AS INT) AS k, CAST(0 AS INT) AS flag, CAST(1 AS INT)
AS h
+ ) AS o
+ WHERE EXISTS (
+ SELECT i.k
+ FROM (
+ SELECT CAST(1 AS INT) AS k
+ UNION ALL
+ SELECT CAST(2 AS INT) AS k
+ ) AS i
+ GROUP BY i.k
+ HAVING o.h = 1
+ QUALIFY row_number() OVER (ORDER BY i.k) = 1
+ AND o.flag = 1
+ )
+ ORDER BY o.k;
+ """
+
+ // positive counterpart of select_40: only k = 10 satisfies both o.h = 1
and o.flag = 1.
+ qt_select_41 """
+ SELECT o.k
+ FROM (
+ SELECT CAST(10 AS INT) AS k, CAST(1 AS INT) AS flag, CAST(1 AS INT)
AS h
+ UNION ALL
+ SELECT CAST(20 AS INT) AS k, CAST(0 AS INT) AS flag, CAST(0 AS INT)
AS h
+ ) AS o
+ WHERE EXISTS (
+ SELECT i.k
+ FROM (
+ SELECT CAST(1 AS INT) AS k
+ UNION ALL
+ SELECT CAST(2 AS INT) AS k
+ ) AS i
+ GROUP BY i.k
+ HAVING o.h = 1
+ QUALIFY row_number() OVER (ORDER BY i.k) = 1
+ AND o.flag = 1
+ )
+ ORDER BY o.k;
+ """
+
+ // qualify -> project where the qualify references a project alias (f)
whose producer is a
+ // correlated outer column (o.flag). The alias-producer dependency must be
preserved so the
+ // correlation is still extracted even though the project contains a
window expression.
+ qt_select_42 """
+ SELECT o.k
+ FROM (
+ SELECT CAST(10 AS INT) AS k, CAST(1 AS INT) AS flag
+ UNION ALL
+ SELECT CAST(20 AS INT) AS k, CAST(0 AS INT) AS flag
+ ) AS o
+ WHERE EXISTS (
+ SELECT i.k, o.flag AS f, row_number() OVER (ORDER BY i.k) AS rn
+ FROM (
+ SELECT CAST(1 AS INT) AS k
+ UNION ALL
+ SELECT CAST(2 AS INT) AS k
+ ) AS i
+ QUALIFY rn = 1
+ AND f = 1
+ )
+ ORDER BY o.k;
+ """
+
+ // negative counterpart of select_42: with o.flag = 0 everywhere the
alias-resolved
+ // correlation must still filter out every outer row.
+ qt_select_43 """
+ SELECT o.k
+ FROM (
+ SELECT CAST(10 AS INT) AS k, CAST(0 AS INT) AS flag
+ UNION ALL
+ SELECT CAST(20 AS INT) AS k, CAST(0 AS INT) AS flag
+ ) AS o
+ WHERE EXISTS (
+ SELECT i.k, o.flag AS f, row_number() OVER (ORDER BY i.k) AS rn
+ FROM (
+ SELECT CAST(1 AS INT) AS k
+ UNION ALL
+ SELECT CAST(2 AS INT) AS k
+ ) AS i
+ QUALIFY rn = 1
+ AND f = 1
+ )
+ ORDER BY o.k;
+ """
+
+ // DISTINCT subquery where the having and the qualify carry separate
correlated outer
+ // predicates: both must stay on the same decorrelatable side of the
distinct barrier,
+ // otherwise one of them is left dangling in the apply's right subtree.
+ // o.h = 0 < count(*) (2 distinct groups) and o.flag = 1 both hold only
for k = 10.
+ qt_select_44 """
+ SELECT o.k
+ FROM (
+ SELECT CAST(10 AS INT) AS k, CAST(1 AS INT) AS flag, CAST(0 AS INT)
AS h
+ UNION ALL
+ SELECT CAST(20 AS INT) AS k, CAST(1 AS INT) AS flag, CAST(5 AS INT)
AS h
+ ) AS o
+ WHERE EXISTS (
+ SELECT DISTINCT i.k
+ FROM (
+ SELECT CAST(1 AS INT) AS k
+ UNION ALL
+ SELECT CAST(2 AS INT) AS k
+ ) AS i
+ HAVING o.h < count(*)
+ QUALIFY row_number() OVER (ORDER BY i.k) = 1
+ AND o.flag = 1
+ )
+ ORDER BY o.k;
+ """
+
+ // a project over a constant UNION ALL that references a correlated outer
column: the
+ // project must not be pushed through the union (the outer slot has no
producer inside
+ // the union children), and the correlation must still be decorrelated
correctly.
+ qt_select_45 """
+ SELECT o.k
+ FROM (
+ SELECT CAST(10 AS INT) AS k, CAST(1 AS INT) AS flag
+ UNION ALL
+ SELECT CAST(20 AS INT) AS k, CAST(0 AS INT) AS flag
+ ) AS o
+ WHERE EXISTS (
+ SELECT i.k, o.flag AS f
+ FROM (
+ SELECT CAST(1 AS INT) AS k
+ UNION ALL
+ SELECT CAST(2 AS INT) AS k
+ ) AS i
+ QUALIFY row_number() OVER (ORDER BY i.k) = 1
+ AND f = 1
+ )
+ ORDER BY o.k;
+ """
+
+ // a correlated having predicate that depends on the aggregate result
cannot be evaluated
+ // below the window project and must be rejected instead of being silently
dropped or
+ // moved above the window (which would change the evaluation order).
+ test {
+ sql """
+ SELECT o.k
+ FROM (
+ SELECT CAST(10 AS INT) AS k, CAST(0 AS INT) AS h
+ ) AS o
+ WHERE EXISTS (
+ SELECT i.k
+ FROM (
+ SELECT CAST(1 AS INT) AS k
+ UNION ALL
+ SELECT CAST(2 AS INT) AS k
+ ) AS i
+ GROUP BY i.k
+ HAVING count(*) = o.h
+ QUALIFY row_number() OVER (ORDER BY i.k) = 1
+ )
+ """
+ exception "in HAVING depending on the aggregate result is not
supported together with QUALIFY"
+ }
+
+ // an aggregate output alias that only depends on outer correlated columns
cannot be
+ // produced by the aggregate and must be rejected explicitly.
+ test {
+ sql """
+ SET sql_mode = '';
Review Comment:
[P1] Execute the SQL-mode change as a separate statement
`test.sql` sends this entire string once through `JdbcUtils.executeToList`,
which uses one prepared statement. The normal and p0 regression URLs do not
enable `allowMultiQueries`, so `SET ...; SELECT ...` fails as a
multi-statement/parser error before reaching
`resolveCorrelatedAggregateOutputAlias`; it cannot match the intended exception
and leaves this negative path untested. Run `sql "SET sql_mode = ''"`
separately before the `test` block and keep only the target SELECT here.
--
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]
