[fix](fe) Preserve external schema generations and timestamp precision - #67904
Open
Gabriel39 wants to merge 9 commits into
Open
[fix](fe) Preserve external schema generations and timestamp precision#67904Gabriel39 wants to merge 9 commits into
Gabriel39 wants to merge 9 commits into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Request changes: four findings remain (three P1, one P2). The NTZ constructor change itself is sound, but the schema/data-generation handling is not end-to-end safe.
Critical checkpoint conclusions:
- Goal and proof: The goal is to retain the current Paimon schema while fencing data and to preserve NTZ sub-millisecond predicates. M1, M2, and M4 show the schema/MVCC goal is not achieved in branch, cached-table, and schema-derived system paths; M3 shows the end-to-end precision regression does not exercise NTZ.
- Scope and focus: The patch is otherwise focused across three production files and targeted tests. No additional user focus was supplied, so the full changed-file scope was reviewed.
- Concurrency: No threads or locks are added. External data/schema commits can race statement binding and planning; the unfenced branch transition is M1 and exact-schema drift is M2.
- Lifecycle and static initialization: Table handles, transient Paimon tables, and scan-option copies are statement-scoped; no new static initialization or resource-release issue was found. The branch handle reload lifecycle is part of M1.
- Configuration: No production configuration is added. The regression restores force_jni_scanner in finally.
- Compatibility: No public SPI, thrift, storage format, symbol, or rolling-upgrade surface changes. Paimon 1.3.1 provides the APIs used.
- Parallel paths: Normal, branch, options, native, JNI, system-table, privilege, fallback, partition, and statistics paths were traced. Distinct gaps are M1, M2, and M4.
- Special conditions: The new FileStoreTable/preserve-marker condition is documented but insufficient: it excludes schema-derived wrappers (M4), and the marker carries no exact schema generation (M2).
- Test coverage: Added unit cases cover a fresh ordinary table and the converter helper, but miss branch mutation, warm catalog cache, schema-derived system aliases, and a true NTZ end-to-end setup (M1-M4). No additional negative/empty-branch test covers the first-commit race.
- Test results: Expected rows are deterministic for the stable setup, but the precision assertions can pass via residual filtering because the Spark column is LTZ (M3). No result file is changed.
- Observability: No new logging or metrics are required for this local read-path change; no separate observability defect was found.
- Transactions and persistence: No Doris transaction, EditLog, failover, or persisted-metadata path is modified.
- Data writes and atomicity: Production changes are read-only; no write/crash atomicity path is modified.
- FE/BE variables: No new cross-process variable or thrift field is introduced. Existing serialized-table/native paths are inconsistent only as described in M2/M4.
- Performance: The added map/table copies are bounded per statement; no distinct CPU, memory, or asymptotic regression was found.
- Other issues: No additional correctness, error-handling, security, or style blocker remained after three converged review rounds.
Static review only: per the review contract, no builds or tests were run, so author/CI test claims were not independently executed.
Contributor
TPC-H: Total hot run time: 16965 ms |
Contributor
TPC-DS: Total hot run time: 82255 ms |
Contributor
ClickBench: Total hot run time: 14.79 s |
### What problem does this PR solve? Related PR: apache#67904 Problem Summary: Cached Paimon tables and system wrappers could use a different schema than statement binding, and branch reads could observe later commits. Carry the exact schema generation and positive or empty branch data fence through binding, native planning and JNI serialization. Use explicit NTZ literals in the timestamp precision regression. ### Release note Paimon statement reads retain consistent schema and data generations. ### Check List (For Author) - Test: Paimon package build (570 passed, 1 skipped); FE MVCC unit tests (68 passed); FE Checkstyle. External regression updated, not run locally. - Behavior changed: Yes, schema and data fences remain consistent through reads. - Does this need documentation: No.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review of exact head 4ffdc1fc1d5b4dbf94ad6dac896b9c24a9976bb3 against base 73b2a153cb5eb34b474386d678ee4a02af5788bd. I am requesting changes for four P1 correctness defects and one P2 regression-standard defect, detailed inline.
Checkpoint conclusions:
- Goal and proof: the ordinary stale-table, system-wrapper, branch-fence, and microsecond cases are addressed, but schema-owned options, fallback topology, TIMESTAMP(7..9), and Iceberg's generic latest pin remain inconsistent; the release-note claim is therefore not yet established for supported paths.
- Focus and scope: all 12 changed files plus relevant callers and consumers were reviewed; no user-specific focus was supplied. The change is otherwise focused.
- Concurrency and lifecycle: the schema memo is statement-scoped, concurrently safe, and discarded with metadata; handle identity separates base/system/branch tables. No lock-order or teardown issue found.
- Configuration and compatibility: no new user configuration, persisted format, public symbol, storage protocol, or FE-BE field is introduced. The internal marker is stripped before Paimon/backend options.
- Parallel paths and conditions: plain, OPTIONS, branch, empty, system-wrapper, fallback/decorator, native dictionary, catalog-loader removal, JNI serialization, and Iceberg paths were traced. The surviving defects are the five inline points; prior GitHub threads were treated as duplicate fences.
- Tests and results: the changed unit tests statically reach the main intended paths but miss renamed schema-owned options, stale fallback pairs, precision above 6, and mixed Iceberg cache generations. Stable regression rows also bypass the required
qt_/order_qt_generated-output workflow. This review ran no builds or tests. The author reports 570 Paimon tests, 68 MVCC tests, and Checkstyle passing; the external Spark/Doris regression was not run locally. - Observability and performance: existing errors/logging are adequate, the per-statement map and schema lookup are bounded, and no separate observability or performance blocker was found.
- Transactions, persistence, and data writes are unchanged. Memory-safety and BE nullable checkpoints are not applicable to this FE Java/test-only diff.
Contributor
TPC-H: Total hot run time: 17215 ms |
Contributor
TPC-DS: Total hot run time: 83453 ms |
Contributor
ClickBench: Total hot run time: 14.97 s |
### What problem does this PR solve? Related PR: apache#67904 Problem Summary: Restoring a bound Paimon schema could retain field-referencing options from a different generation or overwrite a fallback branch's schema and identity. Exact nanosecond timestamp pushdown could reject values that compare equal after Doris truncation. Iceberg cached schema pins could expose live partition names and specs. Preserve each schema's options and fallback provenance, retain privilege checks, keep high-precision comparisons residual, and pin Iceberg partition specs with their schema. ### Release note External scans retain consistent schema and partition metadata. Paimon nanosecond timestamps are compared at Doris precision without unsafe source pruning. ### Check List (For Author) - Test: 1,963 connector unit tests passed, 6 skipped; one existing Iceberg test failure reproduced before the changes and excluded. FE Checkstyle and plugin packages passed. Generated and verified all five regression baselines through the regression harness. - Behavior changed: Yes, preserve schema provenance and avoid false timestamp pruning. - Does this need documentation: No.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Gabriel39
changed the title
Contributor
Author
|
Addressed the five review findings in 75ceac1:
Validation: 1,963 connector tests passed and 6 skipped; one pre-existing Iceberg MERGE test failure was reproduced on the previous PR head and excluded. FE Checkstyle, FE build, and plugin packages passed. Local regression used the current FE/JNI extension with an existing BE and disabled the FE local-shuffle planner only on that isolated test instance for compatibility. The PR description has been updated. GitHub review-reply and thread-resolution APIs are returning server errors, so this top-level comment records the fixes while those discussion updates remain unavailable. |
Contributor
TPC-H: Total hot run time: 16922 ms |
Contributor
There was a problem hiding this comment.
Static review outcome: request changes on exact head 75ceac12b856aca84f572e2337b52870837950b7.
Goal and focus: this PR separates data, schema, and Iceberg spec generations for statement-stable external-table reads and fixes Paimon timestamp literal precision. No additional review focus was supplied. The implementation addresses the previously reported branch, wrapper, bound-schema, schema-builder, timestamp-test, and regression-baseline gaps, but three statement-consistency blockers remain.
Blocking findings:
- Paimon exact-schema discovery/materialization/restoration performs schema-file I/O after the connector authentication/TCCL scope has closed.
- Iceberg records the latest spec ID for schema assembly but drops it before partition-view construction and cache identity.
- The generic MVCC layer computes an exact local pinned schema, then builds partition items through ambient no-argument schema accessors before the pin is registered.
Checkpoint conclusions:
- Goal alignment and tests: the changed tests exercise warm-table Paimon schema restoration, branch/options/native/JNI paths, NTZ precision, Iceberg schema assembly, and generated regression output. They stop before the three failing boundaries described inline.
- Scope, clarity, and reuse: the changes are focused and reuse the statement snapshot abstraction, but the coordinate is not propagated through every generic/connector consumer.
- Concurrency and synchronization: statement maps and immutable/single-flight caches show no new lock-order or mutation race; omitting
specIdfrom the derived cache identity is nevertheless a cross-generation correctness error. - Lifecycle and cleanup: statement metadata ownership and connector close paths are sound. Snapshot registration timing leaves partition construction unable to observe the local pin.
- Configuration: no new configuration contract is introduced. Existing metadata-cache behavior and secured connector deployments expose the reported gaps.
- Compatibility and parallel paths: no persisted or FE/BE wire format changes were found. Base tables, OPTIONS/system/fallback/branch variants, empty snapshots, native/JNI reads, and RANGE/LIST Iceberg paths were checked; existing comments remain duplicate fences.
- Conditions and error handling: timestamp comparison, IN/OR, null propagation, and residual filtering are consistent. The remaining schema I/O can escape authentication, while mixed partition arity/types can fail analysis or silently omit partitions.
- Test execution: per the review contract, I did not build or run tests; conclusions are static. Changed tests were inspected, including the generated ordered regression baseline.
- Observability: no separate logging or metrics gap was identified; fixing context and generation propagation is preferable to diagnosing the resulting failures later.
- Persistence, transactions, and writes: not applicable beyond read-side metadata coordinates; no EditLog, storage-format, or data-write change is present.
- Resource and null safety: Java-only changes introduce no BE ownership or memory-safety issue; nullable timestamp/filter paths retain residual evaluation.
- Performance: statement memoization and bounded caches are reasonable. The spec-view cache key must include the complete generation coordinate.
- Security and other risks: the Doris threat model was applied. Privilege-wrapper retention is sound and no security vulnerability is asserted; the Paimon issue is authenticated connector correctness/availability in secured deployments.
The three inline P1 findings must be fixed and covered before approval.
Contributor
TPC-DS: Total hot run time: 83385 ms |
Contributor
ClickBench: Total hot run time: 15.27 s |
Contributor
There was a problem hiding this comment.
Static review of exact head 1cf784fb4a972f2ec242919db51c05c4cfa121c0 found two blocking correctness issues; the inline comments contain the concrete failure paths and requested coverage.
Checkpoint conclusions:
- Goal and scope: I reviewed all 19 changed files and traced schema/snapshot/spec binding end to end. The main schema-generation and timestamp-precision direction is coherent, subject to the two gaps below.
- Concurrency, lifecycle, and static state: statement maps and connector caches remain bounded/owned by their existing lifecycles. The Iceberg table-name/access-TTL pin can outlive an external recreate while the raw table is deliberately reloaded, producing the missing-spec failure.
- Configuration and dynamic behavior: no public configuration contract changed. Paimon internal bound-schema markers are stripped before SDK validation, but two later system-source transformations escape the connector authentication scope.
- Compatibility, rolling upgrade, and FE-BE values: no SPI/Thrift/edit-log format change was found. Native/JNI/backend schema paths otherwise agree, and high-precision Paimon timestamps remain residual instead of being unsafely pushed.
- Parallel and special paths: latest/empty, snapshot/time/tag/branch/options, normal/system, fallback/privilege, native/JNI, Iceberg first-append, RANGE/LIST, and synthesized/generated-column paths were checked. The two inline issues are the remaining distinct divergences; the known Iceberg partition-view spec issue was not duplicated.
- Tests and result correctness: the unit and regression additions cover the principal schema and precision paths, and deterministic regression rows use generated
order_qt_baselines. Results cited in the PR are author-reported; I ran no build or tests because this review environment expressly prohibited them. Both accepted issues lack the production-shaped negative coverage described inline. - Observability and error handling: existing validation generally fails loud, but the Iceberg gap surfaces as an unhelpful null dereference and the Paimon gap as a catalog authentication failure during property construction.
- Persistence, replay, writes, and transactions: not applicable to these read-side metadata/planning changes; no new durable or transactional state was introduced.
- Performance: cache/memo capacities remain bounded and no separate hot-loop regression was established. The Paimon manifest-cap path performs an avoidable source reconstruction, but its correctness impact is covered by the auth comment.
- Security/trust model: external catalogs are administrator-trusted under the repository threat model. The authentication-scope finding is an operational connector correctness failure, not a Doris security vulnerability.
User focus: no additional focus was supplied, so the full PR was reviewed without narrowing scope. Two complete review rounds converged with no additional distinct findings.
…reparation ### What problem does this PR solve? Related PR: apache#67904 Problem Summary: A warm Iceberg latest pin can retain a partition spec ID that disappears after an external table recreation. Fall back to the reloaded table's current spec, matching the existing missing-schema behavior. Paimon scan properties can also restore bound system-source schemas after backend-table preparation leaves its authenticated scope. Authenticate both later source transformations with the existing classloader-aware helper. ### Release note Avoid null partition specs after Iceberg table recreation and preserve the Paimon authentication scope while preparing bound system-source scan properties. ### Check List (For Author) - Test: 801 related connector and metastore tests passed, one connectivity test skipped. Both new regressions failed before their fixes. FE build and connector package validation passed, including Checkstyle. - Behavior changed: Yes, preserve missing-history fallback and schema-read authentication. - Does this need documentation: No.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review of exact head a56a252a07840b42527a527f865e15d400168329 found two blocking table-generation correctness issues; the inline comments contain the concrete failure paths and requested coverage.
Checkpoint conclusions:
- Goal and proof: the patch largely separates schema and data coordinates and preserves timestamp precision, but two generation-reuse gaps remain. Existing tests cover same-generation and missing-ID behavior, not numeric-ID reuse after recreation.
- Scope and focus: all 19 changed files and the full authoritative diff were reviewed; no additional user focus was supplied.
- Concurrency and lifecycle: no new lock, thread-safety, or deadlock issue was found. Statement metadata, connector caches, invalidation, and external DDL lifecycle were traced; both findings are missing generation-identity fences.
- Configuration and compatibility: no new configuration or public storage, SPI, Thrift, or EditLog compatibility issue was found. Existing cache/TTL and vended-credential paths were checked.
- Parallel paths: plain/options/branch/system Paimon reads; latest/empty/time-travel Iceberg reads; native/JNI; RANGE/LIST; and synthesized/generated columns were traced.
- Special conditions and error handling: marker handling and timestamp precision gates are sound, but reused numeric IDs bypass the intended fallback/guard in the two reported paths.
- Tests and results: unit and regression coverage is broad, and expected outputs are consistent on static inspection, but the two production-shaped recreate/reuse cases are absent. Per the review prompt, I did not build or run tests; CI evidence is author-reported rather than independently executed here.
- Observability: no new metrics or logging requirement was identified; direct invariant coverage is preferable.
- Persistence, transactions, and writes: this is read-side planning/metadata work; no Doris persistent-state or write-path defect was found.
- FE/BE variables: the new Paimon schema coordinate and Iceberg spec selection were traced across their consumers; no additional carrier issue remains beyond existing hard duplicate fences.
- Performance: no separate hot-loop or asymptotic regression was found.
- Other: exactly two findings were accepted after two bounded review rounds; all other candidates were disproven or duplicate-fenced.
Contributor
TPC-H: Total hot run time: 16713 ms |
Contributor
TPC-DS: Total hot run time: 80839 ms |
Contributor
ClickBench: Total hot run time: 14.48 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
### What problem does this PR solve? Related PR: apache#67904 Problem Summary: Eager latest-schema pinning bypassed the schema cache loader's validation. A Paimon table with case-insensitive duplicate columns therefore reached connector planning and reported a different error. Validate the mapped pinned schema before partition enumeration to preserve the existing duplicate-column error. ### Release note Preserve duplicate-column validation when pinning external table schemas. ### Check List (For Author) - Test: 116 FE unit tests; local Paimon duplicate-column regression in Native and JNI modes; FE build and Checkstyle. - Behavior changed: Yes. Restore the existing duplicate-column error on the new latest-schema pinning path. - Does this need documentation: No.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review at exact head b6f108f8388b502e508ae33cfd6066e0acf756fd: request changes.
Two P1 correctness blockers remain unresolved, both already reported inline on this head:
- Paimon generation-blind schema memo: external drop/recreate can reuse schema ID 0, allowing the long-lived name/schema-ID memo to bind generation B's scan to generation A's schema indefinitely.
- Iceberg reused cached spec ID: an externally recreated table can reuse a cached numeric spec ID, causing current schema assembly to consume an unrelated historical spec from the replacement generation.
I did not duplicate these inline comments. All required full-coverage and risk-focused sweeps returned NO_NEW_VALUABLE_FINDINGS; every residual candidate was independently reduced to an existing thread, a fixed earlier thread, pre-existing behavior, or dismissed with concrete code evidence.
Checkpoint conclusions:
- Goal and evidence: the patch largely implements independent schema/data fencing for Paimon and Iceberg and preserves Paimon NTZ microseconds, with focused unit and regression coverage. The two generation-identity gaps above mean the goal is not yet complete under external recreation.
- Scope and clarity: the production changes are broad but mostly follow the necessary metadata, scan, wrapper, native/JNI, and generic-FE consumers. No additional user-provided focus was present.
- Concurrency and lifecycle: no new lock-order, deadlock, or shared-mutation issue was found. The blocking lifecycle problem is semantic: catalog-lifetime caches outlive physical table generations while their keys carry only names and reusable numeric IDs.
- Configuration and compatibility: no new user configuration, storage format, persisted metadata, public SPI, or FE/BE protocol change was found. Connector-private schema markers remain FE-side or are materialized into the existing serialized table representation.
- Parallel paths and conditions: ordinary, OPTIONS, branch, fallback, privilege-decorated, system-table, native, JNI, Iceberg statement-scope, partition, and schema-dictionary paths were traced. New special conditions are documented; the scan guard exempts only connector-classified
SYNTHESIZEDcolumns, while generated and physical mismatches remain fail-loud. - Tests and results: targeted tests cover schema-only evolution, empty-table transition, missing IDs, partition-column pinning, timestamp precisions/operators, native/JNI reads, and deterministic regression output. The missing cases are external recreation with a reused Paimon schema ID and a reused Iceberg spec ID. Per task constraints this was a static-only review: I ran no build or test; test/build results in the PR and thread replies are author-reported, not independently verified.
- Observability, persistence, writes, and performance: no new logging/metrics requirement, Doris transaction/EditLog path, or Doris data-write path applies. No obvious hot-loop regression was found; cache/memo lookup is cheap, but the under-specified cache identity is correctness-blocking. Authentication/TCCL restoration for the new Paimon schema reads appears consistently covered.
Review convergence is complete for this head; resolving the two existing P1 threads is required before approval.
Contributor
TPC-H: Total hot run time: 16690 ms |
Contributor
TPC-DS: Total hot run time: 81033 ms |
Contributor
ClickBench: Total hot run time: 14.58 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
924060929
reviewed
Sep 14, 2026
Contributor
There was a problem hiding this comment.
Reviewed exact head b6f108f8388b502e508ae33cfd6066e0acf756fd.
The overall schema/data-fence direction is sound, and the current CI is green, but two P1 generation-identity gaps remain unresolved:
- Paimon schema memo: the catalog-lifetime memo is keyed by table name plus reusable schema ID. After an external drop/recreate, generation B can reuse schema ID 0 and remain bound to generation A's cached schema until an explicit Doris invalidation.
- Iceberg cached spec: the missing-spec fallback does not cover a replacement table that reuses the cached numeric spec ID. In that case the current schema can be combined with an unrelated older spec from the replacement generation.
The existing tests cover same-generation schema evolution and missing IDs, but not external recreation with ID reuse. Please bind the cached coordinates to a stable physical-table generation identity and add both recreate/reuse regressions before approval.
### What problem does this PR solve? Related PR: apache#67904 Problem Summary: External table recreation can reuse schema and partition-spec IDs. The new Paimon latest-schema path could bind metadata from a historical memo belonging to the previous table, while Iceberg could apply a cached spec ID to an unrelated replacement table. Retain the actual Paimon statement schema for metadata and column handles. Bind Iceberg latest pins to the table identity, reject a mismatch before interpreting its schema/spec IDs, and invalidate the stale cache entry so a retry resolves the replacement table. ### Release note Preserve Paimon latest-schema binding after external table recreation. Detect stale Iceberg pins across table replacement and allow recovery by retrying the statement. ### Check List (For Author) - Test: 1,008 related unit and SDK tests passed, one connectivity test skipped; FE build, connector packages, and Checkstyle passed. Recreation regressions failed before the fix. - Behavior changed: Yes. Paimon latest metadata agrees with the replacement table. Iceberg rejects a pin from a different table identity and clears it for the next statement. - Does this need documentation: No.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
924060929
reviewed
Sep 14, 2026
924060929
left a comment
924060929
left a comment
Contributor
There was a problem hiding this comment.
Re-reviewed current head 54a5294. The Iceberg table-identity fix and the Paimon latest-schema fix address the two previously reported cases. One Paimon explicit time-travel path still allows a recreated table to reuse a schema ID and hit the previous table schema; details are inline.
Contributor
TPC-H: Total hot run time: 16810 ms |
Contributor
TPC-DS: Total hot run time: 82486 ms |
Contributor
ClickBench: Total hot run time: 14.73 s |
Contributor
Author
|
/review |
924060929
reviewed
Sep 14, 2026
924060929
left a comment
924060929
left a comment
Contributor
There was a problem hiding this comment.
Re-reviewed the current head with the local pipeline-style review. The previously reported schema, timestamp, authentication, and Iceberg identity fixes are present, but four distinct Major regressions remain: captured-schema lifetime across INSERT replanning, fallback-child schema restoration, catalog option provenance, and HMS Iceberg capability forwarding. Details are inline.
| // existing direct-construction tests compile unchanged; production goes through the 5-arg ctor. | ||
| private final PaimonLatestSnapshotCache latestSnapshotCache; | ||
|
|
||
| // Metadata is statement-scoped: aliases sharing a data fence must also share one schema generation. |
Contributor
There was a problem hiding this comment.
The captured schema is owned by one ConnectorMetadata instance, but an INSERT planning retry resets the connector scope while retaining the source MVCC snapshot. The replacement metadata has an empty statementSchemas map, so a preserved latest pin with a schema ID falls through to the catalog-lifetime schemaAtMemo. With an old table generation still memoized and a recreated same-name table reusing that schema ID, attempt 1 can bind the new table while attempt 2 obtains old column handles and fails. Please make the exact captured schema live as long as the retained MVCC pin, or re-read it once in the replacement scope without consulting the historical memo. Add an INSERT replan regression with an old-generation memo.
| FallbackReadFileStoreTable pair = (FallbackReadFileStoreTable) table; | ||
| // Schema IDs are branch-local. Broadcasting the main schema through copy(TableSchema) | ||
| // overwrites the fallback's provenance and can even reset its branch to main. | ||
| return new FallbackReadFileStoreTable(restoreBoundSchema(pair.wrapped(), schemaId), pair.fallback()); |
Contributor
There was a problem hiding this comment.
This restores only the main child and keeps the cached fallback child unchanged. If a warm M0/F0 fallback table has since received the same compatible nullable ADD COLUMN on both real branches, the current state is M1/F1 but Doris constructs M1/F0. Paimon 1.3.1 validates the complete row types before projection, so even SELECT id fails although the external branches are compatible. Capture and restore each child using its own branch-local schema coordinate; a main schema ID cannot be broadcast to the fallback branch.
| Map<String, String> merged = new HashMap<>(bound.options()); | ||
| // Field-referencing options evolve with the schema (e.g. bucket-key and sequence.field | ||
| // on rename). Only replay the catalog/runtime delta, never another generation's options. | ||
| table.options().forEach((key, value) -> { |
Contributor
There was a problem hiding this comment.
Value equality cannot recover option provenance. For example, an explicit catalog read.batch-size=64 is indistinguishable here from an old physical value of 64. After an external option-only change makes the physical value 128, this filter drops the explicit catalog override and the scan incorrectly uses 128; a new value of 0 can instead make a previously valid query fail validation. Preserve explicit catalog/runtime overrides separately and merge bound physical options, then catalog/runtime overrides, then relation overrides.
| if (connectorSnapshot.getSchemaId() >= 0) { | ||
| // Latest data and schema can advance independently. Keep the connector's exact schema | ||
| // on the statement pin so analysis cannot fall back to a different cached generation. | ||
| ConnectorTableSchema atSchema = metadata.getTableSchema(session, pinnedHandle, connectorSnapshot); |
Contributor
There was a problem hiding this comment.
The newly retained latest schema calls the snapshot-aware metadata overload. Through an HMS catalog, Hive delegates that overload to the Iceberg sibling but does not apply the capability reflection used by the ordinary two-argument overload. The resulting pinned schema has an empty capability set and shadows the enriched schema, disabling TopN lazy materialization, nested-column pruning, and storage predicate inference for ordinary HMS Iceberg reads. Please reuse the existing sibling capability inheritance helper in the snapshot-aware Hive path and add a retained-latest publication test for all three capabilities.
Contributor
There was a problem hiding this comment.
Request changes. Static review of exact head 54a5294b6cb8490693a157d0d465a47e5e15c9a6 found two additional P1 correctness/availability failures. An equivalent P2 catalog-option provenance issue was posted meanwhile in discussion r4004077352, so I have not duplicated it.
Critical checkpoints
- Goal and proof: The PR aims to keep external-table data, schema, spec, and predicate semantics coherent across schema-only evolution and recreation. The direct paths are substantially covered, but the goal is incomplete: Paimon can cross physical generations within one statement, and valid UUID-less Iceberg evolution is rejected.
- Focus/minimality: The 19-file diff is focused on Paimon/Iceberg MVCC pinning, generic FE consumers, and their tests; no unrelated edits were found.
- Concurrency/thread safety: External catalog ALTER/commit/DROP/CREATE operations are the relevant concurrent actors. Cache values are immutable and statement maps use the existing concurrent machinery; no new lock-order or deadlock issue was found. The Paimon logical handle key nevertheless lacks a physical-generation fence, producing the first inline issue.
- Lifecycle: Statement metadata, independently resolved Paimon handles, Iceberg statement-frozen tables, cache invalidation/retry, leases, authentication scopes, and decorator teardown were traced. The two inline issues are lifecycle/cache-boundary defects; no separate leak, double close, circular lifetime, or static-initialization issue was found.
- Configuration: No new public configuration is added. Existing no-cache/reload Paimon behavior makes the generation crossing reachable; REST vended credentials intentionally keep
latestSnapshotCachewhile disablingtableCache, making the Iceberg failure reachable. Existingpaimon.table-option.*precedence has a separate equal-value provenance gap already covered byr4004077352. - Compatibility: No persisted format, SPI symbol, EditLog, or FE-BE wire change is introduced. UUID-less format-V1 Iceberg metadata is an explicitly supported compatibility path, and the new exact-filename identity fallback breaks valid same-table commits there. No additional rolling-upgrade issue was found.
- Parallel paths: Latest, empty, reader-only OPTIONS, explicit selectors, branches, supported system wrappers, RANGE/LIST/UNPARTITIONED materialization, synchronous/streaming/count scans, native dictionaries, JNI serialization, and metadata tables were checked. No additional distinct issue survived; the known explicit Paimon historical-memo and Iceberg partition-view/spec points are already fenced by existing discussions.
- Conditions and error handling: The Iceberg mismatch error includes the table and retry guidance, but its condition is over-broad for ordinary UUID-less commits. Paimon numeric-ID/equality checks cannot prove physical identity. No separate swallowed status/exception or speculative defensive branch was found.
- Test coverage: The added unit/regression tests broadly cover cache hits, empty-to-first-append, recreation, schema rename, partition arity/type, synthesized versus generated columns, authentication, fallback/branch behavior, native/JNI paths, and timestamp precision. They miss recreation between one statement's pin and later scan-handle resolution, and an ordinary UUID-less M1-to-M2 commit under warm-pin/fresh-table caching.
- Test results/output: Query labels and expected regression output were checked statically. Per the review contract, I did not run builds or tests; author/CI-reported results are not independent verification.
- Observability: Existing errors carry useful table/retry context. Paimon generation mixing has no fail-loud signal, while the Iceberg signal fires on valid evolution; fixing those conditions is the actionable need. No separate logging or metrics gap was substantiated.
- Transactions/persistence: The changed code is read-metadata/planning logic and adds no Doris transaction, journal, failover, or persistent-state behavior.
- Data writes: No Doris data-write path is modified. Existing Iceberg writer identity ancestry was inspected as evidence for valid V1 evolution; no separate write atomicity issue was found.
- FE-BE variables: No new transmitted variable or scattered protocol send path is added; snapshot identity/schema/spec properties remain FE-internal. Native/JNI consumers were included in the consistency trace.
- Performance, memory, and BE nullability: This is FE Java code; no BE allocator/nullability issue applies. Schema/identity work is bounded to statement/cache operations. Apart from the recurring valid-query retry and the already-reported lost reader policy, no material CPU or memory regression was found.
- Other data correctness: Paimon timestamp precision 0-6 preserves the visible wall clock; precision 7-9 pushdown fails closed while Doris retains residual evaluation. The synthesized-column exemption remains narrow enough that physical/generated skew fails loudly. No further distinct issue was found.
User focus: No additional user-provided focus was supplied; the full authoritative diff was reviewed.
Completion: Complete after three convergence rounds. All candidates were accepted, dismissed with code evidence, or duplicate-fenced; this review submits the two remaining distinct findings.
| } | ||
|
|
||
| private long statementSchemaId(PaimonTableHandle handle, Table table) { | ||
| return statementSchemas.computeIfAbsent(handle, |
Contributor
There was a problem hiding this comment.
[P1] Fence the physical Paimon generation across one statement. statementSchemas is keyed by PaimonTableHandle equality, which omits the transient Table/generation. materializeLatest can capture generation A, but PluginDrivenScanNode.create later resolves the handle again; if an external DROP/CREATE occurs between them and B reuses the snapshot/schema IDs, schemaForPin returns A while withScanOptions preserves B and restoration skips because the IDs match. The FE guard therefore passes on A/A while native/JNI planning reads B's rows. This is distinct from the existing cross-statement memo thread because it occurs after the current statement pinned A. Please freeze or validate the physical generation through scan resolution, and cover recreation between pinning and scan-handle resolution in one metadata instance.
| return metadata.uuid(); | ||
| } | ||
| // Legacy V1 metadata may lack a UUID. Only the exact metadata file can safely reuse its IDs. | ||
| return "metadata:" + Objects.requireNonNull(metadata.metadataFileLocation(), |
Contributor
There was a problem hiding this comment.
[P1] Do not equate a UUID-less table's identity with its current metadata file. An ordinary Iceberg commit writes a new metadata file for the same table. With REST vended credentials, latestSnapshotCache stays enabled while tableCache is disabled, so the next statement can receive cached M1 coordinates, freshly load the same table at M2, and fail this identity check; active V1 tables then require a retry after every commit. IcebergWriteSchemaContext already handles this case by accepting a retained metadata ancestor. Please disable latest-pin caching for UUID-less tables or validate ancestry, and test a normal same-table commit under the warm-pin/fresh-table cache combination while retaining recreate protection.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Paimon statement data fences could rewind schema-only changes, allow later branch commits to become visible, or restore fields with another generation's options. Timestamp predicates could also prune matching rows after losing precision. The connector-generic schema pin exposed an Iceberg mismatch between cached columns and live partition metadata.
order_qt_cases and a harness-generated.outfile.Scope: this PR fixes Paimon schema-generation and timestamp handling, plus direct consequences of retaining connector schemas in the generic FE MVCC bridge. Historical Iceberg partition-spec isolation in RANGE/LIST views and derived cache keys is outside this change. A spec-only partition-style drift reproduces identically on the pre-PR implementation at
ee4a91720f; review thread records the evidence.Release note
Paimon reads retain consistent schema and data generations across analysis and execution, including schema-only changes and branch reads. Timestamp predicates preserve microseconds without incorrectly pruning nanosecond values. Iceberg pinned schemas retain consistent partition metadata after schema-only changes.
Check List (For Author)
Validation:
Latest generation-identity fixes at
54a5294b6c: Paimon retains the actual statement schema instead of using the historical memo for latest binding; Iceberg checks the physical-table identity before applying cached schema/spec IDs, invalidates mismatches, and reports a retryable statement error. Recreation tests cover reused IDs, replacement-row reads, and retry recovery. 1,008 related unit/SDK tests passed, one connectivity test skipped; FE build, connector packages, and Checkstyle passed.Earlier external regression fix at
b6f108f838: preserve schema-cache validation when eagerly pinning latest schemas. All 116 related FE unit tests passed; the duplicate-column SQL assertion was reproduced before the fix and passed afterward in Native and JNI modes against a local Paimon table. FE build and Checkstyle passed. The existingtest_paimon_catalogassertion is unchanged.Earlier review fixes at
a56a252a07: 801 related tests passed (572 Paimon, 199 Iceberg metadata/cache, 30 metastore); one Paimon connectivity test skipped. Warm-cache table recreation and the full Paimon JNI scan-property authentication path each failed before the fix and passed afterward. Final FE build, connector packages, and Checkstyle passed. The native historical schema-dictionary authentication path is unchanged from pre-PRee4a91720fand is outside this fix, as explained in the review reply.Paimon: 576 tests passed, one live-connectivity test skipped; package build passed. Three new tests verify real schema-file reads run in the authentication/TCCL scope, including a warm latest-cache hit and scan restoration. These use a guarded local FileIO, not a live Kerberos cluster.
Earlier Iceberg validation at 75ceac1: 1,390 tests passed, five skipped; package build passed.
IcebergWritePlanProviderTest.planMergePreservesExplicitlyEmptyReadAcrossConcurrentFirstAppendalso fails on the previous PR head and was excluded after reproducing that baseline failure.FE MVCC: 71 tests passed, including three new RANGE/LIST partition type/arity guards. The unrelated existing
IvmNormalizeMTMVJoinTestcompile failure was temporarily excluded; the build configuration was restored afterward.Current fixes at
1cf784fb4a: all 29 originally failing Iceberg suites passed locally after the fix, using the final FE and matching BE artifacts. This includes hidden row IDs, UPDATE/DELETE/MERGE, v3 row lineage, schema time travel, and initial defaults. Existing test baselines were unchanged.Current fixes: 234 FE scan/MVCC tests and 251 Iceberg metadata/MVCC/scan-provider tests passed. The synthesized-column and empty-table evolution regressions were reproduced with failing tests before their fixes.
FE Checkstyle and the final FE build passed.
test_paimon_schema_only_snapshot_precision(validated at 75ceac1): generated the baseline and reran successfully, covering plain, OPTIONS, branch, native precision, and JNI precision queries.Local regression used the current FE and Paimon JNI extension with an existing BE.
enable_local_shuffle_planner=falsewas set on the isolated test instance for plan-node compatibility; no environment configuration is included in this PR.Check List (For Reviewer who merge this PR)