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:
##########
@@ -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)).
+ if (!column->has_null(std::max<int64_t>(frame_start - 1,
partition_start),
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.
--
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]
