[refactor](catalog) decouple external catalogs from FE core into loadable connector plugins - #64304
Merged
Conversation
morningman
requested review from
924060929,
CalvinKirs,
englefly,
gavinchou,
morrySnow,
mymeiyi and
starocean999
as code owners
June 9, 2026 09:50
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29382 ms |
Contributor
TPC-DS: Total hot run time: 169289 ms |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
morningman
force-pushed
the
branch-catalog-spi
branch
2 times, most recently
from
June 16, 2026 23:02
f09b6df to
e9c5b3e
Compare
morningman
requested review from
Gabriel39,
dataroaring and
liaoxin01
as code owners
June 20, 2026 13:02
Contributor
Author
|
run buildall |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 29277 ms |
Contributor
TPC-DS: Total hot run time: 174666 ms |
Contributor
ClickBench: Total hot run time: 25.17 s |
morningman
force-pushed
the
branch-catalog-spi
branch
3 times, most recently
from
July 3, 2026 13:03
291d192 to
494ec17
Compare
morningman
force-pushed
the
branch-catalog-spi
branch
from
July 7, 2026 09:35
a653478 to
3ba75b7
Compare
Contributor
Author
|
run buildall |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
|
PR approved by anyone and no changes requested. |
924060929
approved these changes
Jul 30, 2026
Gabriel39
approved these changes
Jul 30, 2026
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
Author
|
/review |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
morrySnow
approved these changes
Jul 30, 2026
liaoxin01
approved these changes
Jul 30, 2026
gavinchou
approved these changes
Jul 30, 2026
Contributor
FE Regression Coverage ReportIncrement line coverage |
12 tasks
![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. This cutover is broadly decomposed and its current architecture gates pass, but I found 12 blocking correctness, lifecycle, classloading, and compatibility issues in the inline comments. The PR merged while this review was running, so these are post-merge follow-up blockers against the reviewed head f1d6f1f.
Critical checkpoint conclusions:
- Goal and scope: the connector extraction/cutover is substantially implemented, but the 1,680-file transition is not safe as merged because production routes activate unresolved transaction and classloader boundaries.
- Data correctness and error handling: Iceberg v3 lineage names can become ambiguous real/synthetic columns; HUDI mixed-version requests and Trino commit failures can report success after failure; replay catches fatal JVM errors.
- Concurrency and lifecycle: the Iceberg async split pump loses the plugin TCCL, external-write rollback ownership is removed before failure cleanup, and Trino scan/handle transactions have broken ownership.
- Configuration and compatibility: Trino's untouched default no longer finds legacy plugin layouts; old-FE/new-BE HUDI metadata compatibility is broken; Paimon/Jindo/AWS plugin closures are incomplete or multiply owned.
- Parallel paths and FE/BE protocol: eager, partition-batch, streaming, write, and JNI/native paths were traced. The separate batch-callback suspicion has no current shipped resource owner, but the streaming and retained-HUDI branches above are real.
- Persistence, failover, and writes: legacy image/journal subtype fixtures are extensive and no additional mapping defect was found; the replay error boundary and failed-commit rollback ordering remain unsafe.
- Testing and observability: negative coverage is missing for these failure modes, and several paths silently return/log success. The review-only instructions prohibited builds and test execution. I ran
check-fe-connector-imports.shandcheck-fe-core-metadata-funnel.sh; both passed. - Performance and memory: no separate algorithmic regression was substantiated, but leaked Trino transactions and failed source cleanup are resource-lifecycle regressions.
User focus: no additional focus was provided, so the full PR was reviewed.
| LOG.debug("Plugin-driven transaction begun with SPI ConnectorTransaction: {}", txnId); | ||
| return txnId; | ||
| } | ||
|
|
||
| @Override | ||
| public void commit(long id) throws UserException { | ||
| PluginDrivenTransaction txn = transactions.remove(id); |
Contributor
There was a problem hiding this comment.
[P1] Keep the transaction reachable until commit failure cleanup runs
commit() removes the wrapper before calling the connector, and the wrapper also closes the connector transaction in finally. If that call throws, AbstractInsertExecutor invokes onFail() and calls rollback(txnId), but the map no longer contains the transaction, so rollback is a no-op. This is reachable in the current Hive connector: HiveConnectorTransaction.commit() calls finishInsertTable() before creating its self-rolling-back HmsCommitter, and explicitly relies on the engine's subsequent rollback to abort pending multipart uploads when that phase fails. Please retain the transaction through the failure path or roll it back inside commit() before closing and deregistering it.
| VLOG_CRITICAL << "MetaScanner::_fetch_metadata"; | ||
| TFetchSchemaTableDataRequest request; | ||
| switch (meta_scan_range.metadata_type) { | ||
| case TMetadataType::HUDI: |
Contributor
There was a problem hiding this comment.
[P1] Preserve the old-FE HUDI metadata request during rolling upgrades
TMetadataType.HUDI and TMetaScanRange.hudi_params remain on the wire as deprecated values, so an old FE can still send this request to a newly upgraded BE. With this switch arm removed, the retained enum falls into the default branch, which sets EOS and returns OK; hudi_meta() therefore reports a valid empty timeline instead of its rows. Please keep this request bridge for the mixed-version support window (or at least fail the retained enum explicitly) and cover the old-FE/new-BE path.
| catalogType, props, new DefaultConnectorContext(name, catalogId)); | ||
| } catch (RuntimeException | Error e) { |
Contributor
There was a problem hiding this comment.
[P1] Do not turn fatal JVM errors into degraded catalogs
The fallback is meant for legacy validation failures and a missing connector class, but catching Error also swallows OutOfMemoryError, StackOverflowError, ThreadDeath, AssertionError, and unrelated VM/linkage failures during replay. Continuing startup after those failures can leave FE in an unsafe state while misreporting the cause as one degraded catalog. Please catch only the expected runtime/configuration and narrowly selected linkage conditions (for example NoClassDefFoundError) and let fatal/invariant errors propagate.
| // Pull ranges with backpressure (needMoreSplit) and pump them one at a time, exactly like | ||
| // legacy doStartSplit. The bounded SplitAssignment queue throttles the lazy source so FE | ||
| // heap stays bounded for million-file scans. | ||
| while (splitAssignment.needMoreSplit() && source.hasNext()) { |
Contributor
There was a problem hiding this comment.
[P1] Keep the plugin TCCL pinned while consuming the lazy split source
The pin here covers only streamSplits() construction. The SPI explicitly requires heavy planning to be deferred until consumption, and Iceberg's source opens tasks.iterator() and maps file tasks inside hasNext(); its manifest-cache path can reach ManifestFiles.read(...) on this same engine pump thread. next() and close() also execute plugin/SDK code. All of those calls run after onPluginClassLoader has restored the app TCCL (Iceberg's separate worker-pool pin does not pin this engine thread), reintroducing the reflective/service-loading split-brain this helper is intended to prevent; a resulting LinkageError also bypasses catch (Exception). Please pin the full source lifecycle and route its failures through SplitAssignment.
| IcebergColumnChange change = toAddColumnChange(column); | ||
| try { | ||
| context.executeAuthenticated(() -> { | ||
| catalogOps.addColumn(iceHandle.getDbName(), iceHandle.getTableName(), change, position); |
Contributor
There was a problem hiding this comment.
[P1] Restore the v3 row-lineage name guard on schema mutations
These ALTER paths no longer apply the removed validateRowLineageColumnMutation check. On a v3 table, ADD COLUMN _row_id BIGINT (or renaming a field to _last_updated_sequence_number) can therefore commit a normal Iceberg field with an ordinary ID; buildTableSchema() then unconditionally appends Doris's synthetic field with the same name and its reserved ID. The resulting duplicate names make slot/handle binding ambiguous and can treat user data as the row-lineage passthrough field. Please restore the case-insensitive, format-version-aware guard across every top-level mutation route and retain the former negative coverage.
| Optional<ConnectorExpression> filter, | ||
| long limit) { | ||
| TrinoTableHandle trinoHandle = (TrinoTableHandle) handle; | ||
| public List<ConnectorScanRange> planScan(ConnectorSession session, ConnectorScanRequest request) { |
Contributor
There was a problem hiding this comment.
[P1] End every Trino scan transaction
This cutover makes this provider the production Trino scan path, but the transaction started below is never committed or rolled back. cleanupQuery() only closes the metadata query scope; the provider inherits the no-op releaseReadTransaction(), and BE can only close its own page source after consuming the serialized handle. Each SELECT therefore leaves connector transaction state behind, and a planning exception leaks it immediately. Please register the transaction by query ID, release it exactly once from the existing query-finish hook (with an immediate rollback on pre-registration/planning failure), and cover success, failure, and cancellation.
| dbName, tableName, trinoHandle, | ||
| columnHandleMapBuilder.buildOrThrow(), | ||
| columnMetadataMapBuilder.buildOrThrow())); | ||
| } finally { |
Contributor
There was a problem hiding this comment.
[P1] Keep Trino handles with the transaction that created them
The opaque table and column handles returned here were created under txn, but this finally commits that transaction before the TrinoTableHandle escapes. Later schema calls, pushdowns, and split planning open unrelated transactions and reuse the saved handles; connectors are not required to make their handles valid across transaction boundaries, and the removed legacy path kept metadata, handles, and transaction paired. Please perform discovery/refinement/split planning in one query-scoped transaction, or reacquire every handle under the scan transaction before using it.
| * connector's transaction manager. A release failure is logged, never rethrown, so it cannot mask | ||
| * the real exception a {@code finally} block runs after. | ||
| */ | ||
| private void releaseQuietly(io.trino.spi.connector.ConnectorTransactionHandle txn) { |
Contributor
There was a problem hiding this comment.
[P1] Propagate release failures when the metadata call succeeded
releaseQuietly() is also used after successful metadata calls, so a connector commit() failure is logged and the caller still receives a successful list/schema/handle result while the transaction may remain live. Avoiding a finally exception from masking a primary failure is useful only when a primary failure exists. Please propagate the release error on the success path; when already unwinding, preserve the original exception and attach/log the release failure as secondary.
| hadoop-aws 3.4.2 wires ApacheHttpClient and it is only test-scoped in s3's closure. Versions are | ||
| BOM-managed (awssdk ${awssdk.version}, matching fe-core). software.amazon.awssdk MUST stay | ||
| child-first (NOT in the parent-first allowlist) — that separate child copy of the SDK is the | ||
| whole point. (STS/assumed-role would need software.amazon.awssdk:sts added the same way.) --> |
Contributor
There was a problem hiding this comment.
[P1] Include STS in Paimon's child-first AWS closure
The supported s3.role_arn / AWS_ROLE_ARN path selects Hadoop's AssumedRoleCredentialProvider, but this isolated plugin bundles child copies of hadoop-aws, s3, sdk-core, and auth while omitting the STS module that this comment says AssumeRole needs. STS then falls back to the parent and links against parent SDK types while the provider uses child SDK types (or is simply absent in another distribution), so role-based Paimon S3 access fails at runtime. Please bundle the same BOM-aligned software.amazon.awssdk:sts closure used by the isolated Iceberg/S3 plugins and cover it through the isolated loader.
| @@ -253,10 +264,34 @@ private PluginHandle<F> loadFromPluginDir(Path pluginDir, ClassLoader parent, Cl | |||
| } | |||
| // Re-load and instantiate the factory class from the runtime classloader so that it | |||
| // has full access to lib/ classes (e.g. CosFileSystemProvider needs S3 classes). | |||
| Class<?> discoveredClass; | |||
| try { | |||
| discoveredClass = classLoader.loadClass(factoryClassName); | |||
Contributor
There was a problem hiding this comment.
[P1] Contain unloadable factories within their plugin directory
loadClass() runs before the API-version check, but this catch does not cover NoClassDefFoundError, UnsupportedClassVersionError, or other LinkageErrors. asSubclass() can also throw ClassCastException, and static initialization during construction can throw a linkage error. Since loadAll() catches only PluginLoadException, one incompatible plugin escapes the documented per-directory boundary and prevents all later plugins from loading. Please read admission metadata before defining the factory where possible and translate plugin-attributable linkage/type failures into the staged load failure while still allowing fatal VM errors to propagate.
This was referenced Jul 30, 2026
This was referenced Jul 30, 2026
morningman
added a commit
that referenced
this pull request
Jul 31, 2026
…y the harness (#66195) ### What problem does this PR solve? FE unit tests dominate CI wall time, and most of that time is not spent running test logic. Full attribution of one `fe-core` surefire window (4092s x 12 forks = **49,104 fork-seconds**, 86% of an 79.7-min build): | component | fork-seconds | share | nature | |---|---|---|---| | JVM lifecycle *between* classes (1219x) | 17,317 | 35.3% | fixed overhead, proportional to class count | | non-FE classes: real test execution | 13,259 | 27.0% | **real work** | | FE startup + teardown (304x) | 7,066 | 14.4% | fixed overhead, proportional to FE-starting classes | | FE classes: class loading before FE start | 4,463 | 9.1% | fixed overhead | | FE classes: real test execution | 3,797 | 7.7% | **real work** | | non-FE classes: class-level setup | 3,172 | 6.5% | fixed overhead | **Real work 17,055s (34.7%) vs fixed overhead 32,019s (65.2%).** Unattributed: 30s. The overhead is per *class*, so the only lever with real magnitude is **reducing class count by merging test classes**. Eliminating one class is worth **55.2 fork-seconds if it starts an FE** (304 such classes) versus **16.0 if it does not** (915 classes) — FE-starting classes are 3.5x more valuable to merge. ### What is changed? Six commits, in dependency order: 1. **Avoid a 10s heartbeat wait in every FE unit test class.** `Daemon.run()` runs one cycle before sleeping, so `HeartbeatMgr`'s first cycle completes before `createDorisCluster()` registers a backend; `checkBEHeartbeat()` then waits a full `heartbeat_interval_second` (default 10). Sets the interval to 1 before the `Env` singleton is created, and makes `checkBEHeartbeatStatus()` check-then-sleep at 20ms granularity instead of sleeping 1s first. Timeout budget unchanged. 2. **Merge 12 tiny nereids test classes into 3 suites.** 3. **Give `DistributeHintTest` assertions and cut its runtime by 95%.** It enumerated a large parameter space while asserting almost nothing; now it asserts the distribution actually chosen and covers the space at a sane size. 4. **Restore spied `Env` fields after every test method**, to stop unbounded spy nesting leaking across methods. 5. **Merge six command privilege tests into one suite.** 6. **Migrate 25 legacy `UtFrameUtils` test classes to `TestWithFeService`.** This one saves no time by itself — it is the precondition for merging, since only classes on the same harness can share an FE. ### Measurements (CI, build 1009445 unless noted) | change | measured | note | |---|---|---| | heartbeat fix | -3.7s/class, **within noise** | **not the lever** — see caveat below | | merge 12 -> 3 classes | 614.4s -> 168.5s | -72.6% | | `DistributeHintTest` | 458.7s -> 64.2s | -86% | | merge 6 -> 1 class | 302.8s -> 63.9s | -78.9%, 13 test cases unchanged | | migrate 25 classes | `DropTableTest` 4.483s -> 4.566s | no gain by design; enables merging | The three verified items save roughly **840 test-seconds** in total. **Caveat, stated plainly:** at ~7x effective parallelism, and with per-build noise of 2 sigma = 151s, that lands as only about **2 minutes of wall clock and is partly masked by noise**. Do not expect the total-seconds number in the build summary to move cleanly. The measurement method that does work is per-class comparison against an unaffected control group, not the build total. The heartbeat fix in commit 1 in particular looked large in local single-fork measurement and turned out to be noise-level on CI — it is kept because it is correct and harmless, not because it is a win. Full-suite result on build 1009445: **SUCCESS, 8773 passed, 0 failed, 1 muted.** The muted one is `ForwardToMasterTest.testAddBeDropBe` (ClassCastException), which is known upstream issue #66004 and unrelated to this branch. ### Rebase note Rebased onto `af6dcff9051` today. Two files conflicted with upstream, both against commit 6: - `FrontendServiceImplTest` — #64304 moved `MCTransaction` / `MaxComputeExternalCatalog` out of `fe-core`; upstream's `WriteBlockAllocatingTransaction` mock kept verbatim. - `DynamicPartitionTableTest` — #65219 added 1478 lines of TIMESTAMPTZ tests. Resolved by taking upstream's file whole and replaying the harness migration onto it, then diffing against upstream to confirm only harness lines moved. All 241 `Assert.*` calls left untouched. Post-rebase verification: `test-compile` clean, `checkstyle:check` clean, and the two conflicted classes run **69/69 passing, 0 skipped** — counts matching upstream's annotation counts exactly. ### A note for reviewers on how this was verified The JUnit4 -> JUnit5 migration in commit 6 has two failure modes that **pass silently** rather than erroring: - `@Rule ExpectedException` is ignored outright by JUnit5, turning six exception tests into tests that assert nothing. Converted to `ExceptionChecker.expectThrowsWithMsg`, which matches JUnit4 semantics (`isInstance` for subclasses, `contains` for messages). - JUnit4 `Assert.assertEquals(msg, expected, actual)` versus JUnit5 `Assertions.assertEquals(expected, actual, msg)` have **reversed argument order**. When all three arguments are Strings this compiles fine and silently compares the wrong things. 36 such call sites exist across the migrated classes; message-first overloads were identified by argument *count*, not by whether the first argument looks like a string. Because of this, `BUILD SUCCESS` is not sufficient evidence for these commits. Every migrated class was accepted only after its `Tests run:` count matched the pre-migration count exactly. Reviewers checking this PR should apply the same standard. ### Release note None ### Check List - [x] Test - [x] Regression test — not applicable, this changes only the FE unit test harness and test classes - [x] Unit Test — full FE UT suite green on CI (build 1009445: 8773 passed, 0 failed); post-rebase spot check 69/69 on the two conflicted classes - [x] Behavior changed: No (test-only, no production code paths altered) - [x] Does this need documentation? No
morningman
added a commit
that referenced
this pull request
Jul 31, 2026
…66303) ### What problem does this PR solve? Issue Number: close #xxx Related PR: #64304 Problem Summary: Follow-up cleanup for #64304. After external catalogs moved behind the connector plugin SPI, fe-core still declared a name for every data source it no longer knows anything about. This removes those names: - **`TableIf.TableType`**: the per-source `*_EXTERNAL_TABLE` constants (`HMS`, `ES`, `JDBC`, `ICEBERG`, `PAIMON`, `MAX_COMPUTE`, `HUDI`, `TRINO_CONNECTOR`, `LAKESOUl`) and the deprecated internal-catalog `ICEBERG` / `HUDI`. An external table served by a connector plugin is a `PLUGIN_EXTERNAL_TABLE`; the source's own name is answered by `PluginDrivenExternalCatalog#getDisplayEngineName`, never by a mapping held in fe-core. - **`TableFormatType`**: `hive`, `iceberg`, `hudi`, `paimon`, `max_compute`, `transactional_hive`, `lakesoul`, `trino_connector`. A connector names its own format string through `ConnectorScanRange#getTableFormatType()`, which `PluginDrivenScanNode` forwards to BE verbatim. Only `tvf` and `remote_doris` are still produced by fe-core itself. The enum is not persisted anywhere — it only builds the thrift string. - **`InitDatabaseLog.Type`**, and with it `ExternalDatabase#dbLogType`, a field that was assigned and never read. `OP_INIT_EXTERNAL_DB` has been ignored on replay since 4.0 and `EditLog#logInitExternalDb` has no callers, so nothing writes one any more; the payload class stays only so `JournalEntity` can still consume such an entry out of an old journal. - **`InitCatalogLog.Type.HUDI`**, which was never produced: hudi tables have always lived in an hms catalog and no `HudiExternalCatalog` ever existed. - **`Database#discardHudiTable`**, dead once `TableType.HUDI` is gone. The remaining `InitCatalogLog.Type` values are kept on purpose. That enum is also the type of the persisted `ExternalCatalog#logType` field, which `PluginDrivenExternalCatalog#gsonPostProcess` reads to backfill the catalog type for the resource-backed catalogs (`es`, `jdbc`) that never persisted one. Deleting a name there would make it deserialize to `null` and lose that catalog's type on upgrade. **Upgrade compatibility.** An image written before the cutover still carries the old `TableType` names. It stays readable because the persisted table class is remapped by `GsonUtils`' compatible-subtype registry, and the stale `type` string deserializes to `null` — GSON returns `null` for an enum name it does not know rather than throwing — which `PluginDrivenExternalTable#gsonPostProcess` then normalizes to `PLUGIN_EXTERNAL_TABLE`, the same normalization it already applied to a recognized legacy name. Nothing in the compiler enforces that two-step property, and a regression would surface not as a build error but as an FE replaying a persisted external table with a `null` type. So the new `LegacyExternalTableTypeReplayTest` pins it for every deleted name, deliberately spelled as string literals so the constants cannot come back just to keep a test compiling.
This was referenced Aug 3, 2026
morningman
pushed a commit
that referenced
this pull request
Aug 4, 2026
### What problem does this PR solve? Related PR: #64304 (catalog SPI) Related PR: #65126 Problem Summary: External metadata caching currently has parallel implementations in FE core and connector modules. Cache policy, statistics, invalidation, lifecycle, and concurrency behavior are therefore duplicated, while connector implementations still depend on FE-owned concepts. This PR introduces an implementation-free MetaCache SPI module and a shared Caffeine-backed runtime module. Common lifecycle, entry, registry, statistics, invalidation, and ID/name coordination are moved into these modules. FE core retains only FE-specific catalog routing, configuration injection, refresh orchestration, edit-log integration, and schema validation. Hive, HMS, Iceberg, Paimon, and MaxCompute consumers are migrated to the shared implementation. The refactor preserves the existing connector constructor policy and the guarded bulk-load contract. A guarded bulk publication participates in the same short generation protocol as manual miss loads, exact-key actions, and asynchronous refreshes, while REFRESH/flush invalidation continues to win without holding a publication monitor during external I/O.
morningman
added a commit
that referenced
this pull request
Aug 4, 2026
…ng (#66369) ### What problem does this PR solve? Related PR: #64304 (catalog SPI) Problem Summary: A connector never decides which columns to read — it renders whatever list `ConnectorScanRequest.getColumns()` carries. The jdbc connector turns that list verbatim into the remote `SELECT` list and falls back to `SELECT *` when it is empty (`JdbcQueryBuilder#buildQuery`). So column pruning for every plugin-driven external scan rests entirely on `PluginDrivenScanNode#buildColumnHandles()`, which intersects the connector's column handles with this scan's tuple slots. That method had **no direct coverage**, and its failure mode (projecting more columns than the query needs) is a pure performance regression that no result-comparing test can observe. The existing jdbc explain assertions all pass a column list and assert those same columns are present; none of them can fail on an over-wide projection they did not anticipate. This PR started as coverage for that gap. The new coverage immediately found a real bug, so it now carries the fix as well. **1. The bug: the connector's scan properties are computed before column pruning** A plugin-driven scan asks its connector for one property bundle — the jdbc remote `SELECT`, per-column dictionaries, file format, path partition keys — and caches it (`cachedPropertiesResult` / `scanNodeProperties`). That cache is first filled from `init()`: ``` PhysicalPlanTranslator#getPlanFragmentForPhysicalFileScan -> scanNode.init() -> FileQueryScanNode#doInitialize -> initSchemaParams -> getPathPartitionKeys() -> PluginDrivenScanNode#getPathPartitionKeys -> getOrLoadScanNodeProperties() ``` `init()` runs while the translator is still translating this scan — strictly **before** the project above it prunes the tuple down to the columns the query reads (`updateScanSlotsMaterialization`). Everything the connector derives from the projection at that point therefore describes the **full table schema**. The only thing that dropped the cache was `convertPredicate()`, and only when there was a conjunct to push down. Queries with a `WHERE` clause were rebuilt from the pruned tuple by accident; filter-less ones kept the pre-pruning bundle. Result: ```sql -- doris_test.test1 has 12 columns explain select count(*) from test1; -- before: QUERY: SELECT `k1`, `k2`, ..., `k12` FROM `doris_test`.`test1` -- after: QUERY: SELECT `k1` FROM `doris_test`.`test1` ``` This holds for **every** `WHERE`-less query on any plugin-driven external table, not just `count(*)`. The scan itself was not affected — `getSplits()` rebuilds the column handles from the final tuple, so the query actually sent to the source was already pruned. What was wrong is the reported remote query, and anything else a connector derives from the projection through this bundle (`populateScanLevelParams`, `getFileAttributes`). Reviewers of the iceberg connector may want to check the field-id dictionary applied in `IcebergScanPlanProvider#populateScanLevelParams`, which is built from the requested columns and, on a filter-less query, was built over the full schema. Fix: drop the cache in `doFinalize()`, the first point at which the tuple is final. Every filtered query already exercises this rebuild path today — including the second MVCC-snapshot / rewrite-scope pin it implies — so the filter-less path is only being moved onto an already-exercised path. Why it survived since the SPI migration: all 13 remote-query assertions in the tree filter, and a filter is exactly what used to hide this. The one filter-less assertion that exists (`test_gbase_jdbc_catalog`, commented out) expects the pruned single column, i.e. the behavior this restores. **2. The projection decision itself (`fe-core`)** `PluginDrivenScanNodeColumnPruningTest` drives the real `buildColumnHandles()` and pins: - only tuple-slot columns are projected (3-column table, 1 requested → exactly 1 handle); - the order follows the slot order, not the connector's handle-map order — the connector renders this list positionally; - slots with no backing column, and slots with no matching handle, are skipped rather than leaking into the list; - an empty tuple projects nothing — the sole input that reaches the connector's `SELECT *` fallback. Every assertion was mutation-checked against the production method: returning `allHandles.values()` kills 4 of the 5, and making the unmatched-slot path fail loud unconditionally kills the 5th. **3. Explain assertions (`external_table_p0`)** Two additions to `test_mysql_jdbc_catalog`, both of which fail without the fix above: - a filter-less projection (`select k8 from test1`) — the shape no existing assertion covered, and the most direct pin for the caching bug; - `count(*)`, the one shape whose projection would otherwise go empty. The engine keeps a single smallest slot (`PhysicalPlanTranslator#updateScanSlotsMaterialization`) instead of letting the tuple go empty, and an empty tuple is exactly what makes the jdbc connector emit `SELECT *`. The assertion pins that the remote select list stays one column wide and is not `*`; it deliberately does **not** pin which column wins, since that is `getSmallestSlot`'s business and tracks type widths.
hubgeter
added a commit
to hubgeter/doris
that referenced
this pull request
Aug 5, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#64304 Problem Summary: Snapshot-pinned Iceberg scans built the full field-id schema dictionary with lower-cased top-level names while scan slots and identity partition keys retained the source names. Mixed-case partition columns therefore missed the BE schema mapping and could abort the backend. Preserve Iceberg top-level names in full-schema dictionaries, return query errors for malformed mappings instead of aborting, and align Hudi partition carriers with the Hudi lower-case schema convention. ### Release note Fix mixed-case partition reads for Iceberg and Hudi external tables. ### Check List (For Author) - Test: Unit Test - FE Iceberg, Hudi, Paimon, and Hive connector unit tests: 278 passed after rebase - BE Iceberg reader unit tests: passed before rebase; latest-master rerun did not reach test execution because the updated BE dependency graph required a full rebuild - Behavior changed: Yes. Mixed-case external partition columns are matched consistently, and malformed schema mappings fail the query instead of terminating BE. - Does this need documentation: No
16 tasks
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
8 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: close #65185
Related PR: every phase PR is listed in #65185
Problem Summary:
This is the merge of the
branch-catalog-spifeature branch, i.e. the Catalog SPImigration tracked in #65185. Per-phase design, review history and the per-connector
status table live in that issue. The framework itself is documented in
fe/fe-connector/README.md(architecture, module map, how to add a connector) andfe/fe-connector/AGENTS.md(build/test recipes, architecture gates, invariants).Before: fe-core hard-coded every external data source under
fe/fe-core/.../datasource/{hive,iceberg,paimon,hudi,trinoconnector,maxcompute,...},reached from the catalog and planner layers through
switch-caseandinstanceof *ExternalTable. Adding or upgrading a source meant changing FE core, andevery source's client stack lived on the single FE classpath.
After: each source ships as a self-contained plugin under
fe/fe-connector/(hive, hms,iceberg, paimon, hudi, trino, maxcompute, jdbc, es), loaded child-first from
Config.connector_plugin_root. fe-core keeps only generic infrastructure — thePluginDriven*catalog / database / table / scan-node bridge plus one per-statementConnectorMetadatafunnel — and no compile-time knowledge of any specific source:CatalogFactoryroutes onConnectorProvider.getType()alone. Twovalidate-phasegates keep it that way: a connector may not import fe-core internals
(
build-support/check-fe-connector-imports.sh), and onlyPluginDrivenMetadatamaycall
Connector#getMetadata(build-support/check-fe-core-metadata-funnel.sh).Catalogs persisted by an older FE (
IcebergExternalCatalog,PaimonExternalDatabase,...) still deserialize and migrate transparently to the plugin-driven types.
Release note
External catalogs (hive, hms, iceberg, paimon, hudi, trino-connector, maxcompute, jdbc,
es) are now served by connector plugins, shipped as zips under
fe/plugins/connector/(
connector_plugin_root). Existing catalogs keep working across upgrade; no DDL changeis required.
The default of
trino_connector_plugin_dirmoves fromplugins/connectorstoplugins/trino_plugins; the old directory is still read as a fallback.The deprecated
lakesoulcatalog type is removed — creating or replaying one now failswith "Lakesoul catalog is no longer supported".
Check List (For Author)
Test
Covered by the connector modules' own unit tests, fe-core unit tests (including a
4.1.3 metadata upgrade-compatibility suite), the two gate self-test scripts under
build-support/tests/, and theexternal_table_p0/external_table_p2regressionsuites.
Behavior changed:
catalogs, the
trino_connector_plugin_dirdefault, and the removal of thelakesoulcatalog type. Everything else is held to behavior parity.Does this need documentation?
connector_plugin_root,the
trino_connector_plugin_dirdefault, and thelakesoulremoval.Check List (For Reviewer who merge this PR)