[fix](variant) Preserve projected shredded states across exchange - #66446
[fix](variant) Preserve projected shredded states across exchange#66446Gabriel39 wants to merge 2 commits into
Conversation
|
run buildall |
|
/review |
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
### What problem does this PR solve? Issue Number: None Related PR: apache#66446 Problem Summary: Master exposes immutable subcolumn traversal through IColumn::ColumnCallback, while branch-4.1 still uses ImutableColumnCallback. Adapt the forward-port implementation to master’s callback contract so the composite shredded state overrides the interface and compiles. ### Release note None ### Check List (For Author) - Test: Unit Test - Behavior changed: No - Does this need documentation: No
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested. I found three P1 correctness failures in the new cross-file/exchange state handling:
- Per-file projection can mix complete and incomplete shredded states, but the fallback only preserves incomplete/incomplete pairs and otherwise invokes the incomplete state's forbidden root materializer.
- Heterogeneous supported leaves are re-encoded without their Parquet primitive width, changing INT64/INT32 values to value-sized integers and low-precision decimals to 16-byte decimals.
- A projected shredded column preserved by an exchange gather has no valid remote wire representation: the Variant SerDe writes empty encoded buffers with a nonzero saved row count, and the receiver rejects it.
Critical checkpoint review:
- Architecture/layering: the format-neutral
VariantShreddedStateboundary remains appropriately owned, but the new core composite can outlive the FileReader path without a corresponding serialization contract. - Lifecycle/selection: arbitrary/repeated selectors, range/filter slicing, empty segments, COW detachment, cache invalidation, and outer-null alignment were traced and no separate defect was found. The blocking lifecycle defect is the complete/incomplete transition described inline.
- Schema/materialization/external compatibility: mapper eligibility, per-file footer finalization, residual fallback, Parquet annotations, integer widths, and decimal precision-derived widths were checked. The mixed-state and physical-identity regressions are both production-reachable.
- Predicates/filtering/nulls: selection is applied consistently to nested values and outer null maps, and the element kernel merges outer nulls correctly. Missing paths and present residuals still trigger the mixed-state failure rather than a safe per-segment fallback.
- Performance/observability/tests: profile-counter and bounded-copy paths did not produce another actionable regression. The added tests do not cover production mapper-eligible heterogeneous primitive IDs, mixed complete/incomplete files, or a multi-backend gather/SerDe boundary.
- User focus:
review_focus.txtcontains no additional guidance, so the whole PR was reviewed without narrowing scope. - Process/completion: the requested repository-local code-review skill is absent in this checkout; the authoritative prompt, required
be/src/format_v2/AGENTS.md, and its three mandatory design guides were applied instead. This was a static-only review as required, with no builds or source edits. Two full rounds converged with both normal scopes and the risk-focused scope returningNO_NEW_VALUABLE_FINDINGSagainst this final comment set.
| _check_invariants(); | ||
| return; | ||
| } | ||
| if (!_shredded->can_materialize() && !selected_source->can_materialize()) { |
There was a problem hiding this comment.
[P1] Handle complete and incomplete file states together
Projection completeness is decided per file: the mapper keeps a full Variant when the key/type cannot be leaf-projected, and the Parquet finalizer also restores a full projection when residual all-null statistics are unavailable. A gather can therefore adopt an incomplete state from file A and then receive a complete state from file B. try_append() rejects the _complete mismatch, this condition is false, and the fallback calls ensure_encoded()/materialized_column() on the incomplete side, which throws instead of returning the requested rows (the reverse order fails too). The new binary test manually forces a partial binary plan, whereas the production mapper makes that file complete, so it misses this path. Please preserve mixed complete/incomplete segments with a per-segment direct/canonical fallback (including missing-path nulls), or make the projection decision uniform across files, and add production-path coverage.
| nulls->insert_range_from(nullable.get_null_map_column(), 0, nullable.size()); | ||
| continue; | ||
| } | ||
| auto typed = ColumnVariantV2::create_typed(match.column, match.type); |
There was a problem hiding this comment.
[P1] Preserve the Parquet primitive width when normalizing mixed leaves
For two mapper-eligible partial files with different leaf types (for example INT64 7 and INT32 8), this heterogeneous branch wraps each match as a generic typed Variant and immediately encodes it. That encoder calls VariantScalarRef::integer(value) without the Parquet width, so both values become INT8; TYPE_DECIMAL128I is likewise always emitted as 16 bytes instead of the precision-derived Parquet width. The normal Parquet reconstruction path explicitly passes integer_width(schema, type) / decimal_width(...), and the existing baseline asserts that an INT64 leaf remains VariantPrimitiveId::INT64. Carry a schema-aware normalized value/physical identity into this branch and add a production-eligible heterogeneous test that asserts the primitive IDs, not only get_int().
| return; | ||
| } | ||
| if (!_shredded->can_materialize() && !selected_source->can_materialize()) { | ||
| // Exchange channels can gather projected rows from files whose shredded leaf types |
There was a problem hiding this comment.
[P1] Serialize a valid representation for remote exchange
This branch preserves a projected _shredded column inside the exchange channel's MutableBlock, but non-local channels immediately pass that block to the Variant binary SerDe. The SerDe only distinguishes typed from encoded state; for a shredded column it writes the still-empty metadata/id/value buffers while saving the nonzero _shredded->size(). Deserialization reconstructs zero rows and throws saved row count ... does not match decoded row count .... This is reachable with an outer element access above a subquery ORDER BY ... LIMIT: access-path collection still projects the leaf at the file scan, no project-pushdown rule moves the expression through LogicalTopN, and one-phase TopN requires a remote gather first. Please add an explicit shredded wire representation, materialize when complete, or fence incomplete projection from remote exchanges, and cover a multi-backend gather.
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run buildall |
|
/review |
2 participants
What changed
Root cause
An exchange channel can buffer selections from multiple scanner blocks. After the first projected shredded batch was adopted, the next indexed insertion forced the destination to encode. A leaf-only projection has no complete root metadata/value pair, so it cannot reconstruct the logical Variant. Different files can also shred the same path with incompatible physical types, which prevents the existing exact-schema append from handling the batch.
Impact
Projected Variant leaves can now cross exchange boundaries without premature root reconstruction. Non-Variant columns, complete Variant states, and compatible shredded schemas retain their existing paths.
Validation
VariantColumnReaderTest.*:ColumnVariantV2Test.*: 69 tests passed.VariantElementV2EncodedTest.*:VariantElementV2TypedTest.*: 7 tests passed.