github-actions[bot] commented on code in PR #67878:
URL: https://github.com/apache/doris/pull/67878#discussion_r4002124934
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/ExpressionUtils.java:
##########
@@ -1205,9 +1207,66 @@ public static boolean isInjective(Expression expression)
{
return expression instanceof Slot;
}
- // if the input is unique, the output of agg is unique, too
+ /**
+ * Whether an aggregate preserves uniqueness for a group containing
exactly one row.
+ *
+ * <p>Checking the aggregate kind and its input slots is not sufficient.
An aggregate argument
+ * may contain a non-injective expression, and the aggregate return type
may also collapse
+ * distinct argument values. Keep this proof deliberately narrow: the
argument must be a bare
+ * slot, and the one-row result conversion must be a proven lossless
numeric conversion.
+ */
public static boolean isInjectiveAgg(Expression agg) {
- return agg instanceof Sum || agg instanceof Avg || agg instanceof Max
|| agg instanceof Min;
+ if (!(agg instanceof Sum || agg instanceof Avg || agg instanceof Max
|| agg instanceof Min)) {
+ return false;
+ }
+ Expression argument = agg.child(0);
+ return argument instanceof Slot
+ &&
isProvablyInjectiveNumericConversion(argument.getDataType(), agg.getDataType());
+ }
+
+ /**
+ * A deliberately small whitelist of numeric conversions that preserve
every source value.
+ * Do not use the general cast compatibility predicates here: some
conversions accepted by
+ * them can truncate values (for example, casting an integer to a bounded
character type).
+ */
+ private static boolean isProvablyInjectiveNumericConversion(DataType
source, DataType target) {
Review Comment:
[P2] Preserve exact-type nonnumeric MIN/MAX injectivity
This numeric check runs before exact type equality, so a bare `MAX`/`MIN` of
a unique non-null `VARCHAR` (similarly temporal or boolean inputs) loses a
valid unique trait even though `Max.customSignature()`/`Min.customSignature()`
return that type unchanged. A reachable retained shape is:
```text
Aggregate(groupBy=[m, g], output=[m, group_concat(g)])
Aggregate(groupBy=[k], output=[max(k) AS m, group_concat(v) AS g])
Scan(unique non-null VARCHAR k)
```
`group_concat` keeps both aggregates live. Previously `m` was unique, so
`Plan.computeDataTrait` supplied `m -> g` and `EliminateGroupByKey` could drop
the redundant `g` key; this early return prevents that optimization. Please
handle `source.equals(target)` before the numeric-family restriction (or
special-case exact-type `MAX`/`MIN`) and add a positive trait/shape test for
this path.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/ExpressionUtils.java:
##########
@@ -1205,9 +1207,66 @@ public static boolean isInjective(Expression expression)
{
return expression instanceof Slot;
}
- // if the input is unique, the output of agg is unique, too
+ /**
+ * Whether an aggregate preserves uniqueness for a group containing
exactly one row.
+ *
+ * <p>Checking the aggregate kind and its input slots is not sufficient.
An aggregate argument
+ * may contain a non-injective expression, and the aggregate return type
may also collapse
+ * distinct argument values. Keep this proof deliberately narrow: the
argument must be a bare
+ * slot, and the one-row result conversion must be a proven lossless
numeric conversion.
+ */
public static boolean isInjectiveAgg(Expression agg) {
- return agg instanceof Sum || agg instanceof Avg || agg instanceof Max
|| agg instanceof Min;
+ if (!(agg instanceof Sum || agg instanceof Avg || agg instanceof Max
|| agg instanceof Min)) {
+ return false;
+ }
+ Expression argument = agg.child(0);
Review Comment:
[P2] Preserve analyzer-inserted injective casts around slots
This checks the bound argument shape, so bare-slot SQL such as
`SUM(boolean_key)` and `MAX(decimalv2_key)` is rejected after analysis: their
signatures introduce a non-explicit BOOLEAN-to-TINYINT or same-range/scale
DecimalV2-to-V3 `Cast`, and `NormalizeAggregate` leaves that wrapper around the
slot. Both mappings, and the subsequent one-row aggregate result conversions,
are injective. In a two-level aggregate retained by a live `group_concat`,
losing the inner aggregate output's unique trait also removes its determinant
FD, so `EliminateGroupByKey` retains a redundant outer grouping key. Please
distinguish analyzer-inserted casts from explicit SQL casts and prove both
original-slot-to-bound-argument and argument-to-result conversions; add direct
analyzed-trait/shape coverage for these coerced bare-slot cases.
--
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]
