Repository navigation
[fix](fe) Resolve dropped Iceberg equality-delete keys after a schema ID is reused - #69536
Conversation
… ID is reused ### What problem does this PR solve? Issue Number: None Related PR: apache#66538, apache#69394 Problem Summary: When an Iceberg table drops a column so that its schema becomes equal to an earlier one, Iceberg reuses the earlier schema ID instead of adding a new schema, and the metadata schema list keeps that schema at its original, earlier position. The equality-delete schema carrier in the Iceberg connector located the scan schema in that list by ID and searched only the schemas listed before it. A live equality delete keyed on the dropped column therefore could not find the schema that still defines the key, and every scan of the table failed with "Iceberg equality-delete fields are absent from schema history". This happens in steady state, not only during an upgrade. Reproduce: create a table with column id, add column k, write data and an equality delete keyed on k, drop k (the schema returns to the initial one and reuses its ID), then query the table. branch-4.1 (apache#66538) searches the scan snapshot's parent chain first, so the definition recorded on that lineage wins, and falls back to the whole metadata list, newest first, for schema-only changes and expired ancestors. This change ports that order to IcebergScanPlanProvider. Each lineage schema is indexed at most once and the walk stops as soon as every key is resolved. It also ports the branch-4.1 tests for a reused schema ID (with and without an expired parent) and for time travel past a later rename. The existing test equalityCarrierKeepsLatestDefinitionOfDroppedEqualityKey (apache#69394) renamed and then dropped the key with no write in between. It now writes a snapshot after each schema change, as the branch-4.1 test it was ported from does, and still expects k2. ### Release note Fix Iceberg queries that failed with "equality-delete fields are absent from schema history" after a column used as an equality-delete key was dropped and the table schema returned to an earlier one. ### Check List (For Author) - Test: Unit Test - fe-connector-iceberg: 4 new cases and 1 adjusted case in IcebergScanPlanProviderTest; the two reused-ID cases fail without the fix; the whole module passes - Behavior changed: Yes. Such queries no longer fail. When a dropped key has several historical definitions, the carrier now prefers the one recorded on the scan snapshot's lineage, as branch-4.1 does. - Does this need documentation: No Co-authored-by: daidai <[email protected]> Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
|
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.
One P1 correctness regression is reported inline: resolving a dropped key solely from snapshot lineage can select a type older than a pending writer's schema, causing a widened equality-delete value to become NULL and match an unrelated NULL-key row. Changes are requested. No existing review threads or previously reported blockers were present.
Reviewed both changed files and the affected planning -> schema carrier -> scan-parameter -> native-reader flow at dc21748. Critical checkpoint conclusions:
- Goal and proof: the real-table tests address schema-ID reuse and expired parents, including dictionary transport. The pending-writer promotion case in the inline finding remains incorrect and lacks coverage.
- Scope and reuse: the change is focused on one lookup algorithm and its unit tests, reusing the existing historical-field merge helpers.
- Concurrency: new collections are local and reads use statement-frozen table metadata; no new locks or worker threads. The finding concerns external writes committing after schema-only changes, which the snapshot schema does not fully describe.
- Lifecycle: statement/table/FileIO ownership is unchanged. Null schema IDs and expired parents reach the schema-history fallback; SDK expiration retains schemas referenced by retained snapshots.
- Configuration: no configuration changes.
- Compatibility: no public SPI, storage-format or wire-format change. Existing backend-semantics fencing remains in use.
- Thrift/protobuf IDs: no gensrc changes; branch field-ID comparison is inapplicable.
- Parallel paths: production and testing callers pass the selected snapshot; normal, pinned/ref and streaming scans share the carrier. Both native reader generations were inspected; system-table handling is separate and unchanged.
- Conditions and errors: repeated schema IDs are deduplicated, the no-missing-key path skips ancestry, and missing schemas/unresolved IDs fail loudly. Early completion after resolving an older type causes the reported defect.
- Test coverage: four new JUnit tests and the amended rename/drop test were inspected alongside existing nested-key, default, reused-name and reader-promotion tests. Proxy histories establish local selection rules, not end-to-end reader results. No test directly covers the reported sequence or large missing-key ancestry.
- Test results: no generated result files changed. No builds or tests were run under the static-only review contract; the author's reported test count was not independently validated.
- Observability: existing error messages identify missing schema/equality IDs; no new logging or metrics requirement was substantiated for this local planning change.
- Transactions and persistence: no Doris transaction, EditLog or replay changes. External snapshot commit semantics were checked against the pinned Iceberg SDK sources linked inline.
- Writes and crash behavior: production changes only reconstruct read metadata; they introduce no writes, commit protocols or resource handles.
- FE/BE variables: only a local Java Snapshot argument is added; dictionary fields and transport paths remain unchanged. Its historical type affects the existing BE comparison contract as described inline.
- Performance: schema indexing is deduplicated and carrier size remains bounded by the current schema and relevant historical fields. Missing keys can require O(retained ancestry) lookups and REST refs-mode can load deferred snapshot history; no separate actionable performance regression was established or benchmarked.
- Other concerns: nested field-ID merging, dropped/re-added names, stable initial defaults, schema-only updates, time travel, branches, expiry, and both reader generations were checked. No additional verified finding remains.
No additional user-provided focus was supplied. Review limits are static inspection only, unexecuted runtime tests, and unmeasured large-lineage/REST history cost. All identified candidates and risk focuses have explicit dispositions in the shared ledger. Final convergence is recorded separately by the submission helper after the unchanged-payload reviewer pass.
Review converged after 3 round(s).
| throw new IllegalStateException( | ||
| "Iceberg snapshot schema " + schemaId + " is absent from table metadata"); | ||
| } | ||
| addHistoricalEqualityFields(fields, missing, lineageSchema); |
There was a problem hiding this comment.
[P1] Preserve promoted types when recovering a dropped equality key
Snapshot lineage can omit the type used by a pending writer. Start with optional k INT and an existing row where k IS NULL; promote k to LONG without a snapshot, prepare a LONG equality-delete file for k=2147483648, drop k, then commit the pending RowDelta. Use unpartitioned files without column metrics (including null counts and bounds). The SDK permits this: SnapshotProducer.apply() refreshes metadata and stamps the current schema, and BaseRowDelta does not reject the changed key schema. The delete snapshot therefore records the dropped schema while its parent still records INT.
This call resolves the ID from that INT ancestor and removes it from missing, so the retained schema-only LONG definition is never considered. V2 _resolve_equality_delete_fields takes the carrier's INT as its comparison type; _load_equality_delete_file then casts the LONG tombstone to INT. Overflow becomes NULL in non-strict mode, and NULL-safe equality incorrectly deletes the existing NULL-key row (strict mode fails the query). The previous metadata-history lookup selected LONG and preserved that row. Preserve the applicable promoted comparison type when choosing lineage definitions, and cover the promotion/drop/pending-delete sequence in tests.
There was a problem hiding this comment.
Confirmed, and fixed in 71f51ed. With Iceberg 1.12 a pending RowDelta is stamped with the schema that is current when it commits (SnapshotProducer.apply → base.currentSchemaId()), and nothing validates its equality field IDs against that schema, so a key promoted by a schema-only change and dropped before the delete commits has no snapshot on the lineage that records the promoted type. FileScannerV2 compares equality keys in the carrier's type, so the INT carrier narrowed the LONG tombstone to NULL (non-strict) and matched NULL keys; the V1 reader compares in the delete file's own type and was not affected.
The carrier now keeps the lineage definition of a dropped key (name, optionality, initial default) but takes the widest type its field ID has in any retained schema, where wider means an Iceberg promotion (TypeUtil.isPromotionAllowed: INT→LONG, FLOAT→DOUBLE, decimal precision). Field IDs are never reused and promotion is monotonic and lossless, so that type covers every file written for the field, and widening is harmless when it comes from a schema after a time-travel target. equalityCarrierKeepsPromotedTypeOfKeyDroppedBeforeItsDeleteCommits replays the sequence with the SDK (promote, prepare the delete, drop, commit) and fails without the widening (expected: <long> but was: <int>).
branch-4.2 resolves keys the same way (IcebergScanNode.getSchemaFieldsForScan) and has the same FileScannerV2 comparison, so it carries the same defect; it will be passed to the 4.x maintainers. One residual case is shared by every version: expireSnapshots().cleanExpiredMetadata(true) can remove a schema-only schema that no retained snapshot references, after which no metadata records the promoted type; closing that needs a BE-side guard when a delete file's key type is wider than the carrier, which is outside this PR.
### What problem does this PR solve? Issue Number: None Related PR: apache#66538 Problem Summary: Review follow-up. The previous commit resolves an equality-delete key that the scan schema no longer has from the scan snapshot's parent chain first. With Iceberg 1.12 a pending RowDelta is stamped with the schema that is current when it commits, and nothing checks its equality field IDs against that schema. So for a key k promoted from INT to LONG by a schema-only change (no snapshot), a writer that wrote LONG tombstones, and k dropped before that delete commits, the delete snapshot records the schema without k and its parent the INT one: no snapshot on the lineage records LONG. The carrier then said INT, FileScannerV2 compares equality keys in the carrier's type, and a tombstone such as 2147483648 was cast to NULL in non-strict mode and deleted every row whose k is NULL (strict mode failed the query). The previous metadata-list lookup found LONG. A dropped key now keeps its lineage definition (name, optionality, initial default) but takes the widest type its field ID has in any retained schema. Field IDs are never reused and promotion is monotonic and lossless, so that type covers every file written for the field, also when it comes from a schema after a time-travel target. ### Release note None ### Check List (For Author) - Test: Unit Test - IcebergScanPlanProviderTest#equalityCarrierKeepsPromotedTypeOfKeyDroppedBeforeItsDeleteCommits replays the sequence with the SDK; it fails without the widening - fe-connector-iceberg module tests - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
FE UT Coverage ReportIncrement line coverage `` 🎉 |
TPC-H: Total hot run time: 28841 ms |
TPC-DS: Total hot run time: 150849 ms |
TPC-H: Total hot run time: 28114 ms |
TPC-DS: Total hot run time: 150394 ms |
ClickBench: Total hot run time: 24.3 s |
|
run buildall |
|
/review |
There was a problem hiding this comment.
Request changes for two verified regressions in dropped equality-key recovery: v3 temporal promotion can produce incorrect deleted rows, and schema-only nullability relaxation can make valid scans fail. Both are reported inline.
The existing P1 4359014047 is fixed for its reported INT-to-LONG scenario: retained promoted types now override a stale lineage type. It is not reposted or counted as an existing blocker. The temporal finding is distinct: the new helper overwrites an already-correct promoted lineage definition. No additional user focus was supplied.
Static review covered both changed files, their callers, carrier serialization, snapshot/schema selection, relevant Iceberg 1.12.0 contracts, and both native reader paths. No builds or tests were run, as required by this review environment.
Critical checkpoint conclusions:
- Goal and proof: lineage recovery and whole-history fallback address reused schema IDs and expired parents. Added unit tests assert these metadata carriers and the pending INT-to-LONG delete case; the two inline cases remain incorrect.
- Scope and reuse: the change is confined to the carrier builder, its call sites and tests. Existing historical-field merging and encoding are reused. The promotion helper's supported-type assumptions need correction.
- Concurrency and locks: new collections are invocation-local; production statement tables retain one metadata generation. No locks, worker threads or lock ordering change. Concurrent external schema evolution is relevant to the nullability finding's pending writer.
- Lifecycle and initialization: no new owned resources, static initialization dependencies or cleanup paths. Ancestor traversal stops at a missing/expired parent and falls back to retained schemas.
- Configuration: no added settings or dynamic-configuration behavior.
- Compatibility: no SPI, storage-format or symbol changes. Existing scan capability fencing remains; legal v3 temporal evolution exposes the reported comparison-domain regression.
- Thrift/protobuf: no definition or field-ID changes; cross-release ID comparison is not applicable.
- Parallel paths: synchronous/streaming native scans share the scan-node carrier. V2 uses the carrier comparison type; V1 takes physical delete-file types. Requiredness reaches missing-field construction in both readers. System-table paths are unchanged.
- Conditions and error handling: missing-schema/key failures remain explicit and documented; the nullability finding instead rejects valid missing-key data using stale requiredness. No new exception is silently ignored.
- Test coverage: new real-table tests cover ID reuse, parent expiration and a pending promoted delete; existing tests cover nested defaults and bounded carriers. Synthetic time-travel histories test ordering, not realistic writer reachability. Physical-file row results, temporal promotion and schema-only optionality are not exercised by the added cases.
- Test results: no regression output files changed; new assertions are metadata/schema assertions. Execution results reported in the PR description were not independently reproduced.
- Observability: existing metadata failures and reader errors remain available; no new distributed operation or logging/metric requirement was identified.
- Transactions and persistence: Doris write, EditLog and failover paths are unchanged. External snapshot/schema commit behavior was traced to validate the reported scenarios.
- Data writes and crash behavior: the patch only changes read planning; it adds no data write, atomicity, crash cleanup or write-lock obligation.
- FE-to-BE values: the existing dictionary and encoding carry the changed key type/requiredness to the readers; no new transmitted variable or send path is introduced.
- Performance and memory: missing-key recovery now scans retained schema history and snapshot ancestry, with each lineage schema indexed once; it does not enumerate data-file splits and carrier size stays bounded by the full selected schema plus referenced historical keys and their ancestors. No benchmark was run and no separate actionable performance regression was established.
- Further review: default conversion, current-field precedence, nested identity matching, name reuse, time travel and fallback were rechecked. No additional distinct defect was verified beyond the two inline findings.
Coverage limits: this is a static review, without physical-file execution or an external writer run. The author's already-disclosed loss of an unreferenced schema-only promotion after cleanExpiredMetadata(true) is a pre-existing limitation, not a new finding in this invocation.
Review converged after 3 round(s).
| continue; | ||
| } | ||
| Type widest = widestTypes.get(fieldId); | ||
| if (widest == null || TypeUtil.isPromotionAllowed(widest, type.asPrimitiveType())) { |
There was a problem hiding this comment.
[P1] Preserve v3 temporal promotions when selecting the comparison type
Iceberg v3 permits DATE to unzoned TIMESTAMP/TIMESTAMP_NS promotion, but the pinned 1.12.0 TypeUtil.isPromotionAllowed only recognizes INT/LONG, FLOAT/DOUBLE and decimal precision (spec, helper). Iterating retained schemas oldest-first therefore keeps DATE, and withWidestTypes replaces even a correctly selected TIMESTAMP lineage field with DATE.
For an unpartitioned v3 table, retain a row with k=DATE '2024-01-02', promote k to TIMESTAMP, commit an equality delete for 2024-01-02 12:00:00, then drop k. Omit column metrics so the delete reaches the reader. V2 _resolve_equality_delete_fields chooses carrier DATE and _load_equality_delete_file casts the noon tombstone to DATE: it incorrectly deletes the original row, whose promoted value is midnight. The previous history lookup retained TIMESTAMP. Handle the v3 temporal comparison domains explicitly, avoid narrowing the lineage type, and cover this sequence. This differs from the existing INT/LONG thread because the promoted schema is present on the lineage.
There was a problem hiding this comment.
Confirmed, and fixed in cbf21fb0585. The widening added in 71f51ed relied on TypeUtil.isPromotionAllowed, which in Iceberg 1.12.0 (and on main) knows only INT→LONG, FLOAT→DOUBLE and decimal precision. For a v3 key promoted from DATE to TIMESTAMP, the first retained type therefore won and replaced the TIMESTAMP definition the lineage had already found. FileScannerV2 then cast the noon tombstone to DATE with an ordinary truncating cast and deleted the midnight row. V1 compares in the delete file's own type and was not affected.
Each field of the selected definition now starts from its own type and moves only along a recognized promotion: the SDK rules plus DATE→unzoned TIMESTAMP/TIMESTAMP_NS. It can no longer narrow a key or switch it to an unrelated type, and the result does not depend on the order of the retained schemas. equalityCarrierKeepsLineageTimestampOfPromotedDateKey replays this sequence, and equalityCarrierWidensDateKeyPromotedWithoutASnapshot covers a schema-only DATE→TIMESTAMP_NS promotion followed by a pending delete and a drop. Both publish the promotion with TableMetadata.Builder.setCurrentSchema, as promotedDateBoundsDoNotPruneNanosecondMatches does, because SchemaUpdate.updateColumn in 1.12.0 still rejects it; PyIceberg and iceberg-go reject it too unless incompatible changes are allowed, while a REST catalog's add-schema update only checks the new schema on its own.
Master before this PR and branch-4.2 do not widen, so neither has this regression. branch-4.2 does share the schema-only variant, because its lineage yields DATE; that case goes into the note for the 4.x maintainers.
| throw new IllegalStateException( | ||
| "Iceberg snapshot schema " + schemaId + " is absent from table metadata"); | ||
| } | ||
| addHistoricalEqualityFields(fields, missing, lineageSchema, widestTypes); |
There was a problem hiding this comment.
[P2] Retain schema-only optionality when recovering a dropped key
This consumes a key from missing using its snapshot-lineage definition before the metadata fallback can observe a compatible required-to-optional change. Consider a v2 unpartitioned table with required k INT and no initial default: append S1, make k optional without a snapshot, prepare a NULL equality-delete RowDelta, drop k, append an id-only file as S2, then commit the pending delete as S3. The SDK allows this pending commit and stamps the current dropped schema (SnapshotProducer, BaseRowDelta). Use files without column metrics so pruning does not exclude the delete.
S2/S3 omit k; recovery reaches S1 and restores REQUIRED, while the previous metadata lookup selected the intervening OPTIONAL definition. BE then rejects S2 with Missing required field: k instead of materializing NULL for the equality comparison. Type widening does not address this unchanged-INT case. Preserve the compatible schema-only optionality transition and add this pending-delete/drop/append test; globally relaxing still-required fields would hide real errors.
There was a problem hiding this comment.
Confirmed, and fixed in cbf21fb0585. The pending delete and the id-only append both commit with the post-drop schema, so the lineage walk reaches the original REQUIRED definition and never sees the schema-only makeColumnOptional. Both readers then reject the id-only file: FileScannerV2 with Missing required field: k, and V1 with Required Iceberg field 'k' is missing from the data file and has no initial default. Master's lookup found OPTIONAL and read NULL. Column metrics do not prevent this: Iceberg's own DeleteFileIndex resolves k from the highest schema ID, where it is optional, and attaches the NULL tombstone because the new file has no stats for k.
A dropped key is now optional when any retained schema has it optional. Required→optional is the compatible direction (the reverse needs allowIncompatibleChanges), and reading NULL for the missing key is safe either way. A key that is required in every retained schema stays required, so the missing-required check does not change for it. equalityCarrierKeepsSchemaOnlyOptionalityOfDroppedKey replays the pending-delete/drop/append sequence, and equalityCarrierKeepsAlwaysRequiredDroppedKeyRequired checks that an always-required dropped key stays required.
branch-4.2 also resolves keys lineage-first in IcebergScanNode.getSchemaFieldsForScan, and its readers reject the file the same way, so it has the same defect; it goes into the note for the 4.x maintainers.
TPC-H: Total hot run time: 28428 ms |
TPC-DS: Total hot run time: 150560 ms |
ClickBench: Total hot run time: 24.29 s |
FE UT Coverage ReportIncrement line coverage `` 🎉 |
FE Regression Coverage ReportIncrement line coverage |
…d compatible changes ### What problem does this PR solve? Issue Number: None Related PR: apache#69536 Problem Summary: A dropped Iceberg equality-delete key takes its definition from the scan's snapshot lineage, falling back to the whole schema list. The previous commit then replaced the key's type with the widest type it has in any retained schema, comparing types with TypeUtil.isPromotionAllowed. In Iceberg 1.12.0 that helper knows only INT to LONG, FLOAT to DOUBLE and decimal precision, so for a v3 table whose key was promoted from DATE to an unzoned TIMESTAMP the first retained type, DATE, won and replaced the TIMESTAMP definition the lineage had found. FileScannerV2 compares equality keys in the carrier's type, so it cast a noon TIMESTAMP tombstone to DATE and deleted the midnight row of that day. A schema-only DATE to TIMESTAMP_NS promotion of a key dropped before its delete committed kept DATE as well. The lineage also lost a schema-only change of a required key to optional: a pending delete and an append without the key both commit with the post-drop schema, so the lineage reaches the original REQUIRED definition, and both readers reject the appended file with a missing required field, where the lookup before this PR found OPTIONAL. Each field of the selected definition now starts from its own type and optionality and takes, from every retained schema, only a compatible relaxation: a promotion recognized by TypeUtil.isPromotionAllowed or the v3 promotion of DATE to an unzoned TIMESTAMP or TIMESTAMP_NS, and optional when any retained schema has the field optional. A key cannot be narrowed or switched to an unrelated type, and a key required in every retained schema stays required. ### Release note None ### Check List (For Author) - Test: Unit Test - IcebergScanPlanProviderTest: a v3 DATE key promoted to TIMESTAMP keeps the lineage's TIMESTAMP; a DATE key promoted to TIMESTAMP_NS without a snapshot and dropped before its delete commits widens to TIMESTAMP_NS; a key made optional without a snapshot and dropped before its delete commits stays optional; a key required in every schema stays required - fe-connector-iceberg module tests - Behavior changed: No (fixes a regression of this PR's previous commits) - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
|
Addressed the second review round in
Four new tests in |
|
run buildall |
|
/review |
There was a problem hiding this comment.
Reviewed #69536 at cbf21fb against the supplied aggregate diff. Two new P2 findings remain: temporally promoted DATE defaults can abort FE planning, and a future nanosecond promotion can make a historical DATE equality-delete scan fail. Both are inline with concrete histories and static call-chain evidence.
The original scenarios in comments 4359014047 (INT/LONG), 4363903193 (temporal narrowing), and 4363903205 (schema-only optionality) are fixed at this head. They were independently revalidated and are not reposted; no existing P0/P1 comment remains blocking for its reported scenario.
Reviewed flow: statement-frozen table -> exact snapshot/ref scan -> live equality-delete field IDs -> scan schema and ancestry -> retained-schema fallback/relaxations -> field-ID dictionary -> V1/V2 native readers.
Critical checkpoint conclusions:
- Goal/data correctness: ancestry plus whole-history fallback resolves reused schema IDs and expired parents in the inspected SDK histories. The two temporal failures prevent an unconditional correctness conclusion; the added tests do not cover them.
- Scope/reuse: the change is limited to the existing provider and unit suite, reusing schema selection, merging and dictionary encoding.
- Concurrency/locks: no threads or locks are added. Local collections operate on statement-frozen metadata; schema and snapshot lookups share that generation and preserve the selected snapshot's visibility.
- Lifecycle/static initialization: no new external resources, shared mutable state or static initialization dependency. Existing table leases own metadata/FileIO lifetime.
- Configuration: no settings or dynamic-update propagation changes.
- Compatibility: no SPI API, function symbol or storage-format changes. Existing current-backend scan-semantics fencing still covers equality-delete scans.
- Thrift/Protobuf: no definitions or field IDs change.
- Parallel paths: traced dictionary consumers in V1 and V2, including Parquet/ORC. Default reconstruction fails before either reader; the future-range finding specifically follows V2's carrier-domain cast. System/position-delete and no-equality branches retain their existing paths.
- Conditions/error handling: empty-missing fast path, repeated lineage schemas, legacy null schema IDs and expired-parent fallback are consistent. Unresolved IDs and malformed retained-snapshot schema references fail explicitly. Compatible nested ancestor/child restoration was checked without a separate defect.
- Test coverage: real SDK histories cover reused IDs, expiration and pending promotion/optionality/drop; doubles cover historical precedence. The reused-ID test round-trips the dictionary. Temporal default-bearing and future-range cases need coverage. Added file helpers are metadata-only, so these additions do not execute row/delete semantics.
- Results/framework: additions follow existing JUnit conventions; no regression output files are changed. No builds or tests were run, as required by this static-only review contract.
- Observability: missing schema/field errors remain explicit; no new distributed operation or metric requirement was identified.
- Transactions/persistence: no Doris transaction, EditLog, failover or persistent-state write path changes. External pending commits were traced through the pinned Iceberg SDK.
- Writes/atomicity: production changes are read-only planning; no data mutation or crash-cleanup path is added. Doris MoW bitmap/version logic is unaffected.
- FE-BE transport: existing TField type/default/optionality fields reach both readers; no new transmitted variable or sending point is introduced.
- Performance/memory: missing-key recovery walks retained ancestry and schemas, and SDK field lookup can populate schema-wide ID indexes. No new data-file enumeration or remote scan is introduced; carrier output stays limited to current fields plus referenced keys. No material performance defect was substantiated, and no benchmark was run.
- Other concerns/user focus: no additional user focus was supplied. Full changed-file and related-path coverage is recorded in the shared ledger; no other verified issue or unresolved candidate remains.
Validation limits: static source/spec/test inspection only; no physical Parquet/ORC execution, cluster validation or performance measurements. The findings are supported by explicit code paths, not claimed runtime reproductions.
Review converged after 2 round(s).
| type = Types.StructType.of(withRetainedRelaxations(type.asStructType().fields(), schemas)); | ||
| } | ||
| relaxed.add(type.equals(field.type()) && optional == field.isOptional() | ||
| ? field : Types.NestedField.from(field).ofType(type).isOptional(optional).build()); |
There was a problem hiding this comment.
[P2] Convert DATE defaults before rebuilding a promoted carrier
A non-null DATE default makes this rebuild fail. Iceberg 1.12.0's NestedField.from(field) copies the typed default literals, and the constructor converts them to the new type; DateLiteral.to accepts only DATE, so TIMESTAMP/TIMESTAMP_NS produces Cannot cast default value to timestamp.... See the pinned builder/default conversion and DateLiteral.
For an unpartitioned v3 table, give optional k DATE initial default 2024-01-02, append S1, publish a schema-only TIMESTAMP promotion with the equivalent midnight default, prepare an equality delete, drop k, then commit the pending delete. As in the new temporal tests, publish the promotion through TableMetadata.Builder. The lineage selects S1's DATE definition; retained widening then reaches this builder and aborts FE planning before either reader runs. The previous metadata lookup selected the already-typed TIMESTAMP definition. A copied DATE write default also triggers this.
Convert both defaults into the promoted temporal type before rebuilding and add a default-bearing test. This differs from the existing temporal thread's fixed no-default comparison failure.
There was a problem hiding this comment.
Confirmed, and fixed in 36bac832997. NestedField's constructor casts both defaults to the field type (castDefault → Literal.to), and in Iceberg 1.12.0 DateLiteral.to returns null for anything but DATE. Rebuilding a dropped DATE key whose lineage definition carries an initial or write default as TIMESTAMP/TIMESTAMP_NS therefore threw Cannot cast default value to timestamp and failed planning for every scan the delete applies to. Master took the already-typed TIMESTAMP definition and never rebuilt it, and branch-4.2 does not widen, so neither fails this way. The SDK promotions (INT→LONG, FLOAT→DOUBLE, decimal precision) convert their literals already.
The relaxation now maps a DATE default to midnight of that day in the promoted type and then rebuilds the field. A date outside the TIMESTAMP_NS range still fails, as the spec requires for this promotion. equalityCarrierConvertsDefaultsOfDateKeyPromotedToTimestamp and equalityCarrierConvertsDefaultsOfDateKeyPromotedToTimestampNs replay your sequence with both defaults set, for TIMESTAMP and TIMESTAMP_NS, and check the converted default in the scan dictionary too.
There was a problem hiding this comment.
You're right about the copied write default; fixed in 9affde5af66. The rebuilt carrier field took the write default of the lineage definition. Converting a stale one outside the TIMESTAMP_NS range (1000-01-01 in your history) overflowed in DateTimeUtil.microsToNanos before any reader ran, so planning failed with ArithmeticException: long overflow. The carrier only feeds the scan dictionary, which carries the initial default and never a write default, so a rebuilt field now drops its write default instead of converting it. The initial default is still converted: it cannot change once the field is added, so the retained promoted definition already holds the same value. equalityCarrierDropsStaleWriteDefaultOfPromotedDateKey replays your history (write default cleared with updateColumnDefault, TIMESTAMP_NS published without a snapshot, the delete pending across the drop), and the two earlier default tests now expect no write default on the carrier.
| optional |= retained.isOptional(); | ||
| if (type.isPrimitiveType() && retained.type().isPrimitiveType() | ||
| && isWideningPromotion(type.asPrimitiveType(), retained.type().asPrimitiveType())) { | ||
| type = retained.type(); |
There was a problem hiding this comment.
[P2] Preserve the historical DATE domain across future nanos promotions
Scanning every retained schema also imports promotions committed after a time-travel target. DATE-to-TIMESTAMP_NS has a smaller representable range: the v3 specification requires out-of-range values to fail at runtime, so this promotion is not harmless for an earlier DATE read.
Use an unpartitioned v3 table with optional k DATE, a row at 1000-01-01, and a DATE equality delete for 1001-01-01, without column metrics. Drop k and append an id-only file to establish target T. Later restore k's old nullable schema with TableMetadata.Builder.setCurrentSchema(existingId) and publish a TIMESTAMP_NS schema. Nullable-field deletion rollback is allowed by the same spec, and the metadata builder accepts this history.
Reading T should still compare DATEs and retain both rows. Instead, its lineage's DATE carrier is widened by that future schema. V2 _resolve_equality_delete_fields chooses TIMESTAMP_NS and _load_equality_delete_file uses EvolutionCast, whose DATE-to-nanos strict conversion rejects the year-1001 tombstone. The previous lookup stopped at T's schema and kept DATE. Preserve historical comparison domains for range-restricting promotions and add this pinned-read case. This is distinct from the existing temporal thread: the problematic promoted schema is after T, and no defaults are needed.
There was a problem hiding this comment.
The mechanism is right. If a dropped key is restored with set-current-schema and then promoted to TIMESTAMP_NS, a read pinned before the promotion now compares in TIMESTAMP_NS. FileScannerV2's strict DATE→nanos EvolutionCast then rejects any date outside 1677-09-21..2262-04-11 with TIMESTAMP_NS overflow when casting row …. The query fails closed; no row is dropped or kept by mistake.
I'm keeping the widening, because the FE cannot tell whether a retained schema was added before or after the scan target. Schemas carry no timestamp, schema IDs are reused (the defect this PR fixes), and the list position only records when a schema was first added. If we stopped widening along DATE→TIMESTAMP_NS for a schema that might be later than the target, the case covered by equalityCarrierWidensDateKeyPromotedWithoutASnapshot would come back: a delete prepared after a schema-only promotion holds nanosecond tombstones, and a DATE carrier makes FileScannerV2 truncate them to their date and delete rows silently. A fail-closed error is the safer of the two outcomes. Your history also needs a rollback of the drop through set-current-schema, a DATE→TIMESTAMP_NS promotion that the 1.12.0 schema-update API rejects, and dates outside the nanosecond range. After that promotion the table's current read fails on the same values anyway, as the spec requires.
The complete fix belongs in FileScannerV2, which compares every delete file in the one carrier type. Choosing the comparison type per delete file, from its own key type and the data file's (V1 already uses the delete file's type), would handle this case. It would also handle the related one where expireSnapshots().cleanExpiredMetadata(true) removes a schema-only promotion and leaves the carrier too narrow. That is a BE change outside this FE-only PR; I'll track the two together.
TPC-H: Total hot run time: 28314 ms |
TPC-DS: Total hot run time: 151199 ms |
TPC-H: Total hot run time: 28647 ms |
There was a problem hiding this comment.
Request changes: one new P1 was verified at head 36bac83. Recovery of an UNKNOWN-origin equality key can ignore valid v3 promotions and make FileScannerV2 retain a row covered by a committed equality delete. The inline comment gives the complete triggering history.
Reviewed both changed files and their upstream/downstream paths with two complete-review agents and a separate risk reviewer. This is static inspection only: no builds, tests, or runtime reproduction were executed. No additional user focus was supplied.
Existing-thread dispositions:
- P1 4359014047 and 4363903193 are fixed for their reported INT/LONG and DATE-origin temporal scenarios. No existing P0/P1 remains independently confirmed in its reported form, so existing_blocking_comment_ids is empty.
- P2 4363903205 is fixed: retained optionality is preserved, with an always-required control.
- P2 4366960335: the reported in-range DATE initial/write literals now convert correctly, including nanosecond scaling. A residual copied-write-default failure remains: S1 has optional k DATE, initial default NULL, write default 1000-01-01, and explicitly written in-range data. Clear the write default without a snapshot, publish DATE-to-TIMESTAMP_NS with null defaults, prepare a matching in-range delete, drop k, then commit the pending delete. The recovered S1 definition still carries the obsolete write default; converting it at line 2376 overflows before either reader, although readers never use write defaults. Dropping that obsolete write default from the read carrier avoids this failure. This remains under the existing copied-DATE-write-default concern; no duplicate inline is posted.
- P2 4366960337 still applies: a later retained nanos schema can impose a smaller comparison range on a historical dropped DATE key, causing a strict overflow error. This known fail-closed time-travel limitation is not reposted. Metadata cleanup that removes the only promoted schema is also already discussed in existing replies.
The reviewed runtime chain is:
getScanNodeProperties -> frozen table + pinned scan/schema
-> selected-snapshot equality IDs -> lineage + metadata fallback
-> retained relaxations -> history_schema_info
-> V1 Parquet/ORC defaults and V2 equality comparisons
Individual code-review skill Part 1.3 conclusions:
- Goal and proof: the lineage walk and full fallback address reused schema IDs and expired ancestors in the added metadata tests. The new UNKNOWN case remains a correctness gap.
- Scope: changes are focused on two Java files; no unrelated SPI or architecture change.
- Concurrency and locks: immutable statement metadata supplies one generation; all new collections are invocation-local. No new threads, locks, lock ordering, or work under locks.
- Lifecycle and static initialization: fields are immutable request carriers; existing table owners retain FileIO. No new resource handles, ownership cycles, or static initialization dependency.
- Configuration: no new settings; existing type-mapping flags still reach serialization.
- Compatibility: existing dictionary shape and scan-semantics gates are reused. Equality-delete scans already require current backend semantics during rolling upgrades.
- Thrift/protobuf: no definitions or IDs changed.
- Parallel paths: V1 Parquet and ORC share the carrier/default handling; V2 uses the carrier comparison type. The new silent-mismatch finding is on V2. Metadata system tables do not use this data-carrier path.
- Special conditions: repeated schema IDs are indexed once; null/expired ancestry falls back; missing metadata and invalid nonprimitive keys fail explicitly. Nested valid keys preserve stable-ID struct ancestors and current fields first.
- Test coverage: added FE tests cover reused IDs, expired ancestry, lineage preference, pending promotions, optionality and temporal defaults. UNKNOWN-origin recovery and physical-row equality results are not covered. End-to-end Parquet/ORC and BE execution remain validation limits.
- Test results: no recorded result artifacts changed; assertions were inspected and no execution is claimed.
- Observability: existing metadata, requiredness and cast errors cover failure paths; no new metric or INFO logging is needed for this metadata-selection change.
- Persistence, transactions and failover: production changes only plan reads from committed external snapshots; no EditLog or commit mutation. Snapshot/schema selection stays within the frozen statement generation.
- Writes, atomicity and crashes: no production write path changes. The P1 violates committed-delete visibility on reads; no new crash cleanup or write-atomicity obligation was introduced.
- FE-BE variables: existing type, optionality and initial-default fields carry the changes through shared serialization; no new transport variable or sending point.
- Performance: metadata-only ancestry/history work; no data-file enumeration added. Carrier size stays limited to current fields plus referenced keys/ancestors. No measured performance claim; node-scoped delete caching does not mix schemas.
- Other checks/core invariants: committed-delete correctness requires the inline fix. Internal partition versions and MoW bitmaps are not involved; BE allocation/ownership is unchanged. Missing/inconsistent metadata fails loudly. All candidates and risk focuses were accepted, deduplicated, or dismissed with evidence; the known P2 limitations and static validation scope remain explicit.
The review converged after two rounds on the unchanged declared finding set; convergence does not mean the PR is ready to merge.
Review converged after 2 round(s).
| if (TypeUtil.isPromotionAllowed(from, to)) { | ||
| return true; | ||
| } | ||
| if (from.typeId() != Type.TypeID.DATE) { |
There was a problem hiding this comment.
[P1] Follow UNKNOWN promotions when recovering a dropped equality key
V3 also permits UNKNOWN to become any concrete type (promotion table). The pinned SDK helper does not recognize that edge, and this non-DATE return leaves a recovered UNKNOWN unchanged.
Use an unpartitioned v3 table with optional k UNKNOWN and commit S1 with k omitted. Publish schema-only UNKNOWN→DATE, prepare a DATE append containing k=2024-01-02, publish schema-only DATE→TIMESTAMP_NS, prepare a matching midnight nanos equality delete, then drop k and commit the pending append followed by the delete. Publish the schemas through TableMetadata.Builder, as the temporal tests do, and omit all column metrics so pruning retains the delete. Both pending snapshots record the dropped schema, so lineage recovery finds only S1's UNKNOWN.
Retained DATE/NANO definitions never replace UNKNOWN. IcebergSchemaUtils encodes it as STRING; V2 restores that carrier type and casts both keys to strings. The DATE becomes 2024-01-02, while the matching tombstone becomes 2024-01-02 00:00:00.000000000, so the deleted row remains visible. The previous reverse metadata lookup selected the retained nanos definition and matched correctly.
Recognize UNKNOWN→concrete promotions and cover this pending-writer history. This differs from the existing DATE-origin thread: the new helper never reaches its DATE handling when the lineage definition is UNKNOWN.
There was a problem hiding this comment.
Confirmed, and fixed in 9affde5af66. The v3 promotion table lets unknown become any type, but Iceberg 1.12.0's TypeUtil.isPromotionAllowed and SchemaUpdate do not implement it, and isWideningPromotion only added DATE on top. In your history the only snapshot that records k is S1, written while k was unknown, so the carrier kept unknown. The scan dictionary encodes unknown as STRING, and FileScannerV2 then casts both keys to STRING: the DATE key becomes 2024-01-02 and the TIMESTAMP_NS tombstone 2024-01-02 00:00:00.000000000, so the deleted row stays visible. Master's reverse lookup took the retained TIMESTAMP_NS definition, so this PR introduced the regression. V1 compares in the delete file's own key type and was not affected.
isWideningPromotion now treats unknown as promotable to any primitive type, so the recovered definition takes the widest retained promotion, TIMESTAMP_NS here. An unknown field only has null defaults, so no default conversion is involved. equalityCarrierFollowsUnknownKeyPromotedWithoutASnapshot replays your history (both promotions published as schema-only changes, a pending append and a pending delete committed after the drop) and checks the carrier and the scan dictionary; without the new rule the carrier stays unknown.
If metadata cleanup has removed every schema that gave k a concrete type, the carrier has nothing to promote to and stays unknown, as on master. That is the same limitation as the cleanup case discussed earlier: the comparison type then has to come from the files, which I'm tracking as the FileScannerV2 follow-up.
TPC-DS: Total hot run time: 150830 ms |
ClickBench: Total hot run time: 24.31 s |
FE UT Coverage ReportIncrement line coverage `` 🎉 |
…s of a dropped equality-delete key ### What problem does this PR solve? Issue Number: None Related PR: apache#69536 Problem Summary: A dropped Iceberg equality-delete key takes its definition from the scan's snapshot lineage and then takes the widest promotion its field has in any retained schema. The promotion check was TypeUtil.isPromotionAllowed plus the v3 promotion of DATE to an unzoned TIMESTAMP or TIMESTAMP_NS. It missed the v3 promotion of unknown to any type, which Iceberg 1.12.0's schema update does not implement but the format allows (PyIceberg's update_column and a REST add-schema do it). On a v3 table whose key was created as unknown, a snapshot committed while it was unknown, and the key was promoted to DATE and then TIMESTAMP_NS without snapshots and dropped before a pending append and a pending equality delete committed, the only snapshot that records the key has it as unknown. The carrier kept unknown, the scan dictionary sent it as STRING, and FileScannerV2 cast both keys to STRING: a DATE key became 2024-01-02 while its TIMESTAMP_NS tombstone became 2024-01-02 00:00:00.000000000, so the deleted row stayed visible. The lookup before this PR took the retained TIMESTAMP_NS definition. V1 compares in the delete file's own key type and was not affected. The previous commit also converted the write default of a rebuilt key to the promoted type. A stale DATE write default outside the TIMESTAMP_NS range, recorded by the snapshot the lineage selects and cleared later without a snapshot, overflowed in DateTimeUtil.microsToNanos and failed planning with "long overflow". The carrier only feeds the scan dictionary, which carries the initial default and never a write default. unknown now promotes to any primitive type when the carrier takes a key's retained promotions, and a rebuilt key drops its write default instead of converting it. The initial default is still converted: it cannot change once the field is added, so the retained promoted definition holds the same value. ### Release note None ### Check List (For Author) - Test: Unit Test - IcebergScanPlanProviderTest: a v3 key created as unknown, promoted to DATE and then TIMESTAMP_NS without snapshots and dropped before a pending append and a pending delete commit, takes TIMESTAMP_NS in the carrier and the scan dictionary; a DATE key whose recorded write default is outside the TIMESTAMP_NS range, cleared and promoted to TIMESTAMP_NS without snapshots, plans without converting it; a rebuilt DATE key keeps only its converted initial default - fe-connector-iceberg module tests - Behavior changed: No (fixes regressions of this PR's previous commits) - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
|
Addressed the fourth review round in
Two new tests in |
|
run buildall |
|
/review |
There was a problem hiding this comment.
Request changes for one new P1: retained TIMESTAMP and TIMESTAMP_NS alternatives can make a dropped equality key use a lossy comparison type after a schema-only rollback, incorrectly filtering a committed row. The inline finding gives the complete trigger and FE-to-BE path.
The schema-ID reuse fix itself is supported by the lineage-first lookup and full-history fallback tests. I independently revalidated the existing P1 threads 4359014047 (INT/LONG), 4363903193 (direct DATE promotions), and 4367404920 (UNKNOWN): their reported triggers are fixed at this head, so none is carried forward. The optionality, DATE-default and stale write-default fixes are also present. Existing P2 thread 4366960337 still describes the acknowledged future DATE-to-nanos range failure; it is already covered and is not reposted. Loss of schema-only type history after metadata cleanup is also an already-known reader limitation.
Critical checkpoint conclusions:
| Checkpoint | Conclusion |
|---|---|
| Goal and evidence | Dropped-key recovery now handles reused schema IDs and expired parents; the new competing-temporal-target case remains incorrect. Added tests prove FE carrier/dictionary expectations, not final BE rows. |
| Scope and clarity | Changes stay within equality-carrier planning and focused JUnit tests. The claim that relaxation is independent of schema order is contradicted by the inline case. |
| Concurrency and locks | Recovery uses local collections over statement-frozen metadata. No new thread, lock, lock-order or critical-section work. |
| Lifecycle and initialization | No new static initialization, external ownership or closeable resource; existing table/FileIO lifetime is preserved. |
| Configuration | No new configuration or dynamic-setting behavior. |
| Compatibility and upgrades | Existing dictionary format and plugin API are retained. The existing current-backend-semantics fence covers equality-delete scans. |
| Thrift/protobuf IDs | No IDL change; cross-branch field-ID audit is not applicable. |
| Parallel paths | Ordinary, streaming and cached scans share this dictionary. Both readers consume it; V2 uses the carrier comparison type, whereas V1 compares in the delete-file type. |
| Conditions and failures | Missing-key, expired-parent and primitive-key checks follow explicit metadata cases; unresolved IDs fail planning. No ignored status or silent exception fallback added. |
| Test coverage | New histories cover reuse, expiry, lineage preference, promotions, optionality and defaults. Existing tests cover nested recovery and fast paths. The competing temporal alternatives need a new case. |
| Test results | No expected-result files changed. No builds or tests were run under this static-only contract; the author's reported test execution was not independently reproduced. |
| Observability | Existing schema/key-ID errors and scan diagnostics are adequate for the added planning logic; no new per-row logging or metric is needed. |
| Transactions and persistence | No Doris transaction state, EditLog or failover protocol changed; scan metadata remains pinned. |
| Data writes and crashes | No production writer or atomicity change, native allocation ownership or new crash-recovery obligation. The reported defect affects read correctness. |
| FE/BE propagation | Existing TField type, optionality and initial-default members carry the result through the shared encoder/apply path; no new wire field. |
| Performance | No added per-data-file work. Parent traversal skips repeated schema indexing; recovered fields scan retained schemas. Full SDK schema-ID maps are already initialized by existing DeleteFileIndex planning. No separate performance defect verified. |
| Other concerns and limits | Nested struct recovery, name/ID precedence, defaults, null schema IDs and expiration were reviewed. No other new verified finding or unresolved candidate remains. |
User focus: no additional focus was supplied. Both changed files and their upstream/downstream consumers were reviewed. Validation remains static: the pending-writer and v3 temporal/UNKNOWN histories lack newly added physical row-level execution evidence.
Review converged after 2 round(s).
| } | ||
| optional |= retained.isOptional(); | ||
| if (type.isPrimitiveType() && retained.type().isPrimitiveType() | ||
| && isWideningPromotion(type.asPrimitiveType(), retained.type().asPrimitiveType())) { |
There was a problem hiding this comment.
[P1] Resolve competing temporal targets before choosing the carrier type
This single pass can keep an abandoned TIMESTAMP definition and reject the TIMESTAMP_NS definition actually used by the delete. On an unpartitioned v3 table, append S1 with optional k DATE = 2024-01-02; publish a schema-only DATE→TIMESTAMP promotion; restore the original DATE schema with TableMetadata.Builder.setCurrentSchema(existingId) before any new snapshot or data is written; then publish DATE→TIMESTAMP_NS. Both forward promotions are allowed by the v3 promotion table, and setCurrentSchema accepts the retained ID. Prepare a nanos equality delete for 2024-01-02T00:00:00.000000001, drop k, and commit the pending delete. Omit all column metrics so the delete reaches BE.
The lineage supplies DATE and retained order DATE/TIMESTAMP/TIMESTAMP_NS/dropped first selects TIMESTAMP. TIMESTAMP→TIMESTAMP_NS is not a recognized promotion, so the carrier stays DATETIMEV2(6). V2 _resolve_equality_delete_fields uses that carrier type, and _load_equality_delete_file casts the nanos tombstone to it; round_timestamp_ns_fraction rounds 1ns to zero. The original midnight row is incorrectly deleted. The base reverse lookup selected TIMESTAMP_NS and kept it.
Handle competing retained temporal domains without a lossy comparison (or fail explicitly), and cover this rollback history. This differs from the existing DATE thread: both DATE promotion edges are implemented here, but the first retained alternative blocks the other; it is also a current, in-range wrong-result case rather than the existing time-travel range error.
There was a problem hiding this comment.
Confirmed, and fixed in b4bb074bfed. The relaxation compared each retained type with the type it had reached so far, so its result depended on the order of the schema list; my earlier statement that it did not was wrong. This only matters when a key has two retained promotions that do not promote to each other: in v3, DATE to both TIMESTAMP and TIMESTAMP_NS, or unknown to two types, and both can be retained only after the current schema is set back to an earlier one (or after a type change that is not a valid promotion). Your history lists DATE, TIMESTAMP, TIMESTAMP_NS, so the loop stopped at TIMESTAMP, and FileScannerV2 rounded the 1 ns tombstone to midnight in DATETIMEV2(6) and deleted the DATE row. Master's reverse lookup took TIMESTAMP_NS, so this PR introduced the regression; V1 compares in the delete file's own type and was not affected. The unknown rule from the previous round has the same flaw: after unknown→INT, a rollback and unknown→STRING, the carrier was INT and turned a non-numeric STRING tombstone into NULL, which matches every row written while k was unknown.
No single carrier type is lossless for every such history, and the metadata does not record which type a delete file was written in. TIMESTAMP drops nanoseconds, and TIMESTAMP_NS cannot hold DATE or TIMESTAMP values outside 1677-09-21..2262-04-11, which FileScannerV2 rejects for DATE and turns into NULL for TIMESTAMP. The carrier now takes the promotion that every other retained promotion of the key promotes to, and planning fails with Iceberg equality-delete field 2 (k) was promoted to both timestamp and timestamp_ns in schema history when there is none. Only promotions of the definition the lineage selected count, so a delete committed while TIMESTAMP_NS was current still resolves TIMESTAMP_NS next to the abandoned TIMESTAMP schema. New tests replay your history in both orders and the unknown variant, and cover that lineage case.
One related history stays as on master and branch-4.2: if a delete is prepared under a TIMESTAMP_NS schema that is then abandoned, and a snapshot is committed while TIMESTAMP is current, the lineage records TIMESTAMP and the carrier keeps it. Telling those apart needs the delete file's own key type, which is the FileScannerV2 follow-up I'm tracking with the cleanup case.
TPC-H: Total hot run time: 28337 ms |
TPC-DS: Total hot run time: 150473 ms |
ClickBench: Total hot run time: 24.2 s |
FE UT Coverage ReportIncrement line coverage `` 🎉 |
FE Regression Coverage ReportIncrement line coverage |
… key regardless of schema order ### What problem does this PR solve? Issue Number: None Related PR: apache#69536 Problem Summary: A dropped Iceberg equality-delete key takes its definition from the scan's snapshot lineage and then the widest promotion its field has in any retained schema. The relaxation compared each retained type with the type it had reached so far and switched whenever that was a promotion, so the result depended on the order of the metadata schema list. The v3 promotions added by this PR give a type two successors that do not promote to each other: DATE to both TIMESTAMP and TIMESTAMP_NS, and unknown to any two types. Both are retained once the current schema is set back to an earlier one (a REST set-current-schema, or republishing an identical schema, which reuses its ID) and the key is promoted again. On a v3 table whose DATE key was promoted to TIMESTAMP without a snapshot, set back to DATE, promoted to TIMESTAMP_NS and dropped before a pending equality delete written under TIMESTAMP_NS committed, the schema list holds DATE, TIMESTAMP, TIMESTAMP_NS and the loop stopped at TIMESTAMP. FileScannerV2 compares keys in the carrier's type, DATETIMEV2(6), so the tombstone 2024-01-02 00:00:00.000000001 rounded to midnight and deleted the DATE row. The lookup before this PR took TIMESTAMP_NS. The unknown promotion added by the previous commit has the same flaw: after unknown to INT, a rollback and unknown to STRING, the carrier was INT, and a non-numeric STRING tombstone cast to NULL, which matched every row written while the key was unknown. V1 compares in the delete file's own key type and was not affected. No single carrier type is lossless for these histories: TIMESTAMP drops nanoseconds, TIMESTAMP_NS cannot hold values outside 1677-09-21 to 2262-04-11, and the metadata does not record which type a delete file was written in. The carrier now collects the retained types that the selected definition promotes to and takes the one every other one promotes to, whatever the schema order. When there is none, planning fails with "Iceberg equality-delete field <id> (<name>) was promoted to both <type> and <type> in schema history". Only promotions of the lineage's definition count, so a delete committed while TIMESTAMP_NS was current still resolves TIMESTAMP_NS beside an abandoned TIMESTAMP schema. ### Release note None ### Check List (For Author) - Test: Unit Test - IcebergScanPlanProviderTest: a v3 DATE key promoted to TIMESTAMP and to TIMESTAMP_NS around a rollback of the current schema, in either order, and an unknown key promoted to INT and to STRING the same way, fail planning with an error that names both types; a delete committed while TIMESTAMP_NS is current keeps TIMESTAMP_NS beside the abandoned TIMESTAMP schema - fe-connector-iceberg module tests - Behavior changed: Yes (when retained schemas promote a dropped equality-delete key to two types that do not promote to each other, planning fails with an explicit error instead of taking whichever type the schema list holds first) - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
|
Addressed the fifth review round in
Four new tests in |
|
run buildall |
|
/review |
There was a problem hiding this comment.
Reviewed exact head b4bb074bfed7b9bb6337254b29a20ddde76ce683 against the supplied merge-base diff. No additional verified finding warrants a new inline comment.
The four earlier P1 findings are addressed at this head: retained numeric promotions (4359014047), v3 DATE promotions (4363903193), UNKNOWN promotions (4367404920), and competing retained promotion targets (4369032165). I independently checked the implementation and corresponding regression histories; none remains a P0/P1 blocker to carry forward. The optionality and DATE-default threads are also addressed, including discarding stale write defaults.
The existing historical DATE-domain finding still applies: a later retained TIMESTAMP_NS schema can make an earlier DATE read fail for out-of-range keys. This is already reported and intentionally retained as a fail-closed limitation, so it is not reposted. The earlier replies also identify the remaining reader-level limits when metadata cleanup removes the only promoted definition or a pending delete was written under an abandoned type. A carrier derived only from metadata cannot distinguish those file types; this review does not claim those known limits are resolved.
Critical checkpoint conclusions:
- Goal and data correctness: lineage-first lookup recovers live dropped equality keys when schema IDs are reused; retained relaxations preserve supported promotions, optionality and initial defaults. Incomparable retained targets fail explicitly. Tests cover reused IDs, expired ancestors and these recovery cases.
- Scope and reuse: the change is confined to one Iceberg scan-planning class and its FE unit tests, reusing existing manifest discovery, TypeUtil projection and schema-dictionary encoding.
- Concurrency and locks: reconstruction adds method-local state, no threads or locks, and performs no refresh or shared mutation. Existing statement/cache metadata and FileIO ownership mechanisms are unchanged; no new lock-order or concurrency defect was found.
- Lifecycle and memory: no new persistent owner, resource handle, callback or static initializer. Temporary collections remain planning-scoped and existing statement/FileIO ownership is preserved.
- Configuration: no settings or dynamic-update behavior are added.
- Compatibility: no exported connector SPI, storage-format or function-symbol change. Existing backend scan-semantics capability checks continue to apply.
- Thrift and protobuf: no definitions, IDs or enums change.
- Parallel paths: the existing dictionary reaches both V1 and V2; V1 Parquet/ORC hidden-key resolution and V2 comparison-domain selection were traced. System-table handling is unchanged.
- Conditional checks and errors: schema reuse, null schema IDs, expired parents, missing key definitions and competing promotions have concrete paths. Ambiguous domains raise a field-specific exception rather than selecting a lossy type.
- Test coverage: added JUnit5 metadata histories cover numeric/DATE/UNKNOWN promotions, initial and stale write defaults, optionality, incomparable targets in both orders, time-travel names and expired lineage. Existing nested-key, drop/re-add and carrier-size cases remain applicable. These FE fixtures validate metadata and dictionary behavior, not complete file-value interoperability.
- Test-result changes: no regression output files change. Assertions and expected errors were inspected; no build or test was run, as required by this static-only review contract.
- Observability: new ambiguity errors include field ID, name and competing types. No new distributed operation or per-row logging needs instrumentation.
- Transactions and persistence: production changes only read committed external snapshot metadata; Doris EditLog, replay and write-transaction handling are unchanged.
- Writes, atomicity and crash cleanup: no production write path or new cleanup obligation is introduced; metadata commits in the diff are test fixtures.
- FE/BE propagation: recovered fields use the existing type/nullability/initial-default dictionary representation; there is no new transmitted variable or serialization route.
- Performance: current equality keys keep the fast exit. Dropped keys walk retained parents, index each distinct lineage schema once and inspect retained definitions per recovered field. No data-file/split enumeration is added, and output is limited to current fields plus referenced historical keys and ancestors. No new actionable performance defect was verified; this was not benchmarked.
- Other invariants and user focus: no additional user focus was supplied. Internal rowset versions/MoW delete bitmaps are outside this external-table planning change. No further verified regression was found in the reviewed scope.
Promotion and literal semantics were checked against the pinned Iceberg 1.12 specification and literal conversion implementation. Review coverage is static; runtime, build/style execution and external-writer interoperability remain unexecuted validation limits.
Review converged after 2 round(s).
TPC-H: Total hot run time: 28260 ms |
TPC-DS: Total hot run time: 150538 ms |
ClickBench: Total hot run time: 24.36 s |
FE Regression Coverage ReportIncrement line coverage |
…lete and data file types ### What problem does this PR solve? Issue Number: None Related PR: apache#69536, apache#68267 Problem Summary: **In short.** FileScannerV2 now compares an Iceberg equality-delete key in the wider of the delete file's and the data file's own key types, instead of in the one table type the FE sends for the whole scan. For some table histories that single type is wrong: the reader then deletes rows it must keep, keeps rows it must delete, or fails the scan. This follows the review of apache#69536, where these cases came up. **Background.** An equality delete removes a data row whose key values equal a tombstone's. After a type promotion the two sides can be stored in different types: tombstones written before an INT -> LONG promotion hold INT values that must still match LONG data. apache#68267 made FileScannerV2 compare in the current schema's type, and for a key that the current schema no longer has, the FE sends a type recovered from the retained schemas (apache#69536). The reader casts both the tombstones and the data keys to that one type. **The problem, and what it cost.** One type per scan cannot describe every delete file: | History | Table type from the FE | Delete file | Data file | Before | After | |---|---|---|---|---|---| | `expireSnapshots().cleanExpiredMetadata(true)` removed the schema of a schema-only INT -> LONG promotion of a dropped key | INT | LONG 4294967296 | INT, one NULL key | the tombstone becomes NULL in INT and deletes the NULL-key row | row kept | | A read pinned before a later DATE -> TIMESTAMP_NS promotion of the key | TIMESTAMP_NS | DATE 1000-01-01 | DATE 1000-01-01 | scan fails: `TIMESTAMP_NS overflow when casting row ...` | row deleted | | No retained schema gives the key a concrete type, so the FE sends Iceberg `unknown` as STRING | STRING | TIMESTAMP_NS 2024-01-02 00:00 | DATE 2024-01-02 | `2024-01-02` != `2024-01-02 00:00:00.000000000`, the row stays visible | row deleted | | The delete was prepared under a TIMESTAMP_NS schema that the table later abandoned for TIMESTAMP | TIMESTAMP | TIMESTAMP_NS 12:00:00.000000500 | TIMESTAMP 12:00:00.000000 | the tombstone is rounded to microseconds and deletes the row | row kept | Three of the four return wrong rows without any error. The V1 reader is not changed by this PR. **How this PR fixes it.** - The delete block keeps the delete file's own key types. When the predicate for a data file is built, each key is compared in the wider of the delete file's type and the data file's type (`equality_delete_comparison_type`). The accepted widenings are the Iceberg promotions (INT -> LONG, FLOAT -> DOUBLE, decimal precision, DATE -> TIMESTAMP and DATE -> TIMESTAMP_NS), a wider fractional scale of the same date-time kind, wider integers, STRING -> VARBINARY (an Iceberg BINARY key decodes as either one, depending on `enable_mapping_varbinary`), and TIMESTAMP -> TIMESTAMP_NS. The last one is not an Iceberg promotion; it is accepted for the abandoned-schema case above, because scaling microseconds to nanoseconds is exact. Two types that convert in neither direction fail the scan instead of being compared in a third type. - The data keys are cast to that type. When it is the data file's wider type, the tombstones are widened once per scanner, and the converted delete block and its hash index are cached next to the original (`_convert_equality_delete_filter`). A table without type changes is compared as before, with no cast and no copy. - A key that the data file lacks is compared through its initial default, in the wider of the delete file's type and the table type. Without an initial default the key is NULL in that file whatever its table type, so it is compared in the delete file's type (an `unknown` key arrives as STRING). - The comparison casts are strict whatever the session's cast mode, through a new `strict` flag on `EvolutionCast`: a non-strict overflow would turn a key into NULL and let a NULL tombstone delete it. DATE -> TIMESTAMP_NS was already strict; other `EvolutionCast` users are unchanged. - The table types from the FE still resolve table fields and the defaults of missing keys; the delete-file cache key is unchanged. **Results.** - 9 new BE unit tests in `iceberg_reader_test.cpp`: the four histories above; INT tombstones against LONG, INT and LONG data files of one scan (the third split reuses the cached widened copy); a missing key's INT default against a LONG tombstone; a missing key without a default under an `unknown` table type; a microsecond key outside the nanosecond range, which must fail; and incomparable key types. With the reader before this PR, 6 of them fail; the other three pin behavior the old code shares. Removing the TIMESTAMP -> TIMESTAMP_NS widening fails 2, and making the casts non-strict fails 1. - End to end on a local cluster: a v2 table in a `file://` Hadoop catalog, built with the Iceberg 1.12.0 Java API: `k INT` with rows (1, NULL), (2, 0), (3, 1); `k` promoted to LONG without a snapshot; a delete of `k = 4294967296` written under that schema; `k` dropped; the delete committed; then `expireSnapshots().retainLast(2).cleanExpiredMetadata(true)`, which leaves schema 0 (`k INT`) and schema 2 (no `k`). `SELECT id` through FileScannerV2 returned 2, 3 before this PR and returns 1, 2, 3 with it. branch-4.1 and branch-4.2 have the same FileScannerV2 code. ### Release note Fix FileScannerV2 comparing Iceberg equality-delete keys in a single scan-wide type, which could delete or keep the wrong rows, or fail the scan, after a key's type changed (for example a schema-only promotion removed by `cleanExpiredMetadata`, or a key whose retained schemas only have it as `unknown`). ### Check List (For Author) - Test - [ ] Regression test - [x] Unit Test - `run-be-ut.sh --run --filter='IcebergV2ReaderTest.*:*EqualityDelete*:*ColumnMapper*:*EvolutionCast*'`: 273 of 273 pass on macOS arm64 (ASAN), including the 9 new cases. - [x] Manual test (add detailed scripts or steps below) - The local end-to-end check described under Results, with an ASAN BE built from this change. - [ ] No need to test or manual test. Explain why: - Behavior changed: - [ ] No. - [x] Yes. FileScannerV2 compares equality-delete keys per delete file and data file; two key types that convert in neither direction now fail the scan; the comparison casts are strict. - Does this need documentation? - [x] No. - [ ] Yes. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…lete and data file types (#69711) ### What problem does this PR solve? Issue Number: None Related PR: #69536, #68267 Problem Summary: **In short.** FileScannerV2 now compares an Iceberg equality-delete key in the wider of the delete file's and the data file's own key types, instead of in the one table type the FE sends for the whole scan. For some table histories that single type is wrong: the reader then deletes rows it must keep, keeps rows it must delete, or fails the scan. This follows the review of #69536, where these cases came up. **Background.** An equality delete removes a data row whose key values equal a tombstone's. After a type promotion the two sides can be stored in different types: tombstones written before an INT -> LONG promotion hold INT values that must still match LONG data. #68267 made FileScannerV2 compare in the current schema's type, and for a key that the current schema no longer has, the FE sends a type recovered from the retained schemas (#69536). The reader casts both the tombstones and the data keys to that one type. **The problem, and what it cost.** One type per scan cannot describe every delete file: | History | Table type from the FE | Delete file | Data file | Before | After | |---|---|---|---|---|---| | `expireSnapshots().cleanExpiredMetadata(true)` removed the schema of a schema-only INT -> LONG promotion of a dropped key | INT | LONG 4294967296 | INT, one NULL key | the tombstone becomes NULL in INT and deletes the NULL-key row | row kept | | A read pinned before a later DATE -> TIMESTAMP_NS promotion of the key | TIMESTAMP_NS | DATE 1000-01-01 | DATE 1000-01-01 | scan fails: `TIMESTAMP_NS overflow when casting row ...` | row deleted | | No retained schema gives the key a concrete type, so the FE sends Iceberg `unknown` as STRING | STRING | TIMESTAMP_NS 2024-01-02 00:00 | DATE 2024-01-02 | `2024-01-02` != `2024-01-02 00:00:00.000000000`, the row stays visible | row deleted | | The delete was prepared under a TIMESTAMP_NS schema that the table later abandoned for TIMESTAMP | TIMESTAMP | TIMESTAMP_NS 12:00:00.000000500 | TIMESTAMP 12:00:00.000000 | the tombstone is rounded to microseconds and deletes the row | row kept | Three of the four return wrong rows without any error. The V1 reader is not changed by this PR. **How this PR fixes it.** - The delete block keeps the delete file's own key types. When the predicate for a data file is built, each key is compared in the wider of the delete file's type and the data file's type (`equality_delete_comparison_type`). The accepted widenings are the Iceberg promotions (INT -> LONG, FLOAT -> DOUBLE, decimal precision, DATE -> TIMESTAMP and DATE -> TIMESTAMP_NS), a wider fractional scale of the same date-time kind, wider integers, STRING -> VARBINARY (an Iceberg BINARY key decodes as either one, depending on `enable_mapping_varbinary`), and TIMESTAMP -> TIMESTAMP_NS. The last one is not an Iceberg promotion; it is accepted for the abandoned-schema case above, because scaling microseconds to nanoseconds is exact. When the table field is `timestamptz`, a `DATETIMEV2` key (Parquet INT96, which carries no instant annotation, as Spark and Hive write it) and a `TIMESTAMPTZ` key compare as `TIMESTAMPTZ` at the larger scale, the `DATETIMEV2` side converted with the session time zone as the projection converts that column. When the table field is the STRING placeholder the FE sends for an Iceberg TIME, which Doris cannot represent, an ORC TIME key (a LONG annotated `iceberg.long-type=TIME`, read as `BIGINT`) and a Parquet one (`TIME(MICROS)`, read as `TIMEV2(6)`) compare as `TIMEV2(6)`, the `BIGINT` side converted exactly as microseconds from midnight by a dedicated expression (`CAST(BIGINT AS TIME)` reads the integer's digits as HHMMSS), and a value outside 00:00:00..23:59:59.999999 fails the scan. Two types that convert in neither direction fail the scan instead of being compared in a third type. - The data keys are cast to that type. When it is the data file's wider type, the tombstones are widened once per scanner, and the converted delete block and its hash index are cached next to the original (`_convert_equality_delete_filter`). A table without type changes is compared as before, with no cast and no copy. - A key that the data file lacks is compared through its initial default, in the wider of the delete file's type and the table type. When that default is NULL (no initial default, or a struct default `{}` whose leaf has none) the key is NULL in that file whatever its table type, so it is compared in the delete file's type (an `unknown` key arrives as STRING). A non-NULL default behind the STRING placeholder of an Iceberg TIME is Iceberg's text of the value (`00:00`), with at most six fractional digits, so it is parsed exactly in the tombstones' type, `TIMEV2` (`TIMEV2(6)` against an ORC TIME tombstone), by the strict comparison cast. A non-NULL default of a `timestamptz_ns` key fails the scan with `NotSupported`: V2 reads its tombstones as `TIMESTAMPTZ(6)` (Parquet floors the nanoseconds, ORC rounds them), so no parse of the nanosecond default compares exactly; the default epoch would match a tombstone 100 ns later and delete every row of the file. - `AncestorNullDefaultExpr`, which gives a missing child the NULL of its NULL parent and its default otherwise, now opens its children. Its `open()` returned before opening them, because `prepare()` had already set the flag its guard checks, so a default cast to the comparison type below it was never opened and aborted the BE with `Check failed: _open_finished` in `Cast`. - The comparison casts are strict whatever the session's cast mode, through a new `strict` flag on `EvolutionCast`: a non-strict overflow would turn a key into NULL and let a NULL tombstone delete it. DATE -> TIMESTAMP_NS was already strict; other `EvolutionCast` users are unchanged. - A delete file without rows is skipped before its keys are resolved: it deletes nothing, but the predicate evaluates its keys first, so its strict casts could fail the scan (a DATE key 1000-01-01 against an empty TIMESTAMP_NS delete file). - The table types from the FE still resolve table fields and the defaults of missing keys; the delete-file cache key is unchanged. **Results.** - 14 new BE unit tests in `iceberg_reader_test.cpp`: the four histories above; INT tombstones against LONG, INT and LONG data files of one scan (the third split reuses the cached widened copy); a missing key's INT default against a LONG tombstone; a missing key without a default under an `unknown` table type; a microsecond key outside the nanosecond range, which must fail; and incomparable key types. With the reader before this PR, 6 of them fail; the other three pin behavior the old code shares. Removing the TIMESTAMP -> TIMESTAMP_NS widening fails 2, and making the casts non-strict fails 1. Three more came from review: an INT96 data key against an adjusted-UTC tombstone in Asia/Shanghai, a NULL leaf under a struct default `{}`, and an empty TIMESTAMP_NS delete file against a DATE key; removing the INT96 rule, the NULL-leaf walk or the empty-file skip fails its test. Two more came from the third round: Iceberg TIME keys across all four ORC / Parquet data and delete file pairs, with NULL and non-NULL tombstones, and a nested Iceberg TIME key added with a text default, against Parquet TIME and ORC TIME tombstones, under NULL and non-NULL parents. Removing the TIME pair rule or the default parsing, or converting the ORC TIME with a SQL cast, fails them; leaving `AncestorNullDefaultExpr`'s children unopened aborts. One more came from the fourth round: a nested `timestamptz_ns` key with the defaults `...0000001`, the epoch and `...0000005` fails against an epoch, a 100 ns and a 500 ns tombstone, while a key without a default still matches NULL tombstones only; parsing those defaults again fails it. - End to end on a local cluster: a v2 table in a `file://` Hadoop catalog, built with the Iceberg 1.12.0 Java API: `k INT` with rows (1, NULL), (2, 0), (3, 1); `k` promoted to LONG without a snapshot; a delete of `k = 4294967296` written under that schema; `k` dropped; the delete committed; then `expireSnapshots().retainLast(2).cleanExpiredMetadata(true)`, which leaves schema 0 (`k INT`) and schema 2 (no `k`). `SELECT id` through FileScannerV2 returned 2, 3 before this PR and returns 1, 2, 3 with it. branch-4.1 and branch-4.2 have the same FileScannerV2 code. ### Release note Fix FileScannerV2 comparing Iceberg equality-delete keys in a single scan-wide type, which could delete or keep the wrong rows, or fail the scan, after a key's type changed (for example a schema-only promotion removed by `cleanExpiredMetadata`, or a key whose retained schemas only have it as `unknown`). ### Check List (For Author) - Test <!-- At least one of them must be included. --> - [ ] Regression test - [x] Unit Test - `run-be-ut.sh --run --filter='IcebergV2ReaderTest.*:*EqualityDelete*:*ColumnMapper*:*EvolutionCast*:*TableReader*'`: 526 of 526 pass on macOS arm64 (ASAN), including the 14 new cases. - Rebased onto master `43f9ceb48d7`: #69639 moved the strict-mode override of `EvolutionCast` from `prepare()` into `configure_cast_options()`, so the new `strict` flag now sets `CastOptions::strict_mode` there, next to the existing DATE -> TIMESTAMP_NS rule. The run above is on the content before the rebase (the third-round commit applies to both unchanged); the 67 TUs that this PR changes or that master changed since `5caaeb3e3d4` were syntax-checked against the rebased tree with the UT build flags, and CI runs the suites again. - [x] Manual test (add detailed scripts or steps below) - The local end-to-end check described under Results, with an ASAN BE built from this change. - [ ] No need to test or manual test. Explain why: - Behavior changed: - [ ] No. - [x] Yes. FileScannerV2 compares equality-delete keys per delete file and data file; two key types that convert in neither direction now fail the scan; the comparison casts are strict. - Does this need documentation? - [x] No. - [ ] Yes. <!-- Add document PR link here. eg: apache/doris-website#1214 --> ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label <!-- Add branch pick label that this PR should merge into --> --------- Co-authored-by: Claude Opus 5.5 <[email protected]>
What problem does this PR solve?
Issue Number: None
Related PR: #66538, #69394
Problem Summary:
In short. This ports a fix from branch-4.1 #66538 (Iceberg V3 default values). When a dropped column is still referenced by a live equality delete, the Iceberg connector now looks up the column's definition along the scan snapshot's parent chain first, and only then in the whole schema list. Master searched only the part of the schema list before the scan schema. That breaks as soon as Iceberg reuses a schema ID, and every query on such a table failed.
Background. An equality delete names its key columns by field ID. If a key column has since been dropped, the scan schema no longer has it, so the connector adds the column's historical definition to the schema dictionary it sends to BE (
IcebergScanPlanProvider.schemaForPotentialEqualityDeletes). BE needs that field ID, type and initial default to read the key from data files. Master (#65851) took the definition from the metadata schema list, searching backwards from the scan schema's position, on the assumption that the list is in chronological order.The problem, and what it cost. Schema IDs are not a sequence. When a schema change produces a schema equal to an existing one, Iceberg reuses that schema's ID and does not append to the list. Dropping a column that was added after the table was created is the common case: the schema returns to the initial one, so the current schema is the first entry of the list, and the schema that still defines the key comes after it. The backward search never reaches that schema, and the scan fails.
Reproduce: create a table with
id; add columnk; write data and an equality delete keyed onk; dropk; query the table.Iceberg equality-delete fields are absent from schema history: [2]How this PR fixes it. It uses the same order as
IcebergScanNode.getSchemaFieldsForScanon branch-4.1 and branch-4.2. First, walk the scan snapshot's parent chain and take each missing key from the schema that snapshot was written with. Then search the whole metadata schema list, newest first, for keys that only schema-only changes or expired ancestors can explain. Unlike branch-4.x, the walk indexes each schema at most once and stops as soon as every key is resolved, so a long lineage that shares one schema costs only map lookups. When no key is missing, nothing changes and no snapshot is read, as before.When a dropped key has several historical definitions, the one recorded on the scan's lineage now wins, as on branch-4.x. The field ID and the initial default are the same either way. Only a name changed by a schema-only commit after the last write can differ. The existing test
equalityCarrierKeepsLatestDefinitionOfDroppedEqualityKey(from #69394) renamed and then dropped the key with no write in between. It now writes a snapshot after each schema change, as the branch-4.1 test it was ported from does, and still expectsk2. A dropped key nevertheless keeps the compatible changes its field ID has in any retained schema: the widest promotion of its type (the SDK's promotions such as INT→LONG, plus the v3 promotions that Iceberg 1.12.0'sTypeUtil.isPromotionAlloweddoes not know:unknownto any primitive type, and DATE to an unzoned TIMESTAMP or TIMESTAMP_NS), and optional when any retained schema has it optional. The widest promotion does not depend on the order of the schemas. If the retained schemas promote the key to two types that do not promote to each other, which takes setting the current schema back to an earlier one (for example DATE→TIMESTAMP, back to DATE, then DATE→TIMESTAMP_NS), planning fails with an explicit error: neither type holds the other's values exactly, and the metadata does not record which one a delete file uses. A DATE initial default moves to midnight in the promoted type, since Iceberg 1.12.0'sDateLiteralconverts only to DATE. The carrier drops the write default of a rebuilt field: no reader uses it, and a stale one may not fit the promoted type. A delete committed after a schema-only promotion or required→optional change follows the relaxed definition even when no snapshot on the lineage records it: FileScannerV2 compares keys in the carrier's type, and both readers reject a data file that lacks a required key. A key is never narrowed, and a key required in every retained schema stays required.Results. This PR ports four tests from #66538 into
IcebergScanPlanProviderTest:equalityCarrierResolvesDroppedKeyWhenSchemaIdIsReuseduses a real in-memory table and also checks the scan dictionary thatgetScanNodePropertiesbuilds.equalityCarrierResolvesReusedSchemaIdAfterParentSnapshotsExpireis the same, with the snapshots that recorded the key expired.equalityCarrierPrefersScanLineageOverLaterRenameForTimeTravelandequalityCarrierKeepsDefaultWhenTimeTravelParentExpiredBeforeLaterRenameuse table doubles with schema histories that the SDK cannot build.equalityCarrierKeepsPromotedTypeOfKeyDroppedBeforeItsDeleteCommits(new) promotes a key from INT to LONG without a snapshot, prepares an equality delete, drops the key and then commits the delete; the carrier must say LONG. It fails without the widening.equalityCarrierKeepsLineageTimestampOfPromotedDateKeyandequalityCarrierWidensDateKeyPromotedWithoutASnapshot(new) promote a v3 DATE key to TIMESTAMP / TIMESTAMP_NS, published directly sinceSchemaUpdatein 1.12.0 rejects these promotions; the carrier must keep the lineage's TIMESTAMP and widen a schema-only DATE→TIMESTAMP_NS.equalityCarrierKeepsSchemaOnlyOptionalityOfDroppedKeyandequalityCarrierKeepsAlwaysRequiredDroppedKeyRequired(new) drop a required key while a delete on it is pending, with and without a schema-onlymakeColumnOptional; the carrier must be optional and required respectively.equalityCarrierConvertsDefaultsOfDateKeyPromotedToTimestampandequalityCarrierConvertsDefaultsOfDateKeyPromotedToTimestampNs(new) give the v3 DATE key an initial and a write default, promote it to TIMESTAMP / TIMESTAMP_NS without a snapshot and drop it while a delete on it is pending; the carrier must hold the initial default as midnight in the promoted type and no write default, and the scan dictionary must carry2024-01-02 00:00:00. Without the conversion, planning fails withCannot cast default value to timestamp.equalityCarrierFollowsUnknownKeyPromotedWithoutASnapshot(new) promotes a v3 key fromunknownto DATE and then TIMESTAMP_NS without snapshots, and commits a pending append and a pending delete after the key is dropped, so the lineage only recordsunknown; the carrier and the scan dictionary must say TIMESTAMP_NS. Without theunknownpromotion the carrier staysunknown, which the dictionary sends as STRING.equalityCarrierDropsStaleWriteDefaultOfPromotedDateKey(new) records a DATE write default outside the TIMESTAMP_NS range, clears it and promotes the key to TIMESTAMP_NS without snapshots; planning must not fail on the stale write default, which converting fails withlong overflow.equalityCarrierRejectsDateKeyPromotedToTimestampThenTimestampNsandequalityCarrierRejectsDateKeyPromotedToTimestampNsThenTimestamp(new) promote a v3 DATE key to one of TIMESTAMP / TIMESTAMP_NS without a snapshot, set the DATE schema current again, promote it to the other and drop it while a delete on it is pending; planning must fail in either order and name both types. Without the check the carrier takes whichever type the schema list holds first, and a TIMESTAMP carrier rounds a nanosecond tombstone to the microsecond.equalityCarrierRejectsUnknownKeyPromotedToUnrelatedTypes(new) does the same withunknownpromoted to INT and then STRING.equalityCarrierKeepsLineagePromotionBesideAnAbandonedOne(new) commits the delete while TIMESTAMP_NS is current, so the lineage records TIMESTAMP_NS; the abandoned TIMESTAMP schema must not fail planning.With master's search, the two reused-ID tests fail with the error above. Without the lineage walk, the time-travel test gets
k2instead ofk. All 1706 tests of fe-connector-iceberg pass.Original author: @hubgeter
Release note
Fix: queries on an Iceberg table failed with "equality-delete fields are absent from schema history" after a column used as an equality-delete key was dropped and the table schema returned to an earlier one.
Check List (For Author)
Test
Behavior changed:
Such queries no longer fail. When a dropped equality-delete key has several historical definitions, the one recorded on the scan snapshot's lineage wins, as on branch-4.x. If retained schemas promote a dropped key to two unrelated types, which takes a rollback of the current schema, planning fails with an explicit error.
Does this need documentation?
Check List (For Reviewer who merge this PR)