github-actions[bot] commented on code in PR #68134:
URL: https://github.com/apache/doris/pull/68134#discussion_r4036015004
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/types/MapType.java:
##########
@@ -70,7 +69,9 @@ public boolean isInjectiveCastTo(DataType target) {
MapType mapType = (MapType) target;
return keyType.isInjectiveCastTo(mapType.keyType) &&
valueType.isInjectiveCastTo(mapType.valueType);
Review Comment:
**[P1] Keep normalizing MAP casts out of the injective set**
`MapType` cannot derive injectivity solely from its key/value types. For a
non-identity MAP cast, BE's `create_map_wrapper` unconditionally calls
`deduplicate_keys()`. A source map `[(1,"a"),(1,"b")]` is distinct from
`[(1,"b")]` under `ColumnMap` hashing/comparison, but `MAP<INT,STRING> ->
MAP<INT,VARBINARY>` (now accepted through STRING -> VARBINARY) converts both to
`[(1,"b")]`. `PushProjectThroughUnion` can therefore move this cast below
`UNION DISTINCT` and change two output rows into one. Please keep non-identity
MAP casts unsafe (including MAPs nested in ARRAY/STRUCT), or otherwise account
for the BE normalization, and add a duplicate-key UNION regression.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/types/DecimalV2Type.java:
##########
@@ -170,6 +171,20 @@ public boolean isInjectiveCastTo(DataType target) {
DecimalV3Type decimalV3Type = (DecimalV3Type) target;
return decimalV3Type.getRange() >= this.getRange() &&
decimalV3Type.getScale() >= this.getScale();
}
+ // An integral target discards the fractional part, so it is injective
only for scale zero.
+ // The strict range bound also leaves room for the asymmetric negative
endpoint of a signed
+ // integer. Binary floating-point casts round, but adjacent values in
a DECIMAL(P, S) domain
+ // remain distinguishable when P <= 7 for FLOAT or P <= 15 for DOUBLE.
Those conservative
+ // decimal-digit limits hold for every scale, not just for integral
decimals.
+ if (scale == 0 && target instanceof IntegralType) {
Review Comment:
**[P1] Use DECIMALV2's effective execution domain**
Declared `scale`/`precision` are not execution-domain invariants for
DECIMALV2. BE normalizes every such expression to `(27,9)`: `CAST(DOUBLE
1.1/1.2 AS DECIMALV2(7,0))` retains two scale-nine values, but this new
DECIMALV2-to-BIGINT classification maps both to `1`. Pushing that cast below
`UNION DISTINCT` changes two rows to one. The same mismatch invalidates the new
`IntegralType` target branch: BIGINT -> DECIMALV2(19,0) is marked safe from 19
declared integer digits, but the effective type has only 18, so `10^18` and
`10^18+1` from nullable BIGINT slots both become NULL in non-strict mode.
Please keep the new DECIMALV2 source/target routes unsafe unless BE enforces
the declared domain, and add runtime DISTINCT regressions in both directions.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/types/DecimalV3Type.java:
##########
@@ -225,6 +226,20 @@ public boolean isInjectiveCastTo(DataType target) {
DecimalV3Type decimalV3Type = (DecimalV3Type) target;
return decimalV3Type.getRange() >= this.getRange() &&
decimalV3Type.getScale() >= this.getScale();
}
+ // An integral target discards the fractional part, so it is injective
only for scale zero.
+ // The strict range bound also leaves room for the asymmetric negative
endpoint of a signed
+ // integer. Binary floating-point casts round, but adjacent values in
a DECIMAL(P, S) domain
+ // remain distinguishable when P <= 7 for FLOAT or P <= 15 for DOUBLE.
Those conservative
+ // decimal-digit limits hold for every scale, not just for integral
decimals.
+ if (scale == 0 && target instanceof IntegralType) {
Review Comment:
**[P1] Do not expose overflowing group expressions as injective**
`true` is correct for this cast alone, but it newly exposes an unsafe
`SimplifyAggGroupBy` path. That rule peels every cast accepted here and assumes
the surrounding arithmetic is injective without checking overflow. For `x
DECIMAL(18,0)`, keys `CAST(x AS BIGINT) * 1024` and `CAST(x AS BIGINT) * 2048`
are accepted; rows `x=0` and `x=2^54` both produce `(0,0)` after BE's unchecked
BIGINT multiplication. The original plan has one group, while the rewrite
groups by bare `x` and produces two. Please make the aggregate rule prove the
complete arithmetic expression safe (or conservatively fence these newly
admitted casts); the same common fix and regression coverage are needed for
DecimalV2 and the newly accepted temporal-to-integral casts.
--
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]
