[eldenmoon]

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?

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.

Performance

Dedicated metadata microbenchmark, RELEASE UT build, same worktree, old = the #70481
deque implementation, new = this PR. Values below are ns per key for 16-byte keys, and
retained is the tracked memory of one registered+sealed dictionary. 64-byte keys show the
same direction (16K keys: register 105 → 113 ns, retained 5.48 MB → 4.42 MB).

keys	register_key (old → new)	duplicate hit (old → new)	seal µs (old → new)	abort ns (old → new)	retained bytes (old → new)
4	106 → 134	15.5 → 14.6	0.2 → 0.2	31 → 31	1.2K → 0.5K
64	80 → 86	8.6 → 8.2	3.6 → 2.5	22 → 20	11.7K → 8.1K
1,024	76 → 83	8.6 → 8.5	83 → 52	22 → 22	179K → 129K
16,384	90 → 92	15.4 → 12.0	2,952 → 1,980	46 → 29	2.86M → 2.06M

• Unique register_key() costs a few extra ns per key at batch scale (2–9% for 64–16K keys;
  the relative gap looks larger only at 4–16 keys, where the absolute cost is ~100 ns/key).
• Duplicate hits, row abort and the existing million-field ASAN UT (22.9 s → 20.9 s) are neutral
  or better; seal() is 25–35% faster because the key objects are contiguous.
• Retained metadata memory is ~30% lower (no per-key PaddedPODArray padding or deque blocks).
• The ASAN per-key gap (~20%) is amplified by sanitizer instrumentation; the RELEASE numbers above
  are the ones that matter for ingestion.

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: [fix](variant) Preserve memory errors in batch and path builders #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

[eldenmoon]

run buildall

[eldenmoon]

/review

[github-actions]

Codex automated review failed and did not complete.

Error: Your access token could not be refreshed because your refresh token was already used. Please log out and sign in again.
Workflow run: https://github.com/apache/doris/actions/runs/43131406964

This credential version is excluded from future reviews. Replace its refresh token to restore eligibility. Please trigger /review again; another configured account may be available.

[eldenmoon]

run external

[eldenmoon]

run buildall

[eldenmoon]

/review

[github-actions]

Reviewed the full four-file aggregate diff at de768c1, including the stacked changes. No new actionable findings or existing blocking inline comments were identified. No additional user focus was supplied.

Critical checkpoint conclusions:

• Goal and evidence: tracked, two-phase metadata key registration and allocation-free scratch release preserve successful encoding; path allocation errors now bypass JSONB fallback. Added unit tests directly cover these mechanisms by inspection.
• Scope: changes are confined to the two Variant implementations and their unit tests; the aggregate also includes the stacked cleanup/error-propagation fixes.
• Concurrency and locks: these builders use staged, per-owner mutation with one active row. No new threads, production shared state, locks or lock-order changes.
• Lifecycle and initialization: key views survive nonthrowing buffer moves; map erasure precedes freeing key bytes. Terminal guards and finalized counters protect the reset metadata implementation. The empty-key sentinel is constexpr, with no dynamic initialization dependency.
• Configuration: no new configuration; test hooks restore existing debug-point settings.
• Compatibility: dictionary sorting, temporary/final IDs, offsets, version header and encoded value layout are preserved. No external symbol or persistent-format change requiring mixed-version handling was found.
• Thrift/protobuf: no schema, field-ID or enum changes.
• Parallel paths: direct registration, imported values, row abort and previous-object key reuse were traced; scalar, array, JSONB, ordinary/doc shredding and extracted-subcolumn writer error paths were covered.
• Conditional checks: geometric growth and empty-key branches have concrete rationale. Both memory error codes explicitly bypass scalar conversion fallback; null-map publication now occurs outside that catch.
• Test coverage: inspected empty keys, key-address stability, allocation rejection during byte copy/vector growth/map growth, abort rollback, scratch release and failed-seal terminal behavior, plus existing canonicalization, validation and large-dictionary cases. The new path injection covers MEM_ALLOC_FAILED at both sites; MEM_LIMIT_EXCEEDED was checked in source.
• Expected results: unit expectations match the inspected encoding/error flow; no regression result files changed.
• Observability: failures retain Exception/Status propagation through existing caller boundaries; no new operational metrics requirement was identified.
• Transactions and persistence: no edit-log, version-visibility, delete-bitmap, commit or failover protocol changes.
• Write atomicity and memory: owned buffers use tracked allocators. Map growth was checked against the pinned phmap v1.3.8 implementation: allocation precedes table replacement. Failed path appends can leave scratch partial, but the shredder latches failure and extracted writers propagate it to abort the enclosing write before publication.
• FE/BE variables: no new transmitted variables or sending paths.
• Performance: outer key storage grows geometrically and moves buffer owners without copying key bytes; no new per-row scan or avoidable allocation was identified. No benchmark claims were independently validated.
• Additional checks: nullable-column ownership, exact-size key-buffer consumers, stale row/scope handles, destruction order and test cleanup were rechecked without an additional issue. No new header-layer, PCH, explicit-instantiation, source-including test or libc API dependency was introduced.

Review converged after two rounds: every assigned reviewer returned NO_NEW_VALUABLE_FINDINGS, all four changed paths were covered, and all risk focuses were dismissed with concrete evidence. No unresolved candidate or static coverage gap remains. Validation was static inspection only: no builds, tests, benchmarks or executable/ABI checks were run.

Review converged after 2 round(s).

[eldenmoon]

Local pipeline review — ✅ PASS

schema: doris-repo-review/v1
status: PASS
pr: apache/doris#70487
commit: de768c17b5df176a2c49bd5af736e2636167ce36
base: 9e598d38789571a7fc30652aa054f035bd4f1184
reviewed_at: 2026-11-12T11:27+08:00
reviewer: eldenmoon
model: gpt-6-astra
effort: xhigh
findings: {blocker: 0, major: 0, minor: 0, nit: 0}
rounds: 1
converged: true

Notes for maintainers

• Read-only source review of the full six-commit stack, with the final metadata refactor distinguished from the five commits inherited from [fix](variant) Preserve memory errors in batch and path builders #70481. No builds, tests, sanitizers, or benchmarks were run in this review; author-reported results were not independently reproduced.
• be/src/core/value/variant/variant_batch_builder.cpp:220 — key-owner capacity grows geometrically. Old/new descriptor arrays coexist during growth, and abort retains descriptor capacity until batch finish/destruction; this review does not claim universally lower peak memory or a reproduced ASAN speedup.
• The original review scripts and document verifier were used with authenticated GitHub connector read snapshots in place of an unauthenticated native gh session. This receipt was rendered with --dry-run for copying; no GitHub comment or review was submitted.

_{Reviewed locally with the doris-repo-review pipeline. Repository policy may accept this receipt for the matching commit; it is not a human Apache approval.}

[eldenmoon]

run p0

[eldenmoon]

run cloud_p0

[eldenmoon]

run vault_p0

[eldenmoon]

run external

[eldenmoon]

run nonConcurrent

[eldenmoon]

run external

[eldenmoon]

run p0

[eldenmoon]

run external

[eldenmoon]

run nonConcurrent

[eldenmoon]

run p0

[eldenmoon]

run cloud_p0

[eldenmoon]

run buildall

[eldenmoon]

run cloud_p0