github-actions[bot] commented on code in PR #67980:
URL: https://github.com/apache/doris/pull/67980#discussion_r4011310152
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/AggregateStrategies.java:
##########
@@ -693,6 +693,15 @@ private LogicalAggregate<? extends Plan>
storageLayerAggregate(
return canNotPush;
}
+ // File MIN/MAX retains only source endpoints. A cast can turn both
endpoints into NULL
+ // while an interior value remains valid. Ignore source nullability
when checking the cast
+ // itself, so safe widening casts over nullable columns can still use
metadata.
+ if (logicalScan instanceof LogicalFileScan &&
argumentsOfAggregateFunction.stream()
+ .anyMatch(argument -> argument instanceof Cast
+ && Cast.castNullable(false,
argument.child(0).getDataType(), argument.getDataType()))) {
Review Comment:
**[P1] Require endpoint preservation, not only non-nullability**
`Cast.castNullable(false, DOUBLE, FLOAT)` is false, so this still pushes
file `MAX(CAST(v AS FLOAT))`. In a single Parquet row group physically ordered
`+0.0, -1e-320`, row scanning casts to `+0.0f, -0.0f` and MAX retains `+0.0f`,
but metadata emits source min then max, so the upper cast/aggregate sees
`-0.0f, +0.0f` and retains `-0.0f`; `signbit(MAX(...))` changes.
`SetPreAggStatus` already rejects this cast class for exactly this
non-homomorphism. Please admit only casts proven to preserve the exact MIN/MAX
representative (or at least reject DOUBLE/DECIMAL-to-FLOAT) and add a
signed-zero regression.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/AggregateStrategies.java:
##########
@@ -693,6 +693,15 @@ private LogicalAggregate<? extends Plan>
storageLayerAggregate(
return canNotPush;
}
+ // File MIN/MAX retains only source endpoints. A cast can turn both
endpoints into NULL
+ // while an interior value remains valid. Ignore source nullability
when checking the cast
+ // itself, so safe widening casts over nullable columns can still use
metadata.
+ if (logicalScan instanceof LogicalFileScan &&
argumentsOfAggregateFunction.stream()
Review Comment:
**[P1] Apply the unsafe-cast guard to OLAP MIN/MAX too**
This helper also accepts `LogicalOlapScan`, whose `TPushAggOp.MINMAX` path
reduces each eligible segment to the two source zone-map endpoints before the
upper cast runs. With a DUP_KEYS BIGINT segment containing `-2147483649`, `0`,
and `2147483648`, this file-only condition still lets `MIN/MAX(CAST(value AS
INT))` push down: the interior `0` is discarded and both retained endpoints
cast to NULL, so the result is NULL instead of `0`. Please gate the OLAP path
as well (and add a direct/projected OLAP regression), or point to an
OLAP-specific guard that prevents this reduction.
--
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]
