[test](regression) Complete Iceberg/Paimon schema time travel P0 coverage#65960
Conversation
Add P0 Iceberg and Paimon regression suites covering schema evolution across snapshots, tags, branches, deletes, and reader paths. Include Iceberg complex nested schema changes introduced on latest master and combine them with historical references. Issue Number: DORIS-27393
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Review context preparation failed before Codex ran; inspect the 'Prepare authoritative PR context and required AGENTS guides' step. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes. This test/documentation-only PR does not yet prove the complete Iceberg/Paimon schema-evolution × time-travel P0 matrix it advertises, and the new distributed JDBC setup introduces concrete test-failure paths. The 17 inline comments identify the blocking contract, oracle, physical-path, cache, and topology gaps.
Critical checkpoints:
- Goal and proof: The intended comprehensive P0 matrix is not achieved. Confirmed Doris bugs are treated as passing contracts, several advertised cells are absent or unobserved, and reader/DV/cache/pruning assertions do not prove the named path.
- Scope and focus: The 13 changed files are thematically focused on external-table regression coverage, but the 3,231-line patch overstates completion in the checked-in coverage document.
- Concurrency: No production locking changes are present. The regression framework runs these normal JDBC suites concurrently, and both can non-atomically download/copy the same live driver JAR.
- Lifecycle: Suite-scoped connections are closed, so the final scanner-variable assignments do not leak. Two Iceberg cleanup blocks still remove debug state after execution, contrary to the repository's drop-before-only test standard.
- Configuration: No production configuration is added. Test-only catalog/session settings are used, but the Paimon cache-off catalog leaves the separate schema cache enabled.
- Compatibility: No production symbol, protocol, storage-format, replay, or rolling-upgrade behavior changes.
- Parallel paths: Iceberg/Paimon, REST/filesystem/JDBC, scanner V1/V2, and JNI/native/C++ variants are attempted, but the comments identify unproved reader, DV, cache, and distributed-host paths.
- Conditions and errors: Supported format rejections use the expected-error DSL correctly. Acknowledged Doris defects must not be encoded as stable passing negative contracts.
- Test coverage: Positive row/schema checks are missing for fast-forward, metadata/nullability, added/promoted fields, equality-delete promotion, pruning, and physical deletion-vector paths.
- Test results: Fixed deterministic results must use named
qt/order_qtcases with generated.outfiles. This review was review-only and did not run builds or suites; the current GitHub/TeamCity view shows compile and unit checks successful while regression jobs remain pending. - Observability: No production observability change is needed, but these tests need plan/profile or metadata evidence for reader selection, selected partitions, and deletion vectors.
- Persistence, transactions, and writes: No production persistence/transaction path changes. Fixture DDL/data writes are isolated, with missing positive atomicity/schema observations called out inline.
- FE/BE propagation: No product variable is added. JDBC driver staging is intended for all FE/BE hosts, but local identity and remote IPv6 handling are incomplete.
- Performance: No production hot path changes; no separate CPU, memory, or asymptotic regression was found.
- Other: Snapshot/time selection and suite-session leakage were investigated and dismissed with concrete code evidence.
- User focus: No additional review focus was supplied.
| The requested schema-change × historical-operation correctness matrix has no | ||
| unimplemented P0 cell. Unsupported format operations and currently incorrect | ||
| Doris behavior are represented by stable negative regression contracts rather | ||
| than being marked as missing. |
There was a problem hiding this comment.
[P1] Do not freeze known defects as successful P0 contracts
This document explicitly classifies these outcomes as current Doris bugs, while the new suites require the wrong exceptions/types to persist. TestAction fails when an expected exception disappears, so fixing a listed defect turns the green P0 suite red. Assert the intended successful rows/schema after the fixes, or quarantine these reproducers and keep the affected cells marked incomplete.
| from paimon.${dbName}.${branchTable} | ||
| order by id | ||
| """)) | ||
| List<String> fastForwardColumns = sql("""desc ${branchTable}""") |
There was a problem hiding this comment.
[P1] Exercise the Doris scan after fast-forward
After fast-forward, only Spark validates the rows; Doris runs DESC plus an analyzer-time unknown-column check, neither of which reaches the BE path whose abort is documented above. containsAll also allows stale main_only to remain. Add the ordered Doris row query and assert the exact resulting schema.
| ) | ||
| order by id | ||
| """) | ||
| assertEquals(incrementalJni, incrementalCpp) |
There was a problem hiding this comment.
[P1] Prove the incremental result and reader paths
Equality permits both sides to be empty or equally wrong, and incrementalJni runs with force_jni_scanner=false; native raw-file conversion can also win before the JNI/C++ selector. Assert concrete rows and the end schema, then use plan/profile evidence or a fixture that proves each intended reader actually ran.
| // add multiple columns and position one AFTER old_name. | ||
| spark_paimon_multi """ | ||
| ALTER TABLE paimon.${dbName}.${topTable} | ||
| ADD COLUMNS (added STRING AFTER old_name, added_second BIGINT); |
There was a problem hiding this comment.
[P2] Observe every advertised schema cell
added_second and its placement are never queried/described, promoted payload.keep is never read afterward, and the re-added MAP/ARRAY children are not queried at their re-add checkpoint. These S02/S03/S12/S13 cells can regress without changing an assertion; add explicit value, type, order, and historical checks.
| sql(""" | ||
| select id, old_partition, payload | ||
| from ${partitionTable} for version as of ${partitionCp0} | ||
| where old_partition = 'p1' or old_partition = 'p2' |
There was a problem hiding this comment.
[P2] Make the pruning predicate exclude a partition
The checkpoint contains only p1 and p2, and this predicate selects both, so removing it produces the same rows. Restrict the query to one partition and assert selected-partition evidence in EXPLAIN; otherwise the claimed pruning cell passes even when pruning is ignored.
| // add and reorder columns while keeping the pre-change snapshot, tag and branch readable. | ||
| spark_iceberg_multi """ | ||
| ALTER TABLE demo.${dbName}.${topTable} | ||
| ADD COLUMNS (added STRING AFTER old_name, added_second BIGINT); |
There was a problem hiding this comment.
[P2] Add value and type oracles for the created cells
added_second has no historical NULL-backfill/current-value/type assertion, and the promoted nested siblings are omitted from later projections. Add explicit historical/current checks using beyond-INT values where relevant; these advertised cells can currently regress without changing a test result.
| 's3.access_key'='admin', | ||
| 's3.secret_key'='password', | ||
| 's3.path.style.access'='true', | ||
| 'meta.cache.paimon.table.ttl-second'='0' |
There was a problem hiding this comment.
[P1] Disable the schema cache in the cache-off catalog
This only disables meta.cache.paimon.table.ttl-second, but historical binding uses the separate schema entry registered by PaimonExternalMetaCache. Also set meta.cache.paimon.schema.ttl-second=0; otherwise this query does not exercise the advertised cache-off schema path.
| sql """refresh catalog ${catalogName}""" | ||
|
|
||
| // Scenario TC01: validate latest schema/data, explicit new binding, predicate and aggregate. | ||
| assertEquals([[1, null], [2, null], [3, null], [4, null], [5, 5000L], [6, 6000L]], |
There was a problem hiding this comment.
[P1] Commit generated expected results for fixed row matrices
The new standalone suites use many hard-coded deterministic assertEquals row sets and add no .out files. Root AGENTS.md requires determined regression results to use named qt/order_qt cases with auto-generated outputs. Keep direct assertions only for runtime-derived metadata and invariants.
| exception "Unknown column 'new_new_id'" | ||
| } | ||
|
|
||
| // Scenario TC04-EQ-S04/S08: the renamed-key snapshot applies equality deletes by field ID. |
There was a problem hiding this comment.
[P1] Add the type promotion this checkpoint claims
Both referenced equality-delete fixture histories only rename fields and later add another INT; neither contains an S08 type transition. Add a real compatible promotion with a beyond-INT value and assert the pre/post rows, or remove equality-delete × promotion from the coverage claim.
| 60 | ||
| ) | ||
| scpFiles("root", hostIp, localDriverPath, jdbcDriversDir, false) | ||
| } |
There was a problem hiding this comment.
[P1] Bracket IPv6 literals in the SCP authority
The helper builds root@${host}:${path}. For a remote IPv6 literal, OpenSSH treats the first colon as the host/path separator, so SSH mkdir can succeed and this copy then targets the wrong host. Use root@[${host}]:${path} for IPv6, ideally in the shared staging helper.
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
3 participants
What
Scope
Tests and test documentation only. No production code is changed.
Validation
The branch was validated successfully and was not rebased after validation, as requested.