eldenmoon opened a new pull request, #70487:
URL: https://github.com/apache/doris/pull/70487
> **Stacked on #70481.** The first five commits belong to that PR; only
> `[refactor](variant) Make Variant metadata key registration
allocation-safe` is new. Please
> review the last commit. Once #70481 merges this branch will be rebased
onto master and the diff
> will shrink to just that commit.
### Background
VariantBatchBuilder collects one encoding unit in a VariantMetadataBuilder.
The builder interns
every object key once, maps its bytes to a temporary key id while rows are
being collected, and
`seal()` then sorts the keys, fixes the temporary-to-final id mapping and
encodes the dictionary.
Aborting a row removes the keys that row added.
### What problem does this PR solve?
Issue Number: None
Related PR: #70481 (fixes the cleanup path this change builds on)
PR Type: Refactor
Problem Summary: `register_key()` appended a `PaddedPODArray` key to a
`std::deque` and then
inserted its map entry inside a `try/catch` that popped the key again. The
rollback was correct,
but it coupled the two containers through exceptions, and the deque's
default constructor can
allocate in a `noexcept` cleanup path. Store each key in a
`DorisVector<char>` owned by a
`DorisVector`, and register in two phases: copy the bytes and reserve
key-vector capacity first,
publish the map entry second, and move the byte buffer into the vector last.
That move cannot
allocate or throw, so the catch block and the deque are both gone. The
encoded metadata,
dictionary ordering, temporary/final id mapping and row-abort semantics are
unchanged.
### Details
- **Moves vs. logic changes:** Only the in-memory key storage and the
registration order change.
The V2 encoding, unsigned-UTF-8 sort, duplicate handling, UTF-8
validation, id remapping and
abort semantics are untouched.
- **Behavior change:** None.
- **Compatibility:** Existing segments and readers are unaffected because
the encoded dictionary
stays byte-for-byte identical.
- **Performance impact:** The existing million-field ASAN unit test dropped
from 22.9 s to 20.9 s
on the same machine. The key vector grows geometrically; reserving `size +
1` on every
registration would make `std::vector` reallocate for every key.
- **Rollback:** Revert this commit to restore the deque plus `try/catch`
implementation.
### Testing
- `./run-be-ut.sh --run
--filter="VariantBatchBuilderTest.*:VariantMetadataBuilderTest.*"` (ASAN):
49 tests passed, including new empty-key, key-address-stability, abort and
injected-OOM cases.
- `./run-be-ut.sh --run
--filter="VariantBatchBuilderTest.*:VariantBatchBuilderAddValueTest.*:VariantMetadataBuilderTest.*:VariantPathBuilderTest.*:VariantCanonicalTest.*:VariantGoldenVectorTest.*:VariantFieldTest.*:ColumnVariantV2Test.*"`:
175 tests passed.
- `build-support/check-format.sh` and `build-support/check-build-hygiene.sh`
passed.
- Million-key registration A/B on the same ASAN build: 22.9 s before, 20.9 s
after.
- clang-tidy could not complete in this environment because its libstdc++
include path misses
`stddef.h`; the diagnostics it did emit are on lines outside this diff.
### Notes for Reviewers
- **phmap growth is safe to rely on here.** `raw_hash_set::resize()`
allocates the replacement
table before it replaces `ctrl_`/`slots_`/`capacity_`, and
`prepare_insert()` only mutates the
table after that allocation succeeds. Entries move with a non-throwing
transfer. The new
`static_assert(std::is_nothrow_move_constructible_v<KeyMapValue>)` keeps
that assumption
checked.
- **Do not reserve `size + 1` on every registration.**
`std::vector::reserve()` allocates exactly
the requested capacity, so that would reallocate and move every previous
key on every insert.
The code grows geometrically only when the vector is full; the comment in
`register_key()`
records why.
- **Empty keys need a real address.** `DorisVector<char>` has a null
`data()` for an empty key,
while the map entry and `StringRef` hashing/comparison still need a stable
pointer. A single
sentinel byte represents every empty key, and `seal()` skips the
zero-length
`PaddedPODArray::insert`.
- **Scope.** The VariantPathBuilder publication checks (pre-reserving the
null map and validating
nested-column sizes before publishing) are intentionally not in this PR:
#70481 already covers
memory-error propagation there, and those checks touch a different module.
### Release note
None
### Check List (For Author)
- Behavior changed: No
- Does this need documentation: No
- Pre-submission status: Ready
--
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]
