From 1de114b6c60c89de2552a7d593783e8d7e5cf262 Mon Sep 17 00:00:00 2001 From: lihangyu Date: Mon, 21 Sep 2026 10:20:21 +0800 Subject: [PATCH] [improvement](variant) Reduce Variant array shredding CPU during import (#67983) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### 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 - [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 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. - Does this need documentation? - [x] No. - [ ] Yes. ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude --- .../variant/v2/variant_path_builder.cpp | 58 +++++++++++------ .../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 ea609d3bdacd47..efb68f5f6325c2 100644 --- a/be/src/storage/segment/variant/v2/variant_path_builder.cpp +++ b/be/src/storage/segment/variant/v2/variant_path_builder.cpp @@ -215,8 +215,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(*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(); @@ -272,8 +280,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); @@ -283,17 +301,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 @@ -302,17 +321,20 @@ DataTypePtr infer_type(VariantRef value, const DataTypePtr& reusable_type = null if (const auto* reusable_array = reusable_type == nullptr ? nullptr : typeid_cast(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(element_type); + return std::make_shared(resolved_element); } bool is_small_or_regular_integer(PrimitiveType type) { @@ -458,8 +480,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(*target_type).get_nested_type()); + const DataTypePtr& element_type = + array_element_type(assert_cast(*target_type)); const uint32_t count = value.num_elements(); for (uint32_t index = 0; index < count; ++index) { const VariantRef element = value.array_at(index); @@ -769,7 +791,7 @@ void append_array(VariantRef value, const DataTypePtr& target_type, IColumn* tar const auto& array_type = assert_cast(*target_type); auto& array = assert_cast(*target); auto& elements = assert_cast(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 fd9cf2086483ee..15530dd6a5875f 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 #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" @@ -809,6 +810,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(*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( + assert_cast(*materialized).get_nested_column()); + const auto& elements = assert_cast(array.get_data()); + const auto& doubles = + assert_cast(elements.get_nested_column()).get_data(); + const std::vector> expected {1.0, 2.0, 3.0, std::nullopt, + std::nullopt, 4.5, 5.0}; + EXPECT_EQ(std::vector(array.get_offsets().begin(), array.get_offsets().end()), + (std::vector {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();