github-actions[bot] commented on code in PR #67295:
URL: https://github.com/apache/doris/pull/67295#discussion_r4022447709
##########
be/src/storage/tablet/base_tablet.cpp:
##########
@@ -1083,10 +1135,22 @@ 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
+ // Row-store and physical Variant columns are not always
representation-equivalent. A typed
+ // Variant path can be coerced by the column writer after RowStoreFill
(for example, string
+ // "001" becomes integer 1), so rebuilding a conflicting row from physical
Variant columns
+ // would make its row-store value depend on whether a publish conflict
occurred. Keep the
+ // row-store path for projections containing Variant; fixed updates of
ordinary columns can use
+ // the narrower physical-column read.
+ const bool update_contains_variant = std::ranges::any_of(update_cids,
[&](uint32_t cid) {
Review Comment:
**[P1] Keep non-round-trippable CHAR values off this path**
`Variant` is not the only projection that cannot be reconstructed from
physical columns. For `CHAR(4)` bytes `61 00 62`, `RowStoreFill` preserves all
three bytes and the CHAR writer pads the full slice; the CHAR key coder also
indexes that full slice. Every CHAR page pre-decoder then uses `strnlen`, so
this physical read returns only `61`. Fixed updates must include every key, so
if this is a CHAR key the transient writer can rebuild it as a different key
while the delete bitmap retires the original/conflicting rows; for a CHAR
value, the rebuilt row-store result likewise changes only when a conflict
occurs. Please make `COLUMN_STORE` eligibility depend on representation
round-trip safety (at least exclude CHAR and supported complex columns
containing CHAR) and add an embedded-NUL conflict regression. This is distinct
from the existing typed-Variant thread.
##########
be/src/storage/tablet/base_tablet.cpp:
##########
@@ -1083,10 +1135,22 @@ 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
+ // Row-store and physical Variant columns are not always
representation-equivalent. A typed
+ // Variant path can be coerced by the column writer after RowStoreFill
(for example, string
+ // "001" becomes integer 1), so rebuilding a conflicting row from physical
Variant columns
+ // would make its row-store value depend on whether a publish conflict
occurred. Keep the
+ // row-store path for projections containing Variant; fixed updates of
ordinary columns can use
+ // the narrower physical-column read.
+ const bool update_contains_variant = std::ranges::any_of(update_cids,
[&](uint32_t cid) {
+ return rowset_schema->column(cid).is_variant_type();
+ });
+ const auto update_read_strategy =
+ partial_update_info->is_fixed_partial_update() &&
!update_contains_variant
Review Comment:
**[P2] Keep the row-store path for sparse wide projections**
This mode-only choice routes every non-Variant fixed update to physical
columns, including the supported sparse/wide region that the PR's own benchmark
says is slower: at 64 conflict rows, the 50- and 100-column projections lose to
row-store decoding because per-column iterator setup dominates. The new
production-path test locks in only the favorable one-row/two-column case.
Please include projection width and planned rows per segment in a
benchmark-calibrated selection policy and cover both sides of the crossover, so
this optimization does not regress wide fixed updates.
--
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]
