github-actions[bot] commented on code in PR #64849:
URL: https://github.com/apache/doris/pull/64849#discussion_r3528776232
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/rules/rewrite/EliminateGroupByKeyTest.java:
##########
@@ -181,6 +183,25 @@ void testEliminateByEqual() {
&&
agg.getGroupByExpressions().get(0).toSql().equals("name")));
}
+ @Test
+ void testEliminateByPkWithOutputNeeded() throws Exception {
+ // Regression: when a group-by key (name) is FD-redundant (id -> name)
+ // but still appears in SELECT, it should be removed from group-by
+ // and wrapped with ANY_VALUE in the output.
+ addConstraint("alter table t1 add constraint pk2 primary key (id)");
+ PlanChecker.from(connectContext)
+ .analyze("select id, name, count(*) from t1 group by id, name")
+ .customRewrite(new EliminateGroupByKey())
Review Comment:
This regression only runs `EliminateGroupByKey` through
`PlanChecker.customRewrite()`, which wraps it as `RuleType.TEST_REWRITE`. That
proves the visitor logic, but not the production
`custom(RuleType.ELIMINATE_GROUP_BY_KEY, ...)` registration/disable bit, nor
the MV pre-rewrite mask added for `ELIMINATE_GROUP_BY_KEY`. Please add a
production-path check, for example a
`.rewrite()`/`disable_nereids_rules=ELIMINATE_GROUP_BY_KEY` case for this query
and a `PreMaterializedViewRewriterTest` assertion that
`ruleSetApplied(RuleType.ELIMINATE_GROUP_BY_KEY)` triggers `needPreRewrite`.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/EliminateGroupByKey.java:
##########
@@ -38,69 +42,156 @@
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 List<Rule> buildRules() {
- return ImmutableList.of(
- RuleType.ELIMINATE_GROUP_BY_KEY.build(
- logicalProject(logicalAggregate().when(agg ->
!agg.getSourceRepeat().isPresent()))
- .then(proj -> {
- LogicalAggregate<? extends Plan> agg =
proj.child();
- LogicalAggregate<Plan> newAgg =
eliminateGroupByKey(agg, proj.getInputSlots());
- if (newAgg == null) {
- return null;
- }
- return proj.withChildren(newAgg);
- })),
- RuleType.ELIMINATE_FILTER_GROUP_BY_KEY.build(
- logicalProject(logicalFilter(logicalAggregate()
- .when(agg ->
!agg.getSourceRepeat().isPresent())))
- .then(proj -> {
- LogicalAggregate<? extends Plan> agg =
proj.child().child();
- Set<Slot> requireSlots = new
HashSet<>(proj.getInputSlots());
-
requireSlots.addAll(proj.child(0).getInputSlots());
- LogicalAggregate<Plan> newAgg =
eliminateGroupByKey(agg, requireSlots);
- if (newAgg == null) {
- return null;
- }
- return
proj.withChildren(proj.child().withChildren(newAgg));
- })
- )
- );
+ 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);
}
- LogicalAggregate<Plan> eliminateGroupByKey(LogicalAggregate<? extends
Plan> agg, Set<Slot> requireOutput) {
- Set<Expression> removeExpression = findCanBeRemovedExpressions(agg,
requireOutput,
+ @Override
+ public Plan visit(Plan plan, Map<ExprId, ExprId> replaceMap) {
+ plan = visitChildren(this, plan, replaceMap);
+ plan = exprIdReplacer.rewriteExpr(plan, replaceMap);
Review Comment:
This upward rewrite misses aggregates whose only changed reference is in
`GROUP BY`. A reduced shape is:
```text
Aggregate(groupBy=[name#old], output=[count(*)])
Project(name#old)
Aggregate(groupBy=[id#i, name#old], output=[id#i, name#old])
```
After the inner `Project(Aggregate)` rewrites `name#old` to
`any_value(name#old) AS name#new`, the outer aggregate needs its group key
rewritten to `name#new`. But `ExprIdRewriter` delegates to
`ExpressionRewrite.AggExpressionRewrite`, which computes `newGroupByExprs` and
then returns the old aggregate whenever the output list is unchanged, so this
group-by-only change is discarded. The resulting plan keeps grouping by
`name#old` while its child now outputs `name#new`, which `CheckAfterRewrite`
reports as an input slot missing from the child output. Please make the
aggregate expression rewrite rebuild when either group-by expressions or
outputs change, or handle aggregate ancestors directly in this rule before
propagating the replacement map.
--
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]
