github-actions[bot] commented on code in PR #67878:
URL: https://github.com/apache/doris/pull/67878#discussion_r4004917827
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/ExpressionUtils.java:
##########
@@ -1205,9 +1205,63 @@ public static boolean isInjective(Expression expression)
{
return expression instanceof Slot;
}
- // if the input is unique, the output of agg is unique, too
+ private static boolean isInjectiveAggArgument(Expression expression) {
+ if (expression instanceof Slot) {
+ return true;
+ }
+ if (!(expression instanceof Cast)) {
+ return false;
Review Comment:
[P2] Recognize no-op bounded-character wrappers
The parser represents every bounded `CAST(k AS VARCHAR(2))` as
`substring(cast(k AS VARCHAR(2)), 1, 2)`, so this branch rejects it before the
cast proof below. For a unique non-null `VARCHAR(1)` source, that entire
wrapper is injective: every admitted source value fits the wider target and the
substring cannot remove anything. In the retained two-aggregate determinant
tree shown in the review summary, the baseline unique trait supplied `m -> g`;
current head keeps the redundant outer grouping key and its hash/shuffle work.
Please recognize parser-generated no-op truncation wrappers (or simplify them
using the source bound) and add a positive retained-plan case.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/ExpressionUtils.java:
##########
@@ -1205,9 +1205,63 @@ public static boolean isInjective(Expression expression)
{
return expression instanceof Slot;
}
- // if the input is unique, the output of agg is unique, too
+ private static boolean isInjectiveAggArgument(Expression expression) {
+ if (expression instanceof Slot) {
+ return true;
+ }
+ if (!(expression instanceof Cast)) {
+ return false;
+ }
+ Cast cast = (Cast) expression;
+ DataType source = cast.child().getDataType();
+ DataType target = cast.getDataType();
+ // Bounded character casts may truncate to the declared length. Treat
every non-identity
+ // conversion to CHAR/VARCHAR conservatively, including
analyzer-generated casts.
+ return (source.equals(target) || (!target.isCharType() &&
!target.isVarcharType()))
+ && !Cast.castNullable(false, source, target)
+ && isInjectiveTypeConversion(source, target)
Review Comment:
[P1] Reject ambiguous complex-to-string casts
This accepts `ARRAY<STRING> -> STRING`, but that cast is not injective at
runtime. `CheckCast` allows it, `Cast.castNullable(false, ARRAY<STRING>,
STRING)` is false, and `ArrayType.isInjectiveCastTo` returns true for every
character target. BE formats arrays by joining elements with `, ` and quotes
nested raw string bytes without escaping them, so the distinct values
`ARRAY('a", "b')` and `ARRAY('a','b')` both become `["a", "b"]`.
```text
Aggregate(groupBy=[m, g], output=[m, group_concat(g)])
Aggregate(groupBy=[arr], output=[max(cast(arr AS string)) AS m,
group_concat(tag) AS g])
Aggregate(groupBy=[arr], output=[arr, any_value(tag) AS tag])
```
The lowest aggregate makes `arr` unique; this line then falsely marks `m`
unique, derives `m -> g`, and lets `EliminateGroupByKey` merge groups that
should remain distinct. Please reject complex-to-character conversions unless
their concrete serialization is proven collision-free, and add an end-to-end
regression with the colliding arrays.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/ExpressionUtils.java:
##########
@@ -1205,9 +1205,63 @@ public static boolean isInjective(Expression expression)
{
return expression instanceof Slot;
}
- // if the input is unique, the output of agg is unique, too
+ private static boolean isInjectiveAggArgument(Expression expression) {
+ if (expression instanceof Slot) {
+ return true;
+ }
+ if (!(expression instanceof Cast)) {
+ return false;
+ }
+ Cast cast = (Cast) expression;
+ DataType source = cast.child().getDataType();
+ DataType target = cast.getDataType();
+ // Bounded character casts may truncate to the declared length. Treat
every non-identity
+ // conversion to CHAR/VARCHAR conservatively, including
analyzer-generated casts.
+ return (source.equals(target) || (!target.isCharType() &&
!target.isVarcharType()))
+ && !Cast.castNullable(false, source, target)
+ && isInjectiveTypeConversion(source, target)
+ && isInjectiveAggArgument(cast.child());
+ }
+
+ /**
+ * 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. Prove injectivity through both the argument
expression and the
+ * one-row argument-to-result type 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 isInjectiveAggArgument(argument)
+ && isInjectiveTypeConversion(argument.getDataType(),
agg.getDataType());
+ }
+
+ /**
+ * Whether a type conversion preserves every source value. Data types
provide the general
+ * proof; the floating-point cases below supplement it with the exact
integer ranges of IEEE
+ * 754 binary32 and binary64.
+ */
+ private static boolean isInjectiveTypeConversion(DataType source, DataType
target) {
+ if (source.isInjectiveCastTo(target)) {
Review Comment:
[P2] Cover exact cross-family scalar widenings
This delegation misses legal conversions whose source types inherit the
default equality-only proof. Both `DATEV2 -> DATETIMEV2(0)` and `IPV4 -> IPV6`
are total and reversible: BE shifts the complete DATEV2 encoding into a
midnight DATETIMEV2 value, and writes a fixed IPv4-mapped prefix plus all 32
IPv4 bits. Both casts remain non-null, and `MIN`/`MAX` return the target type
unchanged. In the retained determinant tree shown in the summary, baseline
produced the valid `m -> g` FD; current head retains redundant outer grouping
work. Please audit scalar widening pairs used by this generic proof, cover at
least these two, and add positive retained-plan tests.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/ExpressionUtils.java:
##########
@@ -1205,9 +1205,63 @@ public static boolean isInjective(Expression expression)
{
return expression instanceof Slot;
}
- // if the input is unique, the output of agg is unique, too
+ private static boolean isInjectiveAggArgument(Expression expression) {
+ if (expression instanceof Slot) {
+ return true;
+ }
+ if (!(expression instanceof Cast)) {
+ return false;
+ }
+ Cast cast = (Cast) expression;
+ DataType source = cast.child().getDataType();
+ DataType target = cast.getDataType();
+ // Bounded character casts may truncate to the declared length. Treat
every non-identity
+ // conversion to CHAR/VARCHAR conservatively, including
analyzer-generated casts.
+ return (source.equals(target) || (!target.isCharType() &&
!target.isVarcharType()))
+ && !Cast.castNullable(false, source, target)
Review Comment:
[P2] Do not lose temporal scale widenings to coarse nullability
`Cast.castNullable` marks every nonidentity datetime-to-datetime conversion
nullable, so this rejects `MAX(CAST(k AS DATETIMEV2(6)))` for a unique non-null
`DATETIMEV2(0)` key. That widening is total and injective:
`DateTimeV2Type.isInjectiveCastTo` proves the scale relation, and BE's
`transform_date_scale` simply copies and succeeds when target scale is at least
source scale. In the window tree shown in the summary, the baseline unique
trait let `SimplifyWindowExpression` replace `row_number() OVER (PARTITION BY
m)` with 1; current head retains the window. Please recognize same-family scale
widenings while keeping reductions and cross-family casts conservative, with a
positive plan-level test.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/ExpressionUtils.java:
##########
@@ -1205,9 +1205,63 @@ public static boolean isInjective(Expression expression)
{
return expression instanceof Slot;
}
- // if the input is unique, the output of agg is unique, too
+ private static boolean isInjectiveAggArgument(Expression expression) {
+ if (expression instanceof Slot) {
+ return true;
+ }
+ if (!(expression instanceof Cast)) {
+ return false;
+ }
+ Cast cast = (Cast) expression;
+ DataType source = cast.child().getDataType();
+ DataType target = cast.getDataType();
+ // Bounded character casts may truncate to the declared length. Treat
every non-identity
+ // conversion to CHAR/VARCHAR conservatively, including
analyzer-generated casts.
+ return (source.equals(target) || (!target.isCharType() &&
!target.isVarcharType()))
+ && !Cast.castNullable(false, source, target)
+ && isInjectiveTypeConversion(source, target)
+ && isInjectiveAggArgument(cast.child());
+ }
+
+ /**
+ * 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. Prove injectivity through both the argument
expression and the
+ * one-row argument-to-result type 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 isInjectiveAggArgument(argument)
+ && isInjectiveTypeConversion(argument.getDataType(),
agg.getDataType());
+ }
+
+ /**
+ * Whether a type conversion preserves every source value. Data types
provide the general
+ * proof; the floating-point cases below supplement it with the exact
integer ranges of IEEE
+ * 754 binary32 and binary64.
+ */
+ private static boolean isInjectiveTypeConversion(DataType source, DataType
target) {
+ if (source.isInjectiveCastTo(target)) {
+ return true;
+ }
+ if (source.isIntegralType()) {
+ if (target.isFloatType()) {
+ return source.width() <= Short.BYTES;
+ }
+ if (target.isDoubleType()) {
+ return source.width() <= Integer.BYTES;
+ }
+ return false;
Review Comment:
[P2] Preserve full-domain LARGEINT to Decimal256 casts
With `enable_decimal256=true`, `DECIMAL(39,0)` contains every signed
LARGEINT value, and the cast is exact and non-null-producing;
`Cast.castNullable` already reaches the same conclusion from the 39-digit
range. `IntegralType.isInjectiveCastTo`, however, excludes every
LARGEINT-to-DecimalV3 conversion, so this path removes the unique trait from
safe `MAX`/`MIN(CAST(k AS DECIMAL(39,0)))`. In the retained determinant tree
shown in the summary, baseline supplied `m -> g`; current head retains
redundant grouping/hash work. Please accept Decimal256 targets with enough
integer digits and add boundary coverage for 38 versus 39 digits.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/ExpressionUtils.java:
##########
@@ -1205,9 +1205,63 @@ public static boolean isInjective(Expression expression)
{
return expression instanceof Slot;
}
- // if the input is unique, the output of agg is unique, too
+ private static boolean isInjectiveAggArgument(Expression expression) {
+ if (expression instanceof Slot) {
+ return true;
+ }
+ if (!(expression instanceof Cast)) {
+ return false;
+ }
+ Cast cast = (Cast) expression;
+ DataType source = cast.child().getDataType();
+ DataType target = cast.getDataType();
+ // Bounded character casts may truncate to the declared length. Treat
every non-identity
+ // conversion to CHAR/VARCHAR conservatively, including
analyzer-generated casts.
+ return (source.equals(target) || (!target.isCharType() &&
!target.isVarcharType()))
+ && !Cast.castNullable(false, source, target)
+ && isInjectiveTypeConversion(source, target)
+ && isInjectiveAggArgument(cast.child());
+ }
+
+ /**
+ * 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. Prove injectivity through both the argument
expression and the
+ * one-row argument-to-result type 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 isInjectiveAggArgument(argument)
+ && isInjectiveTypeConversion(argument.getDataType(),
agg.getDataType());
Review Comment:
[P2] Preserve the original domain through widening casts
```text
Aggregate(groupBy=[a, g], output=[a, group_concat(g)])
Aggregate(groupBy=[id], output=[avg(cast(id AS bigint)) AS a,
group_concat(v) AS g])
Scan(unique non-null INT id)
```
`isInjectiveAggArgument` proves `INT -> BIGINT`, but this line then tests
the entire `BIGINT -> DOUBLE` domain and rejects it. The actual BIGINT values
all originated as INT, and every INT is exactly representable as DOUBLE, so the
composed one-row AVG mapping is injective. Losing `a` also loses `a -> g`,
retaining the redundant outer key and hash/shuffle work; the new negative
assertion codifies that regression. Please carry the original slot domain
through proven widening casts and cover the retained outer-plan shape.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/ExpressionUtils.java:
##########
@@ -1205,9 +1205,63 @@ public static boolean isInjective(Expression expression)
{
return expression instanceof Slot;
}
- // if the input is unique, the output of agg is unique, too
+ private static boolean isInjectiveAggArgument(Expression expression) {
+ if (expression instanceof Slot) {
+ return true;
+ }
+ if (!(expression instanceof Cast)) {
+ return false;
+ }
+ Cast cast = (Cast) expression;
+ DataType source = cast.child().getDataType();
+ DataType target = cast.getDataType();
+ // Bounded character casts may truncate to the declared length. Treat
every non-identity
+ // conversion to CHAR/VARCHAR conservatively, including
analyzer-generated casts.
+ return (source.equals(target) || (!target.isCharType() &&
!target.isVarcharType()))
+ && !Cast.castNullable(false, source, target)
+ && isInjectiveTypeConversion(source, target)
+ && isInjectiveAggArgument(cast.child());
+ }
+
+ /**
+ * 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. Prove injectivity through both the argument
expression and the
+ * one-row argument-to-result type 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 isInjectiveAggArgument(argument)
+ && isInjectiveTypeConversion(argument.getDataType(),
agg.getDataType());
+ }
+
+ /**
+ * Whether a type conversion preserves every source value. Data types
provide the general
+ * proof; the floating-point cases below supplement it with the exact
integer ranges of IEEE
+ * 754 binary32 and binary64.
+ */
+ private static boolean isInjectiveTypeConversion(DataType source, DataType
target) {
+ if (source.isInjectiveCastTo(target)) {
+ return true;
+ }
+ if (source.isIntegralType()) {
Review Comment:
[P2] Include exact integral-to-DecimalV2 casts
This integral branch never recognizes DecimalV2 targets. Explicit `CAST(k AS
DECIMALV2(19,0))` is a total exact embedding of a BIGINT key: the target has
all 19 required integer digits, `Cast.castNullable` agrees it cannot overflow,
and BE's scale-zero integer-to-DecimalV2 path stores the value exactly.
`MIN`/`MAX` may add the normal exact DecimalV2-to-V3 signature cast, but
recursion still rejects this inner conversion. In the retained determinant tree
from the summary, current head loses `m -> g` and leaves avoidable grouping
work. Please add the same range-based proof for DecimalV2 targets and
positive/negative boundary coverage.
--
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]
