github-actions[bot] commented on code in PR #66714:
URL: https://github.com/apache/doris/pull/66714#discussion_r3775265842
##########
be/src/core/column/variant_v2/column_variant_v2.cpp:
##########
@@ -937,40 +938,210 @@ void ColumnVariantV2::insert_encoded_rows( //
NOLINT(readability-function-size)
}
}
- if (_typed) {
- ensure_encoded();
+ _append_prevalidated_encoded_data(data);
+}
+
+struct ColumnVariantV2::EncodedRowsAppender::Impl {
+ using MetadataHashHeads =
+ std::unordered_map<size_t, uint32_t, std::hash<size_t>,
std::equal_to<size_t>,
+ CustomStdAllocator<std::pair<const size_t,
uint32_t>>>;
+
+ explicit Impl(ColumnVariantV2& destination) : column(&destination) {}
+
+ ColumnVariantV2* column;
+ MetadataHashHeads metadata_hash_heads;
+ DorisVector<uint32_t> metadata_hash_next;
+ size_t expected_rows = 0;
+ bool initialized = false;
+#ifdef BE_TEST
+ size_t metadata_comparisons = 0;
+#endif
+};
+
+ColumnVariantV2::EncodedRowsAppender::EncodedRowsAppender(ColumnVariantV2&
column)
+ : _impl(std::make_unique<Impl>(column)) {}
+
+ColumnVariantV2::EncodedRowsAppender::~EncodedRowsAppender() = default;
+
+ColumnVariantV2::EncodedRowsAppender::EncodedRowsAppender(EncodedRowsAppender&&)
noexcept = default;
+
+#ifdef BE_TEST
+size_t ColumnVariantV2::EncodedRowsAppender::metadata_comparisons_for_test()
const noexcept {
+ return _impl == nullptr ? 0 : _impl->metadata_comparisons;
+}
+#endif
+
+void ColumnVariantV2::EncodedRowsAppender::append( //
NOLINT(readability-function-size)
+ std::span<const VariantRef> rows) {
+ if (rows.empty()) {
+ return;
}
- DORIS_CHECK(_typed_type == nullptr) << "encoded state cannot retain a
typed data type";
- require_exclusive(_meta_ids, "metadata ids");
- require_exclusive(_values, "values");
- auto& values = assert_cast<ColumnString&>(*_values);
- auto& metadata_ids = assert_cast<MetaIdsColumn&>(*_meta_ids);
- reserve_rows(values, metadata_ids, data.value_bytes.size, rows);
+ DORIS_CHECK(_impl != nullptr && _impl->column != nullptr)
+ << "Cannot use a moved-from Variant encoded-row appender";
+ ColumnVariantV2& column = *_impl->column;
+
+ using MetadataIdMap =
+ std::unordered_map<std::string_view, uint32_t,
std::hash<std::string_view>,
+ std::equal_to<std::string_view>,
+ CustomStdAllocator<std::pair<const
std::string_view, uint32_t>>>;
+ MetadataIdMap metadata_ids_by_value;
+ DorisVector<VariantMetadataRef> unique_metadatas;
+ DorisVector<uint32_t> source_metadata_ids;
+ DorisVector<StringRef> source_values(rows.size());
+ size_t total_value_bytes = 0;
+ for (size_t row = 0; row < rows.size(); ++row) {
+ const VariantRef value = rows[row];
+ if (value.metadata.data == nullptr && value.metadata.size != 0) {
+ throw Exception(ErrorCode::CORRUPTION,
+ "Variant encoded metadata has a null data pointer
for {} bytes",
+ value.metadata.size);
+ }
+ if (value.value.data == nullptr && value.value.size != 0) {
+ throw Exception(ErrorCode::CORRUPTION,
+ "Variant encoded value has a null data pointer for
{} bytes",
+ value.value.size);
+ }
- if (data.meta_ids.empty()) {
- const VariantMetadataRef metadata = metadata_at(0);
- const uint32_t id = _find_or_insert_metadata({metadata.data,
metadata.size});
- values.insert_many_continuous_binary_data(data.value_bytes.data,
data.value_offsets.data(),
- rows);
- metadata_ids.insert_many_vals(id, rows);
+ const std::string_view metadata_key(
+ value.metadata.data == nullptr ? "" : value.metadata.data,
value.metadata.size);
+ uint32_t source_metadata_id = 0;
+ if (unique_metadatas.empty()) {
+ validate_variant_metadata(value.metadata);
+ unique_metadatas.push_back(value.metadata);
+ } else if (unique_metadatas.size() == 1 &&
metadata_ids_by_value.empty() &&
+ StringRef(unique_metadatas.front().data,
unique_metadatas.front().size) ==
+ StringRef(value.metadata.data,
value.metadata.size)) {
+ // Iceberg files normally share one metadata dictionary across a
batch. Avoid a hash
+ // table and per-row ids until a second distinct dictionary is
actually observed.
+ } else {
+ if (metadata_ids_by_value.empty()) {
+ const VariantMetadataRef first = unique_metadatas.front();
+ metadata_ids_by_value.emplace(
+ std::string_view(first.data == nullptr ? "" :
first.data, first.size), 0);
+ source_metadata_ids.resize(rows.size());
+ }
+ auto metadata_id = metadata_ids_by_value.find(metadata_key);
+ if (metadata_id != metadata_ids_by_value.end()) {
+ source_metadata_id = metadata_id->second;
+ } else {
+ if (unique_metadatas.size() ==
std::numeric_limits<uint32_t>::max()) {
+ throw Exception(
+ ErrorCode::INVALID_ARGUMENT,
+ "Variant encoded metadata dictionary exceeds the
uint32 id limit");
+ }
+ validate_variant_metadata(value.metadata);
+ source_metadata_id =
static_cast<uint32_t>(unique_metadatas.size());
+ unique_metadatas.push_back(value.metadata);
+ metadata_ids_by_value.emplace(metadata_key,
source_metadata_id);
+ }
+ }
+ if (!source_metadata_ids.empty()) {
+ source_metadata_ids[row] = source_metadata_id;
+ }
+ validate_variant_payload(value);
+ source_values[row] = value.value;
+ if (value.value.size > std::numeric_limits<size_t>::max() -
total_value_bytes) {
+ throw Exception(ErrorCode::INVALID_ARGUMENT,
+ "Variant encoded value bytes exceed the size_t
limit");
+ }
+ total_value_bytes += value.value.size;
+ }
+
+ // Validate the complete input before changing a typed/shredded
destination. Failed and empty
+ // appends must preserve its representation just like the EncodedDataView
overload does.
+ if (_impl->initialized) {
+ DORIS_CHECK(column._typed == nullptr && column._shredded == nullptr)
+ << "ColumnVariantV2 changed outside its encoded-row appender";
+ DORIS_CHECK_EQ(_impl->expected_rows, column.size())
+ << "ColumnVariantV2 changed outside its encoded-row appender";
+ const auto& current_metadatas = assert_cast<const ColumnString&>(
+ *static_cast<const IColumn::Ptr&>(column._metadatas));
+ DORIS_CHECK_EQ(_impl->metadata_hash_next.size(),
current_metadatas.size())
+ << "ColumnVariantV2 metadata changed outside its encoded-row
appender";
+ }
+ if (column._typed || column._shredded) {
+ column.ensure_encoded();
+ }
+ DORIS_CHECK(column._typed_type == nullptr) << "encoded state cannot retain
a typed data type";
+ require_exclusive(column._meta_ids, "metadata ids");
+ require_exclusive(column._values, "values");
+ auto& values = assert_cast<ColumnString&>(*column._values);
+ auto& metadata_ids = assert_cast<MetaIdsColumn&>(*column._meta_ids);
+
+ auto current_metadatas = [&]() -> const ColumnString& {
+ return assert_cast<const ColumnString&>(
+ *static_cast<const IColumn::Ptr&>(column._metadatas));
+ };
+ if (!_impl->initialized) {
+ const auto& metadatas = current_metadatas();
+ _impl->metadata_hash_heads.reserve(metadatas.size());
+ _impl->metadata_hash_next.reserve(metadatas.size());
+ for (size_t index = 0; index < metadatas.size(); ++index) {
+ const auto id = static_cast<uint32_t>(index);
+ const StringRef metadata = metadatas.get_data_at(id);
+ const size_t hash = StringRefHash {}(metadata);
+ const auto existing = _impl->metadata_hash_heads.find(hash);
+ const uint32_t next = existing == _impl->metadata_hash_heads.end()
+ ? UNMAPPED_METADATA_ID
+ : existing->second;
+ _impl->metadata_hash_next.push_back(next);
+ _impl->metadata_hash_heads[hash] = id;
+ }
+ _impl->expected_rows = column.size();
+ _impl->initialized = true;
+ }
Review Comment:
[P2] Reject or snapshot same-column borrowed rows
This public appender accepts `VariantRef`s returned by the same column's
`get_value_ref()`, but it retains only shallow pointers and then
`reserve_rows()` can move the destination value buffer before lines 1124/1131
copy them. A forced-growth self-append can therefore copy from invalidated
storage; a shredded self-borrow can lose its owner even earlier during
`ensure_encoded()`. `insert_encoded_rows()` explicitly forbids aliases and
`insert_range_from()` snapshots self-copies, so please snapshot overlapping
metadata/value bytes or reject them before representation conversion, reserve,
or interning, with encoded and shredded regression coverage.
--
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]
