Repository navigation
[fix](bitmap) fix SIGSEGV when reading zero-length serialized BITMAP/HLL/QuantileState cell - #67473
Merged
Merged
Conversation
…HLL/QuantileState cell
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
|
run buildall |
Contributor
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Requesting changes on 04190660015d0c48bf3a207cf7284f04a7f150e3 after two review rounds; both follow-up reviewers converged with NO_NEW_VALUABLE_FINDINGS. The blocking findings are inline: the all-type zero-byte recovery lacks a BITMAP/QUANTILE_STATE compatibility contract, the BITMAP regression oracle can accept the old invalid state, and the added test fails clang-format 16.
Checkpoint conclusions:
- Goal and proof: the early return removes the invalid zero-length object copy across direct, continuous,
StringRef, V1, V2, and V3 paths, but the recovery semantics and BITMAP test proof remain incomplete. - Scope: the implementation is small and focused, and the new test translation unit is auto-discovered by the BE unit-test target.
- Concurrency, lifetime, COW, and memory: no new shared state, locks, static initialization, ownership edge, or invalid default-object lifetime was found.
- Configuration and compatibility: no config, protocol, symbol, or storage-format change is introduced. HLL has a historical dirty-data exception, but zero-byte compatibility for BITMAP and QUANTILE_STATE is unresolved.
- Error and data correctness: malformed BITMAP/QUANTILE_STATE cells can be silently reported as empty; this is the primary correctness blocker.
- Tests and checks: no build was run under the review runner contract.
clang-format-16 --dry-run --Werrorfails on the added test;git diff --checkandbuild-support/check-build-hygiene.shpass. No regression result files are involved. - Transactions, persistence writes, FE/BE variables, and observability: none are otherwise implicated; if compatibility recovery is retained, its scope and observability need to be explicit.
- Performance: no material regression was found.
- Review focus: no additional focus was supplied, so the full PR was reviewed.
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
|
/review |
Contributor
There was a problem hiding this comment.
Requesting changes on c279c87489e320f5203220c440cde58cc46460f5 after two complete review rounds; all round-2 reviewers converged with NO_NEW_VALUABLE_FINDINGS. I am not adding a new inline comment because the only substantiated blocker is already captured in the existing P1 thread.
The latest head fixes the two earlier P2 items: clang-format 16 now accepts the include order, and the BITMAP test asserts the canonical BitmapTypeCode::EMPTY. The P1 remains unresolved: current writers encode empty BITMAP/HLL/QUANTILE_STATE values as 1/1/5 bytes and nullable rows are omitted from data pages. Only HLL has a documented legacy zero-length recovery contract. Applying the fallback to BITMAP and QUANTILE_STATE can therefore turn truncated/corrupt cells into valid empty query results unless a real per-type compatibility contract is established.
Checkpoint conclusions:
- Goal and proof: the change removes the invalid zero-length object reinterpret/copy and the helper tests cover direct and batched insertion, but the all-type recovery policy is not yet justified; the existing P1 remains blocking.
- Scope and clarity: the source change is small and focused, and the new test is discovered by the BE unit-test CMake glob.
- Concurrency: no threads, locks, atomics, or shared concurrent state are involved.
- Lifecycle and ownership: no static-initialization or special-release lifecycle is introduced;
insert_default()creates destination-owned RAII state and no COW/Block mutation issue was found. - Configuration: no configuration item or dynamic-update behavior is added.
- Compatibility: V1, V2, and V3 all preserve zero cell lengths into the shared decoder path. HLL has a historical compatibility exception; BITMAP and QUANTILE_STATE do not have a demonstrated producer/version contract.
- Parallel paths and conditions:
next_batchandread_by_rowidsconverge through the two tested batch helpers, and nullable rows remain governed by the null bitmap. The zero-length conditional still needs the per-type policy requested by P1. - Tests and results: positive nonempty behavior is covered by existing tests and the added BITMAP oracle is now canonical. No build or test execution was performed because this review runner forbids it; no regression
.outfiles are involved. - Observability and error handling: silently normalizing unsupported zero-length BITMAP/QUANTILE_STATE payloads removes corruption evidence; rejection or an explicit scoped recovery contract is required. No separate logging or metric change is otherwise needed.
- Transactions, persistence, writes, and FE/BE variables: no transaction, EditLog, storage-write atomicity, visibility-version, or transmitted-variable change is involved.
- Performance: the O(1) branch removes an invalid copy and introduces no material hot-path regression.
- Other findings: none after duplicate fencing and the final changed-file sweep.
- User focus: no additional review focus was supplied.
Static checks run: clang-format-16 --dry-run --Werror on both changed files and build-support/check-build-hygiene.sh; both passed.
Contributor
|
run buildall |
Contributor
TPC-H: Total hot run time: 27581 ms |
Contributor
TPC-DS: Total hot run time: 152707 ms |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 27938 ms |
Contributor
TPC-DS: Total hot run time: 152632 ms |
Contributor
ClickBench: Total hot run time: 23.97 s |
hello-stephen
approved these changes
Sep 24, 2026
github-actions Bot
pushed a commit
that referenced
this pull request
Sep 24, 2026
…HLL/QuantileState cell (#67473) When a BITMAP / HLL / QUANTILE_STATE cell on disk has a serialized length of 0, ColumnComplexType::insert_binary_data used to interpret the raw page bytes as an in-memory value_type object via reinterpret_cast and then copy-assign it, which is undefined behavior. The heap-pointer members of BitmapValue / HyperLogLog / QuantileState were dereferenced as if they were valid pointers, and BE processes crashed with SIGSEGV during queries that hit read_by_rowids (e.g. lazy materialization on aggregate / unique-key tables that carry a BITMAP value column). Fix it by keeping the default-constructed empty object produced by insert_default() and returning early when length == 0. Add regression tests in be/test/core/column/column_complex_test.cpp covering all three complex types and all three code paths that flow into insert_binary_data.
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
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.
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
When a BITMAP / HLL / QUANTILE_STATE cell on disk has a serialized length
of 0, ColumnComplexType::insert_binary_data used to interpret the raw
page bytes as an in-memory value_type object via reinterpret_cast and
then copy-assign it, which is undefined behavior. The heap-pointer
members of BitmapValue / HyperLogLog / QuantileState were dereferenced
as if they were valid pointers, and BE processes crashed with SIGSEGV
during queries that hit read_by_rowids (e.g. lazy materialization on
aggregate / unique-key tables that carry a BITMAP value column).
Fix it by keeping the default-constructed empty object produced by
insert_default() and returning early when length == 0.
Add regression tests in be/test/core/column/column_complex_test.cpp
covering all three complex types and all three code paths that flow
into insert_binary_data.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)