bobhan1 commented on code in PR #67295:
URL: https://github.com/apache/doris/pull/67295#discussion_r3947442916
##########
be/src/storage/tablet/base_tablet.cpp:
##########
@@ -1084,10 +1136,15 @@ Status
BaseTablet::generate_new_block_for_partial_update(
// rowid in the final block(start from 0, increase continuously) -> rowid
to read in update_block
std::map<uint32_t, uint32_t> read_index_update;
- // read current rowset first, if a row in the current rowset has delete
sign mark
- // we don't need to read values from old block
+ // Fixed partial updates only need their explicit update projection, so
reading the full
+ // row-store JSONB adds an unnecessary full-row allocation. UPSERT
rewrites keep the row-store
+ // path because it preserves the pre-VariantParse row representation.
+ const auto update_read_strategy =
partial_update_info->is_fixed_partial_update()
+ ?
FixedReadPlan::ReadStrategy::COLUMN_STORE
Review Comment:
**[P1] Preserve row-store values for fixed updates of typed Variant columns**
`VariantParse` does not make a fixed update's row-store and physical Variant
representations equivalent. For example, with full row store, V1 non-doc-mode
`v VARIANT<'a': INT>`, and the typed path materialized, a fixed update of `(k,
v)` containing `{"a":"001"}` preserves the string in row store. The [fixed
transform](https://github.com/apache/doris/blob/3e5428c0ccc4cb79744fd8c12f940d6bbdb943e8/be/src/storage/transform/block_transform.cpp#L213-L221)
runs `VariantParse -> RowStoreFill`, while the [schema-template
cast](https://github.com/apache/doris/blob/3e5428c0ccc4cb79744fd8c12f940d6bbdb943e8/be/src/storage/segment/variant/variant_column_writer_impl.cpp#L351-L361)
happens later in the column writer, making the physical `a` subcolumn integer
`1`.
If a concurrent update of another column on the same key triggers
publish-conflict reconstruction, this new `COLUMN_STORE` path reads `{"a":1}`
and the transient writer puts that value into the rebuilt row store. The
previous row-store path preserves `{"a":"001"}`. A point query of the whole
Variant through row store therefore changes its result depending on whether a
publish conflict occurred.
Please preserve row-store semantics for this projection and add a
publish-conflict regression covering full row store + fixed update + Variant
schema-template coercion. The new UPSERT/raw-boolean test does not cover this
case.
This finding is based on tracing the current code; I have not run an
end-to-end reproducer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
