github-actions[bot] commented on code in PR #64849:
URL: https://github.com/apache/doris/pull/64849#discussion_r3788499559
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/EliminateGroupByKey.java:
##########
@@ -110,17 +250,24 @@ public static Set<Expression>
findCanBeRemovedExpressions(LogicalAggregate<? ext
FuncDeps funcDeps = dataTrait.getAllValidFuncDeps(validSlots);
if (funcDeps.isEmpty()) {
- return new HashSet<>();
+ return new FindResult(new HashSet<>(), new HashSet<>());
}
Set<Set<Slot>> minGroupBySlots = funcDeps.eliminateDeps(new
HashSet<>(groupBySlots.values()), requireOutput);
Set<Expression> removeExpression = new HashSet<>();
+ Set<Expression> wrapWithAnyValue = new HashSet<>();
for (Entry<Expression, Set<Slot>> entry : groupBySlots.entrySet()) {
- if (!minGroupBySlots.contains(entry.getValue())
- && !requireOutput.containsAll(entry.getValue())) {
- removeExpression.add(entry.getKey());
+ if (!minGroupBySlots.contains(entry.getValue())) {
+ // FD redundant: can remove from group-by
+ if (!requireOutput.containsAll(entry.getValue())) {
+ // Not needed in output either: remove completely
+ removeExpression.add(entry.getKey());
+ } else {
+ // Still needed in output: remove from group-by, wrap with
ANY_VALUE in output
+ wrapWithAnyValue.add(entry.getKey());
Review Comment:
**[P1] Suppress invalid scan constraints before this branch**
This added `ANY_VALUE` path turns two existing
`LogicalOlapScan.computeUnique()` ordering gaps into wrong results because
`super.computeUnique()` imports table constraints before the scan-specific
guards run:
- A direct non-base index containing `a,c` for a table-level `UNIQUE(a,b)`
resolves only `{a}` in `findSlotsByColumn()`. The selected-index return then
leaves that singleton advertised as unique, so `GROUP BY a,c` becomes `GROUP BY
a` plus `ANY_VALUE(c)` and merges distinct `(1,'x')`/`(1,'y')` groups.
- A MOR unique-key table with an explicit constraint on `k` and
`read_mor_as_dup_tables='*'` deliberately exposes versions `(1,10)`, `(1,20)`,
and `(1,30)`, but the raw-read return also leaves the superclass `k -> v` trait
intact. This branch collapses those three `(k,v)` groups to one.
Please suppress superclass constraints for raw-version reads and require
every constrained column to be present before registering a constraint on a
selected index, then add executed result regressions for both modes.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/EliminateGroupByKey.java:
##########
@@ -17,90 +17,230 @@
package org.apache.doris.nereids.rules.rewrite;
-import org.apache.doris.nereids.annotation.DependsRules;
+import org.apache.doris.nereids.jobs.JobContext;
import org.apache.doris.nereids.properties.DataTrait;
import org.apache.doris.nereids.properties.FuncDeps;
-import org.apache.doris.nereids.rules.Rule;
-import org.apache.doris.nereids.rules.RuleType;
+import org.apache.doris.nereids.trees.expressions.Alias;
+import org.apache.doris.nereids.trees.expressions.ExprId;
import org.apache.doris.nereids.trees.expressions.Expression;
import org.apache.doris.nereids.trees.expressions.NamedExpression;
import org.apache.doris.nereids.trees.expressions.Slot;
+import org.apache.doris.nereids.trees.expressions.functions.agg.AnyValue;
import org.apache.doris.nereids.trees.plans.Plan;
+import org.apache.doris.nereids.trees.plans.algebra.Aggregate;
import org.apache.doris.nereids.trees.plans.logical.LogicalAggregate;
+import org.apache.doris.nereids.trees.plans.logical.LogicalCTEConsumer;
+import org.apache.doris.nereids.trees.plans.logical.LogicalFilter;
+import org.apache.doris.nereids.trees.plans.logical.LogicalProject;
+import org.apache.doris.nereids.trees.plans.visitor.CustomRewriter;
+import org.apache.doris.nereids.trees.plans.visitor.DefaultPlanRewriter;
-import com.google.common.collect.ImmutableList;
+import com.google.common.collect.LinkedHashMultimap;
+import com.google.common.collect.Multimap;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.HashSet;
+import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Set;
-
/**
* Eliminate group by key based on fd item information.
* such as:
* for a -> b, we can get:
* group by a, b, c => group by a, c
+ *
+ * When a group-by key is FD-redundant but still needed in the output,
+ * it is wrapped with any_value() and assigned a fresh ExprId.
+ * Upper plan references are rewritten via ExprIdRewriter so that
+ * all ancestor nodes see the new ExprIds.
*/
-@DependsRules({EliminateGroupBy.class, ColumnPruning.class})
-public class EliminateGroupByKey implements RewriteRuleFactory {
+public class EliminateGroupByKey extends DefaultPlanRewriter<Map<ExprId,
ExprId>> implements CustomRewriter {
+ private ExprIdRewriter exprIdReplacer;
+
+ @Override
+ public Plan rewriteRoot(Plan plan, JobContext jobContext) {
+ if (!plan.containsType(Aggregate.class)) {
+ return plan;
+ }
+ Map<ExprId, ExprId> replaceMap = new HashMap<>();
+ ExprIdRewriter.ReplaceRule replaceRule = new
ExprIdRewriter.ReplaceRule(replaceMap, false);
+ exprIdReplacer = new ExprIdRewriter(replaceRule, jobContext);
+ return plan.accept(this, replaceMap);
+ }
+
+ @Override
+ public Plan visit(Plan plan, Map<ExprId, ExprId> replaceMap) {
+ plan = visitChildren(this, plan, replaceMap);
+ plan = exprIdReplacer.rewriteExpr(plan, replaceMap);
Review Comment:
**[P1] Rewrite lateral `ON` conjuncts with generator arguments**
This whole-tree replacement can leave `LogicalGenerate` internally
inconsistent. A reduced reachable tree is:
```text
Generate[UNNEST(tags#T2), ON tag#G = name#N] // name#N is stale
Project[keep#K, name#N2, tags#T2, cnt#C]
Aggregate[group=k, output=k, ANY_VALUE(name)#N2, ANY_VALUE(tags)#T2,
count(*)#C]
Project[k, upper(k) AS name#N, split(k, ',') AS tags#T]
Scan
```
The lower deterministic expressions provide valid `k -> {name,tags}` FDs,
and a computed `keep` output retains the upper Project.
`LogicalGenerate.getExpressions()` includes both generators and lateral
conjuncts, but `GenerateExpressionRewrite` rewrites only `getGenerators()`;
`withGenerators()` preserves `ON tag#G = name#N` after the child has switched
to `name#N2`. Final slot validation therefore rejects the query. Please
rewrite/rebuild the conjuncts in the same operation and add a production
rewrite test using a grouped derived table with `JOIN LATERAL UNNEST(...) ...
ON ...`.
--
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]
