Repository navigation
[improvement](be) Reuse memory-tracked scratch when validating Variant payloads - #69019
Merged
morningman merged 2 commits intoOct 15, 2026
Merged
Conversation
…t payloads ### What problem does this PR solve? Issue Number: None Related PR: apache#68959 Problem Summary: validate_variant_payload() checks that the field values of every Variant object exactly tile the object's value section. To do that, require_valid_object() collected the byte spans of the field values into a fresh std::vector for every object it checked, nested ones included, and freed it when the check returned. ColumnVariantV2::insert_encoded_rows() (JNI readers, inserted encoded values, Variant element extraction), the EncodedRowsAppender that apache#68959 uses to import unshredded Parquet Variant values, and the native Arrow Variant reader validate every row this way. A 4096-row batch of object rows therefore paid at least 4096 allocations, and the scratch (16 bytes per field) came from plain malloc, which Doris memory tracking does not see. VariantBatchBuilder, which that importer replaced, checked the same intervals in a reused, tracked DorisVector. Add VariantPayloadValidator, which keeps the spans in one DorisVector used as a stack: an object pushes the span of each field after checking the field, so a nested object pushes above its ancestors and pops back before it returns. The checks, their order and their errors are unchanged. The batch callers hold one validator for the batch, and the encoded-row appender holds one for its whole life; validate_variant_payload() stays as the one-shot form for a single value. The class is declared in its own header so that core/field.h, which includes variant_field.h, does not pull in core/custom_allocator.h. ### Release note None ### Check List (For Author) - Test: Unit Test - VariantFieldTest: the reusable validator accepts and rejects the same payloads as the one-shot call, including a corrupt nested object followed by a valid one; its scratch capacity stays the same over 4096 rows; the scratch is visible to the memory tracker - ColumnVariantV2Test.EncodedRowsAppenderReusesValidationScratchAcrossChunks - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Reviewed all seven changed files at head 714d816. One new P2 finding: removing the known-count span reservation adds repeated allocation and relocation work in existing row-wise deserialization, where both validators are fresh for each row. The inline comment describes the concrete caller chain and fix direction. No existing inline findings or blocking comment IDs apply.
The review converged after 2 rounds: both general reviewers and the separate recursive-stack/memory reviewer returned NO_NEW_VALUABLE_FINDINGS in the latest round; main independently verified the final finding, full changed-file coverage, and all risk dispositions. No unresolved candidate or uncovered changed-file scope remains. No additional user focus was supplied.
Critical checkpoint conclusions:
- Goal and evidence: batch/appender scratch is reused and Doris-accounted. Four new unit tests inspect correctness/recovery, reuse across rows/chunks, and tracked allocation/release; the fresh-caller performance regression remains the P2 finding.
- Scope: the seven-file change is focused on reusable validation and its tests.
- Concurrency/locks: scratch is local to synchronous calls or locally owned appenders; existing Parquet materialization locking is unchanged. No new shared state, thread, or lock order.
- Lifecycle/static initialization: RAII releases capacity; appender moves transfer its unique Impl; actual appenders end within their invoking context. Spans contain only offsets/sizes. No new global initializer or ownership cycle.
- Configuration: no additions or dynamic-update behavior.
- Compatibility: payload/metadata/column formats, validation errors and the one-shot public signature are preserved; no execution-version gate is needed for ephemeral scratch.
- Thrift/protobuf: no definitions or IDs changed.
- Parallel paths: explicit/compact metadata, sanity checks, JNI, native Arrow and both Parquet appender users were traced. Remaining one-shot and single-row callers expose the reported growth regression.
- Conditional checks: rejection conditions remain unchanged. The stack example and failed-call reset comment match the implementation; nested failures safely reset before reuse.
- Test coverage: inspected wide/nested/array/empty-object, malformed span/key, recovery, chunk reuse and tracker tests, plus existing framing/depth/primitive/UTF-8/COW/Arrow cases. Current reuse assertions miss repeatedly fresh validator allocation work.
- Test results: no golden files changed; new unit expectations match their encoded inputs and owner lifetimes by inspection.
- Observability: existing errors, Arrow error conversion and Parquet import timing/counters remain sufficient for this internal change.
- Transactions/persistence/failover: no transaction, visibility-version, delete-bitmap, EditLog or persistence protocol changes.
- Writes/atomicity: new scratch allocations and input validation remain before destination mutation; Arrow publishes a validated temporary batch. Existing COW ownership and null handling are preserved.
- FE-BE variables: none added or changed.
- Performance: retained validators avoid per-row scratch allocation after warmup; fresh wide-object validators repeatedly grow from zero, as reported inline. No throughput magnitude was measured.
- Other/module checks: recursive growth keeps only numeric base indices, constructs spans after children return, and restores ancestor state. No additional correctness issue found. The pure-text build-hygiene gate passed header layering/budgets/PCH, extern-template pairing and unity-skip checks.
Core invariants: malformed input still fails by exception/status and significant scratch is tracked by Doris allocation; committed-data visibility, partition versions and MoW state are untouched. This was a static review: no builds, unit/regression tests, ASAN/fault injection, final ELF ABI audit, or benchmarks were executed.
Review converged after 2 round(s).
| std::vector<ObjectValueSpan> spans; | ||
| spans.reserve(count); | ||
| // Entries below base belong to the ancestors of this object; see _object_spans. | ||
| const size_t base = _object_spans.size(); |
Contributor
There was a problem hiding this comment.
[P2] Preserve the known-count reservation for fresh validators
For wide flat objects, replacing spans.reserve(count) with unreserved pushes adds repeated allocation and span-copy work in callers that never reuse a validator. In particular, ColumnVariantV2::deserialize() loops through insert_data(), which creates a fresh validator in parse_canonical_serialized() and another in its single-row insert_encoded_rows() call; both scratch buffers are destroyed on every row. These paths now repeatedly grow from zero instead of making the previous single span allocation per validation, and the batch-reuse tests miss this regression. Reserve capacity for this object's base + count spans before the loop while keeping it reusable.
Contributor
Author
There was a problem hiding this comment.
Fixed in 54c9572. require_valid_object() reserves the object's base + count spans before it checks the fields, taking at least twice the current capacity. A fresh validator therefore allocates once for a flat object (1000 fields: one allocation of 1000 instead of growing 1, 2, 4, ..., 1024). Taking at least twice the capacity matters for siblings: with an exact base + count, a root of 26 objects of 30 fields would reallocate for every sibling once k + 30 passes the capacity; this grows 26 -> 52 -> 104.
VariantFieldTest.FreshValidatorReservesAnObjectOnce pins both: capacity 1000 after the flat object and 104 after the nested one. Without the reservation it reads 1024 and 64, and with an exact reservation the nested case reads 55.
… fields The reusable validator dropped the reservation the one-shot validator made for each object. Callers that still validate every value with a fresh validator, such as ColumnVariantV2::deserialize() through insert_data(), then grew the span buffer from zero for each wide object: about ten allocations for an object of 1000 fields instead of one. require_valid_object() now reserves base + count spans before checking the fields. It takes at least twice the current capacity, so sibling objects that each need a little more space do not reallocate one entry at a time: in a root of 26 objects of 30 fields, field k needs k + 30 entries, and the buffer grows 26 -> 52 -> 104 instead of once per sibling. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 29435 ms |
Contributor
TPC-DS: Total hot run time: 153701 ms |
Contributor
ClickBench: Total hot run time: 24.6 s |
Contributor
There was a problem hiding this comment.
Reviewed all seven changed files at head 54c9572e943213f7c5e9189c0e9dca01d3100ef4, their upstream/downstream callers, parallel validation paths and tests. No new verified finding. Review converged in 1 round: both complete reviewers and the separate risk reviewer returned NO_NEW_VALUABLE_FINDINGS; all candidates and risk questions were independently reconciled and dismissed with evidence.
The existing P2 reservation discussion is addressed at this head: fresh objects reserve their known field count before traversal, and nested siblings grow capacity geometrically (26 -> 52 -> 104). No existing P0/P1 finding applies. No additional user focus was supplied.
Critical checkpoint conclusions:
- Goal/tests: source implements reused, tracked scratch at all changed batch callers. New unit tests assert acceptance/rejection, failed-call reset, capacities, chunks and tracking. No builds or unit/regression tests were run in this review environment.
- Scope: seven files; a separate validator header avoids adding allocator closure to variant_field.h/core/field.h; no unrelated runtime behavior changes identified.
- Concurrency/locks: no new threads, locks, static shared validator, or cross-column state. Per-call validators are local; appender state follows existing exclusive scoped adapter contract.
- Lifecycle/static initialization: local validators own the vector; appender owns it through unique Impl. Production appender callers create/use/destroy lexically within Parquet conversion calls. No new global initializer or ownership cycle.
- Configuration: no additions or dynamic-setting changes.
- Compatibility: Variant Encoding V1 and canonical bytes untouched, metadata/order/size/error checks preserved, one-shot public symbol retained; no exec-version gate needed for scratch-only refactor. No new libc API/toolchain change identified; release ELF audit not performed.
- Thrift/protobuf: none changed.
- Parallel paths: encoded rows compact/explicit metadata, appender, Arrow and sanity all use batch-local scratch. VariantField/canonical single-value wrappers deliberately keep the same one-shot entry point. Existing typed/shredded paths and builder remain separate, appropriate to their ownership.
- Conditions/comments: changed reserve has concrete flat/sibling examples; validate clear documents exception leftovers; stack header documents ancestor/child example. Existing correctness checks and failures remain in place.
- Test coverage: added tests plus existing noncanonical offset, corruption, maximum-depth, invalid encoded insert/appender and Arrow guards were statically reviewed. Direct runtime allocation-fault and exhaustive end-to-end execution are not evidenced here.
- Results: no regression golden/output files changed; new expected capacity and boolean outcomes match source logic by inspection. Author-reported local test results are not this review's test evidence.
- Observability: existing corruption and allocation failure exceptions retained; refactor adds no new distributed operation requiring logs/metrics.
- Transactions/persistence: no visibility, rowset/delete-bitmap, EditLog, transaction or failover logic touched.
- Data writes/atomicity: scratch mutation only; encoded destination validation still precedes typed/shredded conversion or bytes append, Arrow still validates a temporary batch first. Allocation/error exceptions unwind local scratch; production Parquet scope releases appender on return/throw.
- FE-BE variables: no new variables or send paths.
- Performance/memory: DorisVector uses CustomStdAllocator/Allocator consume/release; geometric reservation retains capacity across rows/chunks while maintaining known-count reservation for fresh objects. Object span sort complexity is unchanged; no references into scratch survive child reallocation.
- Other/BE hygiene: new members' definitions stay in existing variant_field.cpp Core target; no source-including tests or new instantiation/pch/gate-table edits. Pure-text hygiene passed and full integrations result reconciled; compiled linking/runtime behavior remain unverified by contract.
Validation limits: static source review and pure-text build hygiene only. No builds, unit/regression tests, runtime allocation-fault checks or release-artifact audits were run. No unresolved static candidate or uncovered changed-file scope remains.
Review converged after 1 round(s).
Contributor
TPC-H: Total hot run time: 28636 ms |
Contributor
TPC-DS: Total hot run time: 151754 ms |
Contributor
ClickBench: Total hot run time: 24.51 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Gabriel39
approved these changes
Oct 15, 2026
Contributor
|
PR approved by anyone and no changes requested. |
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
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
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.
What problem does this PR solve?
Issue Number: None
Related PR: #68959
Problem Summary:
In short. Validating a Variant value no longer allocates a fresh, untracked
std::vectorfor every object it checks. A newVariantPayloadValidatorkeeps that scratch in oneDorisVectorthat every later row reuses, and the callers that validate whole batches (encoded-row inserts, the encoded-row appender that #68959 uses to import unshredded Parquet Variant values, the native Arrow Variant reader andsanity_check) now hold one validator per batch, or for the appender's whole life. This was raised in the review of #68959.Background. Encoded Variant bytes that come from outside (Parquet files, JNI readers, Arrow batches) are validated before a
ColumnVariantV2takes them. For an object,validate_variant_payload()checks that the field values exactly tile the object's value section: it collects the byte span of every field value, sorts the spans by offset and rejects overlaps, gaps and trailing bytes.require_valid_object()collected those spans into a localstd::vectorwithreserve(field_count), so every object it checked, nested objects included, allocated once and freed the vector when its check returned.VariantBatchBuilder, which the Parquet importer used before #68959, checked the same intervals in aDorisVectorit reuses across rows.The problem, and what it cost.
mallocthroughstd::allocator, which the Doris memory trackers do not seeAllocator, counted by the thread's memory trackerinsert_encoded_rows()(JNI readers, inserted encoded values, Variant element extraction), the encoded-row appender and the native Arrow Variant reader validate every row, so this is per-row work on the read path.How this PR fixes it.
VariantPayloadValidator(new headercore/value/variant/variant_payload_validator.h) keeps the spans in oneDorisVectorused as a stack. An object pushes the span of each field after checking that field, so a nested object pushes its spans above those of its ancestors, sorts and checks only its own range and pops back before returning. The checks, their order and their error messages are unchanged. A call that throws leaves the entries of its unfinished objects behind, sovalidate()starts by clearing the stack.variant_field.cppnext to the checks it shares withvalidate_variant_payload(), which remains the one-shot form for a single value (VariantField::from_ref/from_bytes,parse_canonical_serialized). It is declared in its own header so thatcore/field.h, which includesvariant_field.h, does not pull incore/custom_allocator.h(its include-closure budget has no slack).ColumnVariantV2::insert_encoded_rows(),ColumnVariantV2::sanity_check()and the native Arrow reader inDataTypeVariantV2SerDecreate one validator per call;EncodedRowsAppenderkeeps one in itsImpl, so all the chunks a Parquet reader imports share it.Results.
bthrows after three entries are on the stack, and the next valid payload still validates (ReusableValidatorMatchesOneShotValidation).ReusableValidatorReusesScratchAcrossRows); the appender keeps the same capacity across two 4096-row chunks (EncodedRowsAppenderReusesValidationScratchAcrossChunks).ReusableValidatorScratchIsMemTrackerVisible).EncodedRowsAppenderReusesValidationScratchAcrossChunksfails; with the scratch back onstd::vector, onlyReusableValidatorScratchIsMemTrackerVisiblefails; without theclear()at the start ofvalidate(),ReusableValidatorMatchesOneShotValidationstops at theDCHECKthat the stack is empty after a valid payload.ColumnVariantV2Test.TypedComparisonBoundariesMatchEncodedWithoutMaterializing, which also fails on master on the macOS machine used here and passes in CI.Release note
None
Check List (For Author)
Test
VariantFieldTest.ReusableValidatorMatchesOneShotValidation,ReusableValidatorReusesScratchAcrossRows,ReusableValidatorScratchIsMemTrackerVisible,ColumnVariantV2Test.EncodedRowsAppenderReusesValidationScratchAcrossChunks; the existingVariantFieldTest,ColumnVariantV2Test,DataTypeVariantV2*,ArrowFlightVariantTest,VariantColumnReaderTest,VariantBatchBuilderTest,VariantElementV2EncodedTestandJniDataBridgeTestsuites pass.Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)