github-actions[bot] commented on code in PR #66858:
URL: https://github.com/apache/doris/pull/66858#discussion_r3946562844
##########
be/src/storage/segment/variant/binary_column_extract_iterator.cpp:
##########
@@ -117,36 +87,17 @@ Status
BinaryColumnExtractIterator::_fill_variant_v2_path(MutableColumnPtr& dst,
return Status::Corruption("Variant sparse input is not
Map<String,String>");
}
- const auto& offsets = map->get_offsets();
- const StringRef requested {_path.data(), _path.size()};
- DorisVector<StringRef> cells;
- cells.reserve(num_rows);
- DorisVector<uint8_t> missing;
- missing.reserve(num_rows);
- size_t previous_end = 0;
- for (size_t row = 0; row < num_rows; ++row) {
- const size_t end = offsets[ssize_t(row)];
- if (end < previous_end || end > paths->size()) {
- return Status::Corruption("Variant sparse row {} has invalid
offset {}", row, end);
- }
- const size_t lower =
- find_variant_sparse_path_lower_bound(requested, *paths,
previous_end, end);
- if (lower < end && paths->get_data_at(lower) == requested) {
- cells.push_back(values->get_data_at(lower));
- missing.push_back(0);
- } else {
- cells.emplace_back();
- missing.push_back(1);
- }
- previous_end = end;
- }
- if (previous_end != paths->size()) {
- return Status::Corruption("Variant sparse offsets consume {} of {}
cells", previous_end,
- paths->size());
Review Comment:
The replacement check only compares the final offset with paths->size(), but
it no longer rejects decreasing intermediate offsets. A corrupt map such as
[2,1,2] passes this check; StorageMapRowCursor::size_at then subtracts the
offsets as an unsigned size, underflows, and can index past the path/value
columns instead of returning Status::Corruption. Restore per-row
monotonic/bounds validation (or make the assembler reject it before slicing)
and add a tail-valid decreasing-offset regression with no destination mutation.
##########
be/src/storage/transform/block_transform.cpp:
##########
@@ -215,25 +194,16 @@ BlockTransformChain build_transform_chain(const
RowsetWriterContext& context) {
// always rebuilt.
if (context.partial_update_info->is_fixed_partial_update()) {
stages.push_back(std::make_shared<FixedPartialUpdateFillStage>());
- // The legacy fixed path parsed both provided and missing Variant
- // columns before rebuilding RowStore.
- stages.push_back(std::make_shared<VariantParseStage>());
stages.push_back(std::make_shared<RowStoreFillStage>());
} else {
stages.push_back(std::make_shared<FlexiblePartialUpdateFillStage>());
- // The legacy flexible path rebuilt RowStore before parsing the
- // filled Variant columns.
stages.push_back(std::make_shared<RowStoreFillStage>());
- stages.push_back(std::make_shared<VariantParseStage>());
}
return BlockTransformChain {std::move(stages)};
}
- // Direct and schema-change writers rebuilt RowStore from the raw Variant
- // representation, then parsed Variant for its column writer.
if (rebuild_row_store) {
Review Comment:
This removes the only Variant parse/validation seam, but
PushBrokerReader::_cast_to_input_block explicitly skips target TYPE_VARIANT, so
broker/ETL string input remains a nullable ColumnString when
_convert_to_output_block labels it Variant. The Variant converter then returns
StringRef* while VariantColumnWriterImpl reinterprets it as VariantColumnData
and dereferences it as ColumnVariantV2, causing invalid memory access for
non-null rows. Preserve a parse/cast boundary for this path (or make the writer
accept/parse nullable strings) and add a broker/ETL Variant regression test.
##########
be/src/core/data_type/data_type_factory.cpp:
##########
@@ -243,13 +238,8 @@ DataTypePtr DataTypeFactory::create_data_type(const
PColumnMeta& pcolumn) {
nested = std::make_shared<DataTypeString>();
break;
case PGenericType::VARIANT:
- if (pcolumn.variant_is_v2()) {
- nested =
std::make_shared<DataTypeVariantV2>(pcolumn.variant_max_subcolumns_count(),
-
pcolumn.variant_enable_doc_mode());
- } else {
- nested =
std::make_shared<DataTypeVariant>(pcolumn.variant_max_subcolumns_count(),
-
pcolumn.variant_enable_doc_mode());
- }
+ nested =
std::make_shared<DataTypeVariantV2>(pcolumn.variant_max_subcolumns_count(),
Review Comment:
DataTypeFactory::create_data_type(const PColumnMeta&) now unconditionally
returns DataTypeVariantV2 and ignores variant_is_v2. During a rolling upgrade,
an old BE can still send a variant_is_v2=false Block using the V1 serializer
(subcolumn-count/metadata framing), but the new receiver invokes the V2 SerDe
(const/row header framing), so the payload is rejected or misdecoded. Preserve
a V1 decode path for the false marker, or add an explicit execution-version
gate/bump before removing it, and add an old-V1 Block/exchange round-trip test.
--
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]
