[draft] Master catalog spi review 20 - #66071
Closed
morningman wants to merge 145 commits into
Closed
Conversation
morningman
requested review from
924060929,
CalvinKirs,
Gabriel39,
dataroaring,
englefly,
gavinchou,
liaoxin01,
luwei16,
morrySnow,
mymeiyi,
seawinde and
starocean999
as code owners
July 26, 2026 23:38
Contributor
Author
|
run buildall |
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
TPC-H: Total hot run time: 29336 ms |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-DS: Total hot run time: 178405 ms |
Contributor
ClickBench: Total hot run time: 25.58 s |
morningman
force-pushed
the
master-catalog-spi-review-20
branch
from
July 27, 2026 03:28
59e847b to
4e5cc47
Compare
Contributor
Author
|
run buildall |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 29663 ms |
Contributor
TPC-DS: Total hot run time: 176671 ms |
Contributor
ClickBench: Total hot run time: 25.58 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
This multi-month refactor needs persistent state for progress, decisions, risks, and cross-session agent handoff. Establishes a file-based tracking system including dashboard, ADR decision log, deviation log, risk register, per-stage task files, per-connector tracking, and an agent collaboration playbook covering context budget / subagent usage / handoff norms. Closes 18 design decisions (D-001..D-018) and registers 14 risks (R-001..R-014). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…T27) (apache#63582) ## Summary Lands the P0 SPI baseline for the catalog-SPI migration (master plan §3.1 / RFC §2.1), with zero impact on the already-migrated JDBC + ES connectors. - **Batch 0** (commits 1-2): SPI types + fe-core bridges — `ConnectorMetaInvalidator`, `ConnectorTransaction`, `ConnectorMvccSnapshot`, `ExternalMetaCacheInvalidator`, `ConnectorMvccSnapshotAdapter`, `PluginDrivenTransactionManager` generalization. - **Batch 1** (commit 3): DDL + Partition SPI — `ConnectorCreateTableRequest` + 4 spec POJOs, 4 new defaults on `ConnectorTableOps`, 3 new fields on `ConnectorPartitionInfo`, fe-core converter, `PluginDrivenExternalCatalog.createTable` routing. - **Batch 2** (commit 4): Import-gate + unit tests — `tools/check-connector-imports.sh` wired through exec-maven-plugin; `FakeConnectorPlugin` covering every default fall-through; routing tests for the invalidator; converter tests for all 4 partition styles + 2 bucket flavors. ## Commits - `[feat](connector) add P0 batch 0 SPI baseline: MetaInvalidator / Transaction / MvccSnapshot` (T03-T08) - `[feat](connector) wire P0 batch 0 SPI into fe-core` (T09-T12) - `[feat](connector) add P0 batch 1 SPI: CreateTableRequest + listPartitions` (T13-T20) - `[feat](connector) add P0 batch 2 gate + unit tests` (T21-T23, T26-T27) ## Test plan - [x] `mvn -pl fe-connector/fe-connector-api,fe-connector/fe-connector-spi -am compile` — SPI modules compile - [x] `mvn -pl fe-core -am compile -Dmaven.build.cache.enabled=false` — fe-core compile - [x] `mvn -pl fe-core checkstyle:check` — 0 violations - [x] `mvn -pl fe-connector validate` — import gate runs and passes (baseline clean) - [x] `mvn -pl fe-core -am test -Dtest='FakeConnectorPluginTest,ExternalMetaCacheInvalidatorTest,CreateTableInfoToConnectorRequestConverterTest,ConnectorPluginManagerTest,ConnectorSessionImplTest'` — 39/39 green - [x] `mvn -pl fe-connector/fe-connector-jdbc,fe-connector/fe-connector-es -am compile` — downstream connectors compile unchanged - [ ] JDBC regression-test suite (T24) — to be exercised by this PR's CI pipeline - [ ] ES regression-test suite (T25) — to be exercised by this PR's CI pipeline ## Tracking Full plan, decisions, and risk log live under `plan-doc/` in the repo (introduced by 6315983, already on the base branch). Per-task status: `plan-doc/tasks/P0-spi-foundation.md`. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…pache#63641) P1 batch A — close out scan-node SPI consolidation while keeping migration-period fallbacks in place. Three surgical changes route `PluginDrivenExternalTable` first in the nereids translator hot paths so already-migrated SPI connectors (JDBC, ES) take the SPI route, while the existing `instanceof XExternalTable` chains remain as fallbacks for connectors still pending migration (P3–P7). - **T3** — `PhysicalPlanTranslator.visitPhysicalFileScan`: move the existing `PluginDrivenExternalTable` branch from position 8 to position 1; the 7 connector-specific branches (HMS / Iceberg / Paimon / Trino / MaxCompute / LakeSoul / RemoteDoris) stay in place as migration-period fallbacks - **T4** — `PhysicalPlanTranslator.visitPhysicalHudiScan`: add a `PluginDrivenExternalTable` branch routed to `PluginDrivenScanNode.create(...)`, threading `tableSnapshot` + `scanParams` through `FileQueryScanNode` setters; `incrementalRelation` flagged as a P3 Hudi SPI extension TODO. The new branch is unreachable today (`PhysicalHudiScan` is only built for `HMSExternalTable + DLAType.HUDI`), so this is groundwork for P3 with zero current-day runtime impact - **T5** — `LogicalFileScan`: in `computeOutput()`, add a `PluginDrivenExternalTable` branch calling new helper `computePluginDrivenOutput()` — same shape as `computeIcebergOutput`, using `getFullSchema()` + virtualColumns; in `supportPruneNestedColumn()`, add an explicit `PluginDrivenExternalTable → false` branch. Both behaviorally equivalent for JDBC/ES today since they have no hidden cols and no virtualColumns P1 batch B (T1 — delete 13 legacy `Jdbc*Client` + `JdbcFieldSchema`) is deferred to P8 because the 3 fe-core callers — `PostgresResourceValidator`, `StreamingJobUtils`, `CdcStreamTableValuedFunction` — are live CDC streaming code that requires SPI extension for `getPrimaryKeys` / `getColumnsFromJdbc` / `listTables`, which is out of P1 surgical scope. Background and tracking docs live in `plan-doc/` (Master Plan §3.2 P1, tasks/P1-scan-node-cleanup.md, decisions log). - [x] `mvn -pl fe-core -am compile -Dmaven.build.cache.enabled=false` → BUILD SUCCESS - [x] `mvn -pl fe-core checkstyle:check` → 0 violations - [x] JDBC + ES regression-test passing — baseline established in P0 / PR apache#63582 - [ ] PR CI green on this PR - [ ] Manual scan-node smoke for an SPI connector — JDBC `SELECT *` should fall into the new `PluginDrivenExternalTable` branch first 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
…apache#64096) ### What problem does this PR solve? Related PR: apache#63582 (P0 — SPI baseline), apache#63641 (P1 — nereids plugin-driven routing) Problem Summary: This is **P2** of the catalog SPI migration and targets the `branch-catalog-spi` feature branch (continuing P0 apache#63582 and P1 apache#63641). It fully migrates `trino-connector` off the legacy in-tree `fe-core/datasource/trinoconnector/` implementation and onto the connector SPI module `fe-connector-trino`, making `trino-connector` the first connector to complete the SPI consumption playbook that later connectors will reuse as a template. All five batches land together so there is no intermediate state where a newly-created trino catalog cannot be serialized. **Batch A — complete the SPI surface (`fe-connector-trino` only, no fe-core changes)** - `TrinoConnectorProvider.validateProperties`: enforce the required `trino.connector.name` property at `CREATE CATALOG` time (ported from the legacy `checkProperties`). - `TrinoDorisConnector.preCreateValidation`: call `ensureInitialized()` so plugin loading + connector-factory resolution happen at catalog creation instead of being deferred to the first `SELECT`. - `TrinoConnectorDorisMetadata.applyFilter` / `applyProjection`: bridge Trino native filter/projection pushdown, reusing `TrinoPredicateConverter` to translate a Doris `ConnectorExpression` into a Trino `TupleDomain`. `remainingFilter` is conservatively returned as the original expression to match legacy behavior (conjuncts are not stripped; BE re-evaluates them). **Batch B — fe-core bridge for image compatibility** - `GsonUtils`: atomically replace the three legacy `registerSubtype` entries (`TrinoConnectorExternalCatalog` / `Database` / `Table`) with `registerCompatibleSubtype` redirects onto the `PluginDrivenExternal*` hierarchy. This must be atomic — `RuntimeTypeAdapterFactory` rejects duplicate labels, so keeping both bindings would throw at static init. Mirrors what ES/JDBC already did. - `PluginDrivenExternalCatalog.gsonPostProcess`: extract a `legacyLogTypeToCatalogType()` helper that maps `Type.TRINO_CONNECTOR` → `"trino-connector"`; the generic `name().toLowerCase()` would otherwise produce the wrong `"trino_connector"` (underscore) that `CatalogFactory` does not recognize. - `PluginDrivenExternalTable.getEngine()` / `getEngineTableTypeName()`: add `trino-connector` branches that preserve the legacy engine-name / table-type display across `SHOW TABLE STATUS` and `information_schema`. **Batch C — flip the switch** - Add `"trino-connector"` to `CatalogFactory.SPI_READY_TYPES` so catalog creation routes through the SPI path. **Batch D — remove legacy code** - Drop the `instanceof TrinoConnectorExternalTable` scan branch in `PhysicalPlanTranslator` (the `PluginDrivenExternalTable` SPI branch already handles it). - Drop `case "trino-connector"` in `CatalogFactory`. - Delete `fe-core/datasource/trinoconnector/` (10 files) and the now-dead legacy `TrinoConnectorPredicateTest`. - Route the `TRINO_CONNECTOR` db-build case in `ExternalCatalog` to `PluginDrivenExternalDatabase` (mirrors the migrated JDBC case). - **Retained for image compatibility**: the `InitCatalogLog.Type.TRINO_CONNECTOR` and `TableType.TRINO_CONNECTOR_EXTERNAL_TABLE` enums, the GsonUtils redirects, and the `MetastoreProperties` trino-connector entry. **Batch E — tests + tracking docs** - 29 JUnit 5 unit tests over the plugin-free converters: - `TrinoPredicateConverterTest` — `ConnectorExpression` pushdown trees → Trino `TupleDomain` (EQ / range / NE / IN / IS [NOT] NULL / AND / OR, Slice encoding), plus graceful degradation to `TupleDomain.all()` on null/unsupported input. - `TrinoTypeMappingTest` — Trino SPI type → Doris `ConnectorType` (scalars, decimal precision/scale, timestamp precision clamp, array/map/struct, unsupported-type failure). - `TrinoConnectorProviderTest` — `validateProperties` fast-fails when `trino.connector.name` is missing/empty. - No Trino plugin/cluster required; plugin-dependent paths remain covered by the existing `external_table_p0/p2` `trino_connector` regression suites. - Sync the migration tracking docs under `plan-doc/` (already carried on this feature branch since P0). **Net effect**: 28 files, +1025 / −2681 (~1656 LOC net removed). Old FE images holding legacy trino catalogs / databases / tables deserialize onto the `PluginDrivenExternal*` hierarchy through the GsonUtils string-name redirect, with engine-name display preserved. **Deferred (follow-ups, not in this PR)**: - `trino_connector_migration_compat` regression test (old-image deserialization) — requires a running cluster + Trino plugin + docker, unavailable in this dev environment; tracked as a CI/cluster follow-up. - The plugin-install documentation update lives in the `doris-website` repo and is handled separately. ### Release note None ### Check List (For Author) - Test - [x] Unit Test — 29 new tests in `fe-connector-trino` (predicate converter / type mapping / property validation). - [ ] Regression test — existing `trino_connector` suites cover plugin paths; the new old-image compat regression is deferred to a CI/cluster follow-up. - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason - Behavior changed: - [x] No. Internal routing moves from the legacy fe-core path to the SPI path; image compatibility, engine-name display, and pushdown semantics all mirror the legacy behavior. All batches land together, so there is no serialization-gap window. - Does this need documentation? - [x] Yes. The trino-connector plugin-install doc update is a follow-up in the `doris-website` repo. ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label
…tch design (hybrid, T02-T08) (apache#64143) ## Proposed changes testing with apache#64146 P3 of the catalog-SPI migration (base: `branch-catalog-spi`). Migrates the **hudi** connector following the **hybrid** strategy (D-019): harden the dormant HMS-over-SPI hudi connector to correctness parity, build a test baseline, and write the per-table dispatch design — **all behind the closed gate** (`SPI_READY_TYPES` unchanged). >⚠️ **No user-visible behavior change.** The SPI hudi path stays dormant (gate closed); hudi queries continue to use the legacy `HMSExternalTable.dlaType=HUDI` path. This PR removes correctness blockers ahead of the live cutover (deferred to P7 / batch E). ### What's included **Correctness fixes (hardening dormant code, behind gate):** - **T02** — fix hudi JNI `column_types` double bug: emit full Hive type strings (was Doris bare type names, losing precision/scale/subtypes) and send `column_names`/`column_types`/`delta_logs` as typed lists end-to-end (was comma join/split, which shattered `decimal(10,2)` / `struct<...>`). Matches the BE `hudi_jni_reader.cpp` contract (names `,` / types `#` / delta `,`). - **T04** — fail loud on time-travel / incremental read in the SPI `visitPhysicalHudiScan` branch (was silently returning the latest snapshot / silently full-scanning). - **T05** — real EQ/IN partition pruning in `HudiConnectorMetadata.applyFilter` (was a placeholder that ignored predicates and unconditionally switched the partition source from Hudi-metadata to HMS); faithfully mirrors `HiveConnectorMetadata.applyFilter`. - **T07** — column-name casing fix in `avroSchemaToColumns` (top-level lowercase, mirroring legacy `HMSExternalTable`). **Test baseline (all three connector modules started P3 with 0 tests):** - `fe-connector-hudi` (33): type-mapping / schema-parity (COW/MOR golden) / table-type / partition-pruning / scan-range. - `fe-connector-hms` (12): shared Hive-type-string parser tests. - `fe-connector-hive` (14): file-format / partition-pruning (mirrors T05). - COW/MOR schema is **type-agnostic** (golden parity vs legacy `initHudiSchema`); table type only affects scan planning. **Decisions / design (code-grounded, design-only):** - **T03** — defer `schema_id`/`history_schema_info` field-id evolution to batch E (DV-006; not a model-agnostic SPI fix). - **T06** — keep MVCC/snapshot SPI defaults (opt-out) + document (DV-007). - **T08** — `tableFormatType` dispatch design memo + **D-020**: single `hms` catalog per-table routing via a new backward-compatible `ConnectorMetadata.getScanPlanProvider(handle)` (per-table provider seam); refines D-005. The keystone gap is split into M1 (identity consumption, fe-core reads `tableFormatType` as an opaque string) and M2 (scan routing). ### Deferred to batch E / P7 (not in this PR) Gate flip (`SPI_READY_TYPES += hms/hudi`), fe-core `tableFormatType` consumption (M1+M2 implementation), live cutover, delete legacy `datasource/hudi/`, full incremental/time-travel/MVCC, Iceberg-on-hms via SPI (needs P6 `IcebergScanPlanProvider`), cluster/runtime validation. ### Verification Per task tracking, each code batch landed with: per-module compile + checkstyle 0 (incl. test sources) + connector import-gate pass + new unit tests green. The two most recent commits are docs-only (`plan-doc/`); the code is unchanged since the last green batch. Gate stays closed → the dormant SPI path is unreachable at runtime → zero live-path risk. CI re-verifies. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…off got wrong Two of the three blockers the previous handoff left for this step did not survive contact with the code: the fake-provider isolation it called unsolved is an existing pattern in CatalogFactoryPluginRoutingTest, and the remote-Doris baseline it listed as pinned goes through a legacy fe-core table class this change never touches. The one real cost was not the provider scan it worried about but getProperties() copying the whole property map on every call, inside a per-table loop. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…es their own context ConnectorContext carried eighteen methods, eleven of which were storage and backend concerns: credential normalization, URI normalization, the engine filesystem, broker addresses, the backend probe, managed-location cleanup. Their javadoc alone was two thirds of the file. Most connectors have no storage at all -- jdbc, Elasticsearch, MaxCompute and trino-connector never call one of them -- so every author of one read eleven services that could not apply to them. Move those eleven, javadoc unchanged, onto ConnectorStorageContext, reached through a single getStorageContext(). Nothing changes at runtime: the defaults move with the methods, so a connector that reaches a service the engine does not provide still gets the same benign answer, and the getter answers NOOP rather than null so no connector has to check. The engine implements both halves on one object and hands itself back. Splitting it into two would move a filesystem cache that has a lock and a shutdown flag tied to close(), for no gain the SPI can see. The decorator base class now forwards one getter instead of eleven methods, so a storage service added tomorrow cannot be lost there however many exist -- which is exactly the failure that had already happened twice with hand-written pass-throughs. That holds only while no storage method runs plugin code (none does; they execute entirely engine-side, which is why the plugin-pinning decorators never wrapped them). A future one that did would need those decorators to wrap the storage context itself; both the base class and the new interface say so. Deleting the methods from ConnectorContext makes the migration compiler-enforced: every test double that overrode one failed to compile until it was moved across. The plugin packages must be rebuilt and redeployed with the FE. A mixed deployment resolves these methods against an interface that no longer declares them, which surfaces as a runtime AbstractMethodError rather than a refusal to start. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…checks, and scope its contract The hook a connector routes an outbound address through was called sanitizeJdbcUrl, on an interface that otherwise names no protocol, and its engine implementation is a generic outbound-address check that knows nothing about JDBC. Its contract was also wider than anything can deliver. It read "connectors MUST call this method before using any JDBC URL to establish a database connection", which reads as a guarantee that every outbound address in FE is checked. Exactly one connector can honor it: the JDBC one, which opens its own connection. An Iceberg JDBC metastore and a Paimon JDBC catalog hand the user's address to a third-party SDK that connects internally, where the connector has no hook point at all. That is not something this migration lost -- the same two paths connect the same way upstream -- so the fix is to say what the hook covers, not to claim more. Renamed to sanitizeOutboundUrl and the contract now scopes itself to where a connector connects. Closing the two SDK paths is a separate piece of work: it means reaching inside those SDKs' connection setup, and doing it here would forfeit this change's premise of no behavior change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… rating The task doc rated this the highest-risk item in the line because the interface could not move without both classloader-pinning decorators moving with it. That was written before the forwarding base class landed; those decorators now override one method between them and touched nothing here. Also records what did NOT get run: the plugin-package redeploy smoke test the task doc names as its core gate needs a cluster, so the whole seven-item list is still owed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…imate no engine ever asked for estimateScanRangeCount promised the engine would use it "to pre-allocate resources or decide scan parallelism before calling planScan". No engine code has ever called it: the only override in the tree is jdbc's, which returns the constant 1. A dead SPI method is not free. It is one more thing a new connector author reads, weighs and possibly implements, only to find that nothing consults the answer -- the exact "I implemented it, why did nothing happen" tax this cleanup line exists to remove. Repo-wide grep for the symbol is now zero. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rfaces left to guesswork Three places where the interface says something a connector author would act on, and the implementations say something else: 1. releaseReadTransaction releases state opened by a different call, while Connector states that scan plan providers are built fresh per acquisition. Both are true; what was missing is where the state may live. It belongs to the connector, keyed by query id -- which is what hive already does, and which a provider field would silently break. 2. ConnectorScanRange.getLength() said "the number of bytes to read". It is bytes for the file-backed connectors, a ROW count for MaxCompute's row-offset splits, and -1 for MaxCompute's default and Paimon's JNI ranges. The unit is connector-defined; the engine must not read it as a size, which is why size-based features are capability-gated. 3. ConnectorSession.getProperty searches session variables first and catalog properties second, silently. Fine for a knob a catalog defaults and a user overrides; wrong for anything where the origin matters, and the caller could not tell which side answered. Say so, and name the two accessors that read one namespace only. Documentation only; no behavior change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…riants a real positive sample ConnectorContractValidator's own javadoc admitted the gap: hive is the only connector that declares requiresPartitionHashWrite, and hive's tests did not call the validator, so invariants #4 (hash write implies parallel write AND full-schema write order) and #5 (the two partition-distribution arms are mutually exclusive) were exercised solely by fe-core's fake connector. A fake proves the validator rejects a bad declaration. Only a real connector proves the shipped declaration is a good one -- and hive's is the declaration the sink actually depends on. The provider is built directly (its constructor is pure field assignment) with a null client: the validator reads only declared traits, none of which touch the metastore. The no-arg getWritePlanProvider() is avoided because it builds a real HmsClient, whose Hadoop stack is absent from connector unit tests. Every validator invariant now has a positive sample on a real connector: maxcompute for the local-sort arm, hive for the hash arm and their mutual exclusion. Coverage paragraph updated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… what it carries ConnectorWriteHandle.getWriteContext() was once envisioned as a free-form bag. Nothing ever put anything else in it: the sole producer is PluginDrivenInsertCommandContext .getStaticPartitionSpec, and hive/iceberg/maxcompute all consume it as the static partition spec (iceberg ships it verbatim as TDataSink.static_partition_values). The two sides of one value were spelled differently across the SPI boundary, and its own javadoc had to spend a sentence apologising for the name. Rename it to getStaticPartitionSpec(). Both sides now agree, and the javadoc states the contract instead of the history. A future free-form channel would be a new method, not a re-widening of this one. Mechanical rename; the compiler covers every call site. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…or-neutral name QueryTableValueFunction routes EVERY catalog declaring SUPPORTS_PASSTHROUGH_QUERY to one class, and that class was called JdbcQueryTableValueFunction -- named after the only connector that declares the capability today, though nothing in it is jdbc-specific: it reaches the connector only through the SPI. Rename it to PluginDrivenQueryTableValueFunction, matching the PluginDriven* family it belongs to, and say in its javadoc that a second connector declaring the capability needs no new class. This is the same source-name-in-fe-core cleanup the engine-name batch did, one class it did not reach. Rename only; the factory is the single reference. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… switch supportsCreateDatabase() could only restate whether createDatabase() was overridden -- the four connectors declaring true were exactly the four implementing it -- yet the two were kept in step by hand, and getting them out of step had no compile error and no failing test. A connector that implemented createDatabase but forgot the boolean would skip the remote existence precheck, so CREATE DATABASE IF NOT EXISTS on a database that exists only remotely would fail with the source's "already exists" instead of no-opping. This is the "one question, two overridable answers" shape Connector's own javadoc forbids. Delete the switch and ask databaseExists unconditionally, which is what Trino's CreateSchemaTask does: IF NOT EXISTS means "ensure it is there", so a connector that cannot create databases but reports this one as present has already satisfied the request. Behavior change, deliberate and signed off: on jdbc/es/trino/hudi, CREATE DATABASE IF NOT EXISTS <a database that exists remotely> now succeeds silently instead of reporting "CREATE DATABASE not supported". A connector answering neither question keeps the databaseExists() == false default and still falls through to that refusal, unchanged. The fe-core routing test now pins both of those, replacing the two that pinned the old gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rate, not inherited
supportsCastPredicatePushdown defaults to true -- the one opt-OUT switch in a module whose
rule is that capabilities default to false -- and its own javadoc explains why that is
dangerous: the engine unwraps the CAST before the connector sees it, so a connector that
turns the comparison into source-side filtering under different coercion rules UNDER-returns
rows, and BE cannot recover data that was never scanned. jdbc, paimon and maxcompute opt out.
The rest simply inherited true, with no record that anyone had weighed it.
Owner decision: keep the value (flipping it is a cross-connector performance change), but
make every connector's answer deliberate. Recon shows the switch is not uniform:
- iceberg converts the residual predicate and hands it to the TableScan, pruning
manifests and data files at the source;
- es compiles it into the ES query DSL, evaluated by Elasticsearch;
- trino turns it into a Constraint for the embedded connector's own applyFilter.
These three now state true at their own metadata class, each recording what it does with
the predicate and that true is an accepted risk (parity with the pre-migration scan nodes),
not a claim of safety.
- hive and hudi ignore the residual filter entirely in both planScan and
getScanNodeProperties, so the switch is INERT for them; the predicate they do consume
arrives via applyFilter, which the engine never gates on this method.
An override there would be dead code implying otherwise, so the SPI javadoc records the
whole map instead, and tells a new connector to start from false.
No behavior change: every value stated equals the value inherited.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tional interface executeStmt and getColumnsFromQuery were the last two methods ConnectorTableOps declared itself, and its own comment said what they are: the SQL-dialect escape hatch of one connector family, not a table operation, "slated to become their own optional interface". Every connector inherited them and had to work out that they were none of its business. Move both to ConnectorPassthroughSqlOps, which a connector implements or does not. ConnectorTableOps is now purely the aggregate of its six domains. The capability flag goes with them. SUPPORTS_PASSTHROUGH_QUERY was a second overridable answer to "can this connector run my SQL" -- a connector could declare it and implement nothing, or implement getColumnsFromQuery and stay unadmitted, with no compile error either way. Implementing the interface is now the whole declaration, and the two entry points (query() TVF, CALL EXECUTE_STMT) type-check the metadata instead of reading a flag. Resolving the metadata to type-check it costs nothing extra: the TVF resolves the same per-statement instance moments later, and PluginDrivenMetadata memoizes it. No behavior change: jdbc is admitted exactly as before, every other connector refused exactly as before. jdbc and hive tests now pin the declaration by its real shape. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ot wrong Eight of the ten items the earlier review registered are now closed. The registry's cost column was wrong in both directions: the hive contract sample and the read-transaction contradiction were logged as medium and were one test and one paragraph, while "five connectors inherit the CAST-pushdown default" was simply not true -- two of the five consume no residual filter at all, so the switch is dead for them. Also records the one process trap this batch hit: git commit takes the whole index, so a stale staged rename from an earlier git mv rode along into the first commit and left it non-compiling until the four commits were redone. The remaining item (planScan -> one method, one request object) is specified from this round's recon: 14 connector overrides, ~6 test doubles, two engine call sites. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
planScan was a chain of four overloads, each added when a new signal arrived and each delegating to the shorter one before it. Only the shortest was abstract. A connector author reading the interface implemented that one -- the obvious thing to do -- and silently lost the row limit, the pruned partition set and the COUNT(*) pushdown signal: the scan still worked, just slower, with no error, no warning and nothing to grep for. That is precisely the "I implemented it, why did nothing happen" tax this cleanup line exists to remove, and the chain guaranteed it would recur with the next signal. Replace all four with one abstract method taking ConnectorScanRequest, which carries the table handle, columns, filter, limit, required partitions and the count-pushdown flag, each with a default meaning "the engine is not asking for anything special". A signal added later becomes a field, not a fifth overload, and no connector can silently miss it. Trino reaches the same place from the other side: one getSplits, with the pushed-down facts folded into the handle beforehand. planScanForPartitionBatch takes the same request plus its batch; the default re-scopes via withRequiredPartitions instead of hand-copying five arguments -- the copy that would silently drop a field is now one method with a test on it. Fourteen connector overrides collapse to eight (one per connector, plus hive's batch override): jdbc/maxcompute/trino lose the pure-delegation arms entirely, paimon and iceberg fold their count-pushdown entry into their single one. No behavior change -- every connector receives exactly the values it received before, and the engine passes what it passed before, including the batched path's "no limit, no count pushdown". Verification: full-reactor test-compile BUILD SUCCESS; 402 tests across 26 scan-related classes green; ten modules 0 checkstyle violations; 2 mutations caught (withRequiredPartitions dropping the filter, and the batch default forgetting to re-scope -- the two ways this could have failed silently). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The last registered open item is done, so this records what changed for a connector author (one planScan, one request object, new signals become fields) and what is left: only the backlogged end-to-end runs and the plugin-package redeploy smoke, both of which need a cluster. Also notes the two follow-ups this round deliberately did not take -- streamSplits and the scan-node property methods still take positional arguments, which is fine because neither has an overload chain -- and one pre-existing gap it surfaced: no fe-core test pins the engine's count-pushdown wiring, so dropping it would silently de-optimize COUNT(*). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… test with the planScan request object Rebase onto `1aa5ae9597e` reported zero textual conflicts and still produced a broken tree -- the exact failure mode a "clean" rebase hides. Upstream's `port apache#66008's paimon-cpp removal to the paimon connector scan path` added `PaimonScanPlanProviderTest.cppReaderSessionFlagNoLongerChangesThePlan`, which pins that `enable_paimon_cpp_reader` no longer changes the plan (the flag is `fuzzy = true` and three upstream suites set it explicitly, so a PAIMON_CPP range would hard-fail every paimon query under the default `enable_file_scanner_v2 = true`). It calls the seven-argument `planScan(session, handle, columns, filter, limit, requiredPartitions, countPushdown)`. This branch's later `one planScan, one request object` replaced those four overloads with `planScan(session, ConnectorScanRequest)`. The new test method and the signature change never touched the same lines, so the three-way merge took both sides without a conflict and the break surfaced only at test-compile: "method planScan cannot be applied to given types". The port is exact, not approximate. Every argument upstream passed is the builder's default -- filter `Optional.empty()`, limit `-1`, requiredPartitions `null` (normalizePartitions maps it to empty), countPushdown `false` -- so the test asks the provider for the same scan it asked for before, and the assertions it makes (reader_type stays PAIMON_JNI, `paimon_table` is no longer shipped, split wire format identical with the flag on and off) are unchanged. Call style matches the neighbouring `ignorePaimonCppIsNoOpParity`. Verification: full-reactor test-compile BUILD SUCCESS (75 modules, 0 errors); fe-connector-paimon 390/390 (1 pre-existing live-connectivity skip) including `cppReaderSessionFlagNoLongerChangesThePlan`, fe-connector-iceberg 1151/1151 (5 pre-existing live-endpoint skips), fe-connector-api 110/110. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ce baseline for three intended removals
`ConnectorMetadataSurfaceTest.methodSurfaceMatchesRecordedBaseline` has been red on this
branch since three earlier commits changed the SPI surface without regenerating the recorded
set, which is exactly what the test's own failure message asks for ("update
src/test/resources/connector-metadata-methods.txt in the same commit"). It went unnoticed
because those batches verified with test-compile plus the connector modules they touched, and
never ran fe-connector-api's own suite.
This is NOT rebase fallout: `ConnectorMetadata.java` and the baseline resource are
byte-identical before and after the rebase onto `1aa5ae9597e`, so the test failed the same way
on the pre-rebase tip.
All three removals are deliberate and stay removed:
* `executeStmt` / `getColumnsFromQuery` -- moved to the optional `ConnectorPassthroughSqlOps`
by `give SQL passthrough its own optional interface`; they are still declared, just no longer
on every connector's mandatory surface.
* `supportsCreateDatabase()` -- deleted by `drop the create-database mirror switch`, which
records in `ConnectorSchemaOps`' javadoc why the switch could only ever mirror the schema-ops
implementation.
Nothing is added to the baseline ("new since the baseline" was empty), so this is a pure
three-line deletion and the test keeps its full speed-bump value for the next SPI change.
Verification: fe-connector-api 110/110 green, `ConnectorMetadataSurfaceTest` 4/4.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…eal breaks Rebase onto upstream `1aa5ae9597e` replayed all 72 commits with no conflict and still left a tree that did not compile. The HANDOFF now records what a "clean" rebase does not tell you, so the next session does not read the absence of conflicts as an all-clear. Two breaks, with their causes kept apart: * rebase-induced -- upstream's new seven-argument `planScan` call site vs this branch's request-object signature, invisible to the merge because neither side touched the other's lines, visible only at test-compile. * pre-existing -- the ConnectorMetadata surface baseline had been stale since two earlier batches; proven independent of the rebase because both of the test's inputs are byte-identical across it. The second one carries the reusable lesson, so it is written as a rule rather than an incident: verifying with "full-reactor test-compile plus the connector modules this batch touched" never runs fe-connector-api's own suite, which is where the only recorded-baseline test in the tree lives. Changing the public SPI surface means running that module's tests and refreshing the baseline in the same commit. Also corrects the build-command block: `-pl <module>` without `-am` is unsafe for `test`, not just for compilation, and it fails as "junit-vintage failed to discover tests / Tests run: 0" with the real NoClassDefFoundError buried in target/surefire-reports/*.dump -- an hour-burning error message to meet cold. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ion that carries semantics Two textual conflicts, both in ConnectorPluginManager, both from upstream apache#65644 adding a plugin-inventory registration into the exact loop bodies this branch had already given an admission gate. Resolution is the union, with one deliberate narrowing: registerDiscovered stays outside upstream's catch(RuntimeException), because its fail-loud IllegalStateException reports a build error and would otherwise be silently downgraded to warn-and-skip. Also records what the four verification layers actually covered, and the two things they did not: those loop bodies have no test on either side, and the merged loadPlugins now has one reject path that discards the plugin classloader and one that does not.
… reject path Merging this branch's admission gate into the loadPlugins loop that upstream apache#65644 had just restructured left the loop with two reject paths and only one of them releasing the plugin's classloader: the name-conflict path discarded, the registerDiscovered path did not. FileSystemPluginManager, the sibling this loop mirrors, discards on both of its reject paths. Safe because a false return from registerDiscovered has claimed nothing — the claimedTypes add is short-circuited while a problem is pending, claimedEngineNames is untouched and the provider never reaches the providers list — so no part of the manager still references that classloader. No extra log line: registerDiscovered already reports why it refused. fe-core targeted 26/26, checkstyle 0. The loop body itself remains untested on both sides; see the handoff for why a test there was judged not worth its cost.
…r write seams WHAT BROKE: build 1006199 failed 40 non-muted external regression suites (35 iceberg, 5 hive), every one with the same FE error -- "STRUCT requires at least one child type" / "MAP requires exactly 2 child type(s), got 0" / "ARRAY requires exactly 1 child type(s), got 0". The 41st failure (tvf test_hdfs_parquet_group0, MEM_LIMIT_EXCEEDED) is muted and unrelated. ROOT CAUSE: both connector write seams in PhysicalPlanTranslator built their ConnectorColumn list from ConnectorType.of(col.getType().getPrimitiveType().toString()), i.e. from the column's primitive TAG alone. For an ARRAY/MAP/STRUCT column that yields the bare string "ARRAY"/"MAP"/"STRUCT" and drops every child type. The seams have been wrong since they were written (apache#62183 / apache#64688); it stayed invisible because fe-core silently degrades a childless complex type to ARRAY<NULL>/MAP<NULL,NULL> and no connector reads types off the write handle (jdbc's buildInsertSql uses names, hive's buildColumns reads HmsTableInfo, iceberg never reads them). 0ff75bb added the eager shape check to ConnectorType's canonical constructor, which turned that latent information loss into a hard failure at plan-translation time. BLAST RADIUS: the INSERT seam (visitPhysicalConnectorTableSink) breaks any external write whose target holds a complex column, CTAS included. The row-level DML seam (buildPluginRowLevelDmlSink) breaks EVERY iceberg DELETE/UPDATE/MERGE, even against an all-scalar table, because ExternalRowLevelDeletePlanBuilder takes getBaseSchema(true) and the hidden __DORIS_ICEBERG_ROWID_COL__ is itself a STRUCT. Both seams are generic, so maxcompute writes with a complex column were exposed to the same throw with no test to catch it. FIX: convert the whole type through ConnectorColumnConverter.toConnectorType, which recurses and is already what the CREATE TABLE path (CreateTableInfoToConnectorRequestConverter) uses. The shape check is deliberately NOT relaxed: it caught a real defect, and relaxing it would restore the silence for the first connector that does read these types. TEST: the existing tests in PhysicalPlanTranslatorIcebergRowLevelDmlTest use a scalar-only fixture, which is exactly why they could not catch this. The new test gives the delete sink a STRUCT row-id column and asserts the ConnectorType keeps its children and field names. Verified both directions: 5/5 pass with the fix, and reverting the fix makes the new test fail with the production exception ("STRUCT requires at least one child type"). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tures These 13 failures (5 failures + 8 errors of fe-connector-hive's 375) predate this rebase: HiveConnectorMetadata.java and both test files are byte-identical before and after it, and the pre-rebase tree already carries the rejection that fires. The rebase neither caused nor fixed them; it only made them visible, because running the module's own suite is what surfaces them. Root cause is the same in all three: a check moved out of fe-core and into the connector now runs earlier than the fixture was written for, so the test dies before reaching the behaviour it pins. 1. NOT NULL (12 cases). apache#65893 moved fe-core's "hive catalog doesn't support column with 'NOT NULL'" into HiveConnectorMetadata.validateColumns, which runs first in createTable. The shared request() fixture declared id as nullable=false, so every case using it died on NOT NULL before reaching the transactional / RANGE / explicit partition-value / bucketing rejection it actually asserts -- which is why they surfaced as "expected: <true> but was: <false>" on a message check. Columns are now nullable; the NOT NULL rule keeps its own coverage in HiveCreateTableValidationTest.notNullColumnIsRejected. 2. Childless complex type (1 case). 0ff75bb made ConnectorType enforce that a complex type carries its child types, so the bare ConnectorType.of("ARRAY") in HiveCreateTableValidationTest.col() now throws at construction. complexPartitionColumnIsRejected must hand validatePartition a well-formed ARRAY<INT> -- the point is that a COMPLEX partition column is rejected, not that a malformed type fails to build. Added a ConnectorType overload of the col() helper. 3. Missing partition column (1 case). apache#65893 also moved the partition-key existence check into the connector. createTableListPartitionThreadsPartitionKeys partitions on dt AND region while the shared fixture only declares id + dt, so it now fails with "partition key region is not exists". It declares its own column list with region last, honouring hive's partitions-at-the-end-of-schema rule. No production code touched -- the rejections are upstream-intended; only the fixtures were stale. Verified: fe-connector aggregator, all 16 modules BUILD SUCCESS with checkstyle on, 2939 tests, 0 failures, 0 errors (7 pre-existing live-connectivity skips). fe-connector-hive itself 375/375. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… it exposed Zero textual conflicts and a 78/78 all-equal range-diff, so the entry focuses on the two findings that are not visible from the conflict count: 1. A build-invocation error worth reusing. Full-reactor `clean test-compile` is not a valid verification for this repo and reports false failures: fe-connector-hms-hive-shade has zero java sources and binds shade/jar to the `package` phase, so anything stopping at test-compile (or at `test`) hands fe-connector-hms an empty target/classes. Must use `install`. 2. fe-connector-hive's 13 failures are pre-existing, established by content rather than assumed: the whole pre->post tree delta is upstream's 26 files with no fe-connector-hive input among them, and the relevant sources are byte-identical. Also records why the merge was clean (upstream's net FE delta is one commit confined to fe-connector-paimon, and the two sides' hunks are non-adjacent), why that is the case most worth auditing rather than trusting -- upstream's own apache#65955 doris.* -> jni.* rename was a silent break that compiled and tested green -- and the four-layer audit that cleared it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ap, not a new one The previous entry called the shade-runs-at-package failure a new gotcha. It is not: it is already recorded, and the same root cause has now cost a cycle in four separate sessions. Reframed as a recurrence, with the recognisable symptoms of both faces (cannot-find-symbol on Hive metastore types under test-compile; a junit-vintage discovery error under test, whose real NoClassDefFoundError shows only in the surefire dump) and the standing instruction to read the build-gotchas note before the first maven invocation of a session. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
morningman
force-pushed
the
master-catalog-spi-review-20
branch
from
July 27, 2026 09:47
5aa18d4 to
7e90aea
Compare
Contributor
Author
|
run buildall |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
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.
No description provided.