github-actions[bot] commented on code in PR #67884:
URL: https://github.com/apache/doris/pull/67884#discussion_r4079002715
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/SimplifyAggGroupBy.java:
##########
@@ -20,117 +20,99 @@
import org.apache.doris.nereids.rules.Rule;
import org.apache.doris.nereids.rules.RuleType;
import org.apache.doris.nereids.trees.expressions.Add;
-import org.apache.doris.nereids.trees.expressions.BinaryArithmetic;
import org.apache.doris.nereids.trees.expressions.Cast;
-import org.apache.doris.nereids.trees.expressions.Divide;
import org.apache.doris.nereids.trees.expressions.Expression;
import org.apache.doris.nereids.trees.expressions.Multiply;
import org.apache.doris.nereids.trees.expressions.Slot;
import org.apache.doris.nereids.trees.expressions.Subtract;
+import org.apache.doris.nereids.trees.expressions.functions.scalar.Abs;
+import org.apache.doris.nereids.trees.expressions.functions.scalar.IsInf;
+import org.apache.doris.nereids.trees.expressions.functions.scalar.IsNan;
import org.apache.doris.nereids.trees.expressions.literal.Literal;
-import org.apache.doris.nereids.util.ExpressionUtils;
-import org.apache.doris.nereids.util.Utils;
+import org.apache.doris.nereids.types.DataType;
import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
+import java.util.LinkedHashSet;
import java.util.List;
import java.util.Set;
/**
- * Simplify Aggregate group by Multiple to One. For example
+ * Remove deterministic grouping expressions whose inputs are already bare
grouping slots.
* <p>
- * GROUP BY ClientIP, ClientIP - 1, ClientIP - 2, ClientIP - 3
+ * GROUP BY ClientIP, ClientIP + 1, ClientIP + 2
* -->
* GROUP BY ClientIP
+ *
+ * <p>Retain existing bare slots so aggregate outputs can still reference
them. A cast group key
+ * cannot generally provide its original slot to those outputs. Never
synthesize a slot from
+ * derived keys, since doing so can split groups formed by non-injective
expressions.</p>
*/
public class SimplifyAggGroupBy extends OneRewriteRuleFactory {
- private static final ImmutableSet<Class<? extends Expression>>
supportedFunctions
- = ImmutableSet.of(Add.class, Subtract.class, Multiply.class,
Divide.class);
-
@Override
public Rule build() {
return logicalAggregate()
- .when(agg -> agg.getGroupByExpressions().size() > 1
- &&
ExpressionUtils.allMatch(agg.getGroupByExpressions(),
- SimplifyAggGroupBy::isBinaryArithmeticSlot))
+ .when(agg -> agg.getGroupByExpressions().size() > 1)
.then(agg -> {
- List<Expression> groupByExpressions =
agg.getGroupByExpressions();
- ImmutableSet.Builder<Expression> inputSlots
- =
ImmutableSet.builderWithExpectedSize(groupByExpressions.size());
- for (Expression groupByExpression : groupByExpressions) {
- inputSlots.addAll(groupByExpression.getInputSlots());
- }
- Set<Expression> slots = inputSlots.build();
- if (slots.size() != 1) {
+ List<Expression> simplified =
simplifyGroupBy(agg.getGroupByExpressions());
+ if (simplified == null) {
return null;
}
- return
agg.withGroupByAndOutput(Utils.fastToImmutableList(slots),
agg.getOutputExpressions());
+ return agg.withGroupByAndOutput(simplified,
agg.getOutputExpressions());
})
.toRule(RuleType.SIMPLIFY_AGG_GROUP_BY);
}
@VisibleForTesting
- protected static boolean isBinaryArithmeticSlot(Expression expr) {
- if (expr instanceof Slot) {
- return true;
- }
- if (!(expr instanceof BinaryArithmetic)) {
- return false;
- }
- if (!supportedFunctions.contains(expr.getClass())) {
- return false;
+ protected static List<Expression> simplifyGroupBy(List<Expression>
groupByExpressions) {
+ Set<Expression> distinctGroupBy = new
LinkedHashSet<>(groupByExpressions);
+ Set<Expression> determinants = distinctGroupBy.stream()
+
.filter(Slot.class::isInstance).collect(ImmutableSet.toImmutableSet());
+ // Keep at least one key: removing all constant keys changes the
result for empty input.
+ if (!determinants.isEmpty()) {
+ distinctGroupBy.removeIf(expression -> !(expression instanceof
Slot)
+ && !expression.containsVolatileOrNoneMovableExpression()
+ && !expression.containsNondeterministic()
+ && determinants.containsAll(expression.getInputSlots())
+ && preservesGroupingEquality(expression));
}
+ return distinctGroupBy.size() == groupByExpressions.size() ? null :
ImmutableList.copyOf(distinctGroupBy);
+ }
- // Float/double arithmetic: precision loss for all operations
- if (expr.child(0).getDataType().isFloatLikeType()
- || expr.child(1).getDataType().isFloatLikeType()) {
+ /**
+ * Doris grouping equality merges signed zeros and NaN payloads. A
dependent expression
+ * can be removed only if it maps each such equivalence class to one
grouping result.
+ * Addition, subtraction, multiplication, abs, floating casts, isnan, and
isinf
+ * preserve those classes. Signbit, atan2, pow, and string casts can
distinguish
+ * their members, so unreviewed float-dependent operations stay.
+ */
+ private static boolean preservesGroupingEquality(Expression expression) {
+ Set<Slot> inputSlots = expression.getInputSlots();
+ if (inputSlots.stream().anyMatch(slot ->
!hasExactGroupingEquality(slot)
+ && !slot.getDataType().isFloatLikeType())) {
return false;
}
-
- Expression slotExpr;
- Literal literal;
- if (expr.child(0) instanceof Literal) {
- literal = (Literal) expr.child(0);
- slotExpr = expr.child(1);
- } else if (expr.child(1) instanceof Literal) {
- literal = (Literal) expr.child(1);
- slotExpr = expr.child(0);
- } else {
- return false;
+ if (inputSlots.stream().noneMatch(slot ->
slot.getDataType().isFloatLikeType())) {
+ return true;
}
-
- if (!canExtractSlot(slotExpr)) {
- return false;
+ if (expression instanceof Slot || expression instanceof Literal) {
+ return true;
}
-
- return checkLiteral((BinaryArithmetic) expr, literal);
- }
-
- @VisibleForTesting
- protected static boolean checkLiteral(BinaryArithmetic expr, Literal
literal) {
- if (literal.isNullLiteral()) {
+ if (expression instanceof Cast &&
!expression.getDataType().isFloatLikeType()) {
return false;
}
- if (expr instanceof Multiply || expr instanceof Divide) {
- if (literal.isZero()) {
- return false;
- }
+ if (!(expression instanceof Cast || expression instanceof Add ||
expression instanceof Subtract
+ || expression instanceof Multiply || expression instanceof Abs
Review Comment:
[P1] Preserve the canonicalized value of removed float keys
With `ELIMINATE_GROUP_BY_KEY` disabled, consider `SELECT signbit(v * -1.0),
count(*) FROM t GROUP BY v, v * -1.0`. Before this change the normalized tree
is:
```text
Project(signbit(k), count)
Aggregate(keys=[v, k])
Project(v, v * -1.0 AS k)
```
Every BE aggregation sink calls `replace_float_special_values()` on each
key, so `k = -0` is canonicalized to `+0` and `signbit(k)` is `false`.
Admitting `Multiply` here removes `k`; `NormalizeAggregate` instead builds
`Project(signbit(v * -1.0)) -> Aggregate(keys=[v])`. The aggregate returns
canonical `v = +0`, then the upper project recomputes `-0`, changing the result
to `true` even though the group count is unchanged. The new count-only
safe-arithmetic regression cannot catch this. Please retain float-derived keys
whose canonicalized value is consumed by aggregate outputs (directly or as a
subtree), or otherwise preserve that canonicalized value, and add an
output-sensitive regression.
##########
regression-test/suites/nereids_rules_p0/simplify_agg_group_by/simplify_agg_group_by.groovy:
##########
@@ -0,0 +1,268 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+suite("simplify_agg_group_by") {
+ sql "drop table if exists simplify_agg_group_by_decimal"
+ sql """
+ create table simplify_agg_group_by_decimal (
+ x int not null,
+ y int not null,
+ d decimal(38, 0) not null
+ )
+ duplicate key(x)
+ distributed by hash(x) buckets 1
+ properties("replication_num" = "1")
+ """
+ sql """
+ insert into simplify_agg_group_by_decimal values
+ (1, 10, 99999999999999999999999999999999999999),
+ (2, 20, 1)
+ """
+
+ order_qt_decimal_dependency """
+ select count(*)
+ from simplify_agg_group_by_decimal
+ group by d, d * 10
+ """
+
+ order_qt_decimal_alias_dependency """
+ select count(*)
+ from (select d, d * 10 as risky from simplify_agg_group_by_decimal) t
+ group by d, risky
+ """
+
+ explain {
+ sql """
+ select count(*)
+ from simplify_agg_group_by_decimal
+ group by x, x + 1, x + 2
+ """
+ contains "group by: x[#"
+ notContains " + 1)"
+ notContains " + 2)"
+ }
+
+ explain {
+ sql """
+ select count(*)
+ from simplify_agg_group_by_decimal
+ group by x + 1, x + 2
+ """
+ contains "(cast(x as BIGINT) + 1)"
+ contains "(cast(x as BIGINT) + 2)"
+ notContains "group by: x[#"
+ }
+
+ explain {
+ sql """
+ select count(*)
+ from simplify_agg_group_by_decimal
+ group by x, x + 1, y, y + 1
+ """
+ contains "group by: x[#"
+ contains "y[#"
+ notContains " + 1)"
+ }
+
+ explain {
+ sql """
+ select count(*)
+ from simplify_agg_group_by_decimal
+ group by cast(x as bigint), x + 1
+ """
+ contains "cast(x as BIGINT)"
+ contains " + 1)"
+ }
+
+ explain {
+ sql """
+ select count(*)
+ from simplify_agg_group_by_decimal
+ group by x, y, x + y, abs(x)
+ """
+ contains "group by: x[#"
+ contains "y[#"
+ notContains " + "
+ notContains "abs("
+ }
+
+ order_qt_multiple_input_dependency """
+ select x + y, abs(x), count(*)
+ from simplify_agg_group_by_decimal
+ group by x, y, x + y, abs(x)
+ """
+
+ order_qt_constant_grouping_empty_input """
+ select count(*)
+ from simplify_agg_group_by_decimal
+ where x < 0
+ group by 'constant', 2 + 3
+ """
+
+ order_qt_existing_slot_determinant """
+ select x, count(*)
+ from simplify_agg_group_by_decimal
+ group by x, x + 1, x + 2
+ order by x
+ """
+
+ order_qt_multiple_determinants """
+ select x, y, count(*)
+ from simplify_agg_group_by_decimal
+ group by x, x + 1, y, y + 1
+ order by x, y
+ """
+
+ order_qt_injective_cast_and_dependent_outputs """
+ select cast(x as bigint), x + 1, count(*)
+ from simplify_agg_group_by_decimal
+ group by cast(x as bigint), x + 1
+ order by cast(x as bigint), x + 1
+ """
+
+ explain {
+ sql """
+ select cast(x as bigint), x + 1, count(*)
+ from simplify_agg_group_by_decimal
+ group by cast(x as bigint), x + 1
+ """
+ contains "group by: cast(x as"
+ contains ", x + 1[#"
+ }
+
+ order_qt_injective_cast_dependent_output """
+ select x + 1, count(*)
+ from simplify_agg_group_by_decimal
+ group by cast(x as bigint), x + 1
+ order by x + 1
+ """
+
+ explain {
+ sql """
+ select x + 1, count(*)
+ from simplify_agg_group_by_decimal
+ group by cast(x as bigint), x + 1
+ """
+ contains "group by: cast(x as"
+ contains ", x + 1[#"
+ }
+
+ explain {
+ sql """
+ select count(*) as n
+ from simplify_agg_group_by_decimal
+ group by x / 1000000.0, x / 2000000.0
+ order by n
+ """
+ contains "(cast(x as DECIMALV3(15, 5)) / 1000000.0)"
+ contains "(cast(x as DECIMALV3(15, 5)) / 2000000.0)"
+ notContains "group by: x[#"
+ }
+
+ order_qt_decimal_group_key_collision """
+ select count(*) as n
+ from simplify_agg_group_by_decimal
+ group by x / 1000000.0, x / 2000000.0
+ order by n
+ """
+ order_qt_try_cast_dependency """
+ select try_cast(d as bigint) as k, count(*)
+ from simplify_agg_group_by_decimal
+ group by d, try_cast(d as bigint)
+ order by k
+ """
+
+ explain {
+ sql """
+ select count(*)
+ from simplify_agg_group_by_decimal
+ group by x, cast(x as string)
+ """
+ contains "group by: x[#"
+ notContains "cast(x as TEXT)"
+ }
+
+ // Non-movable grouping functions must still run, even if a bare slot
determines them.
+ test {
+ sql """
+ select count(*)
+ from simplify_agg_group_by_decimal
+ group by x, assert_true(x > 1, 'bad')
+ """
+ exception "bad"
+ }
+
+ // score() is rejected by a later validator; simplification must not erase
it first.
+ test {
+ sql """
+ select count(*)
+ from simplify_agg_group_by_decimal
+ group by x, score()
+ """
+ exception "score() function requires WHERE clause with MATCH function"
+ }
+
+ sql "drop table if exists simplify_agg_group_by_signed_zero"
+ sql """
+ create table simplify_agg_group_by_signed_zero (
+ id int not null,
+ v double not null
+ )
+ duplicate key(id)
+ distributed by hash(id) buckets 1
+ properties("replication_num" = "1")
+ """
+ sql "insert into simplify_agg_group_by_signed_zero values (1, cast('+0.0'
as double)), (2, cast('-0.0' as double))"
+ sql "set disable_nereids_rules='ELIMINATE_GROUP_BY_KEY'"
Review Comment:
[P1] Exercise the default FD rewrite in these regressions
Disabling `ELIMINATE_GROUP_BY_KEY` masks a second production path that
bypasses the new guard. With the default rules, normalization produces:
```text
Project(s, count)
Aggregate(keys=[v, s])
Project(v, signbit(v) AS s)
```
`LogicalProject.computeFd` records `{v} -> {s}`, and the later default
`EliminateGroupByKey` removes `s` (wrapping it in `any_value` when selected)
without consulting `preservesGroupingEquality`. The remaining `v` key
canonicalizes `+0/-0` together, so the expected two count-1 groups become one
count-2 group. `atan2`, `pow`, and the string cast take the same path. This is
distinct from the existing analyzer-rule thread, which explicitly assumed this
rule was disabled. Please make the projected-expression FD/elimination
grouping-equality-safe and keep a default-rule regression that proves the final
plan retains these keys.
--
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]
