morningman opened a new pull request, #66135:
URL: https://github.com/apache/doris/pull/66135
### What problem does this PR solve?
Issue Number: #65185
Problem Summary:
Part of the catalog-SPI migration (#65185). Review round 20, targeting
`branch-catalog-spi`.
This round is one self-contained work line: normalize the interface design
of the two public modules
`fe-connector-api` and `fe-connector-spi`, so that adding a new connector
means implementing declared
interfaces and **does not require editing `fe-core`, `fe-connector-api` or
`fe-connector-spi`**.
It started from a full audit of those two modules (172 findings), which was
turned into 25 numbered
tasks plus 10 separately registered open items. All 35 are now done, one
commit per task. The audit
report, the task documents and the rolling handoff are included under
`plan-doc/connector-public-interface-cleanup/`.
**1. Correctness defects found by the audit and fixed**
| module | defect | observable today? |
| --- | --- | --- |
| trino | an OR predicate with 3+ arms was folded from only the first two
arms | **yes** — silently drops rows; a regression introduced by this migration
|
| trino | STRUCT/complex-type field names were dropped, so the engine
invented `col0` / `col1` | **yes** — a regression introduced by this migration |
| paimon | `col <=> value` was pushed down as `IS NULL` | latent: a rewrite
rule normally converts it before the connector sees it; drops rows once that
rule is off |
| paimon | a LIKE pattern containing `_`, an escape, or a mid-string `%` was
narrowed to a prefix match | drops rows |
| hudi | the "last modified" field carried the raw instant string instead of
epoch millis, so the query cache never engaged for partitioned hudi tables |
yes, when the session cache switch is on |
| hive/hudi wrappers | two context wrappers forwarded only the methods that
existed when they were written, so every future engine-context method would
silently lose its classloader pin | latent by construction — fixed at the
mechanism (a forwarding base class), not per method |
| fe-core | both connector write seams in `PhysicalPlanTranslator` built
their column list from the primitive tag alone, dropping every child type of an
ARRAY/MAP/STRUCT column | **yes** — pre-existing since #62183 / #64688, latent
while fe-core silently degraded a childless complex type; the eager shape check
added earlier in this line turned it into a hard failure, and it broke 40
external regression suites (35 iceberg, 5 hive) on build 1006199. Blast radius:
any external write to a table with a complex column (CTAS included), and
**every** iceberg DELETE/UPDATE/MERGE, since the hidden row-id column is itself
a STRUCT |
| fe-core | `ConnectorPluginManager`'s load loop had two reject paths and
released the plugin classloader on only one of them | classloader leak on a
rejected plugin registration; `FileSystemPluginManager`, the sibling it
mirrors, discards on both |
| fe-core | the query profile printed two data-source rows that are never
populated | yes, cosmetic |
Two more user-visible gaps, both on the "one HMS catalog serving two table
formats" delegation path:
nested-column DDL was not forwarded to the iceberg sibling (the same table
accepted the statement in a
standalone catalog but reported "not supported" through the gateway), and an
iceberg table's comment
came back empty through the gateway (the getter has no table handle, so the
gateway's handle-based
routing could not apply).
**2. Adding a connector no longer touches the common modules**
- The catalog-type allow-list is deleted: a registered connector plugin is
now reachable by
`CREATE CATALOG` without any common-module edit. Built-in type names
became reserved words, rejected
at plugin registration time, so shadowing is impossible rather than
tolerated (the Trino model: one
registry, duplicate name refused).
- The remaining source-name branches in `fe-core` are gone. `CREATE TABLE`
is routed by catalog instead
of by engine name, each catalog answers for its own accepted `CREATE
TABLE` engine names, the dead
`MODIFY ENGINE` subsystem is deleted, and the engine name a table displays
is declared by the
connector (`ConnectorProvider.displayEngineName()`, defaulting to the
catalog type name; only
MaxCompute overrides it). **After this round `fe-core` contains no branch
keyed on a data-source
name.**
- Per-table capabilities moved from loose strings to a typed set, and the
write-trait mirror methods on
the entry interface are deleted.
- BE file-cache admission and two engine branches previously matched by type
name are now connector
capability declarations.
**3. Dead interface surface deleted**
Four batches: six pieces of unused connector surface, the LIMIT pushdown
entry point, the unwired
property-descriptor mechanism, the scan-range-type enum family, the
push-model cache-invalidation SPI,
`estimateScanRangeCount`, the create-database mirror switch, and four
zero-consumer SPI groups. Every
deletion was re-scanned across the whole repository (fe-core, 8 connectors,
tests,
`be-java-extensions`, service-discovery config, reflection strings) for zero
hits.
`ConnectorTableOps` is split into domain interfaces with a stated minimum
implementation set, SQL
passthrough moved to its own optional interface, and the four `planScan`
overloads collapsed into one
abstract method plus a `ConnectorScanRequest` object (14 connector overrides
became 8). A recorded
surface baseline (`ConnectorMetadataSurfaceTest`, 72 methods) now guards the
interface against silent
growth.
**4. Neutralization**
Elasticsearch-specific surface and the ES branch inside the generic scan
node are relocated, the
partition null sentinel gets a neutral name with the directory-name rule
sunk into the hudi connector,
the scan-node property-key contract is centralized, the distributed
procedure's result columns are
handed back to the connector, and a catalog's storage services are split
into their own context object.
**5. Contracts written down**
Package-level design rules for both public modules, the predicate-pushdown
contract (per-operator
semantics, plus "if you cannot translate it exactly, do not push it down" —
the root cause of three of
the fixes above), the read-transaction lifecycle, the two property
namespaces and their precedence, the
unit of `getLength()` (connector-defined; the engine must not read it as
bytes), and a batch of
interface docs corrected where they contradicted the implementation.
### Release note
Fixed several external-catalog correctness defects: a Trino-connector OR
predicate with three or more
arms could silently drop rows; Trino complex-type columns lost their field
names and were reported as
`col0` / `col1`; a Paimon LIKE pattern containing `_`, an escape character
or a non-trailing `%` was
narrowed to a prefix match and could drop rows; Paimon pushed null-safe
equality down as `IS NULL`; the
query cache never engaged for partitioned Hudi tables. On an HMS catalog
serving both Hive and Iceberg
tables, nested-column DDL is now forwarded to the Iceberg sibling and an
Iceberg table's comment is
reported correctly.
### Check List (For Author)
- Test
- [x] Regression test
- [x] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason
Local verification of this branch: full 74-module FE reactor `clean install
-DskipTests` (test sources
still compiled) BUILD SUCCESS; the `fe-connector` aggregate green across all
16 modules with checkstyle
enabled — 2939 tests, 0 failures, 0 errors (the 7 skips are pre-existing
live-connectivity tests),
including `fe-connector-iceberg` 1151, `fe-connector-paimon` 401,
`fe-connector-hive` 375 and
`fe-connector-api` 110; plus the targeted `fe-core` connector tests 59/59.
No BE file is touched by
this PR, so BE was not rebuilt.
End-to-end suites added or updated in this PR —
`external_table_p0/paimon/test_paimon_like_pushdown`,
`external_table_p2/hudi/test_hudi_sqlcache`,
`external_table_p0/iceberg/test_iceberg_on_hms_gateway_ddl_parity`,
`external_table_p0/hive/ddl/test_hive_ddl`,
`external_table_p0/iceberg/write/test_iceberg_create_table`,
and three rebased `.out` baselines for the connector-declared engine name —
have not been executed
locally (no cluster available here) and rely on CI.
- Behavior changed:
- [x] Yes.
1. `ENGINE=` in `SHOW CREATE TABLE` and in the table's displayed engine now
come from the connector
rather than from a `fe-core` switch. Both display sites now show the same
name; three regression
`.out` baselines are rebased accordingly.
2. `CREATE TABLE` on an external catalog is now routed and rejected by the
catalog, not by engine name,
so for three combinations the rejection happens earlier and the message
differs.
3. `CREATE DATABASE IF NOT EXISTS <a database that already exists remotely>`
on a jdbc / es / trino /
hudi catalog now succeeds silently instead of reporting "CREATE DATABASE
not supported" (the
`supportsCreateDatabase()` mirror switch is deleted and the precheck is
unconditional, matching
Trino). Connectors that answer neither question are unaffected.
4. The query profile no longer prints two data-source rows that were never
populated.
5. The jdbc connector sends one fewer scan-range key that BE does not read.
- Does this need documentation?
- [x] No.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
