[fix](paimon) Read Paimon tables on BE without reconstructing the catalog metastore - #65867
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
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.
Automated review completed. I found no substantiated blocking issue in the current one-file
change.
Part 1.3 checkpoint conclusions:
- Goal and proof: this restores the unrelocated
HiveConfand shims classes needed when FE
serializes an HMS-backed Paimon table and the BE Paimon JNI scanner deserializes it. Both
legacy and Format V2 affected reads converge on that boundary. The PR reports rebuilt-artifact
smoke coverage fornew HiveConf(), theSerializableHiveConfround trip, andgetUser.
Existing HMS regression coverage exercises forced JNI, incremental, time-travel, and
system-table modes; the named branch product test was not run here. - Scope and clarity: this is a focused one-file dependency repair. Wildcard transitive pruning
keeps the addition to the two small Hive artifacts rather than restoring the former catalog
shade. - Concurrency and lifecycle: no threading, locking, mutable-state ownership, or resource
lifecycle changes. The existing process-lifetime scanner classloader behavior is unchanged. - Configuration and compatibility: no configuration, protocol, storage-format, persistence, or
serialized-wrapper contract changes. The Hive versions remain within the existing 3.1 family. - Parallel paths: legacy and Format V2 JNI modes use the same Paimon scanner artifact; native
and C++ reader paths do not cross this Java deserialization boundary, and other scanners are
isolated. - Tests and results: no result files changed. Visible CI style, license, and dependency checks
pass, but they did not execute the deployed Paimon fat jar. The review instructions prohibited
local builds/tests, so validation here is static plus the PR's reported manual artifact tests. - Observability, transactions, writes, FE-BE variables, and performance: no logging/metric,
transaction, data-write, or FE-BE variable behavior changes. The only identified cost is the
modest jar/startup footprint; no query hot path changes. - Risk closure: the wildcard-pruned external dependency exercised by
HiveConfis already
supplied by the supported BE parent classpath, and the direct Hive jars follow the existing
assembly, deployment, and parent-first loading path. I found no substantiated closure,
collision, or version issue.
User focus: no additional focus was provided.
Contributor
Author
|
run buildall |
Contributor
FE UT Coverage ReportIncrement line coverage `` 🎉 |
Contributor
TPC-H: Total hot run time: 29445 ms |
Contributor
TPC-DS: Total hot run time: 176734 ms |
Contributor
ClickBench: Total hot run time: 25.04 s |
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29714 ms |
Contributor
TPC-DS: Total hot run time: 176867 ms |
Contributor
ClickBench: Total hot run time: 24.88 s |
morningman
changed the title
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 23371 ms |
Contributor
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Jul 30th, 2026 7:58 AM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
TPC-DS: Total hot run time: 118007 ms |
Contributor
ClickBench: Total hot run time: 18.55 s |
Contributor
Author
|
/review |
yiguolei
previously approved these changes
Jul 24, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
|
PR approved by anyone and no changes requested. |
Contributor
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Jul 30th, 2026 7:58 AM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Jul 30th, 2026 7:58 AM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
Author
|
/review |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Request changes. Three distinct P1 issues remain on head dcc69322483f55d3bb385f2750f3df8db8e86cf0:
- Final INCR option reapplication deletes the catalog-visible snapshot pin for version-managed
$partitions, allowing BE to widen to filesystem-latest state. $filestransfers authorization only for a fallback table's main branch before rebuilding the fallback branch with an empty catalog environment.- System-table startup OPTIONS can be resolved on cache generation
Band then applied and serialized on the wrapper/base pair from generationA.
Part 1.3 checkpoint conclusions:
- Goal and proof: The patch coherently removes catalog/metastore loaders from ordinary BE table reconstruction and co-locates the Paimon FileIO SPI with its providers, but the three inline paths do not preserve the original snapshot, authorization, or generation semantics. Each was traced through Doris FE binding/serialization and the Paimon 1.3.1 BE-side reader path.
- Scope and clarity: All eight changed files and their related runtime paths were reviewed. The dependency move and archive guards are focused; loader removal necessarily crosses catalog snapshot, fallback, authorization, and cache-lifecycle boundaries.
- Concurrency: Volatile publication of
sysBaseTablebeforepaimonSysTableis ordered correctly, and no lock-order or deadlock issue survived. The concrete concurrency defect is the second refreshable cache lookup described inline. - Lifecycle: Per-plan system wrappers, FE split planning, Java table serialization, BE deserialization/reader creation, fallback dispatch, catalog publication/rollback, and process-lifetime extension classloaders were traced. No resource leak or reset defect was found beyond the reported state mismatches.
- Configuration: No new Doris setting is introduced. Existing version-management,
query.auth-enabled,scan.fallback-branch, relation OPTIONS/INCR, and optional native/JNI reader settings must retain their configured behavior; the first two findings show cases where they do not. - Compatibility: No storage format, persisted record, public symbol, Thrift field, serialized class version, or Paimon version changes. An old BE simply lacks this classloader fix until upgraded; no separate rolling-upgrade defect was substantiated.
- Parallel and special paths: Ordinary tables, all 17 Paimon system-table kinds, normal/OPTIONS/INCR relations, fixed versus base-shaped schemas, native/JNI paths, direct/decorated fallback tables, empty/latest/version-managed snapshots, extension ignore modes, and S3/Jindo providers were checked. Existing
$snapshots/$buckets,$filesmarker, fallback-bound, and packaging-test threads were not duplicated. - Conditions and errors: Null system-table loading, null catalog-visible latest state, explicit selector precedence, and unsupported option combinations were checked. The accepted failures can return too-new, unauthorized, or cross-generation metadata without an error, so ordinary exception handling does not contain them.
- Tests and results: The changed FE tests cover ordinary reconstruction, pinning, one-environment authorization, option reapplication, and fallback pairing, but not
INCR + partitions + version management, main-allowed/fallback-denied dispatch, or anA-then-Bcache barrier. No builds or tests were run because the supplied review contract prohibits them; assembled extension artifacts are absent, and the visible Build Extensions workflow only ran change detection. No result file changed. - Observability: Existing scanner errors and metrics are adequate for explicit failures, but they cannot detect successful semantic widening or missed authorization. No separate logging or metric issue was found.
- Transactions, persistence, and writes: Not applicable. This patch changes read planning, serialization, authorization transfer, and packaging; it adds no Doris transaction, EditLog, failover, or data-write path.
- FE/BE state transfer: No new protocol variable exists; relation options and the rebuilt Paimon table travel through the existing
serialized_tablepayload. That is why the deleted snapshot pin and frozen cross-generation selector persist to BE. - Performance: Reconstruction and authorization are per scan node, not per row. One additional fallback authorization call is correctness-required; no independent material CPU, memory, I/O, or hot-loop regression survived.
- User focus: No additional review focus was supplied, so the complete PR was reviewed without a narrower theme.
The multi-agent review converged in Round 2: both normal reviewers and the risk-focused reviewer returned NO_NEW_VALUABLE_FINDINGS for this exact three-comment set, wording, positions, live duplicate state, and head SHA. The final changed-file and unresolved-candidate sweep found no unclassified point.
| return rebuiltSysTable; | ||
| } | ||
| if (theScanParams.incrementalRead()) { | ||
| return rebuiltSysTable.copy(getIncrReadParams()); |
Contributor
There was a problem hiding this comment.
[P1] Keep incremental $partitions bounded by the catalog-visible snapshot
For a version-managed catalog, this method has just pinned the catalog-visible snapshot N, but getIncrReadParams() goes through isolateIncrementalRead(), which supplies scan.snapshot-id=null; Paimon's copy removes that pin. The supported start-only timestamp form also becomes incremental-between-timestamp=start,Long.MAX_VALUE. On BE, PartitionsRead calls newScan().listPartitionEntries(), so its incremental scanner now resolves the upper endpoint through the catalog-less SnapshotManager and can expose filesystem N+1 while REST/DLF still publishes N (or after rollback). Please resolve/cap the incremental endpoint while the catalog loader is present, or reject this composition if it cannot be represented safely, and add a catalog-N/filesystem-N+1 phase-barrier test.
Contributor
Author
There was a problem hiding this comment.
Confirmed and fixed in e692e97.
Verified the chain against paimon release-1.3.1, not taken on trust:
- The pin really is dropped:
PaimonScanParams#isolateIncrementalReadnulls every inherited
read-state key,scan.snapshot-idincluded, andcopy(...)removes a key that maps to null
(AbstractFileStoreTable#copyInternal:328-335). But it could not have bounded this scan either
way:AbstractDataTableScan#createStartingScanner(:204-286) switches onoptions.startupMode(),
andINCREMENTALroutes tocreateIncrementalStartingScanner, which never reads
options.scanSnapshotId(). - The timestamp form is the exposed one.
IncrementalDeltaStartingScanner#betweenTimestamps
(:168-185) turns both endpoints into snapshot ids through
SnapshotManager#earlierOrEqualTimeMills, whose binary search runs up tolatestSnapshotId()
(SnapshotManager:288), and falls back tolatestSnapshot().id()when the end resolves to
nothing;AbstractDataTableScan:407-425readslatestSnapshot()again for its empty-range guard.
All three go throughsnapshotLoaderwhen it exists (SnapshotManager:168-192) - the catalog
pointer - and list the snapshot directory once it does not. - Scope is exactly
$partitions:INCREMENTAL_SYSTEM_TABLESintersected with
resolvesSnapshotOnBackendis{partitions}.$audit_log/$binlog/$ro/$row_tracking
are planned on the FE and the BE only materializes theirDataSplits, and the snapshot-id form
(startSnapshotId/endSnapshotId) names its endpoints outright.
Fix: bindIncrementalRangeToCatalog resolves the range here, while the loader is still around, and
hands the BE the explicit incremental-between=startId,endId that betweenTimestamps would have
computed from the catalog's own view. Two things keep that faithful rather than approximate:
- Only the delta scanner's start rule is reachable -
incremental-between-scan-modeis cleared with
the rest of the read state and its defaultAUTOnever selects the diff scanner
(AbstractDataTableScan:427-436,toSnapshotScanMode). - The rewrite only fires when the requested end reaches the catalog's snapshot. An older end already
resolves to the same id on both sides, because every snapshot the catalog has not published yet is
younger than the one it points at - which also keeps theendTimestamp < earliest.timeMillis()
empty-range corner on Paimon's own path.
Tests: testIncrementalRangeIsResolvedOnTheCatalogVisibleSnapshot - catalog snapshot 42 at t=5000
with 43 already on the filesystem resolves to 20,42, plus the past-end and non-version-managed
no-ops - and testIncrementalPartitionsScanBindsItsRangeBeforeTheBackend, the catalog-N /
filesystem-N+1 phase barrier through getPaimonTableForBackend. The second one fails on the
previous code: earlierOrEqualTimeMills is never called, because nothing resolved the endpoint
before the BE.
| FallbackReadFileStoreTable fallbackReadTable = (FallbackReadFileStoreTable) undecorated; | ||
| return new FallbackReadFileStoreTable( | ||
| rebuildWithoutCatalogLoader(fallbackReadTable.wrapped()), | ||
| rebuildWithoutCatalogLoader(fallbackReadTable.fallback())); |
Contributor
There was a problem hiding this comment.
[P1] Authorize this fallback branch before erasing its catalog loader
authorizeDeferredScan() above invokes only dataTable.catalogEnvironment(), which a FallbackReadFileStoreTable delegates to its main branch. Paimon constructs this fallback branch with a separate branch-qualified identifier, and normal FallbackReadScan.plan() authorizes main and fallback plans independently. Rebuilding this branch with CatalogEnvironment.empty() makes the missed fallback check a permanent allow: a user allowed on main but denied on fallback can still read fallback $files metadata. This is reachable for a shared partition when a bucket/level predicate eliminates the main split and the fallback plan supplies it. Please authorize both captured branches with their own environments before dropping the loaders, and cover a main-allowed/fallback-denied dispatch.
Contributor
Author
There was a problem hiding this comment.
Confirmed and fixed in e692e97.
Verified against paimon release-1.3.1:
- The fallback branch does get its own environment:
FileStoreTableFactory#create(:107-123)
builds it withcatalogEnvironment.copy(new Identifier(db, table, fallbackBranch)), i.e. a
branch-qualified identifier. - Paimon does authorize the two branches independently:
FallbackReadFileStoreTable#newScan
(:184-187) isnew FallbackReadScan(wrapped.newScan(), fallback.newScan()), each
AbstractFileStoreTable#newScan(:268-275) passes its own
catalogEnvironment.tableQueryAuth(coreOptions())intoDataTableBatchScan, andplan()calls
authQuery()(DataTableBatchScan:93). - And the check really was lost:
DelegatedFileStoreTable#catalogEnvironment(:168-169) returns
wrapped.catalogEnvironment(), so the single call only ever reached the main branch, while
CatalogEnvironment.empty()#tableQueryAuthreturnsselect -> emptyList()
(CatalogEnvironment:145-148) - a permanent allow on the BE.
Fix: authorizeDeferredScan peels decorators and authorizes each captured branch with its own
environment and its own coreOptions(), since query-auth.enabled is per-branch too. Test
testFallbackBranchIsAuthorizedBeforeDroppingTheCatalogLoader asserts Catalog#authTableQuery is
called once per identifier - db.tbl and db.tbl$branch_fb. The fallback expectation fails on the
previous code.
| ); | ||
| sourceTable.makeSureInitialized(); | ||
| // System tables ignore snapshot semantics, so the empty snapshot resolves the base table. | ||
| Table baseTable = sourceTable.getPaimonTable(Optional.empty()); |
Contributor
There was a problem hiding this comment.
[P1] Resolve system-table OPTIONS on the base generation captured here
This publishes the system wrapper and sysBaseTable from generation A, but getSysPaimonTable(TableScanParams) later resolves startup OPTIONS through another sourceTable.getBasePaimonTable() lookup. Refresh-after-write or explicit invalidation can make that lookup return generation B; TableScanParams then freezes B's snapshot/tag and applies it to the wrapper over A, which is also what PaimonScanNode serializes. Across a drop/recreate or location change this can fail on a selector missing from A, or read the old incarnation when snapshot IDs collide. Please resolve OPTIONS against the captured base associated with this wrapper and add an A-then-B cache phase-barrier test.
Contributor
Author
There was a problem hiding this comment.
Fixed in e692e97, with one correction to the framing.
The split lookup is not something this PR introduced. getSysPaimonTable(TableScanParams) and its
sourceTable.getBasePaimonTable() resolution are untouched upstream code, and before this PR the
wrapper came from an independent catalog.getPaimonTable(nameMapping, "main", sysTableType) load
rather than from the meta cache - so the wrapper and the resolution base were already two different
sources, diverging across a strictly wider window. What this PR changed is that both now come from
the same cache, at two different times.
Worth closing anyway, and it is this PR's own thesis, so: the resolution now runs against the base
getSysPaimonTable() captured (getOptionsResolutionTable()). Only the catalog fallback - format /
object tables, which have no captured base - still looks the table up. The captured base is the
undecorated one, but every call resolveOptions makes on it (options(), snapshotManager(),
changelogManager(), coreOptions(), latestSnapshot(), copy()) delegates through Paimon's
decorators unchanged, so the resolved selector is identical.
Test testSystemTableOptionsResolveOnTheCapturedBaseGeneration: the captured generation is at
snapshot 1, the refreshed cache at snapshot 2, and @options('scan.mode'='latest') has to freeze
scan.snapshot-id=1. It resolves to 2 on the previous code.
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Request changes: the catalog-less BE table reconstruction still loses branch-local catalog state in two fallback paths.
Blocking findings
- The incremental
$partitions@incrtimestamp range is decided and resolved through the main branch only. The fallback branch can either read beyond its catalog-visible pointer or interpret the main branch's explicit snapshot IDs as its own history. - Reapplying resolved relation
OPTIONSafter both loaders are removed discards the fallback selector already resolved on FE and can make FE and BE use different fallback snapshots.
Both are P1 correctness issues, with branch-skew regression-test requirements in the inline comments.
Checkpoint conclusions
- Goal and scope: Moving the Paimon FileIO providers into the scanner artifact and removing the BE catalog loader directly address the reported classloading and metastore-reconstruction failures. All eight changed files are relevant and the implementation is localized, but fallback behavior prevents the end-to-end objective from being correct in every advertised branch, incremental, and OPTIONS mode.
- Correctness and lifecycle: The captured system/base table publication ordering, decorator peeling, five deferred-reader classifications, and branch-by-branch deferred authorization transfer are coherent. The two inline fallback defects remain blocking. Existing live threads on catalog-visible-empty /
$snapshots/$buckets, implicit fallback pin translation,$filesmarker skew, and the missing artifact-level classloader regression are intentionally not duplicated and are not cleared by this review. - Configuration, compatibility, and special paths: Ordinary single-branch, schema-evolution, authorization, rollback/publication, time-travel, fallback, incremental, and FE-planned versus BE-replanned paths were traced. No journal, persistence, transaction, wire-format, or user-configuration compatibility change was introduced. The relevant RBAC boundary is in-model; the deferred authorization transfer itself did not yield another new finding.
- Concurrency and performance: Volatile publication of the captured table generation is ordered correctly. The added work is planning-time metadata and authorization lookup, with no new hot-loop or execution-path performance blocker found.
- Tests and evidence: The added unit tests cover several generation, authorization, and snapshot cases, but not the two branch-skew cases above, and the existing packaging-level classloader-test concern remains live. This runner is review-only, so no build or tests were run; conclusions are based on static tracing and current GitHub CI state.
- Observability and writes: No new persistence/write protocol or logging/metrics contract applies.
- User focus: No additional review focus was provided; the complete PR was reviewed.
Three bounded review rounds converged on head e692e978351eedf61ad382ff38d37f9a06948029: all reviewers returned NO_NEW_VALUABLE_FINDINGS against this exact two-comment set. This review requests changes on the two new, distinct findings while retaining the existing unresolved threads.
| if (range == null || !dataTable.catalogEnvironment().supportsVersionManagement()) { | ||
| return incrementalParams; | ||
| } | ||
| SnapshotManager snapshotManager = dataTable.snapshotManager(); |
Contributor
There was a problem hiding this comment.
[P1] Bind this incremental range against each fallback branch
This helper makes both the rewrite decision and endpoint resolution through dataTable.snapshotManager(), which a FallbackReadFileStoreTable delegates to its main branch. If the requested end is older than the main latest, line 352 leaves the timestamp range unchanged even when it extends past the fallback catalog pointer; after loader removal, that branch can include an unpublished or rollback-retained snapshot. If this method does rewrite, Paimon's fallback option rewrite copies the resulting main incremental-between IDs unchanged because it translates only scan.snapshot-id, so disjoint branch histories fail out of range and overlapping IDs select the wrong commits. Please resolve/cap the range separately with both captured branch managers (or reject this composition), and test different branch IDs and catalog times.
| return rebuiltSysTable.copy(incrementalParams); | ||
| } | ||
| if (theScanParams.isOptions()) { | ||
| return PaimonScanParams.applyOptions(rebuiltSysTable, |
Contributor
There was a problem hiding this comment.
[P1] Preserve the fallback selector already resolved on FE
The processed FE wrapper has already applied this OPTIONS selector to the catalog-backed fallback pair, where Paimon maps the main snapshot to the fallback branch through that branch's catalog-visible SnapshotManager. Here the code rebuilds an unoptioned pair with CatalogEnvironment.empty() and applies only the main selector again, so FallbackReadFileStoreTable.copy repeats the translation against filesystem state. After fallback rollback/publication skew, FE can plan F7 while the serialized BE table selects retained F8; this also reaches OPTIONS-capable FE-planned system tables such as $ro. Please preserve each selected branch before removing its loader, and add an encode/decode fallback-skew OPTIONS test.
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Request changes remains warranted on reviewed head 0d83512fab416c1b4b53d7441ea7d876b140e521.
I found no new non-duplicate inline issue, so this review intentionally adds zero inline comments. The current head still does not preserve branch-local catalog state across the catalog-less FE-to-BE handoff:
- The fallback selector already resolved on FE is translated again after both branch loaders are removed.
- Fallback incremental ranges cannot be bounded by a main-only ID range or left for loaderless branch managers to resolve.
- The same catalog-bound limitation remains for the fallback snapshot pin and for metadata readers that ignore
scan.snapshot-id, including catalog-visible-empty state. - The classloader move is statically coherent, but the production parent/child artifact boundary still lacks the already-requested assembled-jar
FileIO.getregression.
Critical checkpoint conclusions:
- Task goal and proof: Removing the metastore loader and co-locating the Paimon FileIO SPI address the reported BE classloading failure on ordinary paths. The goal is not fully met for the advertised fallback/OPTIONS/incremental and catalog-bound metadata paths above. The added FE tests exercise generation capture, reconstruction, authorization, and selected system tables, but not those branch-skew cases or the assembled classloader boundary.
- Scope and minimality: The production changes are focused on packaging and table reconstruction. The helper boundaries are understandable, but correctness depends on a broad Paimon system-table/fallback lifecycle rather than only local serialization.
- Concurrency:
sysBaseTableis written under the same monitor before volatile publication ofpaimonSysTable, so readers acquire a matching wrapper/base generation. Scan-node table state is query-local. No new lock ordering or shared-collection race was found. - Lifecycle: Capturing the system wrapper and base together fixes refresh-generation skew, and rebuilding fallback branches independently preserves captured schemas. The unresolved lifecycle break is intentional loader removal before every branch-local selector/range has been frozen.
- Configuration: No Doris configuration item is added or changed; dynamic Paimon options remain relation-local copies.
- Compatibility and parallel paths: No thrift, persisted metadata, storage format, function symbol, or transaction protocol changes are introduced. I traced data tables, fixed-row and schema-derived system tables, native/JNI consumers, version-managed and plain catalogs, decorated/fallback tables, publication/rollback/empty states, and OPTIONS/incremental modes. The live threads above cover the remaining wrong parallel paths.
- Conditional checks: The five pin-capable BE-resolved system tables are classified correctly, and
$filesauthorization is transferred branch by branch. The fallback early return in incremental binding and the post-loader OPTIONS reapplication have concrete failure paths already documented in the live threads. - Tests and results: This was a review-only environment, so no build or test suite was run. Static shell parsing passed. Current CI shows CheckStyle and repository checks green, but the extension workflow did not exercise the assembled scanner/preload classloaders; that does not prove the runtime packaging contract.
- Authorization and error handling: Under the repository threat model, limited-user RBAC is in scope. The earlier fallback authorization loss is fixed: main and fallback
$filesbranches are authorized before their loaders are removed. No new silent error or distinct RBAC bypass was found. - Observability: The new build guard has specific error messages and the FE retains existing planning timing/error reporting. No additional hot-path metric or log is necessary.
- Transactions, persistence, writes, and FE-BE variables: Not applicable beyond the existing serialized-table handoff. There is no Doris write, EditLog, failover, or new FE-BE field.
- Performance and other invariants: Reconstruction, authorization, and snapshot binding occur once per scan initialization rather than per row or split. No distinct CPU, memory, transaction-visibility, or MoW invariant issue was found.
No additional user review focus was provided; the complete PR was reviewed.
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29374 ms |
Contributor
TPC-DS: Total hot run time: 177151 ms |
Contributor
ClickBench: Total hot run time: 24.96 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
8 tasks
… BE reads Reading an object-store-backed Paimon table over a REST/DLF catalog failed on BE when resolving the latest snapshot from the filesystem: NoClassDefFoundError: org/apache/paimon/fs/FileIOLoader The paimon OSS/S3 FileIO plugins (paimon-s3 -> S3Loader, paimon-jindo -> JindoLoader) were bundled in preload-extensions, which sits on BE's JVM system (app) classpath, but the org.apache.paimon.fs.FileIOLoader interface they implement lives in paimon-common, bundled only in paimon-scanner's own JniScannerClassLoader. When a BE read resolves the latest snapshot from the filesystem, RESTTokenFileIO.fileIO() calls FileIO.get() -> ServiceLoader.load(FileIOLoader.class, ...), which eagerly instantiates every registered provider. JniScannerClassLoader is a plain parent-first URLClassLoader, so the app classloader defines S3Loader/JindoLoader and cannot resolve the child-only FileIOLoader interface -> the whole discovery aborts. Co-locate the plugins with the FileIOLoader interface: move paimon-s3 / paimon-jindo from preload-extensions to paimon-scanner. paimon-scanner already carries a complete paimon runtime, and the assembly's metaInf-services handler merges the plugins' service files with paimon-common's, so all five FileIOLoader providers (local/hadoop/viewfs/s3/jindo) and the interface end up in one classloader. The Jindo SDK stays on the app classpath (start_be.sh adds jindofs to DORIS_CLASSPATH), so paimon-scanner still reaches it via parent delegation. Both jars must be rebuilt and redeployed: paimon-scanner (now carries the plugins) and preload-extensions (must stop advertising the un-linkable providers on the shared classpath). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DHwqsiGWNosZCeS4ecK97e
…ld time The FileIOLoader interface (paimon-common) and every provider implementing it must sit in the same jar: JniScannerClassLoader delegates parent-first, so a provider left on the shared preload-extensions (JVM app) classpath cannot resolve the child-only interface and ServiceLoader discovery aborts at runtime with "NoClassDefFoundError: FileIOLoader". Unit tests all run in one classloader and cannot reproduce that, so the invariant is asserted on the built artifacts instead: paimon-scanner's merged META-INF/services/org.apache.paimon.fs.FileIOLoader must list S3Loader and JindoLoader, and preload-extensions must bundle no org/apache/paimon/(s3|jindo) class at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…isting build.sh runs with `set -eo pipefail`. `unzip -l <jar> | grep -q` closes the pipe on the first match, so unzip dies of SIGPIPE and the pipeline reports 141 even though grep matched: the preload-extensions guard then took its false branch and let the forbidden provider through. The listing is 120k entries (11 MB) against a 64 KB pipe buffer, so any match outside the last few lines hits this - the guard failed exactly when a provider had leaked in, and stayed quiet on a clean jar, which is why the build never showed it. Let grep consume the whole listing instead. The paimon-scanner guard had a second, unrelated hole: `unzip -p` exits 11 when the services entry is missing altogether, which under `set -e` killed the build with "caution: filename not matched" before the guard could explain itself - its primary failure mode. Tolerate that exit and let the message print. Also record the fallback-branch limit of the BE-side snapshot pin: for a table with `scan.fallback-branch`, `FallbackReadFileStoreTable#copyWithoutTimeTravel` derives the fallback branch's own bound through `SnapshotManager#earlierOrEqualTimeMills`, and that branch has no catalog loader left either, so it resolves against the snapshot directory like the other metadata-only gaps already noted there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…alog metastore Every table loaded from a metastore-backed Paimon catalog (HMS / DLF) carries a Paimon HiveCatalogLoader in its CatalogEnvironment. The BE only reads - via FE-resolved splits and the object store - and never needs the catalog, yet deserializing that loader forces the whole Hive metastore stack onto the BE classpath: HiveConf, the metastore API, and, when a system table resolves its latest snapshot, even the metastore client (DLF's ProxyMetaStoreClient and its REST stack). PaimonScanPlanProvider now serializes a catalog-less table to the BE. A data table is rebuilt from fileIO / location / schema; a system table is rebuilt over a catalog-less base so SnapshotManager lists the snapshot directory instead of calling the metastore. Everything the loader used to do on the BE is done on the FE first, so dropping it changes no rows: - authorization: $files re-plans on the BE through DataTableScan#plan(), whose Catalog#authTableQuery becomes a permanent allow without the loader, so it is run here - per branch on a scan.fallback-branch pair, whose fallback branch carries its own CatalogEnvironment. - snapshot: for the five system tables that pick their snapshot inside the BE reader, the catalog-visible snapshot is pinned via copyWithoutTimeTravel, so a version-managed (REST / DLF) catalog cannot expose an unpublished or rollback-orphaned snapshot. Row-type-following tables ($audit_log, $ro, ...) are deliberately left unpinned: pinning would rewind the BE schema. - incremental range: @incr on $partitions resolves its timestamp endpoints against the catalog and hands the BE the explicit id range paimon would have computed, except on a fallback pair, where an id range cannot be expressed. - schema generation: getSysTableHandle builds the wrapper over the base Table its handle already carries and keeps that base on the sys handle, so the FE and the BE never plan on two different generations. The decorators paimon stacks on a loaded table are peeled down to the fallback pair, which must stay on top or $ro silently drops every fallback-only partition. - relation scan params: the @options / @incr copy the FE applied to the original wrapper is re-applied to the rebuilt one. With the FE no longer sending a catalog loader, paimon-scanner does not need the Hive artifacts on the BE side either; they are removed in the same change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29367 ms |
Contributor
TPC-DS: Total hot run time: 177636 ms |
Contributor
ClickBench: Total hot run time: 24.79 s |
Contributor
Author
|
/review |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
There was a problem hiding this comment.
Automated review result: request changes.
Review completion: COMPLETE. Three bounded review rounds converged on head 8db5036d2e2db28b10a0d519a2cfb3e1d82ff418 and this exact one-comment payload; all three Round 3 reviewers returned NO_NEW_VALUABLE_FINDINGS. No additional user-provided review focus was supplied, so all ten authoritative changed files were reviewed.
Blocking finding
- The eager fallback-branch authorization can reject a
$filesquery that main completely satisfies, even though Paimon would not invoke fallback's authorizedplan()or return fallback files. The inline P1 is distinct from the earlier missing-authorization thread, which applies when fallback does contribute.
Existing threads already cover the fallback OPTIONS/incremental branch-state defects, catalog-visible snapshot/marker limitations, and the missing assembled parent/child classloader regression; those issues were verified at this head and intentionally not duplicated.
Critical checkpoint conclusions
- Goal, scope, and proof: Removing catalog loaders from the serialized BE table and moving Paimon FileIO providers into the scanner artifact are focused changes that address the reported classloading/metastore reconstruction failure. The accepted P1 shows that loader-free authorization transfer is not yet behaviorally equivalent for every advertised fallback
$filespath. - Correctness, lifecycle, parallel paths, and conditions: Captured system wrapper/base generations, handle copies, decorator peeling, all 17 Paimon system readers, normal/OPTIONS/INCR paths, version-managed and plain catalogs, direct and privilege-decorated fallback tables, and native/JNI consumers were traced. The exact wrapper/base generation survives the production property path; the new unconditional fallback check is the only non-duplicate issue. All other substantiated snapshot/selector cases map to named live threads.
- Concurrency, configuration, and compatibility: No new shared-state race, lock-order issue, Doris setting, persisted format, storage format, public symbol, Thrift field, or rolling-upgrade contract was introduced. The compatibility impact is the user-visible false denial at the existing serialized-table boundary.
- Tests and results: The changed FE tests were reviewed statically and cover reconstruction, generation capture, pinning, and eager branch authorization, but not a main-complete/main-allowed/fallback-denied plan. No build or test suite was run because the authoritative review contract prohibits it and this worktree is not initialized; only
bash -n build.shwas run as a syntax check. The already-requested assembled-jar classloader test remains a separate live thread. - Errors and observability: Existing exceptions and build diagnostics are adequate for explicit failures, but this denial needs a behavioral test rather than a new metric or log. No additional observability defect was found.
- Transactions, persistence, writes, and variable propagation: Not applicable beyond the existing FE-to-BE serialized Java
Tablepayload; this patch adds no Doris transaction, EditLog/failover, data-write, or new variable-propagation path. - Performance and other invariants: The added reconstruction and authorization work is per scan initialization, not per row. No independent material CPU, memory, I/O, or hot-path regression was substantiated.
The final live-state sweep confirmed the PR head/base, normalized diff content, diff position 172, current review threads, and a fully classified ledger with no unresolved candidate.
| if (undecorated instanceof FallbackReadFileStoreTable) { | ||
| FallbackReadFileStoreTable fallbackReadTable = (FallbackReadFileStoreTable) undecorated; | ||
| authorizeBranch(fallbackReadTable.wrapped()); | ||
| authorizeBranch(fallbackReadTable.fallback()); |
Contributor
There was a problem hiding this comment.
[P1] Preserve conditional fallback authorization
FallbackReadScan.plan() always plans the main scan, but invokes fallbackScan.plan() only when listPartitions() finds partitions not completed by main; that listing itself does not authorize. This unconditional call therefore rejects a $files query for a user denied on the fallback branch even when every selected partition is satisfied by main and Paimon would never invoke fallback's authorized plan() or return fallback files. This is the inverse of the earlier missing-auth case: please transfer the fallback authorization with the same branch-use condition (while retaining the check when fallback contributes), and cover a main-complete/main-allowed/fallback-denied case.
CalvinKirs
approved these changes
Jul 31, 2026
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
4 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?
Reading a Paimon table over a metastore-backed or REST catalog (HMS / DLF) fails on
BE whenever the JNI reader is used — most visibly system tables (e.g.
$snapshots,$files), which always go through JNI, and also branch / time-travel / incrementalreads. Two failures were observed, both caused by the BE rebuilding a catalog it does
not need:
Root cause. A table loaded from a metastore-backed Paimon catalog carries a Paimon
CatalogLoader(e.g.HiveCatalogLoader) in itsCatalogEnvironment. When FEserializes that table to BE,
SnapshotManager#latestSnapshotIdresolves the latestsnapshot through the catalog's
SnapshotLoader, which on BE reconstructs the catalog'smetastore client and its whole Hive / DLF-REST stack — even though the BE only reads
(via FE-resolved splits and the object store) and the snapshots already live there.
This was previously masked by
java-udf's ~122MBhive-catalog-shadeon BE's sharedclasspath; #65733 replaced it with the slim
hive-udf-shade, exposing the dependency.Fix 1 — serialize a catalog-less table to the BE (
PaimonScanNode).FileStoreTableFactorywith an emptyCatalogEnvironment.A
FileStoreTableis fully defined by fileIO / location / schema, and its dynamicoptions (time travel, incremental) are already merged into the schema by
copy(...),so nothing is lost except the catalog loader.
SystemTableLoader.With no catalog loader,
SnapshotManager#latestSnapshotIdlists the snapshot directoryon the filesystem instead of calling the metastore, so the BE never reconstructs the
catalog and no longer needs any Hive / metastore classes.
Fix 2 — co-locate the Paimon FileIO plugins with
paimon-scanner(packaging).Reading the snapshot from the filesystem makes the BE materialize the object-store
FileIOlazily viaFileIO.get()→ServiceLoader.load(FileIOLoader.class, ...),which eagerly instantiates every registered provider. The OSS/S3 plugins
(
paimon-s3→S3Loader,paimon-jindo→JindoLoader) were bundled inpreload-extensions, on BE's JVM system (app) classpath, but theorg.apache.paimon.fs.FileIOLoaderinterface they implement ships inpaimon-common,bundled only in paimon-scanner's own
JniScannerClassLoader. That loader isparent-first, so the app classloader defines
S3Loader/JindoLoaderand cannotresolve the child-only
FileIOLoader→NoClassDefFoundError: FileIOLoader, whichaborts discovery. Moving the two plugins into
paimon-scannerputs the whole FileIOSPI (interface + all providers) in one classloader; the Jindo SDK stays on the app
classpath (
start_be.shaddsjindofstoDORIS_CLASSPATH) and is still reachablevia parent delegation.
Regression tests to re-run:
io.trino.tests.product.paimon.TestPaimonSparkCompatibility(system-table reads) andexternal_table_p2/paimon/test_paimon_dlf_rest_catalog.Release note
Fix Paimon reads (system tables, branch / time-travel / incremental) failing on BE over
metastore-backed or REST catalogs (HMS / DLF) with
NoClassDefFoundError(
HiveConf/FileIOLoader) orClassNotFoundException: ProxyMetaStoreClient.Check List (For Author)
Test
Behavior changed:
Does this need documentation?