branch-4.1:[opt](variant) Seek unshredded Parquet Variant paths directly - #66758
branch-4.1:[opt](variant) Seek unshredded Parquet Variant paths directly#66758hubgeter wants to merge 3 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
There was a problem hiding this comment.
Request changes: four blocking issues remain in the complete-unshredded Variant direct-seek path.
Findings, ordered by severity:
- The fast path can accept corrupt scalar or container roots that canonical Variant validation rejects, then publishes them through
append_prevalidated. - Selected-subtree validation resets nesting depth and can accept a depth-129 source that the normal Variant contract rejects.
- The unchanged Iceberg p0 suite still requires old path-miss/reconstruction counters that this branch now intentionally eliminates, so reachable regression assertions fail.
- Independent projections of a wide unshredded container each rescan its complete offset table (and re-sort object offsets), creating multiplicative CPU work versus the former cached materialization.
Critical checkpoints:
- Architecture/layering: the implementation remains within core Variant and the v2 Parquet reader/Profile boundary; I found no v1 decoder, Arrow runtime, or table/file identity boundary violation.
- Correctness and external compatibility: selected-only traversal may intentionally avoid unrelated sibling payloads, but it must still preserve exact root-envelope, scalar-domain, and total-depth invariants. The first two inline findings cover the concrete gaps.
- Lifecycle and resource ownership: the metadata views remain owned by the immutable physical state; filter/range/index selections create fresh states, compatible append COW-detaches and invalidates the cache, and retained aliases retain their backing columns. I found no additional lifetime or allocation-accounting defect.
- Schema/materialization/nulls: mixed normalized-unshredded and typed segments preserve segment order, physical primitive identity, SQL null maps, Variant NULL bytes, and exception-before-publication behavior. No additional issue survived review.
- Filtering and deletes: lazy selection, row alignment, page pruning, and position/equality-delete paths were traced. No result-row defect was found; the stale position-delete Profile assertion is included in the third finding.
- Performance and observability: the new persistent counters are wired through the reader Profile, but they do not prevent the repeated wide-container work in the fourth finding, and existing external counter contracts were not updated.
- Tests: the added unit tests cover direct object/array access, appended metadata dictionaries, selected-sibling validation, malformed object tables, and mixed physical segments. They do not cover corrupt root envelopes, the combined depth boundary, or wide multi-projection reuse. Per the review-runner instructions, no build or test command was run; the unchanged p0 assertions were verified statically against the new control flow.
- User focus: no additional user-provided focus was supplied.
The review converged after three full normal and risk-focused rounds. Every candidate is either represented by an inline finding or dismissed with concrete duplicate/lifecycle/semantic evidence; no suspicious point remains unresolved.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Request changes: one blocking resource/performance regression remains in heterogeneous composite Variant extraction.
Finding:
- A direct unshredded match reused by a heterogeneous composite retains the complete root metadata dictionary in each projected result. The old composite normalization fallback compacted metadata to the selected subtree, so wide dictionaries can now multiply live block memory across projections and make an otherwise valid mixed-file query exceed ColumnString or memory limits.
Critical checkpoints:
- Architecture/layering: the implementation stays within core Variant and the v2 Parquet reader/Profile boundary; I found no v1 decoder, Arrow runtime, table/file identity, or mapper ownership violation.
- Correctness and validation: the current head validates the exact root envelope, every accessed container table, selected-child bounds, and the selected subtree at its root-relative depth. The prior root/depth thread fixes are sound, and no additional corrupt-input acceptance remained.
- Lifecycle and cache ownership: metadata/container views remain backed by immutable physical columns for the cache lifetime. Append and selection paths reset or replace borrowed state under the materialization lock, and entry/depth/promoted-offset budgets are bounded. No additional lifetime or invalidation issue remained.
- Schema/materialization/nulls: homogeneous and heterogeneous segments preserve order, schema-aware primitive identity, outer/leaf null maps, and residual reconstruction fallback. The inline finding is the remaining materialization regression: the value is correct, but its output metadata is no longer compact in mixed composites.
- Filtering and deletes: filter/range/index gathers, lazy predicate scans, page pruning, and Iceberg position-delete alignment preserve row and reconstruction semantics at the reviewed head.
- External compatibility: complete unshredded and projected typed files can legally coexist in the changed Iceberg gather path; that reachable mixed-file state is what exposes the inline finding. No separate Parquet Variant encoding-compatibility defect remained.
- Performance and observability: reusable container lookup state fixes the earlier repeated wide-container scan, but it does not address output metadata copying after a hit.
VariantDirectResidualSeekBytescounts selected value slices and therefore does not expose the retained dictionary cost. - Tests: the changed unit and Iceberg cases cover direct object/array access, malformed roots/tables, depth, cache invalidation/budgets, mixed segment orders, lazy/page/delete paths, and Profile contracts. They verify values and counters but not compact output metadata or allocation. Per the review-runner instructions, no local build or test command was run.
- User focus: no additional user-provided focus was supplied; the whole PR was reviewed.
The review converged in Round 2: every normal and risk-focused reviewer returned NO_NEW_VALUABLE_FINDINGS, the one accepted candidate is represented by the inline comment, and no suspicious point remains unresolved.
| // Traversed containers perform bounded reads. Validate the selected subtree exactly, but | ||
| // intentionally do not visit unrelated siblings in the unshredded root. | ||
| validate_variant_payload(current, current_depth); | ||
| selected_rows.push_back(current); |
There was a problem hiding this comment.
[P1] Compact metadata before combining direct residual results
When a heterogeneous composite reuses this direct unshredded result, each selected scalar/miss still carries the root metadata dictionary into append_prevalidated. Before this change, the composite fallback normalized that segment through normalize_materialized_path, whose VariantBatchBuilder retained only keys reachable from the selected subtree. A wide dictionary is now copied into every projected result (and distinct dictionaries are rehashed/copied per path), so E projections retain O(E * root-metadata bytes) and can hit the block/ColumnString limit even after the container lookup cache hits. Please import the selected value through the compacting builder (empty metadata for scalar/missing results and ID remapping for containers), with a wide-metadata mixed-state test.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Review result: no new actionable findings. Current head f000266 is acceptable for approval.
Critical checkpoints:
- Architecture and layering: the optimization remains within core Variant and the v2 Parquet reader/Profile boundary. It does not introduce v1 decoder, Arrow runtime, table-schema mapping, or file-local identity leakage.
- Correctness and corruption handling: direct seek validates the exact root envelope and scalar semantics, validates every accessed object/array table even on misses or kind mismatches, preserves root-relative nesting depth, validates selected-child bounds, and recursively validates the selected subtree. Only unrelated sibling payloads are intentionally left unvisited.
- Schema, materialization, and null semantics: homogeneous typed segments retain exact physical identity; heterogeneous typed, projected, unshredded, and reconstruction-only segments retain row order and SQL/Variant null distinctions. Direct results compact metadata to reachable keys, and completed unshredded prefixes are not evaluated twice during composite fallback.
- Lifecycle, memory, and performance: borrowed container state is protected by the materialization lock, reset after compatible append, and replaced by fresh state after filter, cut, or gather. Retention is bounded by the documented 16K-entry, four-ancestor, and 4 MiB promoted-offset limits. The current head also fixes the previously reported wide-metadata amplification by re-encoding only the selected subtree.
- Filtering and external compatibility: predicate-only, lazy materialization, page-pruned, projected-root, mixed-file, warm/cold, and position-delete paths preserve row alignment and reconstruction behavior for Iceberg Parquet Variant data.
- Observability: the direct-seek time, row, byte, fallback, build, and hit counters are initialized and forwarded through the Parquet reader profile. The regression helper now distinguishes counters that must exist from counters that must be positive, so intentional zero reconstruction remains testable.
- Tests: focused coverage exercises object/array paths, null and missing values, malformed roots and container tables, maximum depth, distinct appended metadata, cache reuse/invalidation and caps, noncanonical offsets, mixed segment orders, metadata compaction, lazy/page/delete behavior, and Profile contracts. Per the review-runner instruction, no local build or test command was run.
- Existing review context: all five earlier inline concerns are represented in the current head and were not duplicated. The latest metadata-compaction concern is covered by subtree re-encoding plus CompactsWideMetadataForDirectAndCompositeResults.
- User focus: no additional user-provided focus was supplied; the entire PR was reviewed.
The review converged in Round 1: both normal full-review agents and the separate risk-focused agent returned NO_NEW_VALUABLE_FINDINGS after their required rechecks, and the main final sweep found no unresolved candidate.
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)