github-actions[bot] commented on code in PR #66644:
URL: https://github.com/apache/doris/pull/66644#discussion_r3861186568
##########
be/src/exprs/aggregate/aggregate_function_null.h:
##########
@@ -503,7 +508,11 @@ class AggregateFunctionNullUnaryInline final
assert_cast<const ColumnNullable*,
TypeCheckOnRelease::DISABLE>(columns[0]);
const IColumn* nested_column = &column->get_nested_column();
- if (!column->has_null()) {
+ // Scan only [frame_start-1, frame_end) covering the incremental step's
+ // outgoing (frame_start-1) and incoming (frame_end-1) positions,
instead
+ // of has_null() over the whole buffered column (avoids O(n * buffer)).
Review Comment:
[P1] Clamp this scan to physically retained rows
The streaming analytic sink periodically erases old blocks and rebases
`BoundaryPose::start` by subtraction, so an active long partition can have a
negative logical `partition_start` even though the retained column starts at
physical index 0. For a PRECEDING frame wider than the retained prefix, this
`max(...)` is still negative and is implicitly converted to `size_t`;
`ColumnNullable::has_null(begin,end)` then performs unchecked `null_map.data()
+ begin`, causing an out-of-bounds scan/crash. The old whole-column check was
safe for this case, and the nested incremental functions already skip outgoing
logical positions before `partition_start`. Please intersect the scan with
physical `[0, column->size())` bounds and add a
streaming-removal/long-PRECEDING test.
##########
be/src/exprs/aggregate/aggregate_function_null.h:
##########
@@ -465,7 +465,12 @@ class AggregateFunctionNullUnaryInline final
}
const auto* column =
assert_cast<const ColumnNullable*,
TypeCheckOnRelease::DISABLE>(columns[0]);
- bool has_null = column->has_null();
+ // Only scan the frame range for nulls (O(frame)), not the whole
buffered
+ // column (O(n)). Scanning the whole column per row makes the analytic
+ // sliding-window path O(n * buffer) when nulls are sparse/absent.
+ bool has_null = current_frame_start < current_frame_end
Review Comment:
[P1] Apply the bounded scan to the default V2 wrapper too
Normal SQL execution enables `enable_aggregate_function_null_v2` by default,
and `AggFnEvaluator` forwards that setting so unary nullable
`sum`/`avg`/`stddev` instantiate `AggregateFunctionNullUnaryInlineV2`. Its
matching window methods still call whole-column `has_null()`
(`aggregate_function_null_v2.h:535,573`), so the advertised/default query path
keeps the O(n * buffer) scan and only sessions that disable V2 benefit here.
The new helper also leaves the V2 attribute at its local default `false`, so
all added tests cover V1 only and would pass before this optimization. Please
mirror or share the bounded logic in V2 and exercise the default-V2 path.
--
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]
