[feature](paimon) Backport Paimon options and read/write coverage - #66176
Conversation
doc： apache/doris-website#4012 Paimon provides many configurable table options, now introduces the `paimon.table-option.*` namespace. Doris removes the prefix, validates the option through Paimon's `SupportedTableOptions`, and applies it to the Paimon `Table` before serialization. Explicit options, such as `paimon.table-option.read.batch-size`, are preserved by the JNI scanner. Invalid or unsupported options fail fast during Catalog initialization. eg: ``` CREATE CATALOG paimon_catalog PROPERTIES ( "type" = "paimon", "paimon.catalog.type" = "filesystem", "warehouse" = "hdfs://127.0.0.1:8020/user/paimon/warehouse", "paimon.table-option.read.batch-size" = "4096", "paimon.jni.enable_jni_io_manager" = "true", "paimon.jni.io_manager.tmp_dir" = "/data/doris/paimon_jni_tmp" ); ``` 2. some jni params all named with paimon.jni.xxxx
### What problem does this PR solve? Problem Summary: The Paimon P0 regression suite did not distinguish the four primary-key merge engines and did not verify that every unsupported Doris data-write shape leaves both data and snapshots unchanged. This PR: - adds Parquet and ORC coverage for deduplicate, partial-update, aggregation, and first-row tables under automatic and forced-JNI reader routing; - verifies dynamic-bucket cross-partition deduplication; - covers rejected INSERT, INSERT SELECT, INSERT OVERWRITE, UPDATE, DELETE, and MERGE statements with pre/post row and snapshot invariants; - records the Paimon CTAS metadata-atomicity defect as an isolated opt-in negative regression; and - documents the Paimon/Iceberg read/write P0 coverage matrix and why no additional Iceberg P0 case is needed for the currently supported surface.
Paimon scans need a query-scoped way to pass dynamic table options to the Paimon SDK. This is especially useful for system tables such as `$files`, where options like `scan.snapshot-id` determine which snapshot is materialized. This PR: - adds `options` to Doris table scan parameters; - supports the existing relation parameter syntax `@options(...)` for both ordinary Paimon data tables and Paimon system tables; - applies query options through `Table.copy(options)`, so the cached base table is not mutated; - serializes the option-bearing table copy to the BE JNI reader, where lazy system-table splits are materialized; - keeps `@incr(...)` for incremental range reads. Paimon validates option names and values. Paimon system-table time travel with `FOR VERSION/TIME AS OF` and unsupported scan parameter types remain rejected. Query a Paimon system table at one snapshot: ```sql SELECT `partition`, bucket, file_path, file_format, schema_id, level, record_count, file_size_in_bytes, min_sequence_number, max_sequence_number, creation_time FROM paimon_catalog.db.`orders$files` @options('scan.snapshot-id'='12345'); ``` The same syntax works for ordinary data tables: ```sql SELECT * FROM paimon_catalog.db.orders @options( 'scan.mode'='from-snapshot', 'scan.snapshot-id'='12345' ); ``` Options are scoped to each relation, so different aliases can use different snapshots: ```sql SELECT left_orders.id, right_orders.id FROM paimon_catalog.db.orders @options('scan.snapshot-id'='1') left_orders JOIN paimon_catalog.db.orders @options('scan.snapshot-id'='2') right_orders ON left_orders.id = right_orders.id; ``` For incremental changes, use `@incr`: ```sql SELECT * FROM paimon_catalog.db.`orders$snapshots` @incr('startSnapshotId'=1, 'endSnapshotId'=2); ``` `@incr` maps the range to Paimon's `incremental-between` option. In contrast, `scan.snapshot-id` selects one table snapshot. A relation accepts one scan-parameter clause, so `@incr` and `@options` cannot be combined on the same relation. 1. Catalog `paimon.table-default.*` properties provide defaults when a table is created. 2. Explicit table properties take precedence over those catalog creation defaults. 3. Query-level `@options(...)` values override the same keys on the Paimon table copy for that query. 4. The cached table is not modified, so options do not leak to later queries. Support query-level Paimon dynamic options through the table scan parameter syntax `@options(...)` for data tables and system tables. - Test: - Unit Test - Regression test - Behavior changed: Yes. Paimon data-table and system-table scans can pass query-scoped options to the Paimon SDK. - Does this need documentation: Yes - FE build passed. - `NereidsParserTest`, `PaimonScanNodeTest`, and `AbstractPaimonPropertiesTest`: 98 tests passed, 0 failures, 0 errors. - Local regression: - `test_paimon_incr_read`: 1 suite passed, 0 failed, 0 fatal, 0 skipped. - `paimon_system_table`: 1 suite passed, 0 failed, 0 fatal, 0 skipped. - Data-table coverage includes native and JNI scanners, snapshots 1/2/3, relation-scoped aliases, query isolation, and invalid snapshot handling. - Multi-snapshot `$files` verification: - latest snapshot: 3 files; - `scan.snapshot-id=1`: 1 file; - `scan.snapshot-id=2`: 2 files; - `scan.snapshot-id=3`: 3 files. - Performance comparison on a Paimon table with 201 snapshots and 10,001 current files, using 10 alternating runs after warm-up: - latest `$files` mean: 5.704s; - `scan.snapshot-id=1` mean: 1.443s; - 3.95x faster, with 74.7% lower elapsed time.
### What problem does this PR solve? Related PR: apache#65502 Problem Summary: apache#65502 already fixed the Iceberg scan correctness issue by sending the complete current table schema to readers. Although that change was introduced for equality-delete dependencies, the same all-column schema invariant is also required for partition evolution: a column classified as an identity partition column by a newer partition spec can still be stored physically in files written by an older spec. This follow-up does not change scan behavior. It documents the partition-evolution invariant, extracts the existing schema initialization into a testable helper, and adds a focused FE unit test verifying that a non-projected evolved partition column remains in the reader schema. ### Release note None ### Check List (For Author) - Test - [ ] Regression test - [x] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason - Behavior changed: - [x] No. - [ ] Yes. - Does this need documentation? - [x] No. - [ ] Yes. ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
Gabriel39
changed the title
|
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. |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes for 16 substantiated issues (11 P1, 5 P2). The blocking themes are statement-snapshot consistency, retained prepared-command state, Paimon selector/schema canonicalization and query authorization, unsafe catalog defaults and failed-ALTER state, upgrade compatibility, and V1 reader arbitration. The Iceberg complete-schema refactor was independently checked and found behavior-neutral; other processed-table, V2, count, and output mappings were also swept. No additional user review focus was provided. Per the review contract this was a static review only, so no builds or tests were run. The repository-local code-review SKILL.md could not be found; the bundled prompt, required AGENTS.md, and its two FileScannerV2 guides were used as the available contract. The final (third) convergence round found the authorization issue, so the bounded review loop is reported as incomplete even though every candidate found has been adjudicated and included.
| public boolean shouldPreloadLatestSnapshot() { | ||
| return hasLatestOnlyRelation && !hasNonLatestRelation; | ||
| // A historical alias has independent scan state and must not cancel the latest alias warmup. | ||
| return hasLatestOnlyRelation; |
There was a problem hiding this comment.
[P1] Preserve the historical alias's snapshot when preloading latest
When one statement references this table through both a latest alias and a historical alias, returning true here makes PreloadExternalMetadata install the latest snapshot in StatementContext.snapshots. That map is keyed only by table identity, and loadSnapshots() never replaces an existing entry, so the later explicit snapshot/tag load is ignored. The historical alias can consequently bind latest schema/partitions (or fail after a historical rename/drop). Please either skip this preload when any non-latest alias exists or key the cached snapshot by relation/selector, and extend the mixed-alias test through historical binding rather than only asserting that latest preload ran.
| relationRoot = ((InsertOverwriteTableCommand) logicalPlan).getLogicalQuery(); | ||
| } else if (logicalPlan instanceof UpdateCommand) { | ||
| relationRoot = ((UpdateCommand) logicalPlan).getLogicalQuery(); | ||
| } else if (logicalPlan instanceof Command) { |
There was a problem hiding this comment.
[P1] Reset every relation-bearing prepared command
This generic branch includes relation-bearing DML that retains source plans: DeleteFromUsingCommand.logicalQuery is executed through an insert command, and top-level MergeIntoCommand reuses its source for both OLAP and Iceberg targets. Either source can contain a Paimon @options relation, but neither is traversed here, so repeated EXECUTEs keep the snapshot resolved by the first execution. Please introduce a common relation-root contract (or explicitly cover both commands) and test repeated DELETE USING and MERGE after advancing the Paimon source.
| relationOptions -> PaimonScanParams.resolveOptions(resolutionTable, relationOptions)); | ||
| // Startup options are normalized to an immutable snapshot before schema binding. The | ||
| // scan phase reuses that exact resolution instead of consulting a mutable tag or clock. | ||
| Table table = PaimonScanParams.selectsSchema(resolvedOptions) |
There was a problem hiding this comment.
[P1] Preserve current schema when pinning latest state
For selector-free default/latest/latest-full/full, resolution starts from the statement table, which is intentionally a pinned snapshot followed by copyWithLatestSchema(). File-creation-time resolution similarly pins the latest snapshot while native Paimon keeps the current row type. Both paths then carry a scan.snapshot-id into this condition, rebuild from the base table, and time-travel to the data snapshot's older schema. After a schema-only ADD COLUMN, these OPTIONS forms can reject a column native latest/file-creation reads expose as null. Please preserve latest-schema provenance for both pinned states and add schema-only evolution regressions.
| if (tagName != null) { | ||
| // A tag owns a retained Snapshot copy even after the ordinary snapshot file expires. | ||
| // Keep the canonical tag selector so planning reads that retained metadata path. | ||
| return resolvedTagOptions(options, tagName); |
There was a problem hiding this comment.
[P1] Pin the selected tag state, not only its mutable name
This keeps scan.tag-name so expired ordinary snapshots remain readable, but both schema binding and scan initialization later call applyOptions() on fresh base tables. Each Paimon Table.copy resolves the tag name again, so a concurrent tag replacement can make one statement bind the old tag schema and execute the new tag snapshot. Please retain an immutable selected tagged table/snapshot that both phases reuse, and test replacing a tag between output binding and split initialization.
|
|
||
| private static SupportedTableOptions build() { | ||
| Map<String, ConfigOption<?>> exactOptions = new HashMap<>(); | ||
| for (ConfigOption<?> option : CoreOptions.getOptions()) { |
There was a problem hiding this comment.
[P1] Restrict catalog defaults to options safe across every internal copy
This allowlist includes every reflected CoreOptions entry, while validation only parses its type. Canonical immutable keys such as merge-engine fail on the later table.copy(), while fallback aliases can evade that raw-key guard: deduplicate.ignore-delete=true is still resolved by Paimon and can suppress retracts, changing visible PK rows. Even copy-legal startup defaults are unsafe: scan.mode=latest later conflicts with the projection loader's snapshot ID, while a catalog snapshot ID is silently replaced by live latest. Please canonicalize names and define a copy- and lifecycle-safe allowlist that excludes immutable aliases and read-state selectors, tested through real nonempty-table initialization.
| "Paimon system table '" + systemTableType + "' does not support OPTIONS scan params."); | ||
| } | ||
| if (scanParams.isOptions() | ||
| && scanParams.getMapParams().containsKey(CoreOptions.SCAN_FILE_CREATION_TIME_MILLIS.key())) { |
There was a problem hiding this comment.
[P2] Make system-table creation-time support independent of retention
This rejects scan.file-creation-time-millis but still admits scan.creation-time-millis for every OPTIONS-capable system table. The latter resolves to a snapshot only when previousSnapshotId + 1 is retained; at a boundary or after expiration it falls back to the same internal file-creation predicate, which PaimonSysExternalTable then rejects. Identical SQL can therefore switch from success to failure as retention changes. Please reject both forms up front or implement the filter for these wrappers, and test a supported system table at the retention boundary.
| if (CoreOptions.SCAN_CREATION_TIME_MILLIS.key().equals(position)) { | ||
| return "from-creation-timestamp".equals(mode); | ||
| } | ||
| return "from-snapshot".equals(mode) |
There was a problem hiding this comment.
[P2] Canonicalize numeric scan.version before mode validation
A digit-only scan.version is Paimon's alias for scan.snapshot-id; Paimon 1.3.1 converts it during Table.copy() before validating that from-snapshot-full has a snapshot ID. This pre-validation looks only at the raw key, so Doris rejects a combination the bundled dependency accepts and implements. Please canonicalize the version first (or classify a digit-only value as snapshot ID here) and add a positive full-snapshot test.
| return table; | ||
| } | ||
| Map<String, String> resolvedOptions = scanParams.getOrResolveMapParams( | ||
| options -> PaimonScanParams.resolveOptions(sourceTable.getBasePaimonTable(), options)); |
There was a problem hiding this comment.
[P1] Resolve system-table latest OPTIONS from the statement snapshot
Selector-free system-table OPTIONS are classified as latest statement reads, but this resolver uses sourceTable.getBasePaimonTable() directly. Meta-table binding returns before the normal source-table loadSnapshots() path, whereas data-table OPTIONS resolve from the statement-pinned table. An external commit between binding data and system aliases can thus make one statement observe different source commits. Please load/reuse the source table's MVCC snapshot here and add a mixed data/system-alias consistency test.
| } | ||
|
|
||
| try { | ||
| new Options(Collections.singletonMap(key, value)).get(option); |
There was a problem hiding this comment.
[P1] Reject a zero Paimon read batch size
Type parsing accepts paimon.table-option.read.batch-size=0. In Paimon 1.3.1 the Parquet reader then advances by min(batchSize, remaining), returns a non-null zero-row batch, and never increments its row position. PaimonJniScanner exhausts that empty iterator and immediately calls readBatch() again, so a nonempty scan spins without reaching EOF. Please require a strictly positive value and add zero-value CREATE/ALTER plus scanner forward-progress coverage.
| | Paimon | Append table, partitioned table, primitive and nested types | Covered | `test_paimon_catalog`, `test_paimon_partition_table`, `test_paimon_full_schema_change` | | ||
| | Paimon | Primary-key deduplicate, partial-update, aggregation and first-row | Covered | `test_paimon_merge_engine_matrix` | | ||
| | Paimon | Fixed bucket, dynamic bucket and cross-partition update | Covered | `test_paimon_merge_engine_matrix`, `test_paimon_partition_pk_delete_refs` | | ||
| | Paimon | Parquet/ORC and mixed-format reads; JNI/native parity | Covered | `test_paimon_merge_engine_matrix`, `paimon_tb_mix_format`, `test_paimon_cpp_reader` | |
There was a problem hiding this comment.
[P2] Require evidence that the parity runs use different readers
The cited test_paimon_cpp_reader toggles enable_paimon_cpp_reader, but that flag is consumed only by legacy V1 while FileScannerV2 defaults on. The other cited suites compare forced JNI with automatic routing without asserting that the automatic run contains any native split, so all comparisons can exercise JNI and still pass. Please add EXPLAIN assertions for JNI versus paimonNativeReadSplits/NATIVE (as test_paimon_partition_pk_delete_refs does), or cite and extend a suite that proves both routes before marking parity covered.
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
4 participants
Summary
Validation
PaimonJniReaderTestandPaimonReaderTest.