[fix](iceberg) Fix MVCC and nested schema evolution edge cases - #66345
Open
Gabriel39 wants to merge 4 commits into
Open
[fix](iceberg) Fix MVCC and nested schema evolution edge cases#66345Gabriel39 wants to merge 4 commits into
Gabriel39 wants to merge 4 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
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 28860 ms |
Contributor
TPC-DS: Total hot run time: 169312 ms |
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.
| ConnectorTableHandle tableHandle, List<ConnectorColumn> connectorColumns, | ||
| TSortInfo writeSortInfo, WriteOperation writeOperation, boolean requireMergeCardinalityCheck) { | ||
| this(targetTable, writePlanProvider, connectorSession, tableHandle, connectorColumns, | ||
| connectorColumns, writeSortInfo, writeOperation, requireMergeCardinalityCheck); |
Contributor
There was a problem hiding this comment.
[P1] Keep the row-level projection and validation baseline on one schema generation. UPDATE and MERGE each call getBaseSchema(true) once while building [operation,row_id,data...] and again for sink.getCols(). With Iceberg table-cache TTL 0, a reorder can land between them; this overload then aliases the later list as boundTargetColumns, so S1 validates against S1 while BE writes the S0-ordered expressions positionally under S1. A same-shaped FIXED/CHAR reorder silently swaps values. Please capture the schema once and thread that exact list through projection, sink, and validation, with a consecutive-read drift test.
| ConnectorType currentType = IcebergTypeMapping.fromIcebergType( | ||
| current.type(), enableVarbinary, enableTimestampTz); | ||
| if (!current.name().equalsIgnoreCase(bound.getName()) | ||
| || !currentType.equals(bound.getType()) |
Contributor
There was a problem hiding this comment.
[P1] Compare recursive Iceberg field identities. The bound conversion has already lost every nested Column.uniqueId, and ConnectorType.equals intentionally excludes child IDs and optionality. For example, a FIXED(n) struct child can be dropped and re-added with the same name/type/position, pass this check as the same CHAR(n) shape, and have the old bound child payload written under the replacement field ID from the live schema JSON. Please carry and compare recursive IDs/requiredness (or an authoritative schema generation) and add a nested recreate test.
| */ | ||
| public class PhysicalConnectorTableSink<CHILD_TYPE extends Plan> extends PhysicalBaseExternalTableSink<CHILD_TYPE> { | ||
|
|
||
| private final List<Column> boundTargetSchema; |
Contributor
There was a problem hiding this comment.
[P1] Resolve positional consumers from this bound schema. getRequirePhysicalProperties() still reloads Hive/MaxCompute partition names and full-schema positions, then indexes the older child output; the Iceberg write-sort path likewise returns live ordinals and indexes that output before schema validation. A refresh or reorder can therefore hash/sort the wrong expression or throw out of bounds. Please carry stable partition/sort identities with this generation and derive every output position from it.
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 |
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