Repository navigation
[test](iceberg) Port the branch-4.1 Iceberg V3 default tests and reserve TIcebergFileDesc field 13 - #69394
Conversation
…rve TIcebergFileDesc field 13 ### What problem does this PR solve? Issue Number: None Related PR: apache#66538 apache#65851 Problem Summary: branch-4.1 apache#66538 and master apache#65851 implement Iceberg V3 initial / write defaults separately. The 4.1 tests target fe-core classes and V1 reader internals that master moved into the fe-connector-iceberg plugin and iceberg_reader_mixin.h, so most of them never reached master. Comparing the two, master already implements much of the tested behavior but has no test for it: equality-delete schema carriers for dropped keys, the missing-required-field and nested-default fences, the commit fences that reject schema / spec / table-identity changes between binding and commit, pinned writer metadata in merge sinks and file conversion, typed write-default SQL, and DEFAULT resolution by field id. This adds master versions of those tests (BE: 3, FE: 37) and one regression check that information_schema does not expose a connector write default. It also reserves field id 13 of TIcebergFileDesc: branch-4.1 / branch-4.2 send `13: optional ExternalTableSchema.TSchema equality_delete_schema` there, so master must never reuse it. No product code changes. The behavior differences found by the same comparison are fixed separately (for example apache#69369). ### Release note None ### Check List (For Author) - Test: Unit Test, Regression test - BE UT: IcebergV2ReaderTest.IcebergProjectedStructDoesNotReuseReaddedEqualityKeyByName, ParquetReadLinesTest.iceberg_row_id_fetch_materializes_readded_missing_column, VOrcTransformerTest.SkipsInvalidBinaryChildrenHiddenByNullableCollections pass (run on a 10-20 master base; the BE files master changed since do not touch these paths) - FE UT: fe-connector-iceberg module 1656 tests pass; fe-core ConnectorWriteSchemaUtilsTest, ExternalRowLevelUpdatePlanBuilderTest, ExternalRowLevelMergePlanBuilderTest pass; the fence / pinned-metadata tests were checked to fail with the guarded condition removed - Regression: external_table_p0/iceberg/test_iceberg_write_default passes (.out generated by script) - Behavior changed: No - 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 |
There was a problem hiding this comment.
Reviewed all 15 changed paths at 5998dbf. No actionable findings. Both full-scope reviewers and the separate risk reviewer returned NO_NEW_VALUABLE_FINDINGS on the unchanged empty finding set; review converged in one round. There were no existing inline findings or additional user focus points.
Critical checkpoint conclusions:
- Goal and correctness: the additions meaningfully characterize existing Iceberg initial/write defaults, equality-delete field identity, statement-pinned metadata and upgrade fences. No executable production behavior changes.
- Scope and reuse: focused additions to existing test classes/fakes, one regression assertion/output, and a Thrift reservation comment; no dependency changes.
- Concurrency and locking: commit-race tests use deterministic staging hooks with shared atomic state. No new running threads or lock order; arbitrary concurrent interleavings are not exercised.
- Lifecycle and memory: descriptor/schema/column owners outlive reader use; readers/writers close and existing FE context cleanup applies. No new production lifecycle or static-initialization dependency.
- Configuration: no new settings or dynamic-reload behavior.
- Compatibility: no symbol, storage-format or wire-layout change; upgrade tests assert existing scheduling requirements.
- Thrift/protobuf: compared TIcebergFileDesc on HEAD and current branch-3.1, 4.0, 4.1 and 4.2 tips. Reserved ID 13 matches optional equality_delete_schema on 4.1/4.2; no ID reuse, enum change or required field added.
- Parallel paths: nested equality-key coverage exercises Parquet and ORC. Sink/file/commit pinning and INSERT versus UPDATE/matched/not-matched MERGE default behavior are distinguished.
- Conditional checks and errors: helpers model explicit cases; negative assertions check intended errors and rejected publication rather than silently accepting failures.
- Test coverage: every changed path and relevant implementation flow reviewed, including projection controls, required/defaulted children, alias collisions, recreated tables and late commit-time schema changes. Synthetic collection cases cover the pure predicate; the cherry-pick case pins conservative schema-history fencing, not physical ancestor-file inheritance or ancestry traversal.
- Expected results: the new information_schema query selects a single column and correctly expects one NULL COLUMN_DEFAULT. Output-generation provenance was not independently verified.
- Observability: assertion diagnostics are adequate for the additions; no new production path requires logging or metrics.
- Transactions and persistence: tests validate preflight and final metadata-swap fences; no Doris journal/replay behavior changes.
- Writes and atomicity: tests inspect actual committed DataFile/sink metadata and confirm rejected staged writes leave no published snapshot. Crash/failover behavior is unchanged and not exercised.
- FE/BE variables: existing schema/default and sink fields are tested; no transmitted variable added.
- Performance: bounded test fixtures only; no production hot-path or retry change.
- Other issues: fixture identity, nullable collection readback, DEFAULT field-ID binding, and UPDATE/MERGE plan branches were rechecked; no additional defect established.
Validation limits: static review only, as required. No builds, unit/regression runs, format/static-analysis execution, mutation testing or mixed-version cluster execution were performed. Author-reported test passes were not independently reproduced. No unresolved code candidate or uncovered changed path remains.
Review converged after 1 round(s).
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
FE UT Coverage ReportIncrement line coverage `` 🎉 |
TPC-H: Total hot run time: 28415 ms |
TPC-DS: Total hot run time: 150694 ms |
ClickBench: Total hot run time: 25.07 s |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
PR approved by anyone and no changes requested. |
|
PR approved by at least one committer and no changes requested. |
1 similar comment
|
PR approved by at least one committer and no changes requested. |
### What problem does this PR solve? Issue Number: None Related PR: apache#66538 Problem Summary: apache#69394 added two ConnectorWriteSchemaUtilsTest cases that expect resolveDefaultReferences to return the default with the type the parser gives its literal (9 is a TINYINT). The previous commit casts the resolved default to its writer column's type, so for the INT test column the result is now an INT literal 9, and both cases failed in FE UT once this branch met master. The two expectations now use the column's type. ### Release note None ### Check List (For Author) - Test: Unit Test - ConnectorWriteSchemaUtilsTest, RewriteDefaultExpressionTest, InsertUtilsTest - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
### What problem does this PR solve? Issue Number: None Related PR: apache#66538 Problem Summary: apache#69394 added two ConnectorWriteSchemaUtilsTest cases that expect resolveDefaultReferences to return the default with the type the parser gives its literal (9 is a TINYINT). The previous commit casts the resolved default to its writer column's type, so for the INT test column the result is now an INT literal 9, and both cases failed in FE UT once this branch met master. The two expectations now use the column's type. ### Release note None ### Check List (For Author) - Test: Unit Test - ConnectorWriteSchemaUtilsTest, RewriteDefaultExpressionTest, InsertUtilsTest - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
### What problem does this PR solve? Issue Number: None Related PR: apache#66538 Problem Summary: apache#69394 added two ConnectorWriteSchemaUtilsTest cases that expect resolveDefaultReferences to return the default with the type the parser gives its literal (9 is a TINYINT). The previous commit casts the resolved default to its writer column's type, so for the INT test column the result is now an INT literal 9, and both cases failed in FE UT once this branch met master. The two expectations now use the column's type. ### Release note None ### Check List (For Author) - Test: Unit Test - ConnectorWriteSchemaUtilsTest, RewriteDefaultExpressionTest, InsertUtilsTest - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
… ID is reused (#69536) ### 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 column `k`; write data and an equality delete keyed on `k`; drop `k`; query the table. | | Master today | After this PR | |---|---|---| | Query after dropping an equality-delete key, when the schema returns to an earlier one | fails: `Iceberg equality-delete fields are absent from schema history: [2]` | succeeds | | Same, after the snapshots that recorded the key have expired | fails with the same error | succeeds (the key comes from the metadata schema list) | | Time travel to a snapshot whose lineage defines the key, while a later schema renames it | earlier definition | earlier definition (unchanged) | **How this PR fixes it.** It uses the same order as `IcebergScanNode.getSchemaFieldsForScan` on 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 expects `k2`. 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's `TypeUtil.isPromotionAllowed` does not know: `unknown` to 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's `DateLiteral` converts 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`: - `equalityCarrierResolvesDroppedKeyWhenSchemaIdIsReused` uses a real in-memory table and also checks the scan dictionary that `getScanNodeProperties` builds. - `equalityCarrierResolvesReusedSchemaIdAfterParentSnapshotsExpire` is the same, with the snapshots that recorded the key expired. - `equalityCarrierPrefersScanLineageOverLaterRenameForTimeTravel` and `equalityCarrierKeepsDefaultWhenTimeTravelParentExpiredBeforeLaterRename` use 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. - `equalityCarrierKeepsLineageTimestampOfPromotedDateKey` and `equalityCarrierWidensDateKeyPromotedWithoutASnapshot` (new) promote a v3 DATE key to TIMESTAMP / TIMESTAMP_NS, published directly since `SchemaUpdate` in 1.12.0 rejects these promotions; the carrier must keep the lineage's TIMESTAMP and widen a schema-only DATE→TIMESTAMP_NS. `equalityCarrierKeepsSchemaOnlyOptionalityOfDroppedKey` and `equalityCarrierKeepsAlwaysRequiredDroppedKeyRequired` (new) drop a required key while a delete on it is pending, with and without a schema-only `makeColumnOptional`; the carrier must be optional and required respectively. - `equalityCarrierConvertsDefaultsOfDateKeyPromotedToTimestamp` and `equalityCarrierConvertsDefaultsOfDateKeyPromotedToTimestampNs` (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 carry `2024-01-02 00:00:00`. Without the conversion, planning fails with `Cannot cast default value to timestamp`. - `equalityCarrierFollowsUnknownKeyPromotedWithoutASnapshot` (new) promotes a v3 key from `unknown` to 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 records `unknown`; the carrier and the scan dictionary must say TIMESTAMP_NS. Without the `unknown` promotion the carrier stays `unknown`, 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 with `long overflow`. - `equalityCarrierRejectsDateKeyPromotedToTimestampThenTimestampNs` and `equalityCarrierRejectsDateKeyPromotedToTimestampNsThenTimestamp` (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 with `unknown` promoted 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 `k2` instead of `k`. All 1706 tests of fe-connector-iceberg pass. Co-authored-by: daidai <[email protected]>
### What problem does this PR solve? Issue Number: None Related PR: apache#66538 Problem Summary: apache#69394 added two ConnectorWriteSchemaUtilsTest cases that expect resolveDefaultReferences to return the default with the type the parser gives its literal (9 is a TINYINT). The previous commit casts the resolved default to its writer column's type, so for the INT test column the result is now an INT literal 9, and both cases failed in FE UT once this branch met master. The two expectations now use the column's type. ### Release note None ### Check List (For Author) - Test: Unit Test - ConnectorWriteSchemaUtilsTest, RewriteDefaultExpressionTest, InsertUtilsTest - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
### What problem does this PR solve? Issue Number: None Related PR: apache#66538 Problem Summary: apache#69394 added two ConnectorWriteSchemaUtilsTest cases that expect resolveDefaultReferences to return the default with the type the parser gives its literal (9 is a TINYINT). The previous commit casts the resolved default to its writer column's type, so for the INT test column the result is now an INT literal 9, and both cases failed in FE UT once this branch met master. The two expectations now use the column's type. ### Release note None ### Check List (For Author) - Test: Unit Test - ConnectorWriteSchemaUtilsTest, RewriteDefaultExpressionTest, InsertUtilsTest - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
### What problem does this PR solve? Issue Number: None Related PR: apache#66538 Problem Summary: apache#69394 added two ConnectorWriteSchemaUtilsTest cases that expect resolveDefaultReferences to return the default with the type the parser gives its literal (9 is a TINYINT). The previous commit casts the resolved default to its writer column's type, so for the INT test column the result is now an INT literal 9, and both cases failed in FE UT once this branch met master. The two expectations now use the column's type. ### Release note None ### Check List (For Author) - Test: Unit Test - ConnectorWriteSchemaUtilsTest, RewriteDefaultExpressionTest, InsertUtilsTest - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
### What problem does this PR solve? Issue Number: None Related PR: apache#66538 Problem Summary: apache#69394 added two ConnectorWriteSchemaUtilsTest cases that expect resolveDefaultReferences to return the default with the type the parser gives its literal (9 is a TINYINT). The previous commit casts the resolved default to its writer column's type, so for the INT test column the result is now an INT literal 9, and both cases failed in FE UT once this branch met master. The two expectations now use the column's type. ### Release note None ### Check List (For Author) - Test: Unit Test - ConnectorWriteSchemaUtilsTest, RewriteDefaultExpressionTest, InsertUtilsTest - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
What problem does this PR solve?
Issue Number: None
Related PR: #66538 #65851
Problem Summary:
In short. This ports the Iceberg V3 default-value tests of branch-4.1 #66538 to master: 3 BE unit tests, 37 FE unit tests and one regression check, all for behavior master already has. It also reserves field id 13 of
TIcebergFileDesc, which branch-4.1 and branch-4.2 use forequality_delete_schema. No product code changes.Background. Iceberg V3 initial / write defaults were implemented twice: by #65851 on master and by #66538 on branch-4.1, which branch-4.2 inherits. The 4.1 tests target fe-core classes (
IcebergScanNode,IcebergWriteSchemaContext,IcebergTransaction, …) and V1 reader internals that master no longer has. On master the Iceberg FE lives in thefe-connector-icebergplugin and the V1 reader iniceberg_reader_mixin.h. So most of the 4.1 tests never reached master, even where master implements the same behavior.The problem, and what it cost. A scenario-by-scenario comparison of the two test suites found that master implements most of the 4.1-tested behavior with no test of its own. The untested behavior:
TIMESTAMP_NS; only referenced historical fields enter the carrierDEFAULTresolves by field id, not by a reused name; UPDATE and matched MERGE do not inject write defaultsTIcebergFileDescends at field 12 on master, while 4.1 / 4.2 send13: optional ExternalTableSchema.TSchema equality_delete_schema; nothing stops master from reusing 13 for an unrelated fieldWithout these tests a regression in any of these paths would go unnoticed, and a reused field 13 would misdecode 4.x plans.
How this PR fixes it. It rewrites the 4.1 scenarios against master's APIs. Plugin tests use the existing Mockito-free fakes (
InMemoryCatalog,ProxyoverTableOperations). The commit-fence tests hooknewSnapshotIdon the staging operations, so the concurrent change lands after the commit preflight and before the metadata swap. The BE tests are copied unchanged from 4.1 (the helper signatures match on master).PlanNodes.thriftgets a comment reserving field 13.test_iceberg_write_defaultgains aqt_check thatinformation_schema.columns.COLUMN_DEFAULTstays NULL for a column whose default is a connector-only write default. The behavior differences found in the same comparison are fixed in separate PRs (for example #69369).Results. All new tests pass on master. To show they check something, the fence and pinned-metadata tests were re-run with the guarded condition removed from the product code; each failed.
fe-connector-iceberg(all tests)ConnectorWriteSchemaUtilsTest,ExternalRowLevelUpdatePlanBuilderTest,ExternalRowLevelMergePlanBuilderTestIcebergV2ReaderTest.IcebergProjectedStructDoesNotReuseReaddedEqualityKeyByName,ParquetReadLinesTest.iceberg_row_id_fetch_materializes_readded_missing_column,VOrcTransformerTest.SkipsInvalidBinaryChildrenHiddenByNullableCollectionsexternal_table_p0/iceberg/test_iceberg_write_defaultOriginal author: @hubgeter
Release note
None
Check List (For Author)
Test
The BE tests ran locally on a 10-20 master base; the BE files master changed since then (profile counters, zone-map pruning, Paimon timestamps) are outside the tested paths.
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)