[master][pick] Forward-port Parquet nullable selection and Iceberg Variant reads - #66413
[master][pick] Forward-port Parquet nullable selection and Iceberg Variant reads#66413Gabriel39 wants to merge 9 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes: four blocking issues remain.
- Variant footer/page-index pruning can suppress an earlier error-preserving conjunct.
- Late runtime-filter refresh can accept shifted deferred Variant output slots and index outside the active file block.
- Delete-only Variant MERGE is not safe for new-FE/old-BE rolling upgrades.
- One deterministic regression result bypasses the required generated golden file.
Checkpoint conclusions:
- Goal and data correctness: The forward-port covers native Parquet Variant reads, nullable selection, planner/access-path plumbing, metadata COUNT, and delete-only MERGE, with broad unit/regression coverage; the two scan correctness defects above mean the goal is not yet safely achieved.
- Scope and parallel paths: The change is cohesive but large. Footer/page pruning, eager/deferred projection, native/legacy scanner gates, and read/write paths were traced end to end. The mirrored page-index defect is covered by the first inline comment.
- Concurrency and lifecycle: Catalog storage bindings and shredded-state ownership/COW were checked without another defect. Late request activation at row-group boundaries is not safe because deferred positions are not preserved (inline comment).
- Compatibility and protocol: New Thrift plumbing defaults correctly for old-FE/new-BE, but new-FE/old-BE delete-only Variant MERGE lacks a query-wide capability fence (inline comment).
- Tests and observability: The PR reports targeted FE, connector, BE ASAN, and format checks, and adds useful profiles. I did not rerun builds/tests because the authoritative review bundle forbids it. Missing coverage includes unsafe-conjunct metadata pruning, two-root late-RF refresh, mixed-version writer omission, and the signed-selector golden result.
- Transactions/persistence/configuration: No new persistence or dynamic-configuration defect was found; delete-file lifecycle otherwise remains fenced and errors propagate.
User focus: review_focus.txt contains no additional guidance, so the entire PR was reviewed.
Review completion: Three rounds converged on this frozen four-comment set; all other candidates were either disproved by upstream invariants or dismissed with concrete code evidence.
| const std::vector<std::unique_ptr<ParquetColumnSchema>>& file_schema, | ||
| const format::FileScanRequest& request, const cctz::time_zone* timezone) { | ||
| for (const auto& conjunct : request.conjuncts) { | ||
| const auto predicate = extract_variant_shredded_predicate(conjunct); |
There was a problem hiding this comment.
[P1] Fence Variant metadata pruning at the first unsafe conjunct
This loop can skip an earlier non-extractable, error-preserving conjunct and prune from a later Variant comparison before any rows are evaluated. For example, with assert_true(id != 0, 'boom') followed by CAST(v['n'] AS INT) > 3000, a row group containing id = 0 but with shredded n.max <= 3000 is discarded here, so the required error never occurs. TableReader::_is_safe_to_pre_execute() stops split pruning at this barrier, and build_predicate_conjunct_schedule() disables staged compaction for the same reason, but both this footer loop and the page-index loop at 1712 ignore it. Please carry the original safe-prefix boundary into the localized request (slotless barriers may otherwise disappear), apply it to both loops, and add an error-preservation differential test.
| // Signed integer selectors are array indexes, even when a shredded object has a key with the | ||
| // same serialized token. The ambiguous scanner path must retain enough state for both results. | ||
| List<List<Object>> signedSelectorRows = sql """ | ||
| SELECT CAST(v[-1] AS INT), CAST(v['-1'] AS INT) |
There was a problem hiding this comment.
[P2] Record this deterministic result through the golden file
This stable one-row result is currently hardcoded with assertEquals, so it bypasses the suite's generated .out contract. The repository testing rules require determined regression results to use qt/order_qt rather than assertion-style checks. Please make this a named golden query (with deterministic ordering if the fixture can grow) and regenerate test_iceberg_variant_read.out so the signed-selector behavior is reviewed and maintained like the neighboring cases.
| file_request->local_positions = *fixed_local_positions; | ||
| } | ||
| file_request->non_predicate_positions.clear(); | ||
| file_request->conjuncts.clear(); |
There was a problem hiding this comment.
[P1] Preserve deferred output slots across late-filter refresh
refresh_conjuncts() seeds only local_positions, while this clears and later reallocates non_predicate_positions. With two independent Variant roots, the initial layout can be v1 predicate/output at 0/1 and v2 at 2/3; rebuilding from retained locals {0,2} assigns the outputs to 3/4. same_physical_scan_layout() compares only the local map and each root's predicate projection, so it accepts that shifted request even though the active file-block template still has four slots. At the next row group the Parquet scheduler uses the refreshed output positions, overwrites slot 3 and then indexes slot 4 out of range. Please preserve the active deferred-position map and compare both deferred positions and output projection trees before queueing a refresh; add a multi-row-group late-RF test with two deferred roots.
| const auto& merge_sink = _t_sink.iceberg_merge_sink; | ||
| // An old FE cannot produce delete-only plans, so an unset flag retains its data-writer path. | ||
| _writes_data_files = !merge_sink.__isset.writes_data_files || merge_sink.writes_data_files; | ||
| // Missing means an old FE plan, which predates SQL MERGE cardinality validation. |
There was a problem hiding this comment.
[P1] Fence writer omission for old BEs during rolling upgrade
A new FE can now allow a delete-only MERGE on a Variant table and send writes_data_files=false, but an old BE skips this unknown Thrift field and still constructs VIcebergTableWriter. Its init_properties() parses the full Iceberg schema_json, and the old parser has no variant primitive, so a fragment placed on that BE fails while the same fragment succeeds on a new BE. The adjacent cardinality capability is disabled through the query-wide execution version for exactly this rolling-upgrade reason; please add an equivalent capability fence here (or reject this plan in FE until all participating BEs support writer omission) and cover the mixed-version case.
FE UT Coverage ReportIncrement line coverage |
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
) - Fuse nullable definition-level runs with the row filter in one traversal. - Produce physical decode ranges, the selected NULL map, and selected value counts without first materializing and rescanning a row-wise selection map. - Reuse the existing selected-decoder strategies and nullable in-place expansion. - Restrict fusion to batches with at least 1,024 rows, at least 10% NULLs, and materially fragmented definition-level runs. No-NULL, low-NULL, clustered, nested, and non-expandable shapes keep the legacy path. The full benchmark matrix includes no-NULL, low-NULL, and clustered level plans as negative controls. Those shapes do not remove enough legacy work to guarantee a win, so this change deliberately leaves them unchanged. Decoder selection and encoding-specific materialization are not modified. - ASAN: `NativeNullableSelectionTest.*` and benchmark scenario tests: 16/16 passed. - ASAN: `ParquetV2NativeDecoderTest.*`: 118/118 passed. | Coverage | Legacy/fused pairs | Correctness | Regressions | Mean CPU change | Least improvement | |---|---:|---|---:|---:|---:| | Full scenario matrix | 100 | Identical ranges and NULL maps | N/A (includes negative controls) | N/A | N/A | | Production-eligible scenarios | 30 | Identical ranges and NULL maps | 0 | -42.68% | 8.08% | | Scenario | Repetitions | Legacy median CPU | Fused median CPU | CPU change | Legacy CV | Fused CV | |---|---:|---:|---:|---:|---:|---:| | 10% selectivity / 50% NULL, fragmented | 10 | 400,220 ns | 177,905 ns | -55.55% | 0.58% | 1.28% | | 99% selectivity / 50% NULL, high-selectivity boundary | 10 | 687,385 ns | 460,657 ns | -32.98% | 0.60% | 1.32% | The microbenchmark isolates nullable selection planning; it is not presented as an end-to-end query speedup.
…he#66302) Forward-port the Iceberg Variant Parquet reader to the plugin-driven connector on master while preserving mixed-version scan safety and delete-only merge behavior.
### What problem does this PR solve? Issue Number: None Related PR: apache#66392 Problem Summary: Master replaced the legacy Hadoop property cache with a shared storage adapter snapshot. Preserve the original fix invariant by publishing an immutable type-keyed map so connector consumers cannot mutate catalog-wide state after publication. Add deterministic coverage for atomic publication and snapshot immutability. ### Release note Prevent connector consumers from modifying shared catalog storage adapter snapshots. ### Check List (For Author) - Test: Unit Test (`CatalogPropertyTest`) - Behavior changed: No. This hardens the existing snapshot contract. - Does this need documentation: No
154db63 to
6adba50
Compare
|
run buildall |
|
/review |
github-actions
Bot
removed
the
approved
There was a problem hiding this comment.
Requesting changes: three additional P1 blockers remain. I deduplicated against the four existing live threads, which continue to cover unsafe Variant metadata-pruning order, late-filter deferred-slot refresh, the regression golden contract, and old-BE merge-writer omission.
Checkpoint conclusions:
- Goal and proof: The PR aims to forward-port native Iceberg Variant reads/writes, physical-leaf pruning, and fused nullable selection. Broad unit and regression coverage is present, but the connector API, rolling-upgrade scan admission, and metadata-COUNT snapshot gaps mean the goal is not safely complete.
- Scope and focus: The change is large but cohesive around external Variant execution.
review_focus.txtadds no extra guidance, so the whole PR was reviewed. - Concurrency: No new thread is introduced. The private shredded-state materialization-cache mutex, its append/reset/read ordering, catalog publication, scanner scheduling, and late runtime-filter activation were traced; no new concurrency issue survived beyond the existing deferred-slot thread.
- Lifecycle: Connector class loading, scan initialization, statement snapshot pinning, file-local projection, block reuse, and merge sink open/close were traced. The metadata-COUNT escape hatch is incorrectly decided before the handle used for planning is pinned (inline).
- Configuration: No new configuration item or dynamic-reload contract is introduced.
- Compatibility: Two public connector SPI methods were added without the required API-major/baseline update (inline). Read-side Variant admission also mistakes a cloud-only smooth-upgrade marker for a general old-BE capability fence (inline).
- Parallel paths: Cloud and community upgrades, root and leaf projections, native and legacy scanner gates, metadata and real-range COUNT, and all merge clause shapes were compared. Delete-only merge propagation is complete for new participants; its old-BE failure remains covered by the existing live thread.
- Conditional logic: The metadata-only COUNT and backend-marker conditions are not sufficient for the states they claim to prove (inline). Other new projection, fallback, and selection gates were checked against their upstream invariants.
- Error handling and memory safety: Status/exception propagation, footer corruption checks, recursive column exclusivity, direct-leaf ownership, nullable alignment, and conversion-failure remapping were checked without another distinct defect.
- Data correctness: Existing live threads cover unsafe metadata pruning and shifted scan coordinates. The new snapshot and mixed-version findings can also route unsupported Variant decoding and are blocking.
- Tests: Coverage is broad, but it lacks connector-major enforcement for the reachable handle/provider surface, a non-cloud old-BE scan case, and a pinned snapshot whose COUNT summary must fall back to files. The existing live P2 covers the deterministic result that bypasses the generated golden file.
- Test results: I did not run builds or tests because the authoritative review bundle requires a static-only review; reported PR results were therefore not independently verified.
- Observability: New scan profiles cover the important reader paths, and no distinct logging or metrics blocker was found. The upgrade and snapshot mismatches need admission-time correctness rather than post-failure observability.
- Transactions and persistence: No Doris EditLog or transaction-state change is introduced. Iceberg snapshot selection and write lifecycle were reviewed; snapshot consistency is the blocking read-side issue.
- Writes and FE-BE variables: The new write flag survives planner clones and new-version sink lifecycles, and its old-FE/new-BE default is conservative. New-FE/old-BE writer omission remains the existing live blocker; the new read carrier has the separate ordinary-upgrade blocker inline.
- Performance and other risks: Physical projection, page/footer pruning, allocation/COW, and fused nullable hot paths were examined. No additional substantiated performance or correctness issue remained after the final candidate audit.
Review status: static review converged on this frozen three-comment addition plus the four existing live threads.
| * Whether this write can emit data files. A delete-only MERGE returns false so a connector may | ||
| * allow position-delete output even when the table has read-only column types. | ||
| */ | ||
| default boolean isWritesDataFiles() { |
There was a problem hiding this comment.
[P1] Bump the connector plugin API for these new SPI methods
This method and ConnectorScanPlanProvider.canServeMetadataOnlyCount() extend the public connector SPI, but the PR leaves the kernel/plugin API at 3.0. A new Iceberg plugin is therefore admitted by an old 3.0 FE; because connector SPI classes are parent-first, planWrite() then invokes isWritesDataFiles() on the old kernel interface and fails with NoSuchMethodError. Please apply the repository's required major bump (including the test pin/baseline), and include these reachable provider/handle types in the frozen surface so this cannot evade the guard.
| ConnectorScanPlanProvider scanProvider = resolveScanProvider(); | ||
| if (isTableLevelCountStarPushdown() && conjuncts.isEmpty() && scanProvider != null) { | ||
| metadataCountProven = onPluginClassLoader(scanProvider, | ||
| () -> scanProvider.canServeMetadataOnlyCount( |
There was a problem hiding this comment.
[P1] Prove metadata COUNT on the same pinned handle that is planned
This capability check runs before pinMvccSnapshot() updates currentHandle, while planScan() later uses the pinned handle and recomputes whether the summary can serve the count. For a time-travel/reference query (or a snapshot change between phases), the early latest snapshot can return true, disabling the old-BE fence, but the selected snapshot can contain deletes or missing counters and fall back to real Variant file ranges. Please apply the statement pin before using this escape hatch, or base the bypass on the actual planned ranges; add a test where latest is countable but the pinned snapshot is not.
| return; | ||
| } | ||
| for (Backend backend : backends) { | ||
| if (backend.isSmoothUpgradeSrc()) { |
There was a problem hiding this comment.
[P1] Fence ordinary rolling-upgrade backends too
isSmoothUpgradeSrc() identifies only the cloud colocated smooth-upgrade source: its production setter is in CloudSystemInfoService, while ordinary backends default this bit to false. During a normal mixed-version cluster upgrade, an old BE therefore passes this loop and can be assigned an Iceberg Variant scan even though it lacks the new carrier/native reader. Please gate on an explicit BE capability or execution version that covers every eligible backend, and test an old non-cloud backend rather than manually setting the cloud-only flag.
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
FE UT Coverage ReportIncrement line coverage |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
### What problem does this PR solve? Issue Number: None Related PR: apache#66413 Problem Summary: Review follow-ups found that Variant metadata pruning could bypass an earlier error-producing predicate, late runtime-filter refresh could shift deferred Variant output slots, and mixed-version clusters could schedule unsupported Variant scans or delete-only MERGE plans. The connector SPI version also did not reflect its expanded public surface. In addition, debug Boolean validation filtered and copied large nullable complex columns even when they contained no Boolean values, which could exhaust query memory. This change preserves the safe pruning prefix and scan layout, introduces execution-version compatibility gates, bumps and freezes the connector SPI surface, pins metadata-count checks to the selected snapshot, and skips allocation-heavy Boolean filtering when no Boolean subcolumn exists. ### Release note Iceberg Variant scans and delete-only MERGE now enforce rolling-upgrade compatibility, and debug column validation avoids copying non-Boolean complex payloads. ### Check List (For Author) - Test: Unit Test and Regression Test - Focused ASAN BE unit tests for Boolean validation, Variant scan refresh, metadata pruning, page filtering, and MERGE compatibility - FE compatibility and connector SPI surface unit tests - Generated Iceberg Variant regression golden output - FE Checkstyle and BE clang-format - Behavior changed: Yes. Unsafe metadata pruning and mixed-version Variant execution are rejected or conservatively evaluated, and non-Boolean nullable complex columns are validated without copying their payload. - Does this need documentation: No
|
run buildall |
|
/review |
FE Regression Coverage ReportIncrement line coverage |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
There was a problem hiding this comment.
I found two P1 blockers, detailed inline: incomplete shredded Variant state crosses a join shuffle and cannot be accumulated safely, and the rolling-upgrade fence rejects valid non-Variant delete-only Iceberg MERGE.
Checkpoint conclusions:
- Goal and correctness: the native Parquet Variant read/write integration is broadly implemented, but the two production paths above violate query correctness/availability. The existing tests do not cover a two-block join shuffle, conflicting cross-file shredded leaf types, or non-Variant delete-only MERGE at execution version 11.
- Scope and parallel paths: I reviewed all 117 authoritative changed paths across BE, FE, connector SPI, Thrift, unit tests, and regression assets. The change is large but cohesive. I traced scan projection/predicate/deferred paths, metadata COUNT, join/exchange buffering, Iceberg merge writers, and mixed-format paths; no additional blocker survived.
- Concurrency and lifecycle: scanner split/late-filter state, immutable/COW shredded ownership, snapshot pinning, exchange buffering, and merge-writer open/write/close cleanup were checked. No additional lock, race, deadlock, static-initialization, or lifetime issue was found; the first inline is a cross-batch lifecycle defect.
- Compatibility, protocol, and configuration: connector API 4.0, the new Thrift field, query-wide BE execution version, and old/new backend behavior were traced. The second inline is the remaining schema-blind compatibility regression. No other FE-BE propagation or dynamic-configuration issue survived.
- Writes, transactions, and persistence: Iceberg data/delete writer routing, close/commit-data collection, and cleanup paths were reviewed. This change adds no EditLog state, and no additional atomicity, failover, or resource-leak issue was found.
- Performance and observability: fused nullable decoding, direct-leaf projection, pruning, counters, and profiles were reviewed. No separate substantiated performance or observability blocker survived.
- Tests: static review only; no builds or tests were run, as required by the review environment.
- User focus:
review_focus.txtcontained no additional user-provided focus, so the whole PR was reviewed without a narrower focus.
Review completion is incomplete at the three-round cap: both final-round normal reviewers returned NO_NEW_VALUABLE_FINDINGS, but the final risk-focused sweep added the accepted cross-file schema refinement now incorporated into the first inline. A fourth same-state convergence round would exceed the prompt's maximum. Every candidate nevertheless has an explicit accepted, duplicate, or evidence-backed dismissed disposition.
| public void bindDataSink(Optional<InsertCommandContext> insertCtx) | ||
| throws AnalysisException { | ||
| if (writeOperation == WriteOperation.MERGE && !writesDataFiles | ||
| && Config.be_exec_version < SUPPORT_ICEBERG_VARIANT_EXEC_VERSION) { |
There was a problem hiding this comment.
[P1] Keep non-Variant delete-only MERGE available during rolling upgrades
writesDataFiles is false for every delete-only MERGE, so this rejects even tables whose schema has no Variant whenever be_exec_version is 11. Those queries were supported before this change: an old BE opens the table writer, but with no insert rows it creates no partition writers or data files; only a Variant schema makes the old parser fail. The matching BE check is equally schema-blind, and the new test uses an empty/non-Variant column list, so it pins this regression instead of the intended Variant-only fence. Please apply the version fence only when the omitted writer schema contains Variant, and retain coverage that ordinary delete-only MERGE remains allowed at version 11.
| return; | ||
| } | ||
|
|
||
| if (_shredded) { |
There was a problem hiding this comment.
[P1] Do not buffer incomplete Variant states across the exchange
The first indexed selection into an empty destination adopts source._shredded, but the next batch reaches this branch and calls ensure_encoded(); a leaf-only Parquet projection cannot materialize and throws. This is reachable when v['n'] is projected above a partitioned hash join: nested pruning sends the raw partial v through the child shuffle, where one channel buffers selections from consecutive scan blocks. Adding the insert_range_from()-style try_append() only fixes identical schemas: Parquet permits different files to shred the same path with incompatible types, while try_append() requires exact schema equality and the incomplete fallback still cannot encode. Please consume the leaf into a common logical representation before exchange, or use an accumulator that preserves heterogeneous partial states; cover both two same-schema blocks and two files with conflicting shredded leaf types.
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Aug 8th, 2026 3:37 AM. The selected account is excluded until 2026-08-08T03:37:00Z. Please trigger /review again; another configured account may be available. |
### 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
|
run buildall |
|
/review |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-H: Total hot run time: 29288 ms |
TPC-DS: Total hot run time: 166056 ms |
ClickBench: Total hot run time: 23.94 s |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
There was a problem hiding this comment.
Requesting changes: one additional P1 blocker remains.
- Complete Parquet Variant roots stay in shredded state, but the VariantV2 Block SerDe treats every non-typed column as encoded and writes the empty encoded buffers. A complete root or nested Parquet Variant that reaches exchange or spill serialization while still shredded therefore saves a nonzero row count but reconstructs zero rows, then deterministically fails the saved-row-count corruption check. The inline comment identifies the construction boundary and the missing round-trip coverage.
I deduplicated this review against all nine live inline threads. Those threads continue to cover unsafe metadata pruning, the missing golden result, late-runtime-filter output positions, rolling-upgrade writer/scan fences, connector SPI versioning, pinned metadata COUNT, non-Variant delete-only MERGE, and incomplete heterogeneous exchange accumulation. The finding in this review is distinct: it affects a materializable complete root whenever it reaches a serialization boundary before an operation has encoded it.
User focus: review_focus.txt adds no extra guidance, so I reviewed the entire PR without narrowing scope.
Critical checkpoint conclusions:
- Goal and proof: The PR implements Iceberg/Parquet Variant v1 reading, nested projection/pruning, scanner-v2 integration, metadata/page pruning, and guarded external writes. It adds broad BE/FE unit and regression coverage, but it does not prove a full-root Variant
Blockserialization round trip; the accepted P1 means the end-to-end goal is not yet met. - Scope and clarity: The change is large but cohesive across the FE planner, connector SPI, Thrift, BE scanner/column stack, benchmark, and tests. I found no unrelated source change.
- Concurrency: The new shredded materialization caches are protected by local mutexes, and the reviewed scanner/reader state remains operator-local. Lock ordering and heavy work around the materialization cache did not expose a distinct deadlock or race.
- Lifecycle and static initialization: Shredded/composite state uses immutable shared ownership plus explicit COW detachment; scanner and materialization lifetimes terminate through their existing owners. No new cross-TU static-initialization dependency was found.
- Configuration: The FE/BE execution version is bumped to 12; this is a compatibility selector rather than a newly dynamic feature switch. Existing live threads already cover the remaining ordinary rolling-upgrade gating gaps.
- Compatibility: FE/BE execution-version checks, connector surfaces, and the new Thrift write flag were traced through their consumers. Outstanding old-BE, SPI, omitted-field, and non-Variant MERGE issues are already represented by live threads; no new duplicate was submitted.
- Parallel paths: Scanner V1/V2, native Parquet, eager predicate versus deferred output readers, metadata COUNT, mixed file formats, and external MERGE paths were compared. Variant support is deliberately confined to the native V2 path; known divergences are already covered by existing threads.
- Conditional checks: Projection-completeness, residual fallback, metadata-count proof, and write-data classification branches were checked against their callers. Their remaining substantiated gaps are existing-thread issues, apart from the new serialization dispatch gap.
- Test coverage: The PR adds extensive schema, reader, mapper, selector, access-path, merge-sink, connector, and regression cases, including negative schemas and heterogeneous leaf types. A complete-root/nested-Variant
Block::serialize/deserialize test is missing and is required by the inline finding. - Test results: Expected-output changes are ordered and consistent with the reviewed SQL. Per the review-runner contract, this was a static review and no builds or tests were run.
- Observability: Variant projection, direct-leaf, fallback, reconstruction, and fused-selection counters provide useful path-level visibility; no additional logging/metric issue survived review.
- Transactions and persistence: No Doris EditLog or table-version persistence path is changed. Iceberg snapshot pinning and metadata-only planning were traced; the pinned-count issue is already a live thread.
- Data writes, atomicity, and crash behavior: External MERGE classification and writer omission were traced from FE planning through the BE sink. Existing threads cover the remaining version/schema fence defects; no additional atomicity, cleanup, or crash leak was found.
- FE/BE variables and protocol:
writes_data_filesand execution-version changes are sent and consumed on the reviewed planner/sink paths. Compatibility defaults and older consumers are already covered by the live review context. - Error handling: New
Statusand exception boundaries generally propagate failures and use invariant checks for impossible shapes. The P1 is a data-correctness/serialization dispatch failure, not a silently ignored status. - Memory and ownership safety: Recursive COW, nullable/typed/shredded ownership, filter/cut/gather, and composite-state transitions were checked. The suspected filter-side shared-child mutation was dismissed because these paths detach buffers or replace immutable child state.
- Null and nullable handling: Definition-level null runs, selected-null expansion, outer Variant nulls, nullable wrappers, and nested array/map/struct carriers were traced. No distinct nullability or const-column defect was found.
- Performance: Direct leaf reads, independent projection, page pruning, fused nullable selection, and benchmark coverage target the hot paths without a substantiated new CPU/memory regression. The required serialization fix should preserve lazy shredding until an actual serialization boundary.
- Other issues: No additional non-duplicate finding remains after three normal full reviews, one adversarial risk review, and the main-agent final sweep.
|
|
||
| const auto* outer_nullable = check_and_get_column<ColumnNullable>(*physical); | ||
| MutableColumnPtr variants = | ||
| ColumnVariantV2::create_shredded(std::make_shared<ParquetVariantShreddedState>( |
There was a problem hiding this comment.
[P1] Materialize complete shredded Variants before block serialization
This returns a shredded ColumnVariantV2 even when complete is true. That state reports the physical row count from _shredded, but DataTypeVariantV2SerDe::{get_uncompressed_serialized_bytes,serialize} only distinguishes typed from encoded and serializes _metadatas/_meta_ids/_values; those buffers are empty for every shredded state. A complete Parquet Variant that reaches fragment-exchange or spill serialization while still shredded therefore saves N rows but encodes zero; the receiver or spill reader reconstructs zero rows and fails the saved-row-count corruption check. This is separate from the existing partial-leaf accumulator thread: it also affects a complete root projection from a single block. Please make the block SerDe materialize complete shredded states (and fail explicitly for incomplete ones), or encode complete rows before they can reach a serialization boundary, with a full-root Variant block round-trip test.
TPC-H: Total hot run time: 28848 ms |
TPC-DS: Total hot run time: 166066 ms |
ClickBench: Total hot run time: 25.08 s |
FE Regression Coverage ReportIncrement line coverage |
3 participants
Summary
Original pull requests
Verification
CatalogPropertyTest: 2 tests passed for atomic publication and snapshot immutability.git diff --checkpassed.