[fix](paimon) Harden JNI reader options and transport - #66247
Open
Gabriel39 wants to merge 4 commits into
Open
Conversation
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: Paimon dynamic reader settings accepted unsafe or unbounded values, query-scoped settings could resize a JVM-global executor, and failed catalog validation could leave tentative properties installed. JNI projection framing also split quoted column names containing commas, while debug logging and scanner statistics could expose raw credentials or repeatedly scan every JVM thread. This change introduces a reader-only option allowlist and bounded validation, keeps catalog and relation precedence explicit, rolls back every catalog validation failure, and transports projected identifiers with per-field Base64 framing. It also replaces raw parameter logging with a fixed safe summary and shares JVM thread-count samples for one second. ### Release note Paimon catalogs and relation @options support bounded read.batch-size and file-reader-async-threshold tuning. Unsafe catalog table options are rejected, manifest planning parallelism is bounded by JVM processor capacity, and JNI projection now supports quoted identifiers containing delimiters. ### Check List (For Author) - Test - Unit Test - Regression test - Behavior changed: - Paimon dynamic reader settings now use an explicit safe allowlist and bounds. - Does this need documentation? - Yes. This PR documents the supported query options in its description.
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![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: five blocking correctness and compatibility issues remain in the Paimon hardening paths.
Checkpoint conclusions:
- Goal and proof: the patch improves option validation, JNI identifier framing, raw-parameter log redaction, ALTER rollback, and thread-count sampling, but it does not accomplish the goal on all supported inputs and paths. The added tests do not cover the five blockers.
- Scope and clarity: the change is generally focused and reuses shared validators, although the generic
CatalogMgrrollback change broadens the impact of connector-specific validation. - Concurrency: the cached thread counter has sound volatile publication, synchronized refresh, retry, and wrap-safe TTL behavior. Failed ALTER is not atomic against concurrent lazy catalog initialization (MAIN-4).
- Lifecycle: Paimon
Table.copyis relation-local and does not mutate cached base-table options; no C++ static-initialization-order or ownership issue was found. Failed ALTER can nevertheless leave initialized derived catalog state alive after raw-map rollback (MAIN-4). - Dynamic options/configuration: catalog and relation reader overrides are observed without restart and their new bounds are clear, but equivalent values inherited from the physical Paimon table bypass validation (MAIN-3).
- Compatibility: retaining the legacy JNI field is rolling-upgrade-friendly only on the V2 producer. The still-supported V1 producer omits the safe payload (MAIN-1), nested type framing remains delimiter-unsafe (MAIN-2), and persisted pre-upgrade catalog options can prevent initialization (MAIN-5).
- Parallel and conditional paths: V2, V1/load routing, catalog, relation, and physical option sources were traced. The missed V1 and physical-table paths are blockers; the added validation conditions and error conversions are otherwise understandable.
- Tests/results: helper unit tests cover basic parsing, validation, redaction, and cache TTL. The P0 success blocks assert only absence of exceptions, leave Scanner V2 enabled, and omit nested names, unsafe physical options, a latch-driven ALTER race, and image/edit-log upgrade coverage. No generated expected-result file was added.
- Observability: raw scanner parameters are no longer logged and the cached JVM thread gauge is safe; no additional metric or log blocker was found.
- Persistence/transactions/data writes: edit-log ordering is correct in the single-threaded success/failure paths and no data-write transaction path changes here. Concurrent failed ALTER can expose unjournaled derived state (MAIN-4), while reload of existing persisted properties is incompatible (MAIN-5).
- Cross-module transport: no new FE-to-BE variable is introduced, but the BE-to-Java JNI schema parameter was updated in only one producer and its parallel type payload remains unsafe (MAIN-1 and MAIN-2).
- Performance: the JVM thread-count cache removes repeated thread walks without a material lock bottleneck. Unsafe physical manifest parallelism can still create a zero-permit wrapper or enlarge Paimon's JVM-static executor (MAIN-3).
- Additional user focus: no extra focus was supplied; the entire changed-file set was reviewed.
Validation was static only: the authoritative review prompt prohibits builds/tests, and this checkout lacks .worktree_initialized and thirdparty/installed. No build or test result is claimed.
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 29850 ms |
Contributor
TPC-DS: Total hot run time: 177722 ms |
Contributor
ClickBench: Total hot run time: 25.02 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
Use paired delimiter-safe schema framing in both scanner routes, validate effective physical options, validate Paimon ALTER candidates off the live catalog, and preserve load compatibility for legacy catalog options. Add unit and P0 coverage for each review scenario.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 29454 ms |
Contributor
There was a problem hiding this comment.
Two blocking correctness gaps remain:
- The new Base64 list protocol still maps one empty top-level identifier to the same payload as zero projected fields, so the Java constructor rejects a reachable external Paimon schema on field/type cardinality.
- Physical-plus-catalog options are validated before relation-scoped options are copied, so a safe relation override cannot take the documented precedence over an unsafe physical value.
Critical checkpoint conclusions:
- Goal and proof: the PR substantially adds bounded Paimon reader options, physical/catalog validation, relation-local copies, delimiter-safe transport, log redaction, detached ALTER validation, replay compatibility, and cheaper metrics. The two findings above prevent the full advertised identifier and precedence behavior. Existing tests do not cover either failing case.
- Scope and clarity: production changes are focused on Paimon/JNI behavior plus one generic ExternalCatalog validation hook and one shared JNI type parser extension. No unrelated source changes were found.
- Concurrency and locking: detached Paimon validation does not publish tentative CatalogProperty state; accepted updates reset synchronized ExternalCatalog objects and invalidate Paimon caches. The global thread counter uses synchronized refresh and volatile publication with intentional one-second staleness. No additional deadlock or stale-publication defect was found.
- Lifecycle and error handling: V1/V2 scanner construction and existing JNI cleanup remain aligned; mixed encoded schema halves fail loudly. Catalog rollback/replay/reset paths preserve prior or journaled state. No new resource leak or swallowed status was substantiated.
- Configuration and dynamic behavior: no Doris runtime config item is added. Catalog ALTER resets derived state, relation options use per-handle Table.copy, and unsafe values are range-checked; MAIN-2 is the remaining ordering error in the precedence chain.
- Compatibility and parallel paths: legacy schema parameters remain as fallback, V1 and V2 now emit the paired protocol, and persisted legacy catalog keys remain raw but inactive. Ordinary, branch, snapshot, system-table, cache-loader, planning, and JNI serialization paths were traced. MAIN-1 affects both scanner routes; MAIN-2 affects ordinary and system-table relation copies.
- Persistence, transactions, and writes: the catalog edit-log replay path was checked and retains loadability without applying unsafe legacy options. The PR does not alter Doris data transaction or storage-format semantics.
- Testing: review-time builds/tests were forbidden by the authoritative prompt and none were run in this review environment. CI was observed separately immediately before submission; its evolving state is not claimed as review-time test evidence. Changed tests cover the existing five review fixes but omit
{}versus{""}framing and safe relation override of an unsafe physical value. - Observability and performance: raw credential-bearing scanner parameter logging is removed, fixed summary fields remain, and the JVM thread-count sample removes repeated ThreadMXBean allocation. No additional observability or material performance regression was found.
- FE/BE propagation and nullability: both legacy and V2 BE producers publish the new paired Java parameters; no new FE-to-BE thrift variable or nullable-column manipulation is introduced.
- User focus: no additional user-provided focus was supplied; the full PR was reviewed.
The five existing inline threads were rechecked and not duplicated: legacy V1 producer parity, nested type framing, no-override physical option validation, detached ALTER validation, and persisted-catalog compatibility are addressed at the reviewed head.
Contributor
TPC-DS: Total hot run time: 177617 ms |
Contributor
ClickBench: Total hot run time: 24.9 s |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Reviewed the full authoritative 22-file bundle; no additional user focus was supplied.
The four inline P1 findings are distinct remaining routes around the new effective-option guard: a read-only system-table wrapper hides its option-bearing data table, row-count planning runs outside PaimonScanNode, partition snapshot loading scans a raw physical handle before overrides, and a fallback reader exposes only the main branch's options.
Checkpoint conclusions:
- Goal and tests: direct data-table option precedence and both JNI scanner protocols are covered, but the changed tests do not exercise the four real wrapper/pre-planning/fallback routes above.
- Scope, parallel paths, and special conditions: V1/V2 JNI framing, data/system/incremental/options/no-options paths, empty and nested identifiers, partitioned metadata, row-count loading, and fallback branches were traced; only the four inline paths remain defective.
- Concurrency, lifecycle, persistence, and compatibility: detached catalog ALTER validation, single live publication/reset, replay/image compatibility, and legacy-key filtering are sound; no additional race or upgrade defect remains.
- Configuration and performance: valid catalog/relation precedence works on the foreground handle, but the four missed manifest paths can still block on a zero-permit executor or mutate Paimon's JVM-global manifest pool.
- Observability and FE/BE propagation: the credential-bearing raw parameter log is removed, the cached scanner metric is safely published, and the paired encoded schema plus serialized processed-table handoff are consistent.
- Data writes and transactions: no data-write/transaction behavior is changed by this PR, and no additional persistence issue was found.
Validation was static only. Per the authoritative bundle, no builds, tests, source changes, or commits were performed.
### What problem does this PR solve? Issue Number: None Related PR: apache#66247 Problem Summary: Paimon read-only system tables and fallback tables hide the file-store handles that actually plan manifests, while snapshot partition loading and row-count collection can plan before ScanNode. An unsafe physical manifest parallelism could therefore bypass the final visible-table check, and early validation could also reject a safe relation-level override. Validate each effective planning handle, preserve the fully merged table during partition projection, and guard row-count plans before they start. ### Release note Paimon manifest parallelism safeguards now cover system-table, fallback-branch, partition-metadata, and statistics planning paths. ### Check List (For Author) - Test: FE checkstyle, Groovy compilation, and manual Paimon 1.3.1 fixture probe passed; unit and P0 regression coverage added but not run locally because the installed third-party toolchain is absent - Behavior changed: Yes. Unsafe hidden manifest-planning handles are rejected before planning while safe relation overrides remain effective. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 29472 ms |
Contributor
There was a problem hiding this comment.
Automated review summary
Requesting changes for four substantiated issues at 8ea521522915c65fcbcf1239de94d125d89c0ff9: three P1 correctness/compatibility gaps in legacy STRUCT parsing and effective Paimon wrapper validation, plus one P2 repeated partition-enumeration regression for reader-only OPTIONS.
Critical checkpoint conclusions:
- Goal and proof — The guardrail, precedence, JNI framing, and credential-safe diagnostics goals are coherent and mostly implemented. The added FE/BE/P0 tests are broad, but they do not prove the four reported cases.
- Scope and clarity — Most changes are focused and reuse common helpers. The shared
ColumnTypeparser change unintentionally broadens the compatibility impact beyond Paimon, and the OPTIONS projection path does redundant work. - Concurrency — Detached catalog validation, synchronized publication/reset, double-checked system-table initialization, and volatile/synchronized counter sampling were traced. No lock-order or publication defect remains beyond the independently cached handle lifecycle reported inline.
- Lifecycle — Scanner open/close and catalog reset lifecycles are balanced. The source-table cache and system-wrapper cache can nevertheless represent different generations, which is reported inline.
- Configuration — Reader option ranges and relation > catalog > physical precedence are generally enforced, including CREATE/ALTER/replay paths. Privilege-wrapped fallback and divergent system-wrapper children bypass the intended complete effective-table validation.
- Compatibility — The paired Base64 schema payload is emitted by both V1 and V2 Paimon readers and the encoded protocol is rolling-compatible. The unencoded public parser no longer preserves its prior lowercase STRUCT-key contract.
- Parallel paths — Normal, snapshot, tag, branch, startup, incremental, row-count/statistics, partition, system-table, fallback, V1, and V2 paths were traced. The privilege-wrapped fallback topology is the remaining missed path.
- Conditional checks — New fail-closed checks are generally documented and correctly placed; the direct
instanceof FallbackReadFileStoreTablecondition is insufficient for supported delegated wrappers. - Test coverage — Positive and negative unit/P0 coverage exists for ranges, precedence, lifecycle, framing, fallback, system tables, and row-count planning. Missing coverage corresponds to the four inline findings.
- Test results — The changed expected behavior is internally consistent on inspection. No tests or builds were run because the authoritative review prompt prohibits them.
- Observability — Raw scanner parameters are no longer logged; the derived debug summary and counters are appropriately bounded. No additional logging/metrics issue was found.
- Persistence and transactions — Detached ALTER validation, EditLog publication/replay, restart behavior, and legacy property filtering were traced without a separate persistence defect. No data-write transaction path is changed.
- FE/BE propagation — Both BE reader generations publish the new paired fields and the Java scanner consumes them consistently. The compatibility problem is confined to the shared legacy parser behavior described inline.
- Performance — Reader-only OPTIONS unnecessarily bypass the memoized latest projection and re-enumerate partitions; no other material CPU, allocation, or memory-accounting regression was substantiated.
- Other issues — External catalog input is trusted under the repository threat model; no separate security vulnerability was identified. All other candidates were either fixed in existing live threads or dismissed with concrete code evidence.
There was no additional user-provided review focus, so the whole PR was reviewed. All required second-round normal and risk reviewers converged on this exact four-comment set with NO_NEW_VALUABLE_FINDINGS.
| if (lowerCaseType.indexOf("<") == 6 | ||
| && lowerCaseType.lastIndexOf(">") == lowerCaseType.length() - 1) { | ||
| String listFields = lowerCaseType.substring(7, lowerCaseType.length() - 1); | ||
| String listFields = originalType.substring(7, originalType.length() - 1); |
Contributor
There was a problem hiding this comment.
[P1] Preserve the legacy STRUCT-key normalization
parseType(...) still serves every unencoded VectorTable caller, but this now recurses through originalType even when encodedStructFields is false. Before this change, struct<Mixed:int> produced child key mixed; it now produces Mixed. VectorColumn.appendStruct and getStruct use these names as exact HashMap keys, including the Java UDF/UDAF path, so existing functions that read or return the formerly normalized key can silently receive/write NULL. Please use the original spelling only for the encoded Paimon grammar (or introduce an explicit compatibility migration) and add an unencoded mixed-case STRUCT test.
|
|
||
| public static void validateEffectiveTable(Table table) { | ||
| validateEffectiveTableOptions(table.options()); | ||
| if (table instanceof FallbackReadFileStoreTable) { |
Contributor
There was a problem hiding this comment.
[P1] Unwrap privilege delegates before checking fallback children
With Paimon's privilege layer enabled, PrivilegedCatalog.getTable wraps the FallbackReadFileStoreTable in PrivilegedFileStoreTable. This outer object fails the instanceof FallbackReadFileStoreTable check; its options() still exposes only the main child, while newScan() delegates to the wrapped fallback planner. Consequently an unsafe physical fallback value remains unvalidated despite the fix in the existing fallback thread. Please traverse the supported delegated wrapper (wrapped()) before recursing into fallback children and cover a privilege-wrapped fallback fixture.
| } | ||
|
|
||
| public void validateEffectiveDataTable(TableScanParams scanParams) { | ||
| Table dataTable = sourceTable.getBasePaimonTable(); |
Contributor
There was a problem hiding this comment.
[P1] Validate the exact data handle embedded in the system wrapper
sourceTable.getBasePaimonTable() comes from Doris' table cache, whereas getRawSysPaimonTable() loads the system wrapper separately through the Paimon catalog. If the source handle is still cached after Paimon's catalog cache expires/is disabled and the physical options change, the wrapper can contain a newer unsafe child while this method validates only the older safe handle. The wrapper's options() is empty, and $partitions later plans its private child directly, so the final generic guard cannot catch the mismatch. Please construct the wrapper from the exact effective source handle or expose and validate its actual child, and add a divergent-handle test.
| Table effectiveTable = PaimonScanParams.applyOptions(baseTable, resolvedOptions); | ||
| // The shared latest cache was built from the catalog-scoped handle. Relation options | ||
| // need their own projection so partition enumeration uses the final safe table copy. | ||
| return PaimonUtils.loadSnapshotProjection(this, effectiveTable); |
Contributor
There was a problem hiding this comment.
[P2] Keep reader-only OPTIONS on the memoized projection
This branch also handles read.batch-size and file-reader-async-threshold, neither of which changes the selected snapshot, schema, or partitions. Nevertheless every such relation calls the uncached projection loader, whose partitioned path runs CatalogUtils.listPartitionsFromFileSystem; the ordinary latest projection is memoized in PaimonTableCacheValue. On a high-partition table this turns a reader tuning option into a full metadata enumeration for every statement. Please reuse the memoized latest projection after validating the effective handle when the supplied options do not select startup metadata, and add a partitioned counting test.
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
2 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?
Issue Number: None
Related PR: None
Problem Summary:
The Paimon JNI read path had several correctness and operational guardrail gaps:
read.batch-sizeaccepted zero, negative, or excessively large values. In particular, zero can make the vectorized reader return without advancing input.scan.manifest.parallelism=0can block on a zero-permit semaphore, while a value above the CPU-sized default causes Paimon to replace a JVM-static executor and leaves the new pool visible to later queries.`region,code`was decoded as two fields, and nested STRUCT child names containing#,,, or:also collided with the type grammar.enable_file_scanner_v2=falsedid not emit the safe schema payload used by FileScannerV2.paimon.table-option.*accepted options which can select a different table context or affect writes after Doris has already bound the original schema.ThreadInfo[], although the Paimon async-reader pool is JVM-global.table@options(...)relation.How does this PR solve it?
read.batch-size:1..65536file-reader-async-threshold:1 MB..1 GBrelation @options> Doris catalog option > physical table option > Paimon default.read.batch-size=0orscan.manifest.parallelism=0from bypassing Doris-side safeguards.1..Runtime.availableProcessors(), preserving the JVM-global executor-capacity invariant.required_fields_base64andcolumns_types_base64schema payloads from both the legacy and V2 C++ readers. Every complete list item is independently encoded, and every nested STRUCT child name is encoded inside the type descriptor.required_fieldsandcolumns_typesparameters as a compatibility fallback. The Java scanner accepts only a complete encoded pair, so it cannot combine two framing versions accidentally.CatalogPropertybefore the live catalog is changed; connectors without this capability retain the existing tentative-validation path and rollback behavior.paimon.table-option.*entries. The raw persisted property is retained so image/edit-log compatibility is not destructive.For example, the following nested schema is now transported without colliding with either the outer list framing or the STRUCT grammar:
Example query-level tuning:
Two aliases of the same table may use different values in the same statement because each relation receives its own
Table.copy(...):Thread statistics microbenchmark
Dataset and method:
getAllThreadIds()andgetThreadInfo(ids, 0), then scans the returnedThreadInfo[].ThreadMXBean. A checksum blackhole consumes every returned count.This microbenchmark isolates statistics collection rather than scan I/O. It shows that the old cost grows with JVM thread count, while cache hits avoid both the full thread walk and its per-call allocation. The tradeoff is that this profile gauge can be up to one second stale; it does not affect query execution or scheduling.
Tests
Passed locally:
PaimonJniScannerTest: 15 tests, 0 failures, Maven build cache disabled.Added coverage:
Not run locally:
protocincluded). CI buildall and regression checks are requested on this PR.Release note
Paimon catalogs and relation
@optionsnow support boundedread.batch-sizeandfile-reader-async-thresholdreader tuning. Unsafe catalog, physical-table, and manifest-parallelism settings are rejected. Both Paimon scanner routes support quoted top-level and nested identifiers containing delimiter characters. Scanner DEBUG logs no longer include raw parameters, failed Paimon catalog validation is atomic, persisted legacy options remain loadable without being applied, and scanner statistics reuse a short-lived JVM-wide thread-count sample.Check List (For Author)
Review follow-up
The review fixes make empty identifiers and option precedence explicit:
$marker. Therefore an empty projection is encoded as the empty payload, while one empty field name is encoded as$; both the legacy and V2 C++ producers use this shared encoding.read.batch-size=0plus catalog1024, or plus relation@options(...=4096), is accepted with the safe final value; the same physical table without an override is still rejected. The same rule applies to manifest parallelism.{}versus{\"\"}unit coverage, scanner constructor coverage, final-boundary/no-override coverage, safe catalog/relation override coverage, and P0 queries through both scanner routes.PaimonJniScannerTestnow runs 15 tests with no failures. A direct Paimon 1.3.1 runtime probe also accepted the safe relation override and rejected the unsafe final value.Review follow-up: hidden planning handles
This update closes the four remaining pre-ScanNode planning gaps:
$partitionsexposes an empty option map while its private data table performs manifest planning. Doris now applies and validates relation options on that hidden data table, while type inspection remains non-planning so a safe override is not rejected prematurely.FallbackReadFileStoreTableis validated recursively because its main and fallback children plan independently.scan.manifest.parallelism=0is rejected before enumeration, while@options('scan.manifest.parallelism'='1')is preserved instead of being lost through a raw catalog reload.fetchRowCount()validates immediately beforenewScan().plan(); coverage exercises bothExternalRowCountCacheand manual analysis entry points.For example, given a partitioned table whose physical option is unsafe:
Added tests use real Paimon 1.3.1
PartitionsTable,FallbackReadFileStoreTable, and file-store table objects, plus a partitioned P0 fixture. FE checkstyle, Groovy compilation, and isolated Paimon fixture probes pass locally. Full FE unit tests and the external P0 environment remain delegated to CI because this checkout does not contain the installed third-party toolchain, includingprotoc.