Conversation
### What problem does this PR solve? Problem Summary: array_pushback and array_append could read past the end of the NULL bitmap when a constant NULL array was evaluated for multiple rows. Use the constant-column row index consistently for NULL bitmap and array offset access. ### Release note Fix array_pushback and array_append with constant NULL arrays in multi-row execution. ### Check List (For Author) - Test: BE build, 396 focused array unit tests, and query_p0 array regression test - Behavior changed: Yes - Does this need documentation: No
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review opinion: approve; I found no blocking correctness issue in the reviewed head.
Checkpoint conclusions:
- Goal and proof: the PR fixes the mixed constant/vector path where a constant nullable array has one stored NULL-map row but
execute_implprocesses multiple logical rows. Reusingsrc_index = index_check_const(i, src_const)for the NULL map and offsets aligns both subcolumns. The added ordered 16-row cases coverarray_pushbackand itsarray_appendalias. - Scope and clarity: the implementation change is minimal and focused; it also removes duplicate const-index calculations. The expected-output change matches the two new queries.
- Concurrency and lifecycle: this is per-block scalar-function execution with no shared mutable state, threads, locks, static initialization, or special resource lifecycle.
- Configuration and compatibility: no configuration, protocol, persisted format, function symbol, FE-BE variable, or rolling-upgrade contract changes.
- Parallel paths and conditions:
array_appendresolves to the same FE/BE implementation.array_pushfrontis already safe because it materializes constant inputs before row-wise access. The top-level NULL branch sets the result NULL/offset and exits before nested-array access; non-constant sources retain rowibehavior. - Error, memory, and nullable safety: no
Statusis discarded and no ownership/allocation behavior changes.unpack_if_constexposes the constant's one-row payload, andColumnArrayoffsets use the padded container whose row-zero predecessor is intentionally defined as zero. - Tests and results: the queries are deterministic via
ORDER BY number, exercise more than the one physical constant row, and the recorded sixteen NULL results per name are correct. Per the review-runner constraint, this was static review only: I did not build or run the regression suite, and I did not independently regenerate the.outfile. - Observability, persistence, and writes: no new logging/metrics are needed; transactions, EditLog/replay, storage visibility, delete bitmaps, and data-write atomicity are not involved.
- Performance and other risks: the loop keeps the same asymptotic work and slightly reduces repeated branching. No security, build-hygiene, glibc-baseline, or additional correctness concern was found.
- User focus: no additional review focus was supplied; the full PR and the constant-NULL/alias paths were reviewed.
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28336 ms |
Contributor
TPC-DS: Total hot run time: 152878 ms |
Contributor
ClickBench: Total hot run time: 24.05 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
array_pushbackand itsarray_appendalias could read past the end of the NULL bitmap when the array argument was a constant NULL column evaluated for multiple rows. This caused unstable NULL propagation and could lead to undefined behavior. The implementation now uses the constant-column row index for both the NULL bitmap and array offsets, preserving scalar NULL semantics across all input rows.Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)