morrySnow commented on code in PR #67878:
URL: https://github.com/apache/doris/pull/67878#discussion_r4004554929
##########
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:
Fixed in cbfe4e7eaa9. Aggregate argument injectivity now recurses through
both analyzer-inserted and explicit Cast nodes, requires an injective type
conversion that keeps non-null input non-null, and still proves the bound
argument-to-aggregate-result conversion. UniqueTest now covers the analyzed
BOOLEAN-to-TINYINT SUM and DecimalV2-to-V3 MAX cases.
##########
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:
Fixed in cbfe4e7eaa9. Exact source/target type equality is handled before
the numeric range cases, so same-type VARCHAR and other nonnumeric MIN/MAX
results preserve uniqueness. The tests include a positive MAX(VARCHAR)
retained-plan case, while bounded CHAR/VARCHAR casts remain conservative
because they can truncate.
--
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]
