morningman opened a new pull request, #69019:
URL: https://github.com/apache/doris/pull/69019
### 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::vector` for every object it checks. A new
`VariantPayloadValidator` keeps that scratch in one `DorisVector` that 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 and `sanity_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 `ColumnVariantV2` takes
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 local
`std::vector` with `reserve(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 a `DorisVector` it reuses across rows.
**The problem, and what it cost.**
| Validating a batch of 4096 object rows | master | with this PR |
|---|---|---|
| Scratch allocations | at least one per object, so at least 4096 per batch,
plus one per nested object | one buffer per batch (one per appender for the
Parquet importer); it grows to the widest path of objects and is then reused |
| Allocator of the scratch (16 bytes per field) | plain `malloc` through
`std::allocator`, which the Doris memory trackers do not see | the Doris
`Allocator`, counted by the thread's memory tracker |
`insert_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 header
`core/value/variant/variant_payload_validator.h`) keeps the spans in one
`DorisVector` used 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, so `validate()`
starts by clearing the stack.
- The class is defined in `variant_field.cpp` next to the checks it shares
with `validate_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 that `core/field.h`, which includes
`variant_field.h`, does not pull in `core/custom_allocator.h` (its
include-closure budget has no slack).
- `ColumnVariantV2::insert_encoded_rows()`,
`ColumnVariantV2::sanity_check()` and the native Arrow reader in
`DataTypeVariantV2SerDe` create one validator per call; `EncodedRowsAppender`
keeps one in its `Impl`, so all the chunks a Parquet reader imports share it.
**Results.**
- The reusable validator accepts and rejects the same 13 payloads as the
one-shot call: nested and wide objects, objects inside arrays, and the corrupt
objects of the existing tests (duplicate offset, overlapping spans, gap,
trailing bytes, unordered keys). A corrupt object nested in field `b` throws
after three entries are on the stack, and the next valid payload still
validates (`ReusableValidatorMatchesOneShotValidation`).
- Validating a 1000-field object and a nested object 4096 times each leaves
the scratch capacity where the first row put it
(`ReusableValidatorReusesScratchAcrossRows`); the appender keeps the same
capacity across two 4096-row chunks
(`EncodedRowsAppenderReusesValidationScratchAcrossChunks`).
- The scratch of a 4096-field object shows up in the thread's memory tracker
(at least 64 KiB while the validator lives) and is released with it
(`ReusableValidatorScratchIsMemTrackerVisible`).
- Each new test catches the regression it is written for: with the appender
back on the one-shot call, only
`EncodedRowsAppenderReusesValidationScratchAcrossChunks` fails; with the
scratch back on `std::vector`, only
`ReusableValidatorScratchIsMemTrackerVisible` fails; without the `clear()` at
the start of `validate()`, `ReusableValidatorMatchesOneShotValidation` stops at
the `DCHECK` that the stack is empty after a valid payload.
- The 233 tests of the Variant suites listed below pass locally, except
`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 <!-- At least one of them must be included. -->
- [ ] Regression test
- [x] Unit Test
- `VariantFieldTest.ReusableValidatorMatchesOneShotValidation`,
`ReusableValidatorReusesScratchAcrossRows`,
`ReusableValidatorScratchIsMemTrackerVisible`,
`ColumnVariantV2Test.EncodedRowsAppenderReusesValidationScratchAcrossChunks`;
the existing `VariantFieldTest`, `ColumnVariantV2Test`, `DataTypeVariantV2*`,
`ArrowFlightVariantTest`, `VariantColumnReaderTest`, `VariantBatchBuilderTest`,
`VariantElementV2EncodedTest` and `JniDataBridgeTest` suites pass.
- [ ] 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? -->
- 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
--
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]
