[enhancement](variant) Support deep Variant object leaf projection - #66575
[enhancement](variant) Support deep Variant object leaf projection#66575Gabriel39 wants to merge 4 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: All Codex review accounts are usage-limited; earliest retry is 2026-08-08T03:32:00Z. Please trigger /review again after that time. |
bd7fc68 to
4c8217f
Compare
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: All Codex review accounts are usage-limited; earliest retry is 2026-08-08T03:32:00Z. Please trigger /review again after that time. |
Gabriel39
changed the title
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review summary
Requesting changes for four P2 issues: the new ancestor-residual gate suppresses valid Variant statistics pruning; row-group physical projection is finalized after page-index and avoided-byte planning; the mixed-row-group unit fixture does not contain residual data that proves fallback I/O; and the Paimon regression asserts eligibility instead of the actual row-group projection outcome.
Critical checkpoint conclusions:
- Architecture and layering: FE path-segment transport, BE access-tree construction, and recursive physical mapping remain coherent; no separate layering or identity defect was substantiated.
- Lifecycle and state: row-group reader construction, dictionary probes, lazy reads, prefetch/reset, and Variant appends use the row-group request once it exists. The accepted planning issue is that this physical shape is produced after OffsetIndex selection and pruning-byte accounting.
- Schema, compatibility, and result correctness: dotted/case-sensitive keys and numeric, repeated, missing, complex, Iceberg, Paimon, and external-writer fallback paths were traced. The remaining fallbacks are conservative; no separate result-materialization defect was found.
- Filtering and pruning: residual row predicates remain in place, but requiring unrelated ancestor overflow to be null regresses otherwise valid footer and page-index pruning.
- Performance and observability: late finalization leaves stale OffsetIndex and
FilteredBytesplanning artifacts, andVariantLeafProjectionsno longer proves that a row group retained leaf projection. - Tests: the patch adds broad unit and integration coverage, but the two inline test findings leave the full-fallback physical branch and the Paimon leaf-retention outcome unproven.
- User focus: no additional focus was provided; the complete 20-file patch was reviewed.
- Validation: static review only, as required by the task; no build, test, or source modification was performed.
Completion status: INCOMPLETE under the bundle's convergence rule. A valuable OffsetIndex finding emerged in the third and final allowed discovery round. It was independently verified, deduplicated into the late-planning comment, and all current candidates are adjudicated, but a fourth convergence round was not permitted.
| typed_object == nullptr || typed_object->kind != ParquetColumnSchemaKind::STRUCT) { | ||
| return std::nullopt; | ||
| } | ||
| fallback_values.push_back(fallback); |
There was a problem hiding this comment.
[P2] Do not gate leaf statistics on unrelated ancestor overflow
The Variant shredding contract makes a partially shredded object's value keys disjoint from the fields represented by its typed_value. Consequently a root/ancestor residual may legitimately be non-null because it contains unrelated fields while this requested descendant is still completely represented by its own wrapper. Adding every ancestor here changes even a shallow v['n'] predicate from checking n.value to also requiring the root residual to be empty, which disables row-group and page-index pruning for valid rows such as {n: 5, extra: 7}. Please keep the all-null proof on the requested field's corresponding value column; ancestor overflow cannot shadow that shredded key in a conforming Parquet Variant.
| request.count_star_placeholder_columns; | ||
| VariantRowGroupProjectionCounts variant_projection_counts; | ||
| if (file_context.contains_variant) { | ||
| const auto predicate_counts = finalize_variant_projections_for_row_group( |
There was a problem hiding this comment.
[P2] Finalize the physical projection before row-group planning
finalize_native_row_group_read_plan() has already run with the leaf candidate before this fallback is applied. If this row group expands to the full Variant wrapper, newly added leaves are absent from the loaded OffsetIndexes, so their readers take the non-OffsetIndex path and walk page gaps even when page pruning produced sparse selected_ranges. The same ordering also makes a group pruned before reaching this line account FilteredBytes from the leaf candidate instead of the full physical request. Please build the row-group-local physical columns before pruning/page-index planning and use that shape for requested leaf IDs and avoided-byte accounting, while keeping the immutable logical request for conjunct semantics.
| auto second_row_group = metadata.row_groups[0]; | ||
| auto& root_residual = second_row_group.columns[2].meta_data.statistics; | ||
| DORIS_CHECK(root_residual.__isset.null_count); | ||
| root_residual.__set_null_count(second_row_group.num_rows - 1); |
There was a problem hiding this comment.
[P2] Exercise real residual data in the fallback row group
This duplicates the first row group's Column Chunks and changes only footer null_count; the underlying terminal residual page is still all null. As a result, the test's rows, typed n values, and projection counters are unchanged even if reader construction or deferred I/O mistakenly keeps using the leaf-only request for the row group marked as full projection. Please use a real second row group with a non-null terminal fallback value and assert the value reconstructed from that branch, so the test fails when the row-group-scoped physical shape is ignored.
| """ | ||
| assertEquals(2, deepProjectionRows.size()) | ||
| String deepProjectionProfile = new ProfileAction(context).getProfileBySql( | ||
| deepProjectionToken, ["VariantLeafProjections"], 30000L, 500L) |
There was a problem hiding this comment.
[P2] Assert the actual row-group projection outcome
VariantLeafProjections is now incremented from the candidate count in ParquetReader::open(), before any row-group residual statistics decide whether the candidate is retained. This fixture also writes tags/extra outside its shredding schema, so the root residual is populated and the current finalizer can full-project every row group while this assertion still passes. Poll VariantLeafProjectionRowGroupColumns (and ideally require VariantFullProjectionRowGroupColumns == 0) against a fully shredded fixture so an always-fallback regression cannot satisfy the test.
|
run buildall |
|
/review |
|
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. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
run buildall |
|
PR approved by at least one committer and no changes requested. |
github-actions
Bot
added
the
approved
|
run buildall |
1 similar comment
|
run buildall |
|
/review |
There was a problem hiding this comment.
Review summary
Requesting changes for one P1 correctness defect and one P2 performance/resource regression:
- Direct
explode(v)access-path propagation erases the array-container transition, allowing a row-group leaf projection to omit root residual arrays that the generator must materialize. - Row-group planning deep-copies complete recursive projection/request state per candidate and retains the projection-bearing plan twice, multiplying planning CPU and memory for many-row-group, wide/deep scans.
Critical checkpoint conclusions:
- Architecture and layering: FE segment comparison, BE access-tree construction, and exact object-key mapping are otherwise coherent, but the legacy untyped
v/kboundary cannot represent that direct Variantexplodeconsumes the root as an array. - Lifecycle and state: eager and queued planning, footer/dictionary/Bloom/PageIndex/OffsetIndex consumers, reader construction, EOF/reset, and aggregate paths use the intended logical or row-group physical request. No separate cursor, lifetime, or stale-plan correctness defect survived.
- Schema, compatibility, and result correctness: for genuine object-key descent, the Parquet Variant shredding contract supports checking only the corresponding terminal fallback. That proof does not apply after generator provenance is erased; valid heterogeneous external files can then fail direct
explode. - Filtering and pruning: footer and page-index statistics remain conservative for supported object paths, including missing, repeated, contradictory, or non-all-null terminal metadata. No distinct pruning defect remains beyond the accepted generator case.
- Performance and observability: pruning totals and the new opened-row-group projection counters are correctly updated. The accepted P2 is the unbounded recursive copy/dual-retention representation, not a counter-total defect.
- Tests: the patch substantially improves mixed-row-group and external Paimon/Iceberg outcome coverage. It still lacks a direct-
explodeheterogeneous shredded fixture and a many-row-group, wide/deep planning regression check. - User focus: no additional focus was provided; all 21 changed files and their production/runtime consumers were reviewed.
- Validation: static review only, as required by the task; no build, test, or source modification was performed.
Completion status: CONVERGED in Round 2. Three full-coverage reviewers and two targeted adversarial reviewers reported no new distinct finding in the convergence round; every candidate is accepted, dismissed, or deduplicated.
|
run buildall |
|
/review |
github-actions
Bot
removed
the
approved
3 participants
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
File Scanner V2 carried Variant access paths as a list of string segments, but the BE Parquet mapper only projected one top-level shredded leaf. Deep object paths therefore fell back even when every nested
typed_valuewrapper was physically available. The FE access-path comparator also joined segments with dots, so an object key such asa.bcould collide with the nested patha/bbefore the paths reached BE.This change reuses the existing access-path list without any Thrift or Proto change. It compares FE paths segment by segment, merges sibling and prefix paths through the shared BE access-path tree, and traverses arbitrary-depth object
typed_valuewrappers for Iceberg and Paimon native Parquet scans. Numeric segments, repeated ancestors, missing leaves, unshredded files, and ambiguous physical leaf identities conservatively fall back to the complete Variant root. Numeric array indexes remain unsupported with a TODO for typed path segments.Release note
Support arbitrary-depth object-only Variant leaf projection for Iceberg and Paimon native Parquet scans.
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)