github-actions[bot] commented on code in PR #67737:
URL: https://github.com/apache/doris/pull/67737#discussion_r4061073220
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/AggregateStrategies.java:
##########
@@ -809,6 +780,33 @@ private LogicalAggregate<? extends Plan>
storageLayerAggregate(
}
}
+ private boolean isSupportedStorageLayerAggregateArgument(
+ AggregateFunction aggregateFunction, Expression argument) {
+ if (argument instanceof SlotReference) {
+ return true;
+ }
+ if (!isNullPreservingCastOverSlot(argument)) {
Review Comment:
[P1] Reject nested casts that can still fail
With strict casting this admitted plan is not equivalent:
```text
LogicalAggregate(count(CAST(a AS ARRAY<INT>)))
LogicalOlapScan(a ARRAY<STRING> NOT NULL; row ['bad'])
```
`Cast.castNullable(false, ARRAY<STRING>, ARRAY<INT>)` returns false because
it does not recurse, so this COUNT branch accepts the cast. Normal BE array
casting propagates the nested string-to-int error (and TRY_CAST can produce a
top-level NULL). After storage COUNT, OLAP/FileScannerV2 counts `a` and emits
empty/default arrays, so the upper cast succeeds and returns the source count
instead of the required error/zero. Please reject complex casts unless nested
conversions are recursively proven null- and error-preserving, and cover
direct/projected runtime paths.
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/rules/rewrite/PhysicalStorageLayerAggregateTest.java:
##########
@@ -402,6 +438,122 @@ public void
testCountOnIndexRejectsIsNullOnProjectedCountSlot() {
.matches(logicalAggregate(logicalProject(logicalFilter(logicalOlapScan()))));
}
+ @Test
+ public void testCastThatMayProduceNullDoesNotUseStorageLayerAggregate() {
+ LogicalOlapScan olapScan = PlanConstructor.newLogicalOlapScan(2,
"cast_aggregate", 0);
+ Cast cast = new Cast(olapScan.getOutput().get(0),
TinyIntType.INSTANCE);
+ TryCast tryCast = new TryCast(olapScan.getOutput().get(0),
TinyIntType.INSTANCE);
+ Cast stringCast = new Cast(olapScan.getOutput().get(1),
TinyIntType.INSTANCE);
+ TryCast stringTryCast = new TryCast(olapScan.getOutput().get(1),
TinyIntType.INSTANCE);
+ List<AggregateFunction> castAggregates = ImmutableList.of(
+ new Count(cast), new Count(tryCast), new Min(cast), new
Max(tryCast),
+ new Count(stringCast), new Count(stringTryCast));
+
+ for (AggregateFunction function : castAggregates) {
+ LogicalAggregate<LogicalOlapScan> aggregate = new
LogicalAggregate<>(
+ Collections.emptyList(), ImmutableList.of(new
Alias(function, "aggregate")),
+ true, Optional.empty(), olapScan);
Review Comment:
[P2] Assert that the physical alternative is absent
`applyImplementation` copies a transformed alternative into the memo without
removing the original logical expression, and `matches(...)` searches both
logical and physical alternatives. This assertion therefore succeeds even if
the rule incorrectly adds `PhysicalStorageLayerAggregate`. The same issue
occurs in the negative checks around current lines 516 and 534. Please use
`nonMatch(physicalStorageLayerAggregate())` (as the file-scan negative case
does), optionally retaining the logical-shape match as a separate assertion.
--
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]
