github-actions[bot] commented on code in PR #65846:
URL: https://github.com/apache/doris/pull/65846#discussion_r3711828285
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/SetPreAggStatus.java:
##########
@@ -294,6 +405,41 @@ private PreAggStatus createPreAggStatus(LogicalOlapScan
logicalOlapScan, PreAggI
return PreAggStatus.off(String.format("Join conjuncts %s
contains non-key column %s",
joinConjuncts, joinInputSlots));
}
+
+ // Row-stability check: volatile expressions evaluated per partial
row
+ // produce different results than per merged logical row, even when
+ // their input slots are all key columns or empty. Check centrally
+ // before per-scan candidate filtering so the guard also covers
+ // other-table aggregates, slot-less filters, joins, and grouping.
+ for (AggregateFunction aggFunc : aggregateFuncs) {
+ if (aggFunc.containsVolatileExpression()) {
Review Comment:
Thanks—the new retained-expression loop addresses the concrete `v7` producer
above. One instance of the same row-cardinality issue still remains: a retained
output such as `assert_true(random() < 0.5, 'bad') AS checked` has no input
slots, so the `retained.getInputSlots()`/`valueSlots` intersection never
associates it with this scan. The later volatility loops inspect aggregate,
filter, join, and grouping expressions, but not retained outputs. With
duplicate full keys, OFF evaluates this retained assertion once after storage
merge, while ON evaluates it once per partial row, changing whether the query
errors. Please check retained producers for volatility before slot-based
attribution, conservatively fence affected scans when a producer is slotless,
and add a duplicate-full-key EXPLAIN regression verifying that the scan stays
OFF.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/SetPreAggStatus.java:
##########
@@ -326,28 +340,49 @@ private PreAggStatus
checkAggregateFunctions(Set<AggregateFunction> aggregateFun
}
PreAggStatus preAggStatus = PreAggStatus.on();
for (AggregateFunction aggFunc : aggregateFuncs) {
- if (aggFunc.children().isEmpty()) {
+ Set<Slot> aggSlots = aggFunc.getInputSlots();
+ if (aggSlots.isEmpty()) {
preAggStatus = PreAggStatus.off(
String.format("can't turn preAgg on for aggregate
function %s", aggFunc));
- } else if (aggFunc.children().size() == 1 && aggFunc.child(0)
instanceof Slot) {
- Slot aggSlot = (Slot) aggFunc.child(0);
- if (aggSlot instanceof SlotReference
- && ((SlotReference)
aggSlot).getOriginalColumn().isPresent()) {
- if (((SlotReference)
aggSlot).getOriginalColumn().get().isKey()) {
- preAggStatus =
OneKeySlotAggChecker.INSTANCE.check(aggFunc);
+ } else {
+ Pair<Set<SlotReference>, Set<SlotReference>> splitSlots =
splitKeyValueSlots(aggSlots);
Review Comment:
The ownership checks now look scan-relative, but the requested result
regression still does not exercise duplicated storage rows. The full aggregate
key of `preagg_t1` and `preagg_t2` is `(k1, k2, k3, k4, k5, k6)`; rows sharing
`k1` differ in `k6`, and neither table has an exact repeated full key. PREAGG
ON and OFF therefore expose the same rows, so the scalar results cannot catch
this bug even though the EXPLAIN assertions can. In `test_b`, `a = abs(k1)` is
also always positive for the current data, so the `r.v7` ELSE branch is
unreachable. Please use dedicated tables with an identical full key inserted in
separate rowsets and data that takes both branches, retaining the EXPLAIN
checks plus a result that changes under faulty ON.
--
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]
