[refactor](catalog) connector public interface cleanup: no common-module edit to add a connector, dead-surface deletion, 11 correctness fixes - #66135
Merged
morningman merged 81 commits intoJul 27, 2026
Conversation
…sign rules The two public connector modules had no place stating how the surface is meant to be used: fe-connector-api had no package documentation at all (95 types, 9 subpackages) and fe-connector-spi's one-paragraph package doc claimed to be "the SPI contracts that connector implementations must fulfill", which is the opposite of the truth (a connector implements fe-connector-api; fe-connector-spi holds the ServiceLoader entry point plus engine-provided services). A connector author therefore had to infer every rule by reading other connectors. Write the rules down, in the module a connector actually implements: - capability declaration has exactly three layers (nullable subsystem getter / provider switch / static-planning capability enum), with the provider as the single source of truth and the same-named methods on Connector marked as engine read ports that a connector must not override; - which exception family each engine path translates. This is the correction with real consequences: metadata/DDL/DML paths translate DorisConnectorException, but catalog property validation translates ONLY IllegalArgumentException (PluginDrivenExternalCatalog.checkProperties), and several connectors depend on that, so it must not be "fixed" to the other family. Also records HiveDirectoryListingException as the sanctioned reason to subclass (the caller must distinguish skip from fail-loud), and the fail-loud vs silent-degrade criterion; - thrift is allowed only at the BE protocol boundary, with the complete list of the five places it appears today and a ban on new thrift-typed parameters; - api vs spi is split by who implements the type, including the fact that the two module names are inverted relative to common usage and are deliberately not being renamed; - lifecycle and threading: one Connector per catalog, one ConnectorMetadata per catalog per statement with an idempotent close, and — the one contract that crashes rather than misanswers when ignored — statistics ops may run on engine background pools with no thread context classloader pinned by the engine, so a connector that reflects by name must pin it itself. Each rule states its current deviations rather than describing the target state as already true (for example: new switches must default false, yet supportsCastPredicatePushdown defaults true; per-table capability refinement is honored for 5 of the 13 capabilities). Comment and pom <description> changes only; no signature, no behavior change. Verified: fe-connector-api + fe-connector-spi compile and checkstyle:check both BUILD SUCCESS with the build cache disabled. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VK7NGsHDmPGvpHFFK5ojej
…tradict the engine Nine javadoc passages described engine behavior that the engine does not have. A connector author cannot read the engine (plugins are packaged and class-loaded separately), so these are the only behavioral contract available, and three of them lead straight to a wrong implementation: - ConnectorPartitionInfo.orderedPartitionValues promised that leaving the list empty makes fe-core parse the partition name itself. That fallback no longer exists; fe-core arity-checks the values and the per-partition catch turns a miss into silent degradation - every partition skipped, table treated as unpartitioned, partition pruning lost, one warn line. Now documented as mandatory, with the observable consequence spelled out. - ConnectorPushdownOps.supportsCastPredicatePushdown promised the engine strips CAST conjuncts when it returns false. Only the residual-predicate path strips; the applyFilter path never consults the switch. Worse, the forward converter unwraps CAST and pushes the child, so a coerced comparison is indistinguishable from a plain one on arrival. Documented, including that the connector carries the under-return risk when it converts such a comparison into remote pruning, and that the true-by-default polarity is a compatibility exception not to copy. The default value is deliberately NOT flipped here: that is a cross-connector behavior change and needs its own commit with end-to-end coverage. - ConnectorMvccSnapshot claimed to be serialized into BE scan ranges and its properties "propagated to BE". The type is not serializable and the engine puts it in no scan range; the properties are read back only by the connector that produced them. The rest are corrections of overstated engine involvement: ConnectorContractValidator now states its real coverage (four connectors' tests call it; the two partition-hash-write invariants have no positive sample on a real connector) and that it checks connector-level getters while the write path reads the per-table overloads; ConnectorProcedureOps.getSupportedProcedures no longer claims to drive routing or validation; ConnectorMetadata's synthetic scan-predicate contract now states the exact expression subset the reverse converter accepts instead of "whatever the connector returns". Also drop internal code names from the public surface, which is headed upstream: one "Design S8", two "(O5-2)", and six bare issue numbers rewritten as the behavior they were standing in for. Javadoc and comment text only; no signature and no behavior change. Verified: fe-connector-api checkstyle:check + compile BUILD SUCCESS with the build cache disabled, and the internal-code-name sweep over both public modules is now empty. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VK7NGsHDmPGvpHFFK5ojej
…mains with a stated minimum implementation set ConnectorTableOps was a 504-line interface with 46 methods, all of them default, mixing eight unrelated responsibilities. Because every method has a default, the compiler forces NOTHING on an implementor: an empty ConnectorMetadata compiles and loads, and then answers SHOW TABLES with an empty list and a query with "table not found" - so a new connector author has no way to learn what a working connector must implement except by reading another connector. Split it by domain, changing no signature and no default body: ConnectorTableMetadataOps (14) resolve a table, read what it looks like ConnectorViewOps (4) views ConnectorTableDdlOps (5) create / drop / rename / truncate ConnectorColumnEvolutionOps (11) column evolution, flat and nested ConnectorSnapshotRefOps (7) branches, tags, partition-spec evolution ConnectorPartitionListingOps (3) partition enumeration ConnectorTableOps remains as their aggregate, so ConnectorMetadata and all eight connectors are untouched; it keeps only the two JDBC pass-through methods, which belong to no domain yet. Each domain's class javadoc states its minimum implementation set, derived from the shipped override distribution rather than from taste - e.g. all eight connectors override getTableHandle, listTableNames, getTableSchema and getColumnHandles; seven override buildTableDescriptor. Where the evidence contradicted a plausible-sounding rule, the javadoc follows the evidence: the three snapshot-aware methods are NOT an all-or-nothing group (three connectors implement only the schema one, and the interface explicitly blesses that), while the branch/tag four and the flat column six ARE groups. Two mechanical guards come with it: - @ConnectorMustImplement marks the required methods. Nothing in production reads it and there is no build gate; RUNTIME retention exists only so a test can pin the set. - ConnectorMetadataSurfaceTest freezes ConnectorMetadata's 81-method surface against a recorded baseline (generated from the pre-split code) and asserts the unconditionally-required set is exactly the known five. A later batch that deliberately deletes SPI methods must update the baseline in the same commit - that is the intent, a speed bump on the public connector surface. Verified: full-reactor test-compile INCLUDING test sources BUILD SUCCESS with the build cache disabled (this is what proves zero breakage - every @OverRide in the eight connectors and fe-core still binds); fe-connector-api checkstyle:check and all 79 tests pass. Both freeze-test mutations were run and confirmed red: deleting one method from a domain fails the surface assertion, and marking a sixth method unconditional fails the minimum-set assertion. Note for anyone reproducing the reactor build: two modules in fe-connector are shade repackagers, and a reactor-wide test-compile resolves them from their pre-shade target/classes, so the hive-dependent modules fail to find org.apache.hadoop.hive.conf. Exclude them (-pl '!fe-connector/ fe-connector-hms-hive-shade,!fe-connector/fe-connector-paimon-hive-shade') and they resolve from the installed jars. This is unrelated to this change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VK7NGsHDmPGvpHFFK5ojej
…S gateway catalog
An iceberg table registered in HMS is reachable through a dedicated iceberg
catalog and through an 'hms' gateway catalog. The gateway forwards every
handle-carrying ALTER to its embedded iceberg sibling, but only the six flat
column ops were overridden; the five path-addressed ones were not, so fe-core
dispatched them straight at hive's own metadata and they hit the inherited
throwing default:
ALTER TABLE hms_catalog.db.iceberg_tbl ADD COLUMN s.b INT
-> "nested ADD COLUMN not supported"
while the same statement on the same table through the iceberg catalog succeeds.
The affected statements are nested ADD / DROP / RENAME / MODIFY COLUMN and
MODIFY COLUMN ... COMMENT - the last of which is the entry point for the flat
form too, so a plain "MODIFY COLUMN c COMMENT '...'" on an iceberg-on-HMS table
was rejected as well.
Add the five overrides with the same guard-and-forward shape as the flat six: a
foreign handle goes to the sibling and is never cast, a hive handle throws the
exact message it inherited before, so plain hive is byte-identical. No TCCL
pinning is needed here for the same reason the flat ops need none - the sibling
pins inside its own authenticated execution.
Found while auditing the connector interfaces: the split-implementation hazard
that the branch/tag group is only theoretically exposed to is real one domain
over.
Verified: fe-connector-hive checkstyle + tests. The delegation suite's
completeness lock now covers all 22 forwarding methods, and its hive-handle half
asserts the five inherited messages byte-for-byte. Note HiveConnectorMetadataDdlTest
fails on this branch both with and without this change (5 failures, 7 errors,
create-table paths) - pre-existing, unrelated, not touched here.
An end-to-end regression driving these statements through a gateway catalog
ships with the follow-up commit; it needs a live cluster and has not been run.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VK7NGsHDmPGvpHFFK5ojej
…way catalog The same iceberg table shows its comment when read through a dedicated iceberg catalog and shows nothing when read through an 'hms' gateway catalog: a blank COMMENT clause in SHOW CREATE TABLE, a blank information_schema.tables .TABLE_COMMENT and a blank SHOW TABLE STATUS Comment column. The cause is structural rather than an omitted override. The gateway routes foreign tables to its embedded sibling by the concrete table-handle type, but getTableComment takes a database and table NAME and never receives a handle, so there is nothing to discriminate on and the connector fell through to the inherited empty default. This is the general shape of the risk for every handle-less method on the table SPI, not a one-off. Answer it from the metastore table the connector already reads: iceberg's HMS catalog mirrors the table's iceberg properties into the HMS table parameters on every commit, comment included - the same 'comment' parameter the shared HMS SHOW CREATE renderer already lifts into the COMMENT clause. So one (cached) getTable answers it, with no sibling connector built and no iceberg metadata load. That matters because the consumer is an unconditional per-table call on information_schema.tables, i.e. once per table on a full listing. Deliberately scoped to iceberg tables: a plain hive table keeps the empty comment legacy HMSExternalTable returned, so nothing an existing hive deployment displays changes. An unreadable table degrades to the empty comment rather than failing, matching the neighbouring name-addressed lookups - this is a display value fetched opportunistically, not a user-requested operation. Also adds the end-to-end regression for both gateway parity gaps (this one and the nested column DDL forwarding of the previous commit): it drives nested ALTER statements through the gateway, verifies the dedicated catalog sees them, and asserts the comment is identical through both catalogs while a plain hive table stays empty. It needs a live external hive/iceberg cluster, so it has NOT been executed here. Verified locally: fe-connector-hive checkstyle + the four affected suites (49 tests) pass, including four new assertions covering iceberg-with-comment, iceberg-without-comment, plain-hive-keeps-empty, and unreadable-table. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VK7NGsHDmPGvpHFFK5ojej
… nine audit facts Update the working notes for the connector public-interface line after landing the first batch (design rules, stale-doc corrections, domain split) plus the two gateway parity fixes the audit turned up. The load-bearing part is the corrections: before writing anything, every factual claim in the three task documents was re-verified against the code, and nine did not survive. They are recorded per task so the next session does not re-derive them, most importantly: - the engine translates TWO exception families, not one - property validation accepts only IllegalArgumentException, and several connectors depend on it, so "unifying" on the other family would degrade CREATE CATALOG errors; - the scan-range-type getter is NOT dead: fe-connector-api's own default populateRangeParams reads it and emits a BE-visible parameter for jdbc, so the batch that deletes that enum family is a behavior change, not a dead-code removal (a warning now sits at the top of that task); - the "snapshot-aware methods are all-or-nothing" rule is contradicted by three of four connectors and by the interface's own javadoc; - the exception-family counts were main+test, overstating the mixed-family argument by 6x on one family; - statistics methods run on three distinct background pools, not one. Also records the build fact that cost time: a reactor-wide test-compile must exclude the two shade repackaging modules, otherwise the hive-dependent modules fail to resolve hive classes for reasons unrelated to any change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VK7NGsHDmPGvpHFFK5ojej
…ctor surface None of these has an implementor or a consumer anywhere in the tree. They cost every new connector author reading time and, in two cases, actively point at the wrong contract. - ConnectorPartitionHandle / ConnectorTransactionHandle: empty marker interfaces with zero references. ConnectorTransaction's javadoc justified the latter as keeping "existing APIs that traffic in opaque handles" working; no such API exists, so the claim goes with the interface. - ConnectorMvccSnapshot.timestampMillis / description: never set by any of the 15 production builder sites. Equality is unaffected (both were constant across every production object), and the type is neither persisted nor sent to BE. - ConnectorTableStatistics.UNKNOWN / ConnectorColumnStatistics.UNKNOWN: unused sentinels whose javadoc contradicted the signatures. Both getters return an Optional, so "unavailable" is Optional.empty(); the docs now say that, and also record that a present value with a -1 field means "this one number is unknown" (hive returns exactly that shape). Note the sentinel that stays: the live ConnectorPartitionInfo.UNKNOWN is a different, load-bearing convention. - ConnectorTestResult sub-components: withComponents / getComponentResults had zero callers, and equals() already ignored the field, so two unequal results could compare equal. The engine reads isSuccess()/getMessage() and logs toString(), whose output is unchanged because the map was always empty. - ConnectorType's four whole-list child accessors: zero callers. The per-index accessors and withChildrenFieldIds keep the four backing fields alive. ConnectorMvccSnapshotTest keeps its equals/hashCode differential coverage for the four surviving fields; its name and javadoc, which hard-coded "six fields", are corrected rather than left lying. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VK7NGsHDmPGvpHFFK5ojej
… point
ConnectorPushdownOps.applyLimit and its LimitApplicationResult have no
implementor in any of the eight connectors, so the engine's tryPushDownLimit()
call in PluginDrivenScanNode always saw Optional.empty() and never touched the
handle. Removing all three is behaviour-neutral today.
It is worth removing rather than leaving because the entry point sits in the
wrong place. getSplits() calls it BEFORE buildRemainingFilter() may strip
CAST-containing conjuncts and before effectiveSourceLimit() suppresses the
source-side LIMIT for exactly that reason: once a predicate has been stripped,
handing the data source a LIMIT lets it return rows that BE then filters again,
so the query under-returns. A connector that implemented applyLimit and returned
false from supportsCastPredicatePushdown (paimon and maxcompute do return false
today; jdbc decides per session) would hit that combination and lose rows. The
hazard is already on record as an accepted out-of-scope risk in
plan-doc/deviations-log.md (DV-020). Deleting the method closes it: a future
LIMIT pushdown has to add an entry point after the stripping step, where
effectiveSourceLimit is in view.
Two collateral edits the deletion forces:
- pinMvccSnapshot()'s javadoc had a {@link #tryPushDownLimit}. Nothing in the
build resolves it (fe-core skips the javadoc plugin), so compilation alone
would have kept a dangling link.
- connector-metadata-methods.txt, the frozen ConnectorMetadata surface, listed
applyLimit; it is regenerated here in the same commit as its own contract
requires. Verified both ways: the test goes red with the stale line present
and green after regeneration.
The batch and async split paths deliberately never pushed the limit, so they are
untouched.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VK7NGsHDmPGvpHFFK5ojej
… external flag ConnectorCreateTableRequest.isExternal() had a producer and no consumer: none of the eight connectors ever read it, and the class said nothing about what "external" is supposed to mean to a connector. It could not have meant anything useful either, because it is a compile-time constant on every path that reaches a connector. CreateTableInfo.checkEngineName forces isExternal = true for hive/iceberg/paimon/jdbc/maxcompute and friends, and rejects external + ENGINE=olap outright, so a connector would only ever observe true. The decision the flag looks like it should drive — writing EXTERNAL_TABLE vs MANAGED_TABLE in the metastore — does not exist in Doris at all: HmsWriteConverter hardcodes MANAGED_TABLE, byte-identical to the pre-migration HiveUtil.toHiveTable. Wiring a connector up to a constant-true flag would therefore not have documented behaviour, it would have changed it. If a connector ever does need to distinguish managed from external tables, the per-catalog property channel already exists and carries a value the user can actually set. The two ConnectorTableDdlOps javadoc lines that promised "external" in the request go with the field, as do the fe-core converter's producer call and the test fixture's trailing parameter. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VK7NGsHDmPGvpHFFK5ojej
…othing carries ConnectorPartitionSpec took a List<ConnectorPartitionValueDef> that is empty at every one of its 12 construction sites, because the neutral converter never lowers LIST/RANGE partition value expressions across the SPI boundary — the spec's own javadoc said so. What a connector actually needs, and hive actually reads, is the boolean hasExplicitPartitionValues(), which survives untouched. So the value list, its 77-line element type, and the wider of the two constructors go. The 11 connector-side edits are all in TEST sources dropping a Collections.emptyList() argument; no connector production code changes. The converter test previously asserted getInitialValues().isEmpty() in three places, i.e. it only ever guarded the dead half. Deleting those assertions with nothing in their place would have left the live half — the presence flag that makes hive reject explicit partition values — with zero coverage anywhere in fe-core. It now asserts the flag directly, including a new case that feeds the converter a non-empty partition-definition list and requires the flag to be raised. Mutation-checked: hard-coding the converter's boolean to false turns exactly that test red, and nothing else. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VK7NGsHDmPGvpHFFK5ojej
…criptor mechanism ConnectorPropertyMetadata and the two Connector getters that returned it (getTableProperties / getSessionProperties, both returning descriptor LISTS) came across from Trino, where the engine validates CREATE TABLE ... WITH (...) keys against them and SET SESSION catalog.property feeds them. Doris has neither, so here they shipped with zero implementors and one caller: a unit test asserting they return empty. Wiring them up was considered and rejected: it would not solve the thing it looks like it solves. Per-catalog values already arrive whole through ConnectorProvider.create and ConnectorSession.getCatalogProperties(), and ConnectorProvider.validateProperties already exists as the validation hook — hive uses exactly that channel today for hive.ignore_absent_partitions, hive.enable_hms_events_incremental_sync and hive.hms_events_batch_size_per_rpc, none of whose key strings appear in fe-core. So the mechanism goes and the rule it was pretending to encode is written down instead: package-info gains Rule 7, "where a connector's tunable knobs live", covering the three real channels (per catalog, per FE process, per session), the key-prefix convention, and an explicit note that there is deliberately no descriptor mechanism, so the next author does not go looking for one. Same commit drops the "95 types" count from Rule 4: it was exact when written, was already wrong two commits later, and a prose invariant nobody can keep true is worse than no number. Beware two live same-name methods this does NOT touch: PluginDrivenExternalTable.getTableProperties() and ConnectorSession.getSessionProperties(), both returning Map<String, String>. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VK7NGsHDmPGvpHFFK5ojej
…rteen task-doc facts The task doc for the first dead-surface batch was written against an older commit and predates the frozen ConnectorMetadata surface test, so following it verbatim would have shipped a red build twice over. Rather than leave the wrong statements standing, the landing record at the top of that doc lists all fourteen corrections a 22-agent recheck produced, including: - the frozen baseline it never mentions (and the fact that the resource has no ASF header, so regenerating it with one silently keeps the test red); - a fourth engine-side reference in a javadoc link, which full-reactor test-compile is structurally blind to — the doc's claim that compiling proves every reference is cleared is simply false; - the worked example that named the wrong connector (hive does not disable CAST predicate pushdown; paimon and maxcompute do); - the UNKNOWN-sentinel harm story, which does not hold: Optional.of(UNKNOWN) and Optional.empty() behave identically at all three engine consumption sites, and a third, live UNKNOWN sentinel survives in the same module; - the prescribed mutation check, which cannot work because the test it names never runs the converter and is already red on this branch for other reasons. README and open-decisions record the two user decisions this batch needed — delete the unwired property-descriptor mechanism (which also settles the pending decision doc), and delete the create-table request's external flag — plus why "keep it and let hive consume it" was reconsidered: with the flag constant-true, consuming it would have flipped every Doris-created hive table to EXTERNAL_TABLE and stopped DROP TABLE from deleting data. HANDOFF carries forward the four build lessons the batch paid for, and the follow-up findings (a dead adapter class in fe-core, two same-name traps). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VK7NGsHDmPGvpHFFK5ojej
…d connector is reachable CREATE CATALOG consulted a seven-element set of type names compiled into fe-core before it would ask the connector plugins anything. A third-party connector could be written correctly, registered correctly, loaded successfully and named in the startup log, and still fail with "Unknown catalog type": the only way to make it usable was to edit that line and republish FE. That is exactly what the plugin mechanism exists to avoid, and it also made ConnectorProvider.supports(catalogType, properties) unreachable for any type outside the set. The set's entire effect was to exclude hudi and to block every third party: it equalled the eight registered providers minus hudi. So the exclusion moves to the connector that needs it. - fe-connector-spi: ConnectorProvider gains isStandaloneCatalogType(), default true. Returning false says "I exist only as an embedded sibling of another connector", which is hudi's case: a hudi table is parasitic on an HMS catalog and has no fe-core catalog class behind it. getType()'s javadoc now states the uniqueness contract it always relied on. - fe-connector-hudi: declares false, and its comments point at that declaration instead of a deleted private field. - ConnectorPluginManager: createStandaloneCatalogConnector() is the entry point for building a catalog and skips non-standalone providers; createConnector() keeps no filter because sibling lookup is a sibling-only connector's ONLY way in. Both share one traversal. - CatalogFactory: no allow-list. Ask the plugins, then the catalog types the engine implements itself, then fail. The degradation arm's condition changes from "the type is in the set" to "nothing serves this type", which fixes a real defect: replaying a CREATE CATALOG for an unserved type threw, and EditLog's fallback catch turns that into System.exit(-1) unless the op code is in skip_operation_types_on_replay_exception (empty by default). A plugin removed from the plugin directory kept the whole FE from starting instead of making one catalog unusable. It is now registered degraded and fails at first access. - Type names are checked when a provider is discovered: blank, already claimed (case-insensitively) or naming a catalog type the engine implements itself is refused. The classpath batch fails loud (a build error); the plugin-directory batch logs and skips, keeping loadPlugins' partial-success contract. Reserving doris/test/lakesoul is what makes consulting the plugins first safe: a plugin declaring itself "doris" would otherwise quietly take over every remote-Doris catalog a user creates. Trino does the same thing, refusing a connector name that is already registered. - PluginDrivenExternalCatalog's lazy build after image deserialization uses the standalone entry point too, so both doors onto a catalog agree on what may become one. User-visible message changes, no test in the tree asserts either old string: "Unknown catalog type 'x'" and "No connector plugin loaded for catalog type 'x'" become one message naming the installed connector types, so a typo is self-diagnosing. Sibling-only types are filtered out of that list -- naming one would point the user at a type the engine will always reject. In-tree behaviour is otherwise unchanged: all seven previously listed types have providers, and the only registered type outside the set is hudi, which the new declaration keeps out. Verified: full-reactor test-compile with test sources, plus checkstyle. 52 unit tests across the touched modules pass. Three mutations were confirmed red: swapping the standalone filter onto the sibling entry point breaks both directions at once (hudi-style lookup returns null AND a bogus catalog becomes creatable), flipping hudi's declaration to true, and dropping the reserved-name check. e2e still needs a real cluster: create+query for each shipped catalog type and, above all, a hudi read, since unit tests can prove routing but not the read path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: catalog-spi-review-20
The previous commit deleted CatalogFactory.SPI_READY_TYPES. Twenty comments across fifteen files still narrated it, so grepping the symbol a comment names would have found nothing. They are reworded to describe the mechanism instead of the field: a catalog type is served by a connector plugin because a provider claims it, not because it appears in a list. Six of them were already wrong before this work started: the iceberg files still said iceberg was "not yet in SPI_READY_TYPES" and its code paths therefore unreachable, years after that cutover landed -- one of them told the reader no iceberg scan range reaches BE. Those stale pre-cutover caveats are dropped rather than reworded. The claims that were true (hms, paimon and iceberg being plugin-driven, and the reason a capability is or is not inert) are kept and now say why without naming a field that no longer exists. Comments only -- no behaviour, no signatures, no test assertions changed. Kept out of the previous commit so the routing change stays readable on its own. Verified by full-reactor test-compile with test sources plus checkstyle, and grep for the symbol across fe/ is now zero. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: catalog-spi-review-20
… facts Task-space bookkeeping for the two preceding commits. - The task doc gains a landing record: the six facts the pre-flight recheck corrected (the message change reaches the seven previously listed types too, not just unknown ones; the deleted private field left 20 comments dangling across 15 files, six of which were already false; the three-step form needs no new collection; CREATE CATALOG is catalog-level CREATE and not admin, which is what made listing installed types a decision; and reserved words beat accepting shadowing), what the implementation does differently from the doc, and the verification actually run. - Three decisions move to decided: routing order (plugins first, plus reserved built-in names refused at registration), listing installed connector types in the failure message, and splitting the dangling-comment cleanup into its own commit. - The handoff records what changed about this work line's premise, since its central promise is now kept, and adds four build lessons: -pl on any single module resolves sibling modules from the local repository and reports a stale jar as a missing override; checkstyle's method-name pattern requires a lowercase second character; PluginDrivenExternalCatalog.getConnector() initializes the catalog and so cannot be used in a plain unit test; and a task doc saying "no mutation verification needed" is not to be taken at face value when the change has a put-it-in-the-wrong- place failure mode. - The frozen ConnectorMetadata baseline is untouched: the new method is on ConnectorProvider in fe-connector-spi, which that baseline does not cover. Written down so the next batch does not hunt for a regeneration that is not needed. Docs only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: catalog-spi-review-20
… not just the first two WHY IT WAS WRONG: TrinoPredicateConverter.convertOr read getDisjuncts().get(0) and .get(1) and ignored the rest. ConnectorOr is a FLATTENED N-ary list -- all three fe-core producers flatten nested ORs before handing it over -- so `a=1 OR a=2 OR a=3` arrived as one three-element node and was pushed to the source as `a IN (1, 2)`. The converted TupleDomain reaches Trino's own applyFilter and getSplits, so the source genuinely never returns the missing rows, and the residual predicate BE re-evaluates can only remove rows, never add them back. The result is a silently short answer: no error, no warning, nothing in EXPLAIN. The more arms the query has, the more it loses. This is a regression this migration introduced, not an inherited upstream bug. The pre-migration fe-core converter also read two children, but its input was a CompoundPredicate, which takes exactly two operands -- a three-way OR was a nested binary tree there, and recursion covered every arm. The input model changed to a flat list; the read did not. FIX: iterate the whole list and columnWiseUnion it. A failing arm is deliberately NOT caught per-arm, unlike convertAnd: skipping an AND conjunct only widens what is pushed and BE recovers exactness, while dropping an OR arm narrows it -- the very shape of this bug. Letting it propagate degrades to TupleDomain.all() (no pushdown), matching IcebergPredicateConverter's all-or-nothing handling of OR. ConnectorOr now also enforces its documented "two or more disjuncts" at construction and copies the caller's list defensively (as ConnectorIn in the same package already did). A degenerate or later-mutated node does not fail loudly downstream -- it just makes the pushed predicate narrower -- so the check belongs where it is cheap. Every existing producer was checked: two guard with an explicit size()==1 ternary, and the third builds from a CompoundPredicate whose two children each contribute at least one element. Tests: three-way and four-way same-column OR (both red before this change), a cross-column OR that must decline to push, and an OR carrying an untranslatable arm that must decline rather than push the two arms it could read. Mutation-verified: restoring the two-arm read turns the first three red. Not done here (deliberately): ConnectorAnd has the same shallow-wrapper shape, but all its producers already avoid one-element ANDs, so folding it in would dilute a row-loss fix into a tidy-up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: catalog-spi-review-20
…rce the complex-type shape WHY IT WAS WRONG: TrinoTypeMapping's ROW branch collected each field's TYPE and never read field.getName(). fe-core's converter does not reject a nameless STRUCT -- it fills in "col" + index -- so the user saw struct<col0:int,col1:text> in DESCRIBE and every `SELECT s.a` written against the actual source schema was rejected at analysis with "the specified field name a was not found". There is no way to reach the field by its real name at all; only positional access still works, which is exactly why a smoke test would not catch it. Field names are part of this type's equals()/hashCode(), so a nameless STRUCT is not "slightly incomplete" -- it has the wrong identity. This is a regression this migration introduced. The pre-migration fe-core mapping checked field.getName().isPresent() and used the real name when there was one. FIX (connector): collect the names and build through ConnectorType.structOf. Anonymous ROW fields are named by POSITION rather than reusing the legacy mapping's single "col" for all of them -- duplicate names are just as unreachable, and "col" + index matches what fe-core and four other connectors already fall back to. ARRAY and MAP move to the factories too, so "complex types go through a factory" is visible at a glance. FIX (public type): the canonical ConnectorType constructor now checks the shape, which covers every convenience constructor and factory since they all funnel through it: ARRAY has one child, MAP two, STRUCT at least one child with a same-length, null-free, parallel field-name list. This is enforced eagerly because a violation is undetectable downstream -- fe-core also turns a childless ARRAY/MAP into ARRAY<NULL>/MAP<NULL,NULL> in silence -- so the mistake surfaces far from the mapping that made it. One rule is deliberately looser than it might look: the four optional per-child lists (nullability, comment, field id, comment-specified) may not be LONGER than the children, but a SHORTER one stays legal. All four accessors read out of range as "not carried for that index" and fall back to a default; that is documented behavior the legacy factories rely on, so requiring exact length would hard-fail a supported state. Every production construction site was checked: only TrinoTypeMapping (fixed here) and EsTypeMapping (a single-child ARRAY, valid) use a bare constructor; the rest use factories that fill names and types in one loop. The existing complex-type tests across fe-core, hms, hudi, iceberg, maxcompute, paimon and hive act as the regression net and all still pass. USER-VISIBLE: STRUCT columns in a trino catalog now show their real field names instead of col0/col1. External table schemas are not persisted, so a reload is enough. A query that hard-coded col0 to work around this stops working -- it was relying on the defect, and a field genuinely named col0 is unaffected. No test depended on the old names. Mutation-verified: dropping the names again, and separately removing the shape check, each turns the corresponding tests red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: catalog-spi-review-20
…own as IS NULL WHY IT WAS WRONG: convertComparison bailed out early whenever the literal converted to null, and only then reached `case EQ_FOR_NULL: return builder.isNull(idx)`. But a null LITERAL is precisely what that early bail-out had already consumed, so by the time control got there the right-hand side was guaranteed NON-null -- and `col <=> 5` was pushed to paimon as `col IS NULL`. That is not a narrowing, it is an inversion: the two predicates select disjoint row sets. Paimon prunes partitions and data files from this predicate during FE planning, so every file holding col = 5 is skipped before the scan starts, and the residual filter BE applies can only drop rows from what was read. The query returns 0 rows, silently, with a smaller partition count in EXPLAIN. Under a default session this arm is usually unreachable: a Nereids rewrite turns `col <=> literal` into `col = literal` in filter position. It is still a defect worth fixing -- that rewrite can be switched off per session via disable_nereids_rules, and a connector's translation must not depend on an optimizer rule having run first. Inside an OR, a single wrong arm also poisons the whole predicate. This one is an upstream defect carried over faithfully, not something this migration introduced: the pre-migration fe-core converter is written identically. FIX: split the "no value" exit into its two unrelated meanings. Only `<=> NULL` has a translation there, and it is exactly IS NULL. Against a non-null literal, `<=>` and `=` select the same rows (`<=>` yields false, never unknown, when the column is null, and paimon's Equal likewise never matches nulls), so EQ_FOR_NULL now shares the EQ arm. The guard checks BOTH the operator and literal.isNull(), which is load-bearing: the same "no value" exit is also how FLOAT, CHAR and timestamp-with-local-time-zone deliberately decline to push. Keying on the operator alone would turn `f <=> 1.5` on a FLOAT column into IS NULL and recreate this bug elsewhere. There is a test for exactly that, and mutation-verified: relaxing the guard turns it red, and restoring `case EQ_FOR_NULL: return builder.isNull(idx)` turns the main case red. Four sibling connectors (iceberg, trino, es, maxcompute) already handle this operator correctly and are untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: catalog-spi-review-20
…provably a prefix match
WHY IT WAS WRONG: convertLike accepted any pattern that did not start with '%' and did
end with '%', chopped off the last character, and pushed the remainder to paimon as a
literal startsWith. That reads three parts of Doris LIKE as literal text when they are
not: '_' is a single-character wildcard, '\' escapes the next character, and a '%' left
anywhere but the tail is still a wildcard. So `'a_c%'` was pushed as startsWith("a_c")
and stopped matching "abc1"; `'a\%%'` (meaning "starts with the literal a%") was pushed
with the backslash still in it and typically matched nothing; `'a%b%'` was pushed as the
literal "a%b".
Every one of those is a NARROWING, and narrowing is the one direction pushdown may never
go. This predicate drives paimon's partition and data-file pruning during FE planning
and the BE-side JNI row filter, so a file skipped because the pushed prefix was stricter
than the user's pattern is never read back. Rows go missing with no error and no EXPLAIN
signal, and the same table read through another engine disagrees.
An upstream defect carried over faithfully, not one this migration introduced: the
pre-migration fe-core converter has the identical check.
FIX: a small pure function decides whether a pattern is provably equivalent to a literal
prefix, and convertLike pushes only when it is. Any '_' or any backslash rejects the
whole pattern; trailing '%'s are stripped; a remaining '%' or an empty body rejects it.
Rejecting on any backslash is blunt on purpose -- it also guarantees the '%' being
stripped is a real wildcard and not an escaped literal one.
The trade this makes is explicit: patterns with '_', an escape, or an inner '%' are no
longer pruned, so those queries read more data. That is slower-but-correct replacing
faster-but-wrong. The common `'prefix%'` shape is unaffected and still pushes.
Tests cover both directions, including the shapes that were already declined, so
tightening the check cannot have started pushing them. Mutation-verified: removing the
'_' check turns the wildcard case red.
An end-to-end case is added (each value written in its own INSERT so file-level pruning
is deterministically reachable). It has NOT been run -- it needs a live paimon cluster.
The unit tests cover the translation itself.
NOT fixed here: es hands a Doris REGEXP pattern straight to Lucene's regexp query, which
anchors differently. Same root cause, different engine semantics and test environment.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: catalog-spi-review-20
…s, not the raw instant
WHY IT WAS WRONG: ConnectorPartitionInfo's last-modified field is contractually epoch
millis, and the hudi connector filled it with the latest completed instant --
yyyyMMddHHmmssSSS read as a number. Both are a long, but for 2024 one is ~2.0e16 and the
other ~1.7e12: four orders of magnitude apart.
The engine gates external SQL cache eligibility on
(now - newest_update) >= cache_last_version_interval_second, having first clamped `now`
to at least newest_update as a guard against FE/metadata clock skew. Feed it an instant
and that clamp drags `now` up with it, so the difference is 0 FOREVER -- not "0 until the
window passes". A partitioned hudi table could therefore never be served from the SQL
cache, and since the gate takes the maximum across every table a query touches, one hudi
table also blocked caching for anything joined to it. No error, no warning; the cache
simply never engages.
Scope, so nobody hunts for a wider symptom: external SQL cache is opt-in via
enable_hive_sql_cache (default off), and an unpartitioned hudi table lists no partitions
and is already ineligible on a different gate. So this affects partitioned hudi tables
once that session variable is on.
FIX: convert in the connector; the engine is untouched. The conversion happens on the
metaClient that was already built, so it costs no extra remote call, and it reads the
zone from the table's own hoodie.table.timeline.timezone -- an instant string carries no
offset. Deliberately NOT via HoodieInstantTimeGenerator.parseDateFromInstantTime, whose
one-line convenience takes the zone from a JVM-global static and would put a whole
timezone offset of error on a table that explicitly declares UTC. An unparseable value
logs and reports 0 ("no reliable change signal", which the engine already handles) rather
than failing a query from the partition-listing hot path over a statistics field.
The raw instant keeps its own jobs -- snapshot id, timeline lookup, time travel -- and is
unchanged. Both partition-listing paths are converted: hive-sync (which pins the instant
separately) and the hudi-metadata listing.
CONSEQUENCES worth stating rather than leaving a reviewer to derive:
- The same value doubles as hudi's data-version token, which drops from ~2.0e16 to
~1.7e12. The only place a shrinking version throws is the SQL dictionary's staleness
check, and its recorded version is not persisted (no @SerializedName), so it resets on
FE restart -- which deploying this requires anyway. No cross-restart comparison exists.
- Each partition's MTMV freshness snapshot changes unit, so materialized views refresh
once and then settle.
- The instant-to-millis mapping is order-preserving (hudi back-fills second-precision
instants with .999), so the token stays monotonic.
- A table whose timeline zone is LOCAL is read in the FE's zone; if the writer sits in a
different zone the value is off by that offset. That only shifts when caching starts,
never correctness, and LOCAL means "the writer's zone", which Doris cannot know.
Also documents the unit on the public getter, since the failure mode for getting it wrong
is silent.
TEST COVERAGE, stated precisely: the unit tests pin the conversion function -- exact
values for millis- and second-precision instants, the empty-timeline and unparseable
cases, and an intent case asserting that wall-clock minus the reported value reads as
"this table has been quiet about an hour". Mutation-verified: returning the raw instant
turns that one red. What they do NOT reach is the two call sites, because the test
executor stubs out the whole metaClient lambda; that wiring rests on review and the
end-to-end case. That case is added here and has NOT been run -- it needs a live hudi
cluster. It is a clean judgement when it does run: the regression env's hudi data is
static and long past the quiet window, so it is red before this change and green after.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: catalog-spi-review-20
…o no engine call is lost The iceberg and paimon connectors each wrap the engine-injected ConnectorContext in a decorator whose only job is pinning the thread-context classloader to the plugin loader, because both plugins bundle hadoop-common (and fe-kerberos, and for iceberg iceberg-aws) child-first: any name-based reflective load running under the engine's default loader would resolve fe-core's copy and ClassCast against the plugin's. Both decorators implemented the interface and copied each method by hand. That fails OPEN. Nearly every method on ConnectorContext has a default implementation whose semantics are a silent downgrade -- getFileSystem returns null, executeAuthenticated runs the task with no authentication, newStorageUriNormalizer drops the per-scan memoization, getStorageProperties returns nothing. Miss one and the compiler says nothing; the call just stops reaching the engine, and for a pinning decorator it stops being pinned. Both had already drifted: iceberg had lost getFileSystem, paimon had lost getFileSystem and newStorageUriNormalizer. Nothing observable is broken today -- only hive calls getFileSystem, and it holds the engine context directly -- so this is a latent defect, not a live one. It is worth fixing now for two reasons. First, whoever calls it first debugs an NPE, or, if they copy hive's null check, a message reading "catalog has no storage properties" while the catalog's storage properties are perfectly fine. Second and more to the point, the hand-copied shape guarantees the next method added to ConnectorContext gets dropped the same way; this project has already had to fix classloader split-brain in four separate places. ForwardingConnectorContext forwards all 19 methods and exposes the wrapped context to subclasses. Both decorators now extend it and keep only executeAuthenticated (plus iceberg's package-private authenticator accessor, which the write path needs to carry the same single-owner doAs into the table's FileIO). createSiblingConnector still reaches the raw engine context rather than the wrapper -- the sibling applies its OWN pinning to whatever it is handed -- which the base class gives for free. A reflection-driven test requires every interface method to arrive at the wrapped context, so adding a method to ConnectorContext without a matching forward becomes a build failure. It compares on name AND parameter types, which matters: an interface default that quietly delegates to its sibling overload (normalizeStorageUri(String, Map) -> normalizeStorageUri(String)) would still leave a record and pass a name-only check while having dropped its arguments. The failure message says what to do next, including the part the base class cannot promise: if the new method can run plugin code, the pinning subclasses must also override it and apply their pin -- this class only guarantees no call is LOST, not that it is pinned. Deliberately not done: no ConnectorContext default was promoted to abstract. There are 25 implementations, 22 of them test doubles, and making getFileSystem abstract would force every offline test double to invent a filesystem -- while still leaving the decorators to hand-copy each new method, so it would not fix the actual failure mode. Behavior is unchanged: executeAuthenticated moves verbatim (only the delegate access changes), and all four existing authentication/pinning assertions in both connectors still pass. Each decorator gains an assertion that the previously-missing methods come from the wrapped context rather than the SPI default. Mutation-verified: removing a forward turns the reflection test red and names the method. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: catalog-spi-review-20
…sk-doc facts Marks tasks 01-06 done in the task list and rewrites the rolling handoff for the next session: what the four newly-enforced invariants are (complex-type shape, OR arity, forwarding completeness, hudi's last-modified unit), what they oblige a future change to do, and why the pushdown-contract task is now the highest-value next step -- it is the shared root cause of the three predicate defects this batch fixed. Two facts in the task documents were wrong and are corrected in the handoff so the next reader does not re-derive them: 1. Task 04 proposed requiring each optional per-child list on ConnectorType to be either empty or exactly as long as the children. That is stricter than the class actually contracts: all four accessors read out of range as "not carried for that index" and fall back to a default, and the field comments say so explicitly. Implementing it literally would have hard-failed a supported state. Shipped as "may not be LONGER than children; shorter stays legal". 2. Task 05 said the hudi APIs it needs were verified in hudi-common 1.0.2. They are -- but the local repository holds several hudi versions and fixInstantTimeCompatibility exists only in 1.0.2, so checking against the wrong jar yields the opposite conclusion. The build does use 1.0.2. The millisecond back-fill for second-precision instants was also measured (999) rather than guessed. Also records three build/verification lessons this batch paid for: mutation runs can be batched into one build when each mutation lands in a distinct test class (but a mutation can itself break checkstyle and abort the run before any test executes); piping maven through `tail` discards the Tests run: lines, leaving BUILD SUCCESS as no evidence at all; and hudi's stubbed test executor replaces the whole metaClient lambda, so a green partition-listing test does not mean that path is covered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: catalog-spi-review-20
…ntract The pushdown expression tree is the only predicate language a connector receives, and it had almost no contract: one sentence per node class. Every connector therefore re-derived the operator semantics from scratch, and three shipped row-loss bugs came out of that -- a null-safe equality collapsed into IS NULL, a LIKE with a single-character wildcard narrowed into a prefix match, and an OR folded down to its first two arms. None of them fail a query; they just return fewer rows, and EXPLAIN looks normal. What is now written down, all of it re-verified against the code in this commit rather than copied from the earlier audit: - The rule the three bugs violated: translate exactly, or drop the whole conjunct; never push a narrower approximation. Plus the precondition that makes "too wide" survivable -- the engine still holds the conjuncts and BE re-evaluates them -- because a connector that claims full pushdown loses that safety net. - That the safe direction FLIPS by use: dropping a conjunct is safe for scan pushdown and for write-time conflict detection, but for an ALTER TABLE ... EXECUTE ... WHERE rewrite scope it silently widens the set of rewritten files, so the engine-side converter throws instead. - Per-operator semantics on ConnectorComparison, with EQ_FOR_NULL split into its two cases (NULL literal -> IS NULL, non-NULL literal -> plain equality). - The LIKE dialect on ConnectorLike: % / _ / backslash escape, REGEXP is unanchored (so a whole-string-anchored remote regex is not a valid verbatim target), and which patterns are provably prefix matches. - The literal value domain: the eight Java shapes the engine actually produces. The old list was wrong in both directions -- it advertised Integer, which is never produced, and omitted that LARGEINT arrives as a decimal String. - Which residual protocol the engine actually honors. A non-null remaining filter removes NOTHING (fine-grained matching is unimplemented); only a null remaining filter or explicit not-pushed conjunct indices do anything. - That ConnectorFunctionCall doubles as the fallback carrier: an unmodelled expression arrives as a "function" whose name is the rendered SQL text and whose argument list is empty, so matching by function name is unsafe. A three-argument LIKE ... ESCAPE also lands there rather than on ConnectorLike. Three statements that contradicted the code are corrected, not just annotated. The worst was on getScanNodePropertiesResult: it claimed the default "assumes all conjuncts have been pushed", while the default carries no tracking at all and prunes nothing -- the exact opposite claim, and the dangerous direction to be wrong in. The engine-side comment on pruneConjunctsFromNodeProperties had the same inversion, so it is fixed in the same commit; leaving one half wrong is how the next reader ends up back here. No behavior change: comments only, no new class and no new method. Column-name case handling is recorded as connector-decided rather than legislated, since paimon lower-cases and jdbc maps through its own table -- unifying that is a behavior decision, not documentation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: connector-public-interface-cleanup batch 5
… SPI Dropping a metadata cache had two interfaces pointing in opposite directions. Engine-to-connector (Connector.invalidateTable / invalidateAll / invalidateDb / invalidatePartition) is live: 17 call sites across PluginDrivenExternalCatalog, RefreshManager and CatalogMgr. Connector-to-engine (ConnectorMetaInvalidator, reached through ConnectorContext.getMetaInvalidator) had zero connector callers, and the engine could not honor what it promised: - invalidatePartition took partition VALUES while the engine's partition cache is keyed by partition NAME, and the name cannot be reconstructed from values without the partition column names the SPI never carried. The bridge therefore degraded to whole-table invalidation, its own comment calling that "correct but over-broad". - invalidateStatistics was an empty method, because the engine has no "drop statistics but keep schema" entry point (row-count cache is keyed by id, not name), and calling invalidateTable would have violated the interface's own "without dropping schema cache" wording. So of five methods, one was a lie and one was an order of magnitude wider than declared -- and both behaviors were pinned by unit tests, which turned "known to be impossible" into "protected existing behavior". Worse than the dead surface is the trap: the two vocabularies collide by name (invalidateDb vs invalidateDatabase) with OPPOSITE parameter semantics, so a connector author who reasonably assumes "I noticed a remote change, I should call this" writes code that compiles, does not throw, and silently blows away a whole table's cache per partition event. What actually runs is the pull model: MetastoreEventSyncDriver polls the connector's pollOnce for neutral change descriptors and the engine applies them itself. Removed: the SPI interface, ConnectorContext.getMetaInvalidator, the forwarding in ForwardingConnectorContext, the fe-core bridge ExternalMetaCacheInvalidator and its test, the DefaultConnectorContext override, and the test double plus the seven "the connector did not notify the engine" assertions in IcebergProcedureOpsTest. Those assertions are deliberately not re-expressed in another form: with the interface gone their failure condition cannot occur, and a test that can never go red is worse than no test. The design statement they encoded (invalidation is the engine's job, via the standard refresh path) stays in that test's class javadoc. No behavior change: zero production callers before this commit. fe-core loses one class and one test class, consistent with the "only shrinks" rule for this phase. The live direction is untouched, and HiveConnectorPartitionViewCacheTest still pins it (engine calls connector, argument is a partition name). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: connector-public-interface-cleanup batch 5
… family ConnectorScanRange.getRangeType() was the only ABSTRACT method on the public scan-range surface with no engine consumer at all. Every one of the eight shipped connectors implemented it and all eight returned FILE_SCAN -- including the jdbc connector, which did not return JDBC_SCAN -- so three of the enum's four values had zero producers, and four test anonymous classes had to implement it to compile while testing split weight, partition values and explain stats. The provider-side twin, ConnectorScanPlanProvider.getScanRangeType(), had three overrides that all returned the default verbatim and no engine call site either. The documentation was worse than the dead code: the enum claimed each value maps to a specific thrift scan-range variant and the provider method claimed the engine picks the structure from it. Neither is true. The thrift shape comes from whether the range overrides populateRangeParams (iceberg, hudi, paimon, es and three others build their own typed descriptor), and the BE-side reader comes from getTableFormatType() plus the scan-level format property. Anyone following those javadocs would change the enum and find that nothing happens. Both class javadocs are now rewritten to say what actually decides those two things. This is a behavior change, not pure deletion, and the task notes it as zero-consumer were wrong on one point: the default populateRangeParams in fe-connector-api reads getRangeType() and writes connector_scan_range_type=<NAME> into the BE-visible jdbc_params map. jdbc is the only connector that does not override populateRangeParams, so that key really was being sent -- to a reader that never looks at it. Verified before deleting: zero hits for the key across be/, and JdbcJniScanner picks its keys out of the map by name, ignoring extras. The jdbc_params map itself stays (file_scanner.cpp and jdbc_reader.cpp both consume it), so only the one key is removed. That default implementation had no test anywhere in the repo, which is how a BE-visible map ended up carrying freight unnoticed. JdbcScanRangeAndPropertiesTest now covers it from both sides: the keys BE does read (query_sql, jdbc_url, jdbc_user, connector_file_format) must still be there, and the dead key must be gone. Mutation-verified -- putting the key back turns that assertion red. getProperties() is deliberately left as the one remaining abstract method rather than being given an empty default: it is the entire input of BE's jdbc reader on this default path, so losing the compile-time requirement would turn a forgotten implementation into a runtime failure far from its cause. Also untouched: getFileFormat() itself and the connector_file_format key (that surface belongs to the scan-level format question, which routes BE between reader generations), and the "plugin_driven" table-format-type default. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: connector-public-interface-cleanup batch 5
…roups Four method groups the engine never calls, plus the connector implementations and tests written for them. Three are ordinary dead surface; one is a trap. The trap is CREATE TABLE. There were two overloads, and the request overload's default built a ConnectorTableSchema and delegated to the narrower (schema, properties) one -- dropping the partition spec, the bucket spec and IF NOT EXISTS along the way. Nothing implemented the narrow form (all four connectors that support create implement the request overload), so it never fired; but the narrow signature is documented as "creates a new table with the given schema and properties", which is the one a new connector author would naturally pick. Doing so would make CREATE TABLE ... PARTITION BY report success and produce an unpartitioned table. DROP DATABASE had the same shape: the 4-arg overload defaulted to a 3-arg form, discarding `force`, so DROP DATABASE ... FORCE became a non-cascading drop that then failed on a non-empty database with a misleading error. Both throws now live on the single remaining overload, with the message text unchanged so existing error assertions are unaffected. Also deleted: - ConnectorMetadata.getProperties(): zero callers anywhere. Five connectors overrode it (paimon's returned an empty map purely to satisfy the interface). It was hard to spot as dead because "getProperties" also exists on ConnectorTableSchema, ConnectorDatabaseMetadata and CatalogProperty, all live. It was also on the wrong object: it claimed to be connector-level state while ConnectorMetadata is rebuilt per statement. - listPartitionValues: implemented by hudi, paimon and maxcompute, each carefully honoring a documented "inner list order must match the requested columns" contract that nothing ever read. Its own javadoc pointed at the partition_values() table function, which actually reaches connectors through listPartitions (PluginDrivenExternalTable.getNameToPartitionValues), so anyone implementing it per the docs would find their code never runs. - getPrimaryKeys plus the ConnectorTableSchema.PRIMARY_KEYS_KEY reserved property: two parallel channels for the same fact, neither with a reader. Only jdbc implemented the method (engine call sites: none); only paimon wrote the key, and fe-core strips every reserved key from SHOW CREATE TABLE, so that path was write-then-discard. Streaming jobs read primary keys through fe-core's own legacy JDBC client, untouched here, and paimon's user-visible `primary-key` table property (what SHOW CREATE TABLE actually displays) is also untouched. jdbc's internal client-layer getPrimaryKeys implementations stay: they are connector-internal JDBC DatabaseMetaData wrappers with dialect handling, not public surface. Test changes are the acceptance surface, not collateral. The former "default createTable degrades to the legacy overload" test is rewritten to assert the opposite (it must reject), a new test covers the 4-arg dropDatabase default that had no coverage before the throw moved onto it, and the two fe-core tests that used the primary-keys key as their "reserved keys get stripped" sample now use the distribution-columns key so the original intent is preserved rather than deleted. Paimon's shared-partition-cache test keeps its loadCount == 1 invariant. All three mutations verified red: reinstating the degrading create default, removing the drop-database throw, and making a paimon enumeration hook bypass the shared cache. The frozen SPI surface baseline is regenerated in this commit: 80 -> 75 entries, exactly the five deleted signatures gone and nothing else. hudi's cache test had the partition_values() mapping wrong in the opposite direction -- it asserted that path must list FRESH. Corrected to record the real routing (through listPartitions, i.e. the cached path, so it can trail an external partition addition by one TTL). That is pre-existing behavior; whether it should bypass the cache is a separate decision, and the mapping is written down so it is not re-derived wrongly again. hive's now-unread `properties` field is removed with all 31 constructor call sites left intact, which leaves one unused parameter on the widest constructor -- a deliberate trade against churn. No user-visible behavior change: every deleted method had zero engine callers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: connector-public-interface-cleanup batch 5
Rolls the task-space docs forward for the fifth batch (four commits: the pushdown contract, and the last three dead-surface deletions). Marks tasks 09, 12, 13 and 14 done in the README, and records seven decisions that were open: delete both primary-key channels, keep the pushdown contract documentation-only (no new public helper), leave hudi's partition_values() freshness as-is and only fix the comments, fix the mirrored engine-side comment in the same commit, keep hive's 31 constructor call sites at the cost of one unused parameter, and skip both of task 13's stretch items. Four task-doc facts corrected, all found by re-checking symbols on HEAD before touching anything: - The reverse-direction invalidation forwarding had already moved into ForwardingConnectorContext, so the deletion point was one base class plus the fe-connector-spi package doc -- not "one line in each of two connector wrapper classes" as written. - The pushdown package has 17 classes, not 18; the limit-application carrier the task doc cited was deleted in the previous batch. - Task 09's "optional helper" had changed meaning: the LIKE prefix guard now exists as a private method inside paimon, so adding the helper would mean hoisting it into public API for a single caller. - Task 12's line numbers and homes were all stale after the domain split, and it never mentioned that the frozen SPI baseline includes inherited methods and therefore had to be regenerated in the same commit. Two new build gotchas recorded: copying the method list out of the frozen-baseline failure message picks up JUnit's " ==> expected:" suffix on the last line (cost one build to notice), and deleting an SPI method needs a follow-up sweep for imports and private fields that lose their only reader. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: connector-public-interface-cleanup batch 5
…ty key contract A connector hands scan-node-level information to the engine as a Map<String, String>, but the contract of that map lived nowhere in the public modules: four keys were `private` constants in PluginDrivenScanNode (whose comment already claimed they were "shared with connector plugins"), the `query` key was a bare literal on both sides, hive kept a byte-identical copy of three of them, and hudi / iceberg / paimon / jdbc / es simply inlined the strings. The three synthetic engine->connector keys existed twice — once in the engine, once in paimon — kept aligned by a comment saying "byte-identical". A new connector had to read engine source and copy literals, with no compile-time help: mistyping `hive.text.column_separator` compiles, starts, and silently returns every text row in the first column. Declare the contract once, in fe-connector-api's ScanNodePropertyKeys: the keys the engine reads (file format, path partition keys, the `location.` prefix, the remote query text, and the twelve text-family attributes) and the SYNTHETIC_* keys the engine injects for appendExplainInfo. Every literal keeps its exact historical value; only the symbols move. The text prefix keeps the `hive.text.` literal but gets a source-neutral name — it serves TEXT/CSV/JSON for every connector, and changing the value would need a synchronized engine+hive edit for a naming-only gain. Two more contract fixes in the same surface: * The property table has two return faces (the Map one and the ScanNodePropertiesResult one). The engine calls only the latter, whose default delegates to the former, so a connector that overrides both with different content has its Map silently discarded. That rule is now written on both methods, and es's Map-face override — unreachable from the engine, since es also overrides the result face — is deleted. * ScanNodePropertiesResult encoded "does this connector report per-conjunct pushdown" in *which constructor the caller picked*: `new ScanNodePropertiesResult(props)` silently also declared "prune nothing". The constructors are now private behind `of(props)` and `withPushdownTracking(props, notPushed)`, so the claim is visible at the call site. Getting this bit wrong is a wrong-results bug (tracking with an empty set means "prune every conjunct unevaluated"). Finally, drop getSerializedTable(Map) from the SPI. Its only implementation read back the key paimon had itself written into the property table, and paimon already overrides populateScanLevelParams, which receives the very TFileScanRangeParams the engine sends and runs after the generic scan-range construction — so it now just sets the field. The engine-side override and the FileQueryScanNode hook it implemented go away with it (fe-core net-negative; no other subclass overrode it). Behavior: the property maps are byte-identical to before. The one runtime change is who sets the thrift `serialized_table` field for paimon, which is a hard requirement of its BE JNI reader (BE fails the scan with "missing serialized_table" rather than degrading), so it is pinned by a new assertion in PaimonScanPlanProviderTest; the mutation that drops the set was confirmed to fail only that test. The two-faces rule is pinned by a new fe-connector-api test. No test depended on the deleted SPI method, and there is no user-visible message or wire-format change. Verified: full-reactor test-compile including test sources + checkstyle BUILD SUCCESS; 19 test classes across fe-connector-api / es / hive / hudi / iceberg / jdbc / paimon / fe-core green; one mutation run. Not covered by unit tests: the BE side of the paimon serialized table and the hive text attributes actually parsing a file — both need cluster e2e (paimon catalog query regression and a hive text/CSV/JSON read), listed in the handoff as owed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: connector-public-interface-cleanup batch 6
… engine branches matched by type name Two engine decisions are taken while a catalog may still be uninitialized, and both were written as a hardcoded data source name: * the metastore-event driver force-initializes an uninitialized catalog once so it can obtain its event source and seed its cursor — gated on the catalog type being "hms"; * switching to a catalog lands the session in "default_db" — gated on the catalog type being "es". The first one has a real consequence beyond neutrality. Each FE keeps its own cursor, and a follower normally receives no queries at all, so a catalog that is never queried on this FE never gets initialized and never seeds a cursor. Any other connector that exposes an event source therefore does not sync incrementally on followers (nor on any FE between restart and first query), silently, until someone queries it. That also contradicts what Connector#getEventSource() promises in its javadoc — that the engine's driver is connector-agnostic and never selects by type. Both are now declared by the connector: ConnectorProvider gains providesEventSource() (default false) and defaultDatabaseOnUse() (default empty). They sit on the PROVIDER, not on Connector, and that is the whole point: these decisions are made before the connector exists, and asking the connector would force-initialize exactly the idle catalogs the event driver's type check was written to leave alone. Reading them needs a way to reach a provider without building a connector, so ConnectorPluginManager / ConnectorFactory gain findProvider(), using the same selection as createConnector. That entry point is connector-agnostic, and it exists to delete two data source names from fe-core. Connector#getEventSource() remains the single source of truth for whether a connector actually has an event source — the driver still skips a connector whose source is null, so a provider declaring true without one costs at most a wasted initialization. es names its own synthetic database instead of the engine repeating the "default_db" literal, and the switch keeps its position and precedence (still applied after the remembered database, since such a catalog has exactly one database to return to). Behavior is unchanged for every shipped connector: hms declares an event source, es declares its database, everything else keeps the defaults. Verified: full-reactor test-compile with test sources + checkstyle BUILD SUCCESS; the new MetastoreEventSyncDriverSeedingTest asserts both directions by counting whether the catalog was touched at all — a declaring type IS force-initialized, and an idle catalog of a non-declaring or unregistered type is not touched once; two mutations (dropping the declaration guard, and es returning the SPI default) were each caught by the expected test class. Not covered by unit tests: the four-line Env.changeCatalog wiring itself, which needs a live session — owed as e2e (SWITCH <es catalog> then SELECT DATABASE()), together with an event-sync run on an FE that never queries the catalog. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: connector-public-interface-cleanup batch 6
…from a connector capability
FileQueryScanNode governed BE's file-cache admission control by matching the catalog type against the
set {"hms", "iceberg", "paimon"}. The coverage that produces is right today, but the criterion is not
what actually matters: admission governance is meaningful exactly when BE reads the data with its
native file readers, because only those populate the file cache. A new connector that reads parquet
natively would have its tables silently skipped — the user's library/table admission rules would not
apply and nothing but a debug log would say so — until someone edited fe-core.
Add ConnectorScanPlanProvider#supportsFileCache() (default false, same opt-in shape as
supportsBatchScan / supportsTableSample) and have the plugin-driven scan node answer from the provider
resolved through the table handle. Two consequences worth naming:
* the answer now follows the connector that actually serves the table, not the catalog it lives in.
In a heterogeneous HMS catalog each table is answered by its own sibling — which is why hudi has to
declare it explicitly: hudi tables live in an "hms" catalog and used to inherit governance from the
catalog type name, so not declaring it would silently drop them out of admission control. That is
the one trap in this change and it is pinned by a test.
* FileQueryScanNode gets a protected hook defaulting to false, which keeps the table-valued-function
and remote-Doris scan nodes out of governance exactly as the allow-list did, without either of them
saying anything.
hive / iceberg / paimon / hudi declare true; maxcompute / trino / jdbc / es keep the default, matching
the allow-list they were outside of. Behavior is therefore unchanged for every shipped connector.
Verified: full-reactor test-compile with test sources + checkstyle BUILD SUCCESS; the new
PluginDrivenScanNodeFileCacheAdmissionTest covers declared / not-declared / no-provider-resolved plus
the base-node default, and the hudi and es suites pin their own declarations. Two mutations (hudi
returning false, and the node governing every scan) were each caught by the expected test class. Not
covered by unit tests: that admission rules really fire end to end — owed as an e2e on a heterogeneous
HMS catalog (hive + iceberg + hudi) plus standalone iceberg/paimon catalogs with a library-level rule
configured.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: connector-public-interface-cleanup batch 6
…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
requested review from
924060929,
CalvinKirs,
dataroaring,
englefly,
gavinchou,
liaoxin01,
luwei16,
morrySnow,
mymeiyi and
starocean999
as code owners
July 27, 2026 13:41
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
morningman
added a commit
that referenced
this pull request
Jul 27, 2026
…ule edit to add a connector, dead-surface deletion, 11 correctness fixes (#66135) 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.
morningman
added a commit
that referenced
this pull request
Jul 28, 2026
…ule edit to add a connector, dead-surface deletion, 11 correctness fixes (#66135) 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.
morningman
added a commit
that referenced
this pull request
Jul 28, 2026
…ule edit to add a connector, dead-surface deletion, 11 correctness fixes (#66135) 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.
morningman
added a commit
that referenced
this pull request
Jul 29, 2026
…ule edit to add a connector, dead-surface deletion, 11 correctness fixes (#66135) 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.
morningman
added a commit
that referenced
this pull request
Jul 29, 2026
…ule edit to add a connector, dead-surface deletion, 11 correctness fixes (#66135) 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: #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-apiandfe-connector-spi, so that adding a new connector means implementing declaredinterfaces and does not require editing
fe-core,fe-connector-apiorfe-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
col0/col1col <=> valuewas pushed down asIS NULL_, an escape, or a mid-string%was narrowed to a prefix matchPhysicalPlanTranslatorbuilt their column list from the primitive tag alone, dropping every child type of an ARRAY/MAP/STRUCT columnConnectorPluginManager's load loop had two reject paths and released the plugin classloader on only one of themFileSystemPluginManager, the sibling it mirrors, discards on bothTwo 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
CREATE CATALOGwithout any common-module edit. Built-in type names became reserved words, rejectedat plugin registration time, so shadowing is impossible rather than tolerated (the Trino model: one
registry, duplicate name refused).
fe-coreare gone.CREATE TABLEis routed by catalog insteadof by engine name, each catalog answers for its own accepted
CREATE TABLEengine names, the deadMODIFY ENGINEsubsystem is deleted, and the engine name a table displays is declared by theconnector (
ConnectorProvider.displayEngineName(), defaulting to the catalog type name; onlyMaxCompute overrides it). After this round
fe-corecontains no branch keyed on a data-sourcename.
the entry interface are deleted.
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. Everydeletion was re-scanned across the whole repository (fe-core, 8 connectors, tests,
be-java-extensions, service-discovery config, reflection strings) for zero hits.ConnectorTableOpsis split into domain interfaces with a stated minimum implementation set, SQLpassthrough moved to its own optional interface, and the four
planScanoverloads collapsed into oneabstract method plus a
ConnectorScanRequestobject (14 connector overrides became 8). A recordedsurface baseline (
ConnectorMetadataSurfaceTest, 72 methods) now guards the interface against silentgrowth.
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 ofinterface 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%wasnarrowed to a prefix match and could drop rows; Paimon pushed null-safe equality down as
IS NULL; thequery 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)
Local verification of this branch: full 74-module FE reactor
clean install -DskipTests(test sourcesstill compiled) BUILD SUCCESS; the
fe-connectoraggregate green across all 16 modules with checkstyleenabled — 2939 tests, 0 failures, 0 errors (the 7 skips are pre-existing live-connectivity tests),
including
fe-connector-iceberg1151,fe-connector-paimon401,fe-connector-hive375 andfe-connector-api110; plus the targetedfe-coreconnector tests 59/59. No BE file is touched bythis 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
.outbaselines for the connector-declared engine name — have not been executedlocally (no cluster available here) and rely on CI.
ENGINE=inSHOW CREATE TABLEand in the table's displayed engine now come from the connectorrather than from a
fe-coreswitch. Both display sites now show the same name; three regression.outbaselines are rebased accordingly.CREATE TABLEon 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.
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, matchingTrino). Connectors that answer neither question are unaffected.