branch-4.2: [improvement](variant) Reduce Variant array shredding CPU during import #67983 - #68296
Open
github-actions[bot] wants to merge 1 commit into
Open
github-actions[bot] wants to merge 1 commit into
github-actions[bot] wants to merge 1 commit into
Conversation
…rt (#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 17ac3d9; 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: apache/doris-website#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 <noreply@anthropic.com>
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
|
run buildall |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cherry-picked from #67983