[fix](variant) preserve subcolumns after row-store partial update - #62067
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
Adds a regression test and BE write-path changes to ensure VARIANT subcolumns are preserved/materialized correctly after row-store partial updates.
Changes:
- Add a regression scenario that updates a non-VARIANT column and verifies VARIANT subfields remain readable and unchanged.
- Materialize VARIANT columns after filling missing columns during partial-update writes (both SegmentWriter and VerticalSegmentWriter).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| regression-test/suites/variant_p0/delete_update.groovy | Adds a regression case for VARIANT subcolumn preservation across partial updates in row-store tables. |
| be/src/storage/segment/vertical_segment_writer.cpp | Materializes VARIANT columns in the partial-update append path before serializing to row-store. |
| be/src/storage/segment/segment_writer.cpp | Same VARIANT materialization step in the non-vertical segment writer partial-update append path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+111
to
+114
| def wholeVariant = sql """select cast(event_property_variant as string) | ||
| from ${partialUpdateVariantTable} | ||
| where event_id = 1""" | ||
| assertEquals('{"base_id":"100000000009523363","other_key":"abc"}', wholeVariant[0][0]) |
There was a problem hiding this comment.
This assertion is potentially flaky because the string serialization of a VARIANT/JSON object may not guarantee key order or exact formatting. To make the test stable, assert on individual fields (e.g., base_id and other_key) or compare a canonicalized representation rather than strict string equality of the whole object.
Suggested change
| def wholeVariant = sql """select cast(event_property_variant as string) | |
| from ${partialUpdateVariantTable} | |
| where event_id = 1""" | |
| assertEquals('{"base_id":"100000000009523363","other_key":"abc"}', wholeVariant[0][0]) | |
| def wholeVariant = sql """select cast(event_property_variant['base_id'] as string), | |
| cast(event_property_variant['other_key'] as string) | |
| from ${partialUpdateVariantTable} | |
| where event_id = 1""" | |
| assertEquals("100000000009523363", wholeVariant[0][0]) | |
| assertEquals("abc", wholeVariant[0][1]) |
| from ${partialUpdateVariantTable} | ||
| where event_id = 1""" | ||
| assertEquals("100000000009523363", beforeUpdate[0][0]) | ||
|
|
There was a problem hiding this comment.
This test intends to validate behavior after a partial update, but it doesn't explicitly enable partial updates before running the UPDATE (the suite enables enable_unique_key_partial_update later for a different section). Consider setting enable_unique_key_partial_update=true (and syncing) before this UPDATE, so the regression reliably exercises the specific code path this PR changes.
Suggested change
| sql "set enable_unique_key_partial_update=true;" | |
| sql "sync" |
eldenmoon
force-pushed
the
fix-partial-rowstore-var
branch
from
April 2, 2026 14:49
1f48122 to
cbbda43
Compare
Member
Author
|
run buildall |
TPC-H: Total hot run time: 29157 ms |
TPC-DS: Total hot run time: 180841 ms |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Member
Author
|
skip check_coverage |
Contributor
|
PR approved by anyone and no changes requested. |
airborne12
approved these changes
Apr 3, 2026
airborne12
left a comment
airborne12
left a comment
Member
There was a problem hiding this comment.
LGTM. Critical bug fix — variant subcolumns were lost during row-store partial update because parse_and_materialize_variant_columns was not called after fill_missing_columns. Both segment_writer and vertical_segment_writer are consistently fixed. Regression test precisely covers the failure scenario.
Contributor
|
PR approved by at least one committer and no changes requested. |
github-actions
Bot
added
the
approved
eldenmoon
added
usercase
github-actions Bot
pushed a commit
that referenced
this pull request
Apr 3, 2026
…2067) need to materialize variant after reading from row store when partial update
github-actions Bot
pushed a commit
that referenced
this pull request
Apr 3, 2026
…2067) need to materialize variant after reading from row store when partial update
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.
need to materialize variant after reading from row store when partial update