[fix](variant) Prefer sparse variant fields over root value#65660
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Member
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes incorrect VARIANT serialization results in distributed ordered queries by preserving sparse/dense subcolumn structure during row copy, materializing pending default suffixes, and ensuring root-value visibility logic does not mask sparse/object fields.
Changes:
- Add a regression test that exercises TopN lazy materialization with sparse VARIANT subcolumns.
- Add BE unit tests covering pending-default-suffix materialization, sparse-limit preservation, and mixed root+sparse visibility.
- Adjust
ColumnVariantrow-copy and root-visibility logic to better preserve sparse/doc_value structure.
Review Checkpoints (skill Part 1.3)
- Goal & proof: Goal is clear (preserve sparse variant fields during row copy); covered by the added regression + unit tests.
- Scope/focus: Changes are localized to
ColumnVariantcopy/visibility plus targeted tests. - Concurrency: No new concurrency/locking introduced in the touched code paths.
- Lifecycle/static init: No new static/global lifecycle concerns introduced.
- Config items: No new configs added (only test session variables).
- Compatibility/storage format: No explicit on-disk format change shown in this diff; behavior change is in copy/serialization logic.
- Parallel code paths:
insert_from()now funnels mixed-shape copies throughinsert_range_from(); this appears to be the intended consolidation. - Special conditional checks: New visibility logic adds sparse offsets consideration; one indexing detail needs adjustment (see PR comment).
- Test coverage: Regression + 3 unit tests added; seems appropriate for this bug fix.
- Observability: No new logging/metrics; likely fine for this narrowly scoped correctness fix.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| regression-test/suites/variant_p0/test_topn_lazy_materialize_sparse_variant.groovy | Adds a regression scenario for TopN lazy materialization over sparse VARIANT data. |
| regression-test/data/variant_p0/test_topn_lazy_materialize_sparse_variant.out | Captures the expected query output for the new regression case. |
| be/test/core/column/column_variant_test.cpp | Adds focused unit tests for row-copy default suffixes, sparse limits, and root visibility with sparse fields. |
| be/src/core/column/column_variant.cpp | Updates VARIANT subcolumn range-copy, row-copy behavior, and root visibility checks to preserve sparse structure. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: A distributed ordered query using TopN lazy materialization could return an array root such as [] instead of the object fields stored in a VARIANT row. Root visibility already considered dense subcolumns and document snapshots but ignored sparse subcolumns, so a non-null root incorrectly won during serialization. Check the current row sparse offsets before treating the root as visible. Before the fix the reproduced row returned []; after the fix it returns {"n":7,"word":"hot"}.
### Release note
Fix incorrect VARIANT results in TopN lazy queries when object fields are stored in sparse columns.
### Check List (For Author)
- Test:
- Unit Test: ColumnVariantTest.visible_root_does_not_hide_sparse_fields
- Regression test: variant_p0/test_topn_lazy_materialize_sparse_variant
- ASAN BE build
- Behavior changed: Yes. Sparse object fields now take precedence over a non-array root during VARIANT serialization.
- Does this need documentation: No
eldenmoon
force-pushed
the
branch-fix-doris-27147
branch
from
July 15, 2026 12:45
513c12a to
48973fb
Compare
eldenmoon
changed the title
Member
Author
|
run buildall |
Member
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 completed for PR 65660 at head 48973fb.
I found two correctness issues that should be fixed before merge:
- The new sparse-visibility guard can still lose sparse rows when doc-value offsets are present from earlier rows in mixed-format/compatibility rowsets.
- The new regression expected output records
[]for rows inserted as{}, which would lock in object-to-array corruption.
Checkpoint summary: I reviewed the changed ColumnVariant serialization/copying code, the TopN lazy materialization row-id fetch/scatter path, storage variant sparse/doc read/write interactions, and the new BE/regression tests. Existing review context was checked; the existing nrow - 1/PaddedPODArray thread is not duplicated here. User focus file had no additional focus. Validation was static only, per the review prompt; I did not run builds or tests.
| } | ||
| } | ||
|
|
||
| const auto& sparse_offsets = serialized_sparse_column_offsets(); |
Contributor
There was a problem hiding this comment.
This still misses sparse rows after any prior doc-snapshot row. Once this new check returns false for a row with sparse entries, serialize_one_row_to_json_format() immediately tests doc_value_column_map.get_offsets()[row_num] != 0; that is the cumulative end offset, so it remains true for later rows even when the current row has no doc-value entries. If a result column is assembled from mixed-format or compatibility rowsets where an earlier segment has doc-value snapshots and a later row has sparse entries, the sparse row can serialize as {} and return before the sparse map below is considered. Please gate the doc snapshot branch with the current-row delta, e.g. the existing has_doc_value_column(row_num) / offsets[row_num - 1] < offsets[row_num], and add a unit case with a prior doc row and a current sparse row.
Member
Author
There was a problem hiding this comment.
need to fix in new mem layout
| 4 -9223372036854775808 [1,2,3] {"k":1} | ||
| 6 1 [1,2,3] {} | ||
| 7 0 [1,2,3] [1,2,3] | ||
| 9 \N {} [] |
Contributor
There was a problem hiding this comment.
This expected row does not match the inserted value. The suite inserts col_variant = '{}' for pk 9 (and again for pk 15), but the new oracle records []. Other rows inserted as {} in this same output remain {}, so this is not just a display convention; it locks in an object-to-array change in the regression that is supposed to protect VARIANT materialization correctness. Please fix the underlying serialization case and regenerate this as {} (or remove these rows from this regression and cover the object/array issue separately).
Member
Author
There was a problem hiding this comment.
need to fix in new mem layout
Contributor
TPC-H: Total hot run time: 29010 ms |
Contributor
TPC-DS: Total hot run time: 177266 ms |
Contributor
ClickBench: Total hot run time: 25.17 s |
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29406 ms |
Contributor
TPC-DS: Total hot run time: 176905 ms |
Contributor
ClickBench: Total hot run time: 24.84 s |
eldenmoon
changed the title
Contributor
|
branch-4.1 full regression build 203698 reproduced DORIS-27147 with 672 I prepared a focused follow-up for the two unresolved review threads: eldenmoon#5 It changes the doc-value guard to a current-row offset delta, adds the requested prior-doc/current-sparse unit scenario, and removes the two The existing macOS BE UT red on #65660 is unrelated runner setup ( |
Contributor
|
Additional CI triage (2026-07-22): the existing red checks on revision
The two unresolved review issues are addressed separately in eldenmoon#5. After that follow-up is merged into this PR head, please rerun BE UT/macOS BE UT on the new revision. |
csun5285
approved these changes
Jul 23, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Member
Author
|
run p0 |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Member
Author
|
/review |
Contributor
|
PR approved by anyone and no changes requested. |
Contributor
There was a problem hiding this comment.
Automated review completed for PR 65660 at head ed4a264eb9fc7b47e2ee2db678c7c7742eee9fb0.
No new inline findings remain after validation and duplicate suppression. The PR is not ready to merge because two existing correctness threads remain unresolved:
- discussion_r3587564814: cumulative doc-snapshot offsets can still route a later sparse row through an empty current-row doc branch.
- discussion_r3587564816: the regression oracle records
[]for pk 9 and pk 15 even though both rows insert{}.
Checkpoint conclusions:
- Goal and proof: The added current-row sparse-offset check correctly prevents a non-array JSONB root from hiding sparse object fields, and the unit test exercises that representation. The end-to-end regression is not yet valid proof for all selected rows because its two empty-object expectations are wrong.
- Scope and clarity: The implementation change is small, focused, and reuses the existing ColumnMap offset invariant.
- Concurrency and lifecycle: The changed path is read-only serialization; it adds no locks, atomics, ownership transfers, static initialization, or lifecycle coupling.
- Configuration and compatibility: No production configuration, persisted format, wire protocol, symbol, or rolling-upgrade contract changes. The regression uses session-only knobs to force V3 sparse layout and TopN materialization.
- Parallel paths and conditions: Storage row scatter, Variant Block SerDe, TopN response merge, and result SerDes were traced. Persisted roots are Nullable(JSONB); sparse and doc readers default the unused representation. The row-0
nrow - 1access is the documented PaddedPODArray sentinel convention. - Tests and results: The unit is targeted and the p0 query is deterministically ordered and asserts
MaterializeNode, but the two bad expected rows and the missing prior-doc/current-sparse case must be corrected in the existing threads. Per the review-runner contract, I did not build or run tests locally. - Observability, transactions, persistence, and FE/BE variables: Not applicable to this local serialization predicate; no new gap found.
- Performance: The change adds two O(1) offset reads with no allocation or copy after dense fields have been ruled out.
- User focus: No additional user-provided focus was present.
Review coverage is complete for the current head; the two existing correctness issues still need fixes.
github-actions Bot
pushed a commit
that referenced
this pull request
Jul 23, 2026
A distributed ordered query using TopN lazy
materialization could return an array root such as [] instead of the
object fields stored in a VARIANT row. Root visibility already
considered dense subcolumns and document snapshots but ignored sparse
subcolumns, so a non-null root incorrectly won during serialization.
Check the current row sparse offsets before treating the root as
visible. Before the fix the reproduced row returned []; after the fix it
returns {"n":7,"word":"hot"}.
Contributor
|
[review-followup-65957] The separate review follow-up was not part of the merged head. It has now been cleanly reapplied to current master as #65957; further review, CI, and validation are tracked there. |
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: None
Related PR: None
Problem Summary: A distributed ordered query using TopN lazy materialization could return an array root such as [] instead of the object fields stored in a VARIANT row. Root visibility already considered dense subcolumns and document snapshots but ignored sparse subcolumns, so a non-null root incorrectly won during serialization. Check the current row sparse offsets before treating the root as visible. Before the fix the reproduced row returned []; after the fix it returns {"n":7,"word":"hot"}.
Release note
Fix incorrect VARIANT results in TopN lazy queries when object fields are stored in sparse columns.
Check List (For Author)