eldenmoon opened a new pull request, #67983:
URL: https://github.com/apache/doris/pull/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)
--
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]
