github-actions[bot] commented on code in PR #67169:
URL: https://github.com/apache/doris/pull/67169#discussion_r3862259280
##########
be/src/exprs/aggregate/aggregate_function_null_v2.h:
##########
@@ -570,7 +572,9 @@ class AggregateFunctionNullUnaryInlineV2 final
assert_cast<const ColumnNullable*,
TypeCheckOnRelease::DISABLE>(columns[0]);
const IColumn* nested_column = &column->get_nested_column();
- if (!column->has_null()) {
+ // The incremental step only touches the outgoing row and the current
frame.
+ if (!column->has_null(std::max<int64_t>(frame_start - 1,
partition_start),
Review Comment:
[P1] Do not use rebased logical offsets as physical column indices
Streaming cleanup erases a physical prefix and subtracts it from
`BoundaryPose::start`, so an active long partition can have negative logical
starts. With a PRECEDING width still clipped to the original partition start,
this `max(...)` is negative; conversion to `size_t` makes
`ColumnNullable::has_null` perform out-of-bounds pointer arithmetic. The old
full-column probe stayed inside retained storage, and the nested incremental
function can skip the not-yet-outgoing logical prefix. Bound the probe to
materialized rows while preserving state for the erased prefix, and cover
eviction in an analytic test.
##########
be/src/exprs/aggregate/aggregate_function_null_v2.h:
##########
@@ -532,7 +532,9 @@ class AggregateFunctionNullUnaryInlineV2 final
}
const auto* column =
assert_cast<const ColumnNullable*,
TypeCheckOnRelease::DISABLE>(columns[0]);
- bool has_null = column->has_null();
+ bool has_null = current_frame_start < current_frame_end
Review Comment:
[P1] Preserve all-NULL state across a trailing empty slice
With `SUM(v) OVER (ORDER BY k ROWS BETWEEN UNBOUNDED PRECEDING AND 1
FOLLOWING)` on two NULL rows, the first output accumulates both rows and leaves
the nullable-v2 flag false with a reusable result. The second output
contributes `[2,3)`, which clamps to empty `[2,2)`. This expression now returns
false, so the later `set_flag(place)` plus a no-op nested SUM changes the
result from NULL to 0. Return without changing wrapper/nested state for a
reusable empty slice and add this all-NULL trailing-frame regression.
##########
be/src/exprs/aggregate/aggregate_function_count.h:
##########
@@ -346,7 +346,7 @@ class AggregateFunctionCountNotNullUnary final
const auto& nullable_column =
assert_cast<const ColumnNullable&,
TypeCheckOnRelease::DISABLE>(*columns[0]);
size_t count = 0;
- if (nullable_column.has_null()) {
+ if (nullable_column.has_null(frame_start, frame_end)) {
Review Comment:
[P1] Keep COUNT's NULL probe within retained rows
The same streaming rebasing can make this clamped `frame_start` negative.
For an all-non-NULL long PRECEDING frame, the old whole-column check returned
false and computed the correct logical count as `frame_end - frame_start`
without reading erased rows. The new range call converts the negative begin to
`size_t` and probes outside the retained null map. Keep the probe within
physical column bounds (without losing the logical width shortcut) and add an
eviction-boundary COUNT 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]
