Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes for three avoidable hot-path regressions in the new manual NULL/constant handling.
Critical checkpoint conclusions:
- Goal and correctness: The functional fix is sound under static review. ColumnView covers plain, constant, nullable, and Const(Nullable) inputs; date state is reconstructed per row; NULL rows short-circuit before payload parsing; invalid non-NULL weekdays still return InvalidArgument. FE and BE nullable return types and DateV2/TimestampNs behavior align.
- Scope/design: The change is focused on the shared relative-day implementation and its BE unit coverage.
- Concurrency/lifecycle/memory: No new shared mutable state, locking, or special lifecycle is introduced. Result and null-map columns remain size-aligned and column-owned.
- Configuration/compatibility/persistence/data writes: No configuration, protocol/storage format, persistence, transaction, or rolling-upgrade surface is changed.
- Parallel paths: Both next_day/previous_day and DateV2/TimestampNs paths were traced; no distinct correctness issue remains.
- Performance: The accepted inline findings must be addressed: preserve partial-constant fast paths, remove the release assertion from the row loop, and retain the all-NULL constant-result shortcut.
- Tests: Existing next/previous-day regressions cover multi-row constant-date behavior and error cases; the added BE unit test covers actual NULLs in both positions, and existing TimestampNs coverage exercises nullable input. Per the review-runner contract, this was a static review only; no builds or tests were run. The text-only BE build-hygiene gate passed.
- User focus: No additional focus was provided.
Review status: complete after three bounded rounds. Every candidate was independently verified and deduplicated; the final normal and risk-focused round returned NO_NEW_VALUABLE_FINDINGS.
| return result; | ||
| } | ||
|
|
||
| bool use_default_implementation_for_nulls() const override { return false; } |
There was a problem hiding this comment.
Opting out of the framework NULL implementation also drops its only_null shortcut. next_day(NULL, e) is not foldable when e is a slot, so it reaches BE; this path previously returned a constant NULL column immediately, but now reserves/fills a row-sized DateV2 column and null map and executes the scalar loop for every row. Please retain an early only_null result before these allocations (and before constant-weekday validation, to preserve NULL precedence) while keeping the new per-row handling for mixed-null inputs.
| const auto week_view = ColumnView<TYPE_STRING>::create(week_column); | ||
| for (size_t i = 0; i < input_rows_count; ++i) { | ||
| if (date_view.is_null_at(i) || week_view.is_null_at(i)) { | ||
| DORIS_CHECK(null_map != nullptr); |
There was a problem hiding this comment.
This assertion is inside the scalar row loop, while the repository guidance and check.h explicitly require DCHECK in loops where release checks add overhead. The relationship between input nullability and the result null map is fixed for the block, so please either validate it once before the loop or use the debug-only form here. Suggested local fix:
| DORIS_CHECK(null_map != nullptr); | |
| DCHECK(null_map != nullptr); |
| } | ||
| auto dtv = date_v2_from_date_like(date_view.value_at(i)); | ||
| auto week = week_view.value_at(i); | ||
| auto week_day = day_of_week(week); |
There was a problem hiding this comment.
Please preserve the partial-constant fast paths here. For next_day(date_col, 'MON'), the framework's all-constant handling does not run because only one argument is constant, so week_view.is_const reaches this loop. The removed right_const branch ran day_of_week once; this version copies, uppercases, and hashes the same literal for every row, changing that work from O(1) to O(N). The analogous constant-date path now also repeats DateV2/TimestampNs conversion. Cache a constant weekday lazily after the NULL short-circuit, and cache an immutable converted constant date that is copied before each per-row mutation.
There was a problem hiding this comment.
你说的对，可是过去就是因为他自己手写了null和const的处理，导致他变成错的了。
|
run buildall |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
2 participants
next_dayandprevious_dayreused a mutated date value when the date argument was constant, producing cumulative cross-row results. Nullable inputs were also unpacked before execution, causing NULL weekday rows to be parsed as empty strings and fail the entire query. The functions now useColumnViewto handle constant and nullable columns directly, compute each row from an independent date value, propagate NULL results, and continue rejecting invalid non-NULL weekdays.Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)