[fix](variant) Preserve nested Variant append atomicity - #66421
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
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.
Requesting changes: this test expansion has seven actionable coverage/oracle gaps. The logical expected outputs are mostly internally consistent, but several assertions can pass without exercising the behavior they name, and the new atomicity test misses a reachable partial-mutation path.
Critical checkpoints:
- Correctness and lifecycle: blocked by the lazy nested-append atomicity gap and by a deletion-vector checkpoint whose result is unchanged if the delete is ignored.
- Physical fixture and configuration compatibility: blocked because the pinned Iceberg 1.10.1 writer does not establish the requested shredded physical modes, while fallback assertions depend on those properties being ignored.
- Parallel and special-case paths: the purported parallel case changes ceilings but does not prove more than one scanner ran.
- Profile observability and determinism: the helper uses a short non-completion-aware wait, the row-group threshold double-counts profile views, and the lazy-read counter is not specific to the Variant output.
- Expected output and snapshot semantics: I checked the new arithmetic and current/history rows; no additional mismatch was found, and the equality/position-delete row checks are otherwise meaningful.
- Performance, cleanup, and general compatibility: there is no production performance change in this test-only PR, and no separate cleanup/container defect remained after the full sweep.
No additional user focus was supplied, so I reviewed the whole PR. Three review rounds converged on this seven-comment set with no unresolved candidates. This was a static review only: the task forbade builds/tests, so I did not run them. The requested repository-local code-review skill was absent in this checkout, and no AGENTS.md files were required.
| MutableColumns invalid_fields; | ||
| invalid_fields.push_back(nullable_strings( | ||
| {{VARIANT_EMPTY_METADATA.data(), VARIANT_EMPTY_METADATA.size()}}, {0})); | ||
| const Status status = materialize_variant_rows(shredded_int64_schema(), |
There was a problem hiding this comment.
[P1] Exercise the lazy append failure in this atomicity test
This malformed batch has only metadata, so ParquetVariantShreddedState rejects its field count before append_materialized_column() mutates anything. It therefore cannot catch the dangerous nested case: compatibility validation accepts a Variant child without materializing it, STRUCT append mutates earlier siblings in order, and a later incompatible/corrupt shredded Variant can throw during insert_range_from, leaving those siblings grown despite the error Status. Please add a pre-populated STRUCT/LIST/MAP case with a primitive sibling before the Variant and corruption that is discovered only during lazy fallback, then assert every child/null-map/offset remains unchanged (and fix the transactional append path if it fails).
| TBLPROPERTIES ( | ||
| 'format-version'='3', | ||
| 'write.format.default'='parquet', | ||
| 'write.parquet.shred-variants'='true', |
There was a problem hiding this comment.
[P1] Make this fixture's physical Variant mode explicit
This table requests write.parquet.shred-variants=true, but the checks below require VariantDirectLeafPathMisses and reconstruction and call the file unshredded. In the repository's pinned Iceberg 1.10.1 Spark runtime the property is not defined in TableProperties, so the test passes only because it is ignored; a writer that honors it will shred n and make the fallback profile time out. The other new Spark-generated true fixtures likewise do not prove shredded decoding under the current pin. Set false for fallback fixtures, and use a capable/checked physical fixture plus a schema or direct-leaf oracle for cases intended to cover shredding.
| def getProfileByToken = { String token, List<String> positiveCounters = [] -> | ||
| String lastProfile = "" | ||
| for (int retry = 0; retry < 20; ++retry) { | ||
| List profileData = profileAction.getProfileList() |
There was a problem hiding this comment.
[P1] Use the completion-aware profile waiter
This custom loop gives asynchronously published scanner counters only 10 seconds and never requires the profile list/detail to be COMPLETE. ProfileAction.getProfileBySql already provides a completion-aware 60-second wait, and the existing DV suite explicitly notes that detailed scanner counters arrive after query return. On a loaded external CI run these new multi-counter calls can fail even though the query and final profile are correct. Please use the framework waiter (then poll positivity if needed) without shortening its established timeout.
| WHERE v['shared'] >= 20 | ||
| ORDER BY id | ||
| """ | ||
| sql "set parallel_pipeline_task_num=4" |
There was a problem hiding this comment.
[P2] Prove that the parallel case actually uses multiple scanners
These settings only raise concurrency ceilings; min_file_scanners_concurrency remains 1, so the query may run serially and still match the byte-for-byte serial result above. That leaves cross-file state races uncovered. Force a minimum concurrency greater than one, assert the fixture has multiple data files, and bind this query to a completed profile showing actual concurrent/non-empty scanners rather than only a configured maximum.
| String multiRowGroupColdProfile = getProfileByToken(multiRowGroupColdToken, | ||
| ["RowGroupsTotalNum", "VariantDirectLeafPathMisses", "VariantReconstructedRows", | ||
| "FilteredRowsByLazyRead"]).toString() | ||
| assertTrue(counterSum(multiRowGroupColdProfile, "RowGroupsTotalNum") > 1, |
There was a problem hiding this comment.
[P1] Do not double-count merged and detailed profile views
counterSum scans the complete REST profile, which presents scanner counters in both merged and detailed/execution sections. A single real RowGroupsTotalNum: 1 can therefore appear twice, sum to 2, and satisfy this > 1 assertion even though the file exercised only one row group. The existing DV suite avoids this by slicing the authoritative MergedProfile section before aggregation. Scope this threshold to one profile view (or parse one ParquetReader block) and assert its actual row-group count.
| qt_variant_deletion_vector_current """ | ||
| SELECT COUNT(*), MIN(id), MAX(id), SUM(CAST(v['n'] AS BIGINT)) | ||
| FROM variant_deletion_vector | ||
| WHERE v['keep'] = true |
There was a problem hiding this comment.
[P1] Make the DV checkpoint observe the deleted rows
The DELETE removes odd ids, and those rows were initialized with keep=false; this WHERE v['keep'] = true therefore returns the same 2048 even rows and sum even if Doris ignores the delete entirely. It also cannot distinguish a PUFFIN deletion vector from an ordinary position-delete file. Query a predicate that includes the deleted rows (for example v['n'] >= 0) and assert the current even-id result, then verify the live delete metadata is PUFFIN/content-offset DV data for this fixture.
| "The mixed scan did not evaluate rows from the shredded typed leaf") | ||
| assertTrue(counterSum(pagePruningProfile, "VariantReconstructedRows") > 0, | ||
| "The mixed scan did not reconstruct complete Variant output") | ||
| assertTrue(counterSum(pagePruningProfile, "FilteredRowsByLazyRead") > 0, |
There was a problem hiding this comment.
[P1] Make delayed materialization specific to the Variant output
FilteredRowsByLazyRead is incremented whenever any non-predicate column is deferred. Because this query also outputs id, the counter stays positive if only id is lazy while the complete Variant root regresses to eager reconstruction; VariantReconstructedRows > 0 does not distinguish that case. Assert a Variant-specific relationship (for example reconstructed rows equal selected/output rows and are below pre-filter candidates), or add/use a Variant-specific deferred-materialization counter.
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Gabriel39
changed the title
|
run buildall |
|
/review |
There was a problem hiding this comment.
Automated code review summary
No new actionable defect was found after full-scope review and duplicate suppression. The existing inline threads remain the authoritative discussion for transactional Variant append/atomicity and regression fixture/profile proof; I did not repost substantially similar issues.
Critical checkpoints:
- Reader lifecycle and atomicity: traced schema planning through
NativeColumnReader, the Parquet scheduler, andTableReaderoutput ownership. Production clears the output/template before each reader call, so an empty exact-shape Variant destination adopts the converted tree. The remaining direct-helper rollback scenario requests the same transactional/unchanged-output contract as existing discussionr3710264289. - Schema and materialization: checked complete versus partial shredded state, schema/metadata transitions, primitive annotations, UUID/decimal/date/timestamp conversion, SQL NULL versus Variant null, root arrays, and nested STRUCT/LIST/MAP offsets/null maps. No additional correctness issue was substantiated.
- Filtering and deletes: checked page/lazy materialization, direct-leaf fallback, selection/row alignment, PUFFIN deletion vectors, equality deletes, snapshots/time travel, and mixed shredded/unshredded files. The apparent position-delete coverage concern was withdrawn because Iceberg VARIANT is v3-only and v3 writers cannot add new position-delete files.
- Performance and observability: checked the recursive append allocation path, scanner isolation/concurrency, completion-aware merged profile parsing, and Variant-specific reconstruction counters. The apparent allocation regression is not on the current production batch path.
- Tests and external compatibility: checked unit reachability, fixture provenance, exact result oracles, process-harness isolation, and Iceberg v3 behavior. No new coverage gap distinct from the existing review threads remains.
- User focus: no additional focus was supplied; the entire PR was reviewed.
- Convergence: in the final frozen round, both full-scope reviewers and the independent risk reviewer returned
NO_NEW_VALUABLE_FINDINGS; the final inline-comment set is zero.
Limitations: static review only; builds and tests were not run per the runner instructions. The repository did not contain the requested code-review SKILL.md, so the authoritative prompt, required Format V2 AGENTS.md, and mandatory FileScannerV2/Parquet review guides were applied directly.
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
3 participants
What problem does this PR solve?
Iceberg Variant reads need broader coverage across physical shredding states, nested containers, delete files, projection pruning, predicate pruning, and delayed materialization. Review also exposed a reader bug: a corrupt lazily decoded Variant nested after an already appended sibling could leave the destination column partially mutated.
What is changed?
Tests