github-actions[bot] commented on code in PR #64849:
URL: https://github.com/apache/doris/pull/64849#discussion_r3473327629
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/EliminateGroupByKey.java:
##########
@@ -79,28 +81,59 @@ public List<Rule> buildRules() {
}
LogicalAggregate<Plan> eliminateGroupByKey(LogicalAggregate<? extends
Plan> agg, Set<Slot> requireOutput) {
- Set<Expression> removeExpression = findCanBeRemovedExpressions(agg,
requireOutput,
+ FindResult result = findCanBeRemovedExpressionsInternal(agg,
requireOutput,
agg.child().getLogicalProperties().getTrait());
+ Set<Expression> removeExpression = result.removeExpression;
+ Set<Expression> wrapWithAnyValue = result.wrapWithAnyValue;
+
List<Expression> newGroupExpression = new ArrayList<>();
for (Expression expression : agg.getGroupByExpressions()) {
- if (!removeExpression.contains(expression)) {
+ if (!removeExpression.contains(expression)
+ && !wrapWithAnyValue.contains(expression)) {
newGroupExpression.add(expression);
}
}
List<NamedExpression> newOutput = new ArrayList<>();
for (NamedExpression expression : agg.getOutputExpressions()) {
- if (!removeExpression.contains(expression)) {
- newOutput.add(expression);
+ if (removeExpression.contains(expression)) {
+ continue;
+ }
+ if (wrapWithAnyValue.contains(expression)) {
+ // expression is FD-redundant but needed in output: wrap with
any_value
Review Comment:
This creates the exact ExprId shape that the parallel uniform-key rewrite
avoids: for a wrapped slot `name#x`, the aggregate output becomes
`any_value(name#x) AS name#x`, so the same ExprId names both the child input
slot and a new aggregate output expression. `EliminateGroupByKeyByUniform` has
a local comment explaining that `any_value(a#0) AS a#0` violates ExprId
uniqueness and can break modules such as MV rewrite, which is why that rule
allocates a fresh alias ExprId and rewrites upper references. Please follow the
same pattern here, or otherwise keep the original grouping key until the upper
references can be rewritten safely.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/EliminateGroupByKey.java:
##########
@@ -79,28 +81,59 @@ public List<Rule> buildRules() {
}
LogicalAggregate<Plan> eliminateGroupByKey(LogicalAggregate<? extends
Plan> agg, Set<Slot> requireOutput) {
- Set<Expression> removeExpression = findCanBeRemovedExpressions(agg,
requireOutput,
+ FindResult result = findCanBeRemovedExpressionsInternal(agg,
requireOutput,
agg.child().getLogicalProperties().getTrait());
+ Set<Expression> removeExpression = result.removeExpression;
+ Set<Expression> wrapWithAnyValue = result.wrapWithAnyValue;
+
List<Expression> newGroupExpression = new ArrayList<>();
for (Expression expression : agg.getGroupByExpressions()) {
- if (!removeExpression.contains(expression)) {
+ if (!removeExpression.contains(expression)
+ && !wrapWithAnyValue.contains(expression)) {
newGroupExpression.add(expression);
}
}
List<NamedExpression> newOutput = new ArrayList<>();
for (NamedExpression expression : agg.getOutputExpressions()) {
- if (!removeExpression.contains(expression)) {
- newOutput.add(expression);
+ if (removeExpression.contains(expression)) {
+ continue;
+ }
+ if (wrapWithAnyValue.contains(expression)) {
+ // expression is FD-redundant but needed in output: wrap with
any_value
+ expression = new Alias(expression.getExprId(),
+ new AnyValue(expression.toSlot()),
expression.getName());
}
+ newOutput.add(expression);
}
return agg.withGroupByAndOutput(newGroupExpression, newOutput);
}
/**
- * return removeExpression
+ * Return expressions that can be removed from both group-by and output.
+ * Kept for backward compatibility with external callers (e.g.
PushDownAggThroughJoinOnPkFk).
*/
public static Set<Expression>
findCanBeRemovedExpressions(LogicalAggregate<? extends Plan> agg,
Set<Slot> requireOutput, DataTrait dataTrait) {
+ FindResult result = findCanBeRemovedExpressionsInternal(agg,
requireOutput, dataTrait);
+ Set<Expression> all = new HashSet<>();
+ all.addAll(result.removeExpression);
Review Comment:
Returning `wrapWithAnyValue` here leaks the new direct-rule behavior into
the old public API. `PushDownAggThroughJoinOnPkFk.eliminatePrimaryOutput()`
still treats this method's result as safe to remove from the pushed-down
aggregate GROUP BY and does not wrap retained outputs. For example, in a PK/FK
join where the foreign side also has `foreign.id2 -> foreign.name`, a query
grouped by `pri.id1, foreign.id2, foreign.name` and selecting `foreign.name`
will mark `foreign.name` as wrap-only here. The pushdown caller then drops it
from `minGroupBySlotList`, rewrites `pri.id1` to the foreign key, and keeps the
`foreign.name` output as a bare slot because it is not a primary-side output.
The new aggregate below the join can therefore output `foreign.name` without
grouping by it or wrapping it in `any_value`. Please keep this public helper
returning only expressions that are removable by existing external callers, or
update the PK/FK pushdown path to consume the split result and add the
same `ANY_VALUE` output rewrite.
--
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]
