[fix](iceberg) Fix MVCC and nested schema evolution edge cases - #66345
Open
Gabriel39 wants to merge 7 commits into
Open
[fix](iceberg) Fix MVCC and nested schema evolution edge cases#66345Gabriel39 wants to merge 7 commits into
Gabriel39 wants to merge 7 commits into
Conversation
### What problem does this PR solve? Issue Number: close #xxx Related PR: apache#66007 Problem Summary: Several review follow-ups from apache#66007 remained open: empty Iceberg reads could lose their MVCC boundary before a concurrent first append; nested DESCRIBE comments were not emitted as valid SQL literals; strict pre-casts widened required struct fields to nullable; trivial collection children lost inherited parent masks; and connector sink binding repeatedly loaded the latest target schema. This change preserves the empty-read marker through Iceberg write planning, centralizes SQL string literal quoting, makes cast nullability strict-mode aware, projects inherited collection masks through ARRAY/MAP offsets, and captures one case-insensitive connector target-schema snapshot per bind. ### Release note Fix Iceberg MVCC and nested schema-evolution edge cases in connector reads, writes, DESCRIBE output, casts, and sink binding. ### Check List (For Author) - Test - [x] Regression tests - [x] Unit tests - Behavior changed: - [x] Yes. Correctness fixes for the affected edge cases. - Does this need documentation? - [x] No.
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 29212 ms |
Contributor
TPC-DS: Total hot run time: 169738 ms |
Contributor
ClickBench: Total hot run time: 23.86 s |
Keep an explicitly empty MVCC read empty across a concurrent first append and validate RowDelta conflicts from table creation. Add a barrier test for the MERGE/INSERT race. Also fail writes when the bound schema changed, avoid unnecessary nested collection null-mask allocation, and keep Cast nullability independent of thread-local strict mode.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
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. |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 28597 ms |
Contributor
|
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. |
Contributor
TPC-DS: Total hot run time: 168981 ms |
Gabriel39
marked this pull request as ready for review
August 1, 2026 05:48
Gabriel39
requested review from
924060929,
englefly,
morrySnow,
starocean999 and
yiguolei
as code owners
August 1, 2026 05:48
Contributor
ClickBench: Total hot run time: 23.99 s |
Gabriel39
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Request changes: three issues remain.
Checkpoint conclusions:
- Goal and proof: the PR addresses nested schema evolution, Iceberg MVCC/write binding, strict-cast struct nullability, and SQL comment quoting, but it does not fully achieve that goal because valid Iceberg write shapes and strict narrowing casts still fail, and the BE collection path adds a redundant scan. The added tests do not exercise those three cases.
- Scope and clarity: the changes are generally localized and the full 23-file diff was reviewed; no unrelated source change was identified.
- Concurrency and lifecycle: the resolved-empty snapshot marker, handle copies, scan initialization, synchronous/streaming/count paths, and RowDelta base validation were traced. Production pin ordering preserves the empty snapshot, and no lock-order, static-lifetime, or release issue was found.
- Configuration, compatibility, and persistence: no new configuration, storage-format, journal, symbol, or rolling-upgrade compatibility issue was found.
- Parallel paths and data writes: ARRAY/MAP, struct/named_struct, partial/static/full/rewrite/row-level writes, and both SQL backslash modes were checked. The Iceberg validator incorrectly treats a write subset as a full captured schema.
- FE/BE contract: snapshot state propagation is complete, but strict-cast logical nullability disagrees with the BE physical column type for narrowing casts.
- Performance: the new nested collection probe performs an avoidable O(entries) null-map pass for absent/all-clear inherited masks. No other material CPU, memory, or observability issue was substantiated.
- Tests and results: changed unit tests and assertions were statically inspected; no generated result-file issue was found. Builds and tests were not run because the authoritative review instructions prohibit them.
No additional user review focus was provided; the whole PR was reviewed.
| // only when a required descendant can consume them. This avoids scratch proportional | ||
| // to all array entries for the common all-required schema. | ||
| const NullMap* descendant_parent_null_map_ptr = nullptr; | ||
| if (_requires_parent_null_map_for_alignment(nested_column, |
Contributor
There was a problem hiding this comment.
_requires_parent_null_map_for_alignment() runs before this code establishes that the inherited parent mask exists and hides at least one row. For a large ARRAY/MAP with nullable-to-required descendants and a null or all-clear parent mask, the probe scans descendant null maps via has_null(), projection returns no entry mask, and normal alignment scans those descendants again. Please gate the descendant probe on a parent mask that can actually hide rows so this schema-evolution path does not add an avoidable O(number of entries) pass per block.
| List<ConnectorColumn> boundColumns = handle.getColumns(); | ||
| boolean hasSyntheticRowId = boundColumns.size() == currentColumns.size() + 1 | ||
| && DORIS_ICEBERG_ROWID_COL.equals(boundColumns.get(boundColumns.size() - 1).getName()); | ||
| if (boundColumns.size() != currentColumns.size() && !hasSyntheticRowId) { |
Contributor
There was a problem hiding this comment.
handle.getColumns() is the query's bound write list, not the full schema snapshot. BindSink intentionally omits unspecified columns and static partition columns, and PhysicalPlanTranslator copies that subset into the handle, so a valid INSERT INTO t (id) ... for a two-column table (or an INSERT with a static partition) reaches this branch with different sizes and is rejected as schema drift even when the table never changed. Please carry the complete bound target schema separately for this comparison, while retaining the subset for value-to-column mapping.
|
|
||
| /** Nullability of this cast when invalid input fails instead of producing NULL. */ | ||
| public boolean strictModeNullable() { | ||
| return child().nullable(); |
Contributor
There was a problem hiding this comment.
Strict failure semantics do not make every successful cast physically non-nullable. BE's strict BIGINT-to-INT specialization still has type Nullable(Int32) (with an all-clear map for valid rows), while this reports a required field and FE serializes Struct<Int32>. The generic nested-type compatibility check admits that mismatch, then FunctionStruct tries to insert the ColumnNullable argument into a non-null ColumnInt32 field and hits the column-type assertion even for an in-range value. Please keep the field nullable until strict casts actually return non-null types, or normalize the strict cast's physical result as part of this change.
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
ClickBench: Total hot run time: 23.75 s |
Contributor
There was a problem hiding this comment.
Request changes: four blocking write-correctness issues remain. Production type conversion rejects unchanged Iceberg schemas; row-level UPDATE/MERGE can move their validation baseline to a later schema; recursive Iceberg field identity is not fenced; and partition/write-sort planning still resolves live positions against older bound output.
Checkpoint conclusions:
- Goal and proof: the head fixes the earlier partial/static write-list false positive, empty-snapshot lifecycle, strict-cast struct shape, and format-v2 descendant-scan cost, but the end-to-end schema-generation goal is incomplete because of the four inline findings. The added tests do not prove the real translator-to-provider path or the concurrent generation paths.
- Scope and user focus: all 29 changed files and their upstream/downstream consumers were reviewed. The changes are generally localized and the new SPI method is defaulted. No additional user review focus was provided, so the whole PR was reviewed.
- Concurrency: no new in-process shared-state, lock-order, or deadlock issue was found. The surviving races are external metadata updates between bind, row-level projection synthesis, physical partition/write-sort planning, and
planWrite; supported no-cache/refresh behavior makes them reachable. - Lifecycle:
snapshotResolvedand the ordinary bound-schema list survive all traced immutable copies. Mandatory scan initialization pins before batch selection, so the provisional resolved-empty streaming concern is dismissed. No static-initialization, reference-cycle, cleanup, or ownership defect was found. - Configuration: no configuration item is added. Existing cache invalidation and Iceberg table-cache TTL 0 expose the mixed-generation cases; no separate dynamic-configuration issue was found.
- Compatibility and protocol: the write-handle API addition is source-compatible, and no storage format, Thrift payload, EditLog, symbol, or rolling-upgrade carrier changes. The scalar
ConnectorTypeencoding mismatch is nevertheless a direct behavioral compatibility break for ordinary writes. - Parallel paths and conditions: ordinary INSERT/OVERWRITE/REWRITE, explicit/static-partition writes, DELETE, COUNT/system/Top-N/rewrite-scope scans, RowDelta conflict validation, both SQL quoting modes, struct constructors, and ARRAY/MAP/STRUCT null-mask coordinates were checked. UPDATE/MERGE's compatibility constructor and the pre-validation position consumers are the missed parallel paths. The special conditions otherwise fail loudly and are documented; a default-only change is correctly dismissed because its old value is already an explicit legal expression.
- Tests and results: changed tests and assertions were statically inspected and their expected behavior is consistent, but they bypass production type conversion and omit recursive field replacement, consecutive row-level schema reads, and live partition/write-sort positions. No build or test was run because the authoritative review instructions require static review only.
- Observability: existing connector exceptions and scan/write profiles are sufficient once validation executes; no new logging, metric, or hot-INFO issue was found.
- Persistence, transactions, data writes, and crashes: no FE persistence state is added and Iceberg atomic commit/abort behavior remains intact. However, recursive replacement and row-level generation skew can atomically commit payloads under the wrong field identity/order, while the positional paths can choose the wrong slot or fail before the intended retry exception. No separate crash-leak or recovery defect survived review.
- FE/BE variables: no new variable needs FE-to-BE propagation and no scattered send path is missing.
- Performance: the immutable schema copies and validation are O(schema) per write and outside row hot paths. The format-v2 all-clear/absent-mask gates remove the prior redundant descendant scan; no other material CPU or memory regression was substantiated.
- Other issues and deduplication: the three existing threads cover only the earlier format-v2 scan cost, subset/full-schema comparison, and strict-cast nullability. The four submitted findings are distinct, and no other unresolved candidate remains after three converged review rounds.
|
|
||
| private static ConnectorColumn toWriteConnectorColumn(Column column) { | ||
| ConnectorColumn result = new ConnectorColumn(column.getName(), | ||
| ConnectorColumnConverter.toConnectorType(column.getType()), |
Contributor
There was a problem hiding this comment.
[P1] Normalize the bound type before comparing it. A real unchanged Iceberg INTEGER arrives as ConnectorType("INT", -1, -1), becomes ScalarType.INT with 0/0, and is converted here to ConnectorType("INT", 0, 0); ConnectorType.equals compares those parameters, so ordinary INSERT/UPDATE/MERGE/REWRITE is rejected as "schema changed". Timestamps and several other scalars diverge too. The added provider tests construct canonical ConnectorType.of("INT") directly and miss this path; please compare one canonical representation and add a translator-to-provider test.
Contributor
|
Codex automated review failed and did not complete. Error: Review step was failure (possibly timeout or cancelled) Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
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. |
Contributor
TPC-H: Total hot run time: 28933 ms |
Contributor
TPC-DS: Total hot run time: 169135 ms |
Contributor
|
Codex automated review failed and did not complete. Error: Review step was failure (possibly timeout or cancelled) Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
ClickBench: Total hot run time: 23.88 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 28921 ms |
Contributor
TPC-DS: Total hot run time: 169269 ms |
Contributor
There was a problem hiding this comment.
Review summary
The complete static review converged in one round with no new valuable inline findings. The current head/base still match the authoritative bundle (c1509e2081ae39181f6b88ac619c605cde4ed129 / 4ac235afe5586934de5481c524e2e7a8b1947af1). One P1 remains applicable: Iceberg write-sort positions are resolved in the getCols() subset/user-order coordinate domain and consumed against the full-schema sink output. I did not repost it because the same mechanism and outcome are already covered by discussion_r3696025156.
No additional user-provided focus was supplied, so I reviewed the whole 42-file change set.
Critical checkpoint conclusions
- Goal and tests: The MVCC empty-snapshot fence, bound-schema identity propagation, nested nullability alignment, struct cast nullability, and SQL comment quoting changes match the stated goals. Added tests exercise the intended failures; no build or test command was run because this review is explicitly static-only.
- Scope and focus: All authoritative changed files and their relevant callers, consumers, and tests were covered. No unrelated behavior was used to expand the review scope.
- Concurrency: Query-begin empty snapshots remain fenced across eager, COUNT, and streaming dispatch; property loading pins before every production batch decision. Iceberg's null RowDelta baseline validates all versions, and the barrier test covers a concurrent first append.
- Lifecycle and ownership: Immutable Iceberg handle copies preserve snapshot/ref/schema, resolved-empty, rewrite-scope, and Top-N state. Connector session/TCCL and transaction begin/commit ownership remain balanced; no leak or stale-handle path was found.
- Configuration: Batch-mode enable/threshold settings, Iceberg type-mapping flags, SQL escape modes, merge partitioning, and cache-enabled/disabled paths were checked. No new invalid-default or ignored-setting issue was found.
- Compatibility: New SPI methods retain compatible defaults/overloads. Scalar type parameters are canonicalized where parameterless, meaningful parameters remain compared, and recursive IDs/requiredness survive the connector boundary. No new API or serialized-format break was found.
- Parallel paths: Ordinary/partial/static/reordered INSERT, UPDATE, MERGE, REWRITE, eager/COUNT/streaming/system scans, and STRUCT/ARRAY/MAP paths were traced. The only surviving positional defect is the existing Iceberg sort thread cited above.
- Special conditions: Resolved-empty versus unresolved-latest, snapshotless metadata tables, explicit time travel, v3 lineage fields, hidden/all-clear parent masks, nested required fields, and both SQL escape modes were checked without finding a distinct issue.
- Test coverage and result correctness: The tests use deterministic barriers and direct state/output oracles for the changed contracts. The remaining integrated partial/reordered write-sort gap is already requested in the existing thread. Test execution was not permitted for this review.
- Observability: Existing error propagation, EXPLAIN/profile behavior, and connector failure visibility remain intact; no new silent-degrade, logging, or metrics gap was found.
- Transactions and persistence: Empty-read MERGE conflict validation covers the first append, begin remains single-owner, and commit validation retains its conflict filters. No EditLog or other FE persistence format is changed.
- Data writes, atomicity, and crash behavior: Bound schema generations, recursive field identity, and v3 reserved columns are carried through write planning and validation. No new partial-commit or crash-consistency issue was found; the already-threaded sort-coordinate mismatch still needs resolution.
- FE/BE contracts and variables: Struct field nullability now matches nullable BE cast wrappers, connector type conversion preserves nested metadata, and sink sort/partition information was traced end to end. No new variable-propagation issue was found.
- Performance: The format-v2 gate avoids descendant scans and entry-sized scratch allocation when the ancestor mask is absent or all-clear; required ARRAY/MAP projections remain coordinate-correct. No new hot-path regression was found.
- Other issues: No security-sensitive behavior was requested or introduced, and no additional correctness issue survived independent verification and duplicate suppression.
Review status: complete and converged; zero new inline comments.
Contributor
ClickBench: Total hot run time: 23.92 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
This PR fixes correctness and memory-usage edge cases in Iceberg MVCC writes and nested schema evolution:
ColumnNullable.The fix preserves the explicit empty-snapshot marker through scan planning and keeps RowDelta conflict validation anchored at table creation. In the concurrent empty-table MERGE/INSERT window, MERGE keeps reading the original empty view and its commit conflicts with the concurrent first append instead of producing duplicate data.
Write planning now carries an immutable complete target-schema snapshot separately from the write subset, validates column order, type, and Iceberg field id, and excludes engine-generated row-lineage metadata from the user-schema comparison. Nested collection alignment gates descendant scans on an inherited mask that can actually hide a row, and struct constructors preserve the physical nullability of cast results.
Release note
Fix Iceberg empty-snapshot concurrency, partial-write schema validation, nested collection memory usage, cast-result nullability, and nested DESCRIBE comment rendering.
Check List (For Author)
Tests
TableReaderTest: 95 passedgit diff --check: passed