[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 4 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 |
3 participants
Summary
Original pull requests
Verification
CatalogPropertyTest: 2 tests passed for atomic publication and snapshot immutability.git diff --checkpassed.