This is an automated email from the ASF dual-hosted git repository.
yiguolei pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/master by this push:
new efd566353bd [improvement](variant) Reduce Variant array shredding CPU
during import (#67983)
efd566353bd is described below
commit efd566353bd598f92605c12aa94efc988759291c
Author: lihangyu <[email protected]>
AuthorDate: Mon Sep 21 10:20:21 2026 +0800
[improvement](variant) Reduce Variant array shredding CPU during import
(#67983)
### What problem does this PR solve?
Issue Number: None
Related PR: #67551
Problem Summary:
Variant V2 import spends most of its BE CPU in the segment writer's
Variant shredder. Importing 1M rows of sparse JSON (about 20 of 2,000
keys per row: BIGINT, string, double, boolean, BIGINT arrays, and small
objects) into a default `VARIANT` table through a `local()` TVF used
49.94 BE CPU seconds, and 70.1% of the BE samples were in
`VariantShredder::append`. Two defects in the ARRAY path of
`VariantPathBuilder` caused most of it.
1. **Array type reuse never matched.** `infer_type()` tries to reuse the
path's existing `DataTypeArray` when the element type is unchanged.
`DataTypeArray` always wraps its element in `Nullable`, but inferred
element types are never nullable, so `Nullable(BIGINT).equals(BIGINT)`
failed for every array value. Each value then went through
`path_least_common_type()` and `get_least_supertype_jsonb()`, which
builds temporary `DataTypes` vectors, strips `Nullable`, and allocates a
new `DataTypeNullable` only to arrive at the same type. The first commit
compares the unwrapped element type.
2. **Static data types were copied per value.** Memtable flushes of
several tablets shred concurrently. For every ARRAY value the builder
copied process-wide static data types by value: `infer_type()` returned
each element's static type, and `append_array()` and
`value_is_representable()` unwrapped the element with
`remove_nullable()`. Each copy is an atomic reference-count update on a
control block shared by all flush threads. With eight concurrent segment
writers, `perf annotate` put most of `infer_type()`'s own samples, a
third of `value_is_representable()`'s, and a fifth of `append_value()`'s
on lock-prefixed reference-count instructions. The second commit returns
scalar element types by reference, resolves a common element type only
when elements differ, and borrows the unwrapped array element.
Inferred and promoted types are unchanged in both commits.
Results, RELEASE build, one single-node cluster, binaries swapped
between runs in two interleaved rounds, medians of 6 imports each:
| Build | BE CPU s | Wall s | Rows per BE CPU s | Shredder share of BE
CPU |
|---|---|---|---|---|
| master | 49.94 | 8.41 | 20,026 | 70.1% |
| + array type reuse | 38.19 | 7.18 | 26,185 | 51.1% |
| + borrowed static types | 24.71 | 5.61 | 40,461 | 22.4% |
The imported data checksum is identical across all 18 imports.
`get_least_supertype_jsonb()` dropped from 21.6% of BE CPU to not
sampled.
`BM_VariantSparseImport` from #67551, thread CPU seconds per 1M rows,
medians of 5 samples:
| Scenario | master | + array type reuse | + borrowed static types |
|---|---|---|---|
| MixedTypes, 8 concurrent writers | 25.66 | 18.81 | 11.23 |
| MixedTypes, 1 writer | 12.09 | 10.56 | 10.84 |
| NoArrays, 8 concurrent writers | 8.62 | 8.74 | 8.93 |
With both commits the eight-writer cost matches the single-writer cost.
The single-writer and NoArrays rows vary by about 10% between runs on
the shared test host.
### Release note
None
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [x] Regression test
- [x] Unit Test
- [x] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
Validated on base 17ac3d9275f; this branch is rebased onto a newer
master whose two additional commits do not touch these files.
- Unit test:
`VariantPathBuilderTest.*:VariantShredderTest.*:VariantColumnWriterReaderTest.*`,
78 passed and 2 skipped (skipped on master too), including the new
`VariantPathBuilderTest.ArrayPathReusesElementTypeAcrossRows`.
- Regression test on a RELEASE cluster with each commit: `variant_p0`
suites `regression_test_variant`, `regression_test_variant_types`,
`test_variant_array_subscript`,
`regression_test_variant_array_with_predicate`,
`test_variant_array_function`, `variant_compute_v2`,
`regression_test_variant_multi_var`,
`regression_test_variant_predefine_schema` (10 suite files), all passed.
- Manual test: the 1M-row import and `BM_VariantSparseImport`
comparisons above; `build-support/check-format.sh` and
`build-support/check-build-hygiene.sh` pass. clang-tidy on the changed
test file reports nothing; on `variant_path_builder.cpp` the clang
static analyzer crashes on unchanged `__int128` code, and without
`clang-analyzer-*` the only diagnostic is the existing cognitive
complexity of the unchanged `VariantPathBuilder::append`.
- Behavior changed:
- [x] No.
- [ ] Yes. <!-- Explain the behavior change -->
- Does this need documentation?
- [x] No.
- [ ] Yes. <!-- Add document PR link here. eg:
https://github.com/apache/doris-website/pull/1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR
should merge into -->
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude <[email protected]>
---
.../segment/variant/v2/variant_path_builder.cpp | 58 ++++++++++++++------
.../variant/variant_column_writer_reader_test.cpp | 64 ++++++++++++++++++++++
2 files changed, 104 insertions(+), 18 deletions(-)
diff --git a/be/src/storage/segment/variant/v2/variant_path_builder.cpp
b/be/src/storage/segment/variant/v2/variant_path_builder.cpp
index 9c8704cc4b3..5d75ad5750d 100644
--- a/be/src/storage/segment/variant/v2/variant_path_builder.cpp
+++ b/be/src/storage/segment/variant/v2/variant_path_builder.cpp
@@ -219,8 +219,16 @@ const DataTypePtr& cached_decimal_type(uint32_t scale) {
return types[scale];
}
-DataTypePtr infer_type(VariantRef value, const DataTypePtr& reusable_type =
nullptr) {
- const ValueKind kind = value_kind(value);
+const DataTypePtr& array_element_type(const DataTypeArray& array) {
+ // DataTypeArray always wraps its element in Nullable. Borrow the element
instead of copying it
+ // through remove_nullable(): element types are process-wide statics
shared by concurrent
+ // flushes, so each shared_ptr copy is a contended reference-count update.
+ return assert_cast<const
DataTypeNullable&>(*array.get_nested_type()).get_nested_type();
+}
+
+// Every scalar storage type is a process-wide static, so it is returned by
reference.
+const DataTypePtr& infer_scalar_type(VariantRef value, ValueKind kind) {
+ DORIS_CHECK(kind != ValueKind::ARRAY);
switch (kind) {
case ValueKind::NULL_VALUE:
return nothing_type();
@@ -280,8 +288,18 @@ DataTypePtr infer_type(VariantRef value, const
DataTypePtr& reusable_type = null
case ValueKind::ARRAY:
break;
}
+ __builtin_unreachable();
+}
+
+DataTypePtr infer_type(VariantRef value, const DataTypePtr& reusable_type =
nullptr) {
+ const ValueKind kind = value_kind(value);
+ if (kind != ValueKind::ARRAY) {
+ return infer_scalar_type(value, kind);
+ }
- DataTypePtr element_type;
+ // Borrow the static element types and only resolve a common type when
elements differ.
+ DataTypePtr promoted_element;
+ const DataTypePtr* element_type = nullptr;
const uint32_t element_count = value.num_elements();
for (uint32_t index = 0; index < element_count; ++index) {
const VariantRef element = value.array_at(index);
@@ -291,17 +309,18 @@ DataTypePtr infer_type(VariantRef value, const
DataTypePtr& reusable_type = null
element.basic_type() == VariantBasicType::OBJECT)) {
return jsonb_type();
}
- DataTypePtr inferred = infer_type(element);
+ const DataTypePtr& inferred = infer_scalar_type(element, element_kind);
if (inferred->get_primitive_type() == INVALID_TYPE) {
continue;
}
- element_type = element_type == nullptr ? std::move(inferred)
- :
path_least_common_type(element_type, inferred);
- }
-
- if (element_type == nullptr) {
- element_type = nothing_type();
+ if (element_type == nullptr) {
+ element_type = &inferred;
+ } else if (element_type->get() != inferred.get()) {
+ promoted_element = path_least_common_type(*element_type, inferred);
+ element_type = &promoted_element;
+ }
}
+ const DataTypePtr& resolved_element = element_type == nullptr ?
nothing_type() : *element_type;
// A path commonly sees the same ARRAY element type on every row. Reuse
the builder's
// DataTypeArray in that case instead of allocating a temporary shared_ptr
per value. The
@@ -310,17 +329,20 @@ DataTypePtr infer_type(VariantRef value, const
DataTypePtr& reusable_type = null
if (const auto* reusable_array =
reusable_type == nullptr ? nullptr
: typeid_cast<const
DataTypeArray*>(reusable_type.get())) {
- const DataTypePtr& reusable_element =
reusable_array->get_nested_type();
- if (reusable_element.get() == element_type.get() ||
- reusable_element->equals(*element_type)) {
+ // Inferred element types are never nullable, so compare against the
unwrapped element;
+ // otherwise every ARRAY value would miss the equality check and pay
+ // get_least_supertype_jsonb() only to rebuild the same type.
+ const DataTypePtr& reusable_element =
array_element_type(*reusable_array);
+ if (reusable_element.get() == resolved_element.get() ||
+ reusable_element->equals(*resolved_element)) {
return reusable_type;
}
- DataTypePtr common_element = path_least_common_type(reusable_element,
element_type);
+ DataTypePtr common_element = path_least_common_type(reusable_element,
resolved_element);
if (reusable_element->equals(*common_element)) {
return reusable_type;
}
}
- return std::make_shared<DataTypeArray>(element_type);
+ return std::make_shared<DataTypeArray>(resolved_element);
}
bool is_small_or_regular_integer(PrimitiveType type) {
@@ -479,8 +501,8 @@ bool value_is_representable(VariantRef value, const
DataTypePtr& target_type) {
if (kind != ValueKind::ARRAY) {
return false;
}
- const DataTypePtr element_type =
- remove_nullable(assert_cast<const
DataTypeArray&>(*target_type).get_nested_type());
+ const DataTypePtr& element_type =
+ array_element_type(assert_cast<const
DataTypeArray&>(*target_type));
const uint32_t count = value.num_elements();
for (uint32_t index = 0; index < count; ++index) {
const VariantRef element = value.array_at(index);
@@ -807,7 +829,7 @@ void append_array(VariantRef value, const DataTypePtr&
target_type, IColumn* tar
const auto& array_type = assert_cast<const DataTypeArray&>(*target_type);
auto& array = assert_cast<ColumnArray&>(*target);
auto& elements = assert_cast<ColumnNullable&>(array.get_data());
- const DataTypePtr element_type =
remove_nullable(array_type.get_nested_type());
+ const DataTypePtr& element_type = array_element_type(array_type);
// infer_type() made the first borrowed pass. Revisit the encoded children
only after path type
// promotion is complete, appending directly without an owning recursive
scratch tree.
const uint32_t count = value.num_elements();
diff --git a/be/test/storage/variant/variant_column_writer_reader_test.cpp
b/be/test/storage/variant/variant_column_writer_reader_test.cpp
index 1c4dda3f89c..5148ff14e28 100644
--- a/be/test/storage/variant/variant_column_writer_reader_test.cpp
+++ b/be/test/storage/variant/variant_column_writer_reader_test.cpp
@@ -28,6 +28,7 @@
#include <thread>
#include "common/config.h"
+#include "core/column/column_array.h"
#include "core/column/column_nullable.h"
#include "core/column/column_string.h"
#include "core/column/column_vector.h"
@@ -773,6 +774,69 @@ TEST(VariantPathBuilderTest,
PreservesIncomingArrayWhenInferredDecimalPromotionO
"[9999999999999999999999999999999999999.9]");
}
+TEST(VariantPathBuilderTest, ArrayPathReusesElementTypeAcrossRows) {
+ VariantBatchBuilder value_builder;
+ const auto append_array = [&](auto&& fill) {
+ auto row = value_builder.begin_row();
+ auto array = row.start_array();
+ fill(row);
+ array.finish();
+ row.finish();
+ };
+ append_array([](auto& row) {
+ row.add_float(1.0F);
+ row.add_float(2.0F);
+ });
+ append_array([](auto& row) {
+ row.add_float(3.0F);
+ row.add_null();
+ });
+ append_array([](auto& row) { row.add_null(); });
+ append_array([](auto& row) { row.add_double(4.5); });
+ append_array([](auto& row) { row.add_float(5.0F); });
+ VariantBatchBuilder values = value_builder.finish_batch();
+
+ const auto element_primitive = [](const DataTypePtr& type) {
+ const DataTypePtr array = remove_nullable(type);
+ return remove_nullable(assert_cast<const
DataTypeArray&>(*array).get_nested_type())
+ ->get_primitive_type();
+ };
+ segment_v2::VariantPathBuilder builder(PathInData("metric"));
+ // FLOAT arrays, arrays with null elements, and all-null arrays share the
first row's type.
+ for (size_t row = 0; row < 3; ++row) {
+ ASSERT_TRUE(builder.append(values.value_at(row), row).ok());
+ EXPECT_EQ(builder.promotion_count(), 0) << "row=" << row;
+ ASSERT_EQ(element_primitive(builder.type()), TYPE_FLOAT) << "row=" <<
row;
+ }
+ ASSERT_TRUE(builder.append(values.value_at(3), 3).ok());
+ EXPECT_EQ(builder.promotion_count(), 1);
+ ASSERT_EQ(element_primitive(builder.type()), TYPE_DOUBLE);
+ // A narrower FLOAT element after promotion reuses the promoted DOUBLE
array type.
+ ASSERT_TRUE(builder.append(values.value_at(4), 4).ok());
+ EXPECT_EQ(builder.promotion_count(), 1);
+ ASSERT_EQ(element_primitive(builder.type()), TYPE_DOUBLE);
+
+ ColumnPtr materialized;
+ ASSERT_TRUE(builder.materialize(&materialized).ok());
+ const auto& array = assert_cast<const ColumnArray&>(
+ assert_cast<const
ColumnNullable&>(*materialized).get_nested_column());
+ const auto& elements = assert_cast<const
ColumnNullable&>(array.get_data());
+ const auto& doubles =
+ assert_cast<const
ColumnFloat64&>(elements.get_nested_column()).get_data();
+ const std::vector<std::optional<double>> expected {1.0, 2.0, 3.0,
std::nullopt,
+ std::nullopt, 4.5, 5.0};
+ EXPECT_EQ(std::vector<uint64_t>(array.get_offsets().begin(),
array.get_offsets().end()),
+ (std::vector<uint64_t> {2, 4, 5, 6, 7}));
+ ASSERT_EQ(elements.size(), expected.size());
+ for (size_t index = 0; index < expected.size(); ++index) {
+ SCOPED_TRACE(testing::Message() << "element=" << index);
+ EXPECT_EQ(elements.is_null_at(index), !expected[index].has_value());
+ if (expected[index].has_value()) {
+ EXPECT_DOUBLE_EQ(doubles[index], *expected[index]);
+ }
+ }
+}
+
TEST(VariantPathBuilderTest,
StringifiesArrayWithoutTreatingExistingNullAsCastFailure) {
VariantBatchBuilder value_builder;
auto row = value_builder.begin_row();
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
