[refactor](connector) give every connector the same property layout, and make each key have one reader - #66507
Merged
morningman merged 25 commits intoAug 7, 2026
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
1 similar comment
Contributor
Author
|
run buildall |
morningman
force-pushed
the
connector-properties-convention
branch
from
August 6, 2026 08:53
c38c6f5 to
068b106
Compare
Contributor
Author
|
run buildall |
morningman
force-pushed
the
connector-properties-convention
branch
from
August 6, 2026 09:44
068b106 to
7e780e9
Compare
Contributor
Author
|
run buildall |
morningman
force-pushed
the
connector-properties-convention
branch
from
August 6, 2026 11:35
7e780e9 to
d5e33ae
Compare
Contributor
Author
|
run buildall |
CalvinKirs
previously approved these changes
Aug 6, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
|
PR approved by anyone and no changes requested. |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 28760 ms |
Contributor
TPC-DS: Total hot run time: 166476 ms |
Contributor
ClickBench: Total hot run time: 23.89 s |
…vention A-class) Everything a user writes in CREATE CATALOG for an adbc catalog becomes a typed holder: @ConnectorProperty fields bound by ConnectorPropertiesUtils, the derived partitioned-read mode and driver-option map, and the key-name constants. of(map) binds, derives and validates in one step, so an instance that exists has valid properties -- which is what lets every reader downstream use a getter instead of re-parsing the map. It stays free of I/O and of arrow-adbc types because it runs at CREATE, again on the merged candidate when ALTER validates, and once more on every connector rebuild, including on an FE replaying the edit log. Unknown keys are accepted by design: the same map carries engine keys (type, meta.cache.*) and storage keys (s3.*), and ALTER CATALOG merges properties -- it can overwrite a key but never remove one -- so a key refused here would leave a catalog no statement could repair. fe-foundation is now declared directly rather than relied on transitively through fe-connector-spi, so the binding annotations this class needs cannot disappear with someone else's dependency change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… convention B-class) The keys of this plugin's adbc.conf, their defaults, and the two reads that resolve them now live in one class, separate from the per-catalog properties they used to sit beside. The reading logic moves verbatim out of AdbcConnector. The conf test gains real teeth in the process. It used to call ConnectorConf.get with a hand-written null legacy key, so its "no fe.conf key is consulted" assertion only proved that the test passed null -- it would have stayed green if a reader started naming one. Going through AdbcConf's own readers makes that assertion about production code, and the DORIS_HOME default is now exercised rather than restated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s.of The hand-written require/parse sequence and the meta-cache check are gone: both now live in of(), so the provider's door and every other construction of the holder check exactly the same things. A check that existed only here would have been one that ALTER validation and connector rebuild did not run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…es/AdbcConf, drop the constants class AdbcConnectorProperties is gone. The catalog map now has exactly one reader -- AdbcCatalogProperties -- and every consumer takes a getter instead of parsing the map again: AdbcConnector builds the holder in its constructor and passes it to the scan planner, AdbcDialectSelector takes the dialect name as a value, and the driver-path resolver's adbc.conf keys come from AdbcConf. That single reader is the point. Blank-means-unset, trimming and the partitioned-read spelling were each expressed at more than one call site before, which is how two of them start disagreeing without anything failing. The null-to-empty-string change in the defaults is payload-neutral: both AdbcScanRange.Builder.putIfPresent and AdbcClient.buildParameters already treat "" as absent, so nothing new reaches BE or the driver. The old test's cases that the holder test did not already cover are carried over, including the provider-door ones -- validateProperties is one line now, and a body that stopped calling of() would otherwise leave every other test green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AdbcConnectorMetadata handed the catalog's whole property map -- password included -- to every ConnectorTableSchema it built, where it was cached and carried around for the rest of the catalog's life. Nothing reads those entries for an adbc table today, and the one rendering that would print them, SHOW CREATE TABLE (Env.getDdlStmt), prints table properties unmasked but is fenced off by the SUPPORTS_SHOW_CREATE_DDL capability this connector does not declare. So this leaks nothing now -- but one capability declaration is the entire distance between a stored credential and a string a user can read, and that is too thin a margin to leave for data nobody wants. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two named classes per connector and nothing else: <Xxx>CatalogProperties for what a user writes in CREATE CATALOG, <Xxx>Conf for the plugin's own settings file. The rules that are easy to get wrong are the point of writing this down: of() must be pure and idempotent because it runs on paths no validator does, it must refuse bad values but never unknown keys because ALTER cannot remove one, and the raw map holds credentials so it must not reach anything a user can read. Also records why the binding has to happen inside the plugin -- foundation is child-first for connectors, so fe-core reflecting on a plugin object finds no annotations and says nothing about it -- and what to audit when migrating a released connector, where the binder throwing on a malformed number replaces a getInt helper that silently used the default. Fixes the duplicated list number 15 while in the file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… holder HudiConnectorProperties was a constants class plus a getInt helper, read from two places: HudiConnector parsed the metastore URI (with its "uri" short-form fallback) and the client-pool size, and HudiConnectorMetadata separately parsed use_hive_sync_partition off the raw map. Replace it with HudiCatalogProperties, which binds and validates in one step, so the connector and its metadata read getters instead of the map. None of these keys belong to hudi. There is no type=hudi catalog: the connector is always an embedded sibling of an HMS gateway and receives that gateway catalog's whole property map verbatim, so what it reads are hive's keys. The class says so, and keeps the copied literals next to a comment naming their owner, which is the only way to reference another plugin's key. For the same reason no validateProperties is added to the provider: sibling creation does not pass a validation door, so of() runs from the connector constructor only. hadoop.security.authentication stays a private constant next to buildPluginAuthenticator: it is a raw storage key that buildHadoopConf hands to the Configuration wholesale along with every other passthrough key, so reading it there is a peek, not this connector interpreting a property of its own. Behaviour change, one key: hive.metastore.client.pool.size was parsed by a helper that swallowed NumberFormatException and fell back to 8; it is now bound as an int and a malformed value is refused. Note the asymmetry this leaves while the hive connector still parses the same key leniently -- on a catalog whose pool size is misspelled, hive tables keep working and hudi tables do not, until an ALTER CATALOG overwrites the value. Also drops hoodie.datasource.write.table.type, which had no reader anywhere. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
MCConnectorProperties was 25 key constants with no parsing of its own, so the values were re-read at six call sites -- the provider's validateProperties, the connector's doInit and buildSettings, the scan and write plan providers, the endpoint resolver and the client factory -- each with its own getOrDefault plus parseInt pair, and each free to disagree with the others about a default. Replace it with MCCatalogProperties: one bind-derive-validate step, then getters everywhere. validateProperties collapses to building one. The three value vocabularies become enums next to the property they belong to (SplitStrategy, AccountFormat, AuthType), which is what lets the client factory drop its unsupported-type arm: an invalid auth type can no longer reach it. Note the enums keep their pre-existing matching: auth type case-insensitive, the other two exact, since aligning them would change which spellings an existing catalog accepts. mc.endpoint is required of a new catalog but not of a stored one. Catalogs written before it existed carry only mc.region / mc.odps_endpoint / mc.tunnel_endpoint, and of() runs on every FE restart, so requiring the current spelling there would take those catalogs away from their owners with no statement able to repair them. of() therefore requires a *resolvable* endpoint and the CREATE/ALTER-only rule lives in checkCreateTimeOnlyRules(), which only the provider calls -- preserving both doors exactly as they behave today. Numeric key audit (D7). This connector already parsed strictly at CREATE, so no key flips from lenient to strict: key before after mc.connect_timeout parseInt, threw bound int mc.read_timeout parseInt, threw bound int mc.retry_count parseInt, threw bound int mc.split_byte_size parsed only under byte_size bound long, always mc.split_row_count parsed only under row_count bound long, always mc.max_field_size_bytes parsed in the write path only bound long, at CREATE Two user-visible consequences, both in the direction of failing earlier: - A malformed number is now refused by the binder, so the message reads "Failed to set property 'mc.read_timeout' on MCCatalogProperties: For input string: ..." instead of "property mc.read_timeout must be an integer". It still names the key and the bad value. - The split size of the *unselected* strategy is validated too. A catalog with mc.split_strategy=byte_size and a malformed mc.split_row_count is accepted today and refused after this change; ALTER CATALOG overwriting the value repairs it. The binder also treats a blank value as unset, where resolveEndpoint and the auth check used containsKey. So "mc.access_key" = "" now fails at CREATE naming the missing credential rather than at the first request with whatever the service says. Also drops mc.session_token and mc.max_write_batch_rows, which no code read. Neither is removed from the property map that goes to BE -- that is still passed whole -- so a catalog setting them is unaffected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…a conf class JdbcConnectorProperties mixed three unrelated things: the per-catalog property keys, the per-FE deployment settings read from jdbc.conf, and a lenient getInt helper. Its values were then re-read at five call sites -- the provider's validateProperties, the connector, the scan and write plan providers, and the metadata -- each with its own default. Split it into JdbcCatalogProperties (the CREATE CATALOG map, bound and checked) and JdbcConf (the deployment settings), and give every reader a getter. The split that matters here is not between the two new classes but inside the first one. This connector has always validated far more at CREATE/ALTER than at run time: the pool sizes have bounds no reader enforces, the boolean properties must be spelled true/false where every reader takes anything, the database-list consistency rule is checked once and never again, and lower_case_table_names is rejected by name. None of those is an invariant of a working catalog -- a stored catalog breaking any of them runs today -- so putting them in of(), which runs on every rebuild including on an FE replaying the edit log, would take such catalogs away from their owners with no statement able to repair them. of() therefore holds only the required jdbc_url and the framework's type conversion; everything else moved to checkCreateTimeOnlyRules(), which only the interactive doors call. The test file pins each rule on its side of that line. URL normalization stays outside the holder. It depends on both halves of the configuration at once -- the URL is per-catalog, but whether a SQL Server URL gets encrypt=false appended is per-FE -- so a holder of per-catalog properties has no business doing it. of() takes the normalization as a function: the connector passes the real one, the validation doors pass identity. The jdbc. prefix is stripped on the way in, as it was in both places that used to resolve these keys, and the short spelling still wins over the prefixed one. Numeric key audit (D7): the five connection-pool keys. key CREATE door runtime after connection_pool_min_size parseInt, threw getInt, swallowed bound int connection_pool_max_size parseInt, threw getInt, swallowed bound int connection_pool_max_wait_time parseInt, threw getInt, swallowed bound int connection_pool_max_life_time parseInt, threw getInt, swallowed bound int connection_pool_keep_alive true/false only parseBoolean unchanged So CREATE and ALTER already refused a malformed number and still do; what changes is the stored catalog that somehow holds one -- only possible from an image written before this validation existed. It is silently read as the default today and refuses to build after this change, until ALTER CATALOG overwrites the value. The bounds deliberately do NOT follow: they were never enforced at run time, so they stay create-time only, per the paragraph above. The message for a malformed number now comes from the binder, so it reads "Failed to set property 'connection_pool_min_size' ..." rather than "Property 'connection_pool_min_size' must be a valid integer, got: ...". It still names the key and the bad value. Also drops the "type" constant, which nothing read. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…a conf class Applies the connector property convention to the hive (hms) connector: the per-catalog keys become HiveCatalogProperties, a typed holder whose of() binds, derives and validates in one step, and the two deployment-level settings become HmsConf. The 150-line HiveConnectorProperties constants class is gone; every other constant it carried moved next to its single reader (CREATE TABLE keys and session variable names to HiveConnectorMetadata, the remote table-parameter keys to HiveTableFormatDetector). Six of its constants were dead and are simply removed: the metastore type and the four Kerberos keys are owned by HmsClientConfig and by the shared AbstractHmsMetaStoreProperties holder, and copying them here gave those keys two sources of truth. FLINK_CONNECTOR was both dead and wrong -- it spelled the key "connector" while the detector reads "flink.connector". Fixes the "uri" short form. HiveConnector read it, but only to decide whether the user had named a metastore at all: the map then went to HmsConfHelper, which copies it verbatim into a HiveConf that knows only hive.metastore.uris, so a catalog written with the short form was created successfully and then connected nowhere. The holder binds both spellings (canonical wins) and restates the resolved value under the canonical key for the HMS client. The hudi connector and the iceberg sibling already honoured the short form, so this also removes an asymmetry within one catalog. Behavior changes, all user-visible: - A catalog naming no metastore URI is now rejected by CREATE CATALOG instead of failing at first access. Such a catalog could never run: createClient() has always required the property outright. - hive.metastore.client.pool.size and hive.hms_events_batch_size_per_rpc are strict. The removed getInt swallowed a malformed value and used the default, so a stored catalog carrying one now fails to build; ALTER CATALOG repairs it. The pool size also stops being strict-for-hudi / lenient-for-hive on one catalog. - On the lazy build path, a catalog naming a removed metastore type or no URI now raises IllegalArgumentException rather than DorisConnectorException. The messages are unchanged. The two meta-cache TTL checks stay create-time only, in checkCreateTimeOnlyRules: they were added after the hms cutover and have only ever guarded the interactive doors, so a stored catalog that breaks one runs today and must keep running. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… a conf class Applies the connector property convention to the trino-connector bridge, the last connector in this series that had no property class at all: its two per-catalog keys were read inline in the provider and in the connector's lazy init, and the two deployment-level keys lived as constants on the provider. They become TrinoCatalogProperties and TrinoConf, and validateProperties is one line. TrinoBootstrap.resolvePluginDir stops taking a property map. It now takes the catalog's already-bound trino.plugin.dir override, so the holder is the only thing in the module that reads the catalog map. Fixes the deprecated dashed connector name. Trino renamed its connector names to underscores and its ConnectorName constructor now rejects anything outside [a-z][a-z0-9_]*, so the bridge translates the old spelling -- but it did so into a local variable, leaving the map serialized into the BE scan payload with the dashed name still in it. BE feeds that value straight into its own ConnectorName, so a catalog written as e.g. "delta-lake" served metadata correctly on FE and failed every SELECT on BE with "Invalid connector name", which reads as unrelated to the spelling. The correction now happens once, in the holder, and both the FE factory lookup and the BE payload see the corrected value. One further user-visible change: the binder treats a blank value as unset, where the hand-written check only tested isEmpty(), so a whitespace-only trino.connector.name is now refused at CREATE. Such a catalog could never work. No numeric-key audit table for this connector: both of its catalog keys are strings and it had no hand-written number parsing, so the strictness flip that governs the other migrations has no target here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… convergence rests on
Every alias-bearing paimon catalog property has two readers today: the per-flavor
*MetaStoreProperties holder binds it through @ConnectorProperty(names = ...) for
validation, and PaimonCatalogFactory re-scans the raw map with its own firstNonBlank
helper and a parallel String[] alias array for assembly. Retiring the second reader is
only safe if both resolve the same value, and nothing in the compiler enforces that: a
divergence would silently let a catalog validate against one value and connect with
another.
Two guards, no production change:
- fe-foundation ConnectorPropertiesUtilsTest gains the three rules the convergence rests
on -- names() resolves to the first NON-BLANK alias (not the first present one),
declaration order is priority order, all-blank leaves the field at its initial value --
plus the fact that bound values are trimmed.
- fe-connector-paimon PaimonAliasResolutionParityTest drives the same map through both
readers for every converging alias pair (hms uri, rest uri, the five jdbc keys) and
asserts they agree on all four alias shapes.
The readers agree on selection. The single divergence is trim: the binder trims, the
helper returns the value verbatim. That split is already live for the hms flavor, where
the HiveConf is built from the bound holder ("thrift://nn:9083") while the paimon Options
come from the raw scan ("thrift://nn:9083 ") -- one catalog, two values. The convergence
normalizes them instead of preserving the split, so the divergence is pinned explicitly
rather than left to be discovered.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S73AWq9AHxYfKkzWJfTcKs
…re the assembly rework PaimonCatalogFactoryTest asserts that the keys it names are right; nothing there notices a key that appears, disappears, or is spelled differently. That matters more than usual here: paimon silently ignores an option it does not recognize, so a dropped or misspelled key does not throw -- it produces a catalog that connects with different settings than the operator asked for. Folding the per-flavor assembly onto the bound *MetaStoreProperties holders removes the raw alias scan it is currently compared against, so the reference has to be captured before the change and kept afterwards. These eight snapshots assert the ENTIRE Options map for filesystem (explicit and defaulted), hms (defaults and the uri alias with both defaults overridden), rest (both forms) and jdbc (full and minimal). Every input also carries the three namespaces the paimon.* passthrough must exclude -- storage, per-table options, and the BE jni knobs -- so an exclusion that stops working is a diff rather than a silence. Writing them out surfaced one thing the per-key tests never showed: paimon.catalog.type is a Doris-side key, but it matches the generic paimon. passthrough like any other, so every catalog emits a catalog.type option paimon does not define. It is inert and removing it would change what a live catalog is built with, so it is pinned as-is; cleaning it up is a separate, deliberate change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S73AWq9AHxYfKkzWJfTcKs
…rties, not a second raw scan The hms flavor had two readers for the same keys: PaimonHmsMetaStoreProperties bound the metastore uri through @ConnectorProperty for validation and for the HiveConf, while PaimonCatalogFactory re-scanned the raw map with firstNonBlank and a parallel alias array to build the paimon Options. Nothing kept the two in step. They were already out of step. The binder trims, firstNonBlank does not, so a catalog created with "hive.metastore.uris" = "thrift://nn:9083 " talks to the metastore as "thrift://nn:9083" (HiveConf, built from the bound value) while its paimon Options claim "thrift://nn:9083 ". buildCatalogOptions now binds the flavor's typed facts once and assembles from those, so both come from the same value. That is a user-visible change: a padded value now reaches the paimon SDK trimmed. Ownership follows the same rule: - hive.conf.resources was a bare string literal in the connector (and is one in the iceberg connector too). It is an HMS-backend fact like every other field, so it is declared once on AbstractHmsMetaStoreProperties and read back through a getter. It still rides the verbatim hive.* passthrough into the HiveConf, unchanged. - client-pool-cache.eviction-interval-ms and location-in-properties are paimon's own hms options, so they move to PaimonHmsMetaStoreProperties, which now emits the flavor's option keys as a neutral map (the metastore modules stay free of the paimon SDK). They stay Strings: paimon parses them itself, and binding them to a number would turn a value paimon tolerates today into a catalog that cannot be created. - warehouse is declared for all flavors on AbstractMetaStoreProperties, so the common appender reads it from there instead of the raw map. The metastore identifier is resolved before binding so an unknown paimon.catalog.type keeps failing with this factory's own message rather than the dispatcher's. Verified: fe-connector-paimon 525 tests 0 failures (1 skip is the pre-existing env-gated PaimonLiveConnectivityTest); fe-connector-iceberg, fe-connector-hive and fe-connector-metastore-hms green against the shared base change; ConnectorPluginSurfaceTest untouched; checkstyle clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S73AWq9AHxYfKkzWJfTcKs
…erties PaimonRestMetaStoreProperties.toRestOptions() was written for exactly this cutover and then never wired up: the rest flavor kept assembling its options from a second raw scan while the holder that validated them sat unused. This connects it. One ordering change inside toRestOptions(). Both the "paimon.rest." prefix strip and the alias-resolved uri write to the same "uri" key, and the strip used to run last, so a padded "paimon.rest.uri" would connect with a value validate() never saw. The bound value now wins. Everything else the prefix strip forwards stays verbatim -- it is a wildcard passthrough of keys this connector does not interpret, not alias resolution. Verified: fe-connector-paimon 525 tests 0 failures (1 pre-existing env-gated skip); checkstyle clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S73AWq9AHxYfKkzWJfTcKs
…ies everywhere The jdbc flavor resolved its five aliases in four places: the catalog Options, the CREATE-time driver-path check, the FE driver registration, and the BE-bound scan options. All four scanned the raw map with firstNonBlank while PaimonJdbcMetaStoreProperties bound the same aliases for validation. All four now read the bound values. The BE-bound options are the ones that matter most. jdbc.driver_url is turned into a real URL and jdbc.driver_class into a real Class.forName on the backend, so a padded value used to reach BE as "file:///opt/drivers/ mysql.jar " and fail at load time with an error naming neither the property nor the padding. Covered by a new test. Alias priority is unchanged -- PaimonJdbcMetaStoreProperties declares the same names in the same order, which the parity test pins -- and the raw jdbc.* passthrough stays a raw read: it forwards keys the holder does not model, and it still runs after the bound keys so an alias-resolved user or password wins over a bare jdbc.* copy. Verified: fe-connector-paimon 527 tests 0 failures (1 pre-existing env-gated skip); checkstyle clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S73AWq9AHxYfKkzWJfTcKs
…d a conf class PaimonConnectorProperties was a 135-line constants class mixing four unrelated things: the plugin's deployment settings and their readers, the connector-level catalog keys, the flavor literals, and a set of per-flavor alias arrays. The alias arrays went in the preceding commits, when the assembly started reading the bound metastore properties. This splits what is left the way every other connector in this tree is now split, and deletes the class. - PaimonCatalogProperties: the three catalog keys that are not specific to a backend -- paimon.catalog.type and the two enable.mapping.* switches -- as @ConnectorProperty fields, plus the derived lower-cased flavor and the flavor literals. Everything else a user writes belongs to the bound *MetaStoreProperties, so three keys is the whole surface. - PaimonConf: the two paimon.conf settings, their fe.conf fallback keys, and the readers. The metastore client timeout was an inlined three-argument ConnectorConf.get at the call site; it is a named reader now, and its test drives that reader instead of re-spelling it. of() and checkCreateTimeOnlyRules() are deliberately not one method. of() runs on every connector build, including the lazy rebuild after an FE restart, so it only binds and derives -- it cannot throw. The meta-cache checks, the dead-knob warning, the paimon table-option extraction and the backend's own validate() all ran solely against a CREATE/ALTER statement before, and they still do: a catalog created before one of those rules existed has to be able to come back. That is why validateProperties is one line and the connector constructor calls only of(). The dead-knob warning moves with them. The design had it staying on the provider so a lazy rebuild could not reprint it every time; the create-time-only method turns out to be a third place that is off the rebuild path, which keeps the warning in its original position relative to the meta-cache check. It now logs under PaimonCatalogProperties. Also drops MetaStoreParseUtils.firstNonBlank, which had no production caller in any connector (both factories carried their own copy), and its test. Mechanical fallout: PaimonScanPlanProvider and PaimonConnectorMetadata take the holder instead of the raw map, which is 91 construction sites across 18 test files, rewritten by splitting each argument list on bracket balance rather than by pattern -- the arguments contain nested calls. Both keep getRaw() for the namespaces they forward wholesale (paimon.jni.*, jdbc.*, fs./dfs./hadoop.), which are copy-all passthroughs, not properties. Verified: fe-connector-paimon 532 tests 0 failures (1 skip is the pre-existing env-gated PaimonLiveConnectivityTest); iceberg, hive, metastore-hms, metastore-spi and connector-spi 1189 tests 0 failures (5 skips are pre-existing iceberg live-connectivity tests); ConnectorPluginSurfaceTest green; checkstyle clean; @ConnectorProperty count is 3; the freshly built plugin zip carries lib/fe-foundation-1.2-SNAPSHOT.jar and paimon.conf.template. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S73AWq9AHxYfKkzWJfTcKs
…ore the assembly rework The per-flavor assembly is about to be folded onto the bound Iceberg*MetaStoreProperties holders, retiring the parallel raw-map alias scan that IcebergCatalogFactory performs today. Once that scan is gone there is nothing left to compare the new path against, so capture the reference now. IcebergCatalogFactoryTest asserts that the keys it names are right; nothing there notices a key that appears, disappears, or is spelled differently. The iceberg SDK ignores options it does not recognize, so such a drift does not throw -- it produces a catalog that connects with different settings than the operator asked for. These 19 cases assert the ENTIRE option map, one per flavor and per emission branch, so any drift renders as a diff. They stay after the rework as the permanent guard that the holder and the assembly agree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S73AWq9AHxYfKkzWJfTcKs
…perties The rest flavor had two readers of the same keys: IcebergRestMetaStoreProperties declared them with @ConnectorProperty for CREATE-time validation, while IcebergCatalogFactory scanned the raw map again with its own copy of the alias names to build the catalog options. Nothing kept the two in agreement, so an alias or a blank-value rule could be validated one way and assembled another. The holder is now the single declaration. It gains the eight keys the assembly needed and the holder lacked (uri, prefix, vended-credentials-enabled, the two client timeouts, the oauth2 server-uri and token-refresh flag, session-token), plus the three the connector was reading as raw literals (nested-namespace-enabled, view-enabled, session-timeout) -- all three are rest-only, and IcebergCatalogOps already gates the first two on the flavor, so a non-rest catalog reads exactly the defaults it read before. The connector binds it once and reads session mode, delegated-token-mode and the listing flags off it; the scan and write providers read the vended-credentials flag off it too. The now-dead alias constants are deleted rather than left behind. The one visible change: values are trimmed, because the property binder trims and the raw scan did not. A uri written with a trailing space was already VALIDATED trimmed while the catalog was BUILT from the untrimmed string; the two now agree. This is the same trade accepted for paimon, and it reaches the oauth2 token and the rest access keys as well -- pinned as its own snapshot case. A second, narrower change: iceberg.rest.view-enabled set to the empty string now reads as its default (true) rather than false, because the framework treats blank as unset. The whole-map snapshots are unchanged except for that trim case, which is the evidence that the assembly still emits byte-identical options. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S73AWq9AHxYfKkzWJfTcKs
…perties Same convergence as the rest flavor: IcebergGlueMetaStoreProperties declared the glue keys for CREATE-time validation while IcebergCatalogFactory kept its own copy of the alias arrays to scan the raw map with at assembly time. Auditing the two copies against each other first (the alias sets, in order) -- they agreed, so nothing about which alias wins changes here. What the holder lacked was three keys the assembly reads and validation does not: the region (and its two aliases), the session token, and the assume-role external id. Those are added, the seven alias arrays in IcebergConnectorProperties are deleted, and the region-resolution helper now takes the bound region instead of re-scanning for it -- the endpoint-regex and us-east-1 fallbacks stay where they are, since they derive a value rather than read one. Values are trimmed now, as in the rest flavor. The whole-map snapshots are unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S73AWq9AHxYfKkzWJfTcKs
…perties Last of the three flavors, and with it the raw alias scan goes away. IcebergJdbcMetaStoreProperties declared only uri and catalog_name; the seven keys the assembly reads (user, password, init-catalog-tables, schema-version, strict-mode, and the two driver keys) were scanned out of the raw map by the factory and the connector. The holder now declares all of them, and the four readers -- the options appender, the positional catalog-name resolution, the CREATE-time driver-url gate and the driver registration -- go through it. Auditing the alias sets first, as with glue: the uri alias order here is the REVERSE of the rest flavor's (plain uri wins over iceberg.jdbc.uri) and the holder already had it right, so nothing changes. That order is now pinned by a whole-map snapshot, since getting it backwards would silently point a live catalog at a different database. Two loose ends in the same sweep, both of which the retirement depends on: the hms path reads hive.conf.resources off the shared HMS holder (which has declared it since the paimon convergence) instead of as a bare string literal, and IcebergCatalogFactory.firstNonBlank -- with no callers left outside the S3 region scan, whose alias set is a storage concern rather than any one metastore flavor's -- becomes private, its two wrappers deleted. Values are trimmed now, as in the other two flavors. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S73AWq9AHxYfKkzWJfTcKs
…nd a conf class Last step of the iceberg property rework, and the one that retires IcebergConnectorProperties. That class had become a bag of five unrelated things: deployment settings, connector-level catalog keys, cache key names, option keys the assembly emits, and a few names only test fixtures used. Each goes where its reader is. IcebergCatalogProperties holds what a user writes for the catalog itself and no metastore backend in particular -- the backend type, the two type-mapping switches, and the extra namespace level. Four keys, because everything else belongs to a flavor and moved onto the *MetaStoreProperties in the two previous commits. Its two entry points are deliberately unequal: of(Map) binds and derives and never throws, because it runs on every connector build including the lazy rebuild after an FE restart, while checkCreateTimeOnlyRules() carries the meta-cache validation and the backend dispatch that only a CREATE/ALTER statement reaches. A rule in the wrong one of those two does not fail visibly; it makes a catalog created last year stop coming back after a restart. The provider's validateProperties is one line now. IcebergConf holds the two deployment settings, resolved from the plugin's own iceberg.conf with the fe.conf key as fallback -- the same shape PaimonConf has, including the accessors the connector calls instead of spelling the lookup out. The six meta-cache key names move next to the caches they configure on IcebergConnector, which was already spelling two of them out a second time (as was IcebergScanPlanProvider, a third). external_catalog.name is worth a note: it reads like a REST knob, but IcebergCatalogOps honors it for every flavor, so it is connector-level -- and it binds to null rather than "", or every catalog that never set it would acquire an empty namespace level. The metadata, scan and write providers take the bound holder as their first argument, so all four consumers read one bound object rather than four re-derivations of the same map. Behavior changes, all on inputs that are already pathological: an all-blank iceberg.catalog.type now reports "Missing" instead of "Unknown" (both throw); external_catalog.name set to the empty string now means unset; and the flavor comparison sees trimmed values, as in the two previous commits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S73AWq9AHxYfKkzWJfTcKs
…ess hms catalog Folding the hms catalog properties into HiveCatalogProperties moved the "HMS URI is required" check off the lazy createClient path and into of(), which runs at CREATE CATALOG. Two suites build an hms catalog that names no metastore at all and never query it, so until now they created one that could only ever fail on first access. external_table_p0/tvf/test_catalogs_tvf.groovy reads the catalog's properties back through catalogs(), covering the *XXX masking of credential keys and the GRANT/REVOKE visibility filtering. It lost its address when apache#64304 removed DLF 1.0 and commented out "hive.metastore.type" = "dlf", which had been what made a metastore URI unnecessary. The address added here is never dialled. auth_call/test_hive_base_case_auth.groovy uses the catalog purely as something to grant a privilege on. Only the statement root runs would have failed -- the privilege check in CreateCatalogCommand.validate() precedes property validation, so the neighbouring statement expecting "denied" still gets "denied" -- but the two are deliberately identical, and the sibling test_ddl_catalog_auth.groovy already names a metastore on all four of its hms catalogs, the denied one included. Both are updated so they stay identical. Every property the assertions read is untouched, so no .out changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S73AWq9AHxYfKkzWJfTcKs
morningman
force-pushed
the
connector-properties-convention
branch
from
August 6, 2026 15:23
d5e33ae to
16cbfaf
Compare
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28496 ms |
Contributor
TPC-DS: Total hot run time: 165581 ms |
Contributor
ClickBench: Total hot run time: 23.64 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
CalvinKirs
approved these changes
Aug 6, 2026
morningman
added a commit
to morningman/doris
that referenced
this pull request
Aug 7, 2026
…every other connector Upstream apache#66507 gave every connector one `<Xxx>CatalogProperties`: @ConnectorProperty fields bound by ConnectorPropertiesUtils, derived read-only values, and one `of(map)` that binds, derives and validates so each key has exactly one reader. The fluss connector landed on this branch before that and kept the old shape -- a constant class of static readers, each re-parsing the raw map at every call site. It was the only connector left outside the convention. FlussCatalogProperties replaces FlussConnectorProperties. Five bound keys (fluss.bootstrap.servers, fluss.union_read.mode, fluss.union_read.max_tail_rows and the two engine-wide enable.mapping.* switches) and three derived values (the union-read mode, the type-mapping options, the prefix-stripped fluss client config). The union-read mode was being parsed four separate times per scan and the client config twice; both are now derived once. FlussConnector and FlussScanPlanProvider hold the bound object instead of the map, and the provider's validateProperties is one line -- which, through the SPI default validatePropertiesForUpdate, guards ALTER CATALOG with the same line as CREATE. No FlussConf: that class is for a plugin's own <name>.conf, and fluss ships no template and reads no ConnectorConf key. PaimonSiblingProperties stays as it is -- it holds another plugin's key literals copied across the classloader split, which is the IcebergSiblingProperties precedent, not a properties class of this connector. Every rule stayed in of(). The convention's test is "could an existing catalog violate this and still run?", and here nothing can: FlussConnector's constructor already validated, so all three rules were on the rebuild path before this change. There is no create-time-only set to split out. Two behavior differences, both from adopting the shared binder: - A blank value now counts as absent for every key, so `fluss.union_read.max_tail_rows = ""` reads as the default instead of being rejected. That is the binder's framework-wide rule; a connector that made blank mean something else here would be the one place a user has to remember a local exception. It is also what keeps `fluss.bootstrap.servers = " "` reporting "is missing" as it did before. - The client config now gets the bound (trimmed) bootstrap servers rather than the raw string. They are the same key seen twice -- bound, and forwarded with the prefix stripped -- and letting the raw entry win handed the fluss client an address that differed from the one this connector validated and quotes in its errors. Verified: 194 fluss tests, 0 failures, 0 skipped; checkstyle 0; the plugin zip carries fe-foundation (the binder has to run inside the plugin, since org.apache.doris.foundation is child-first for connectors) and still excludes the parent-first modules; FlussConnectorProperties has zero hits repo-wide. Both new invariants were checked red by mutation: dropping the bound-value-last write, and degrading an unknown union-read mode to auto instead of failing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
morningman
added a commit
to morningman/doris
that referenced
this pull request
Aug 8, 2026
…every other connector Upstream apache#66507 gave every connector one `<Xxx>CatalogProperties`: @ConnectorProperty fields bound by ConnectorPropertiesUtils, derived read-only values, and one `of(map)` that binds, derives and validates so each key has exactly one reader. The fluss connector landed on this branch before that and kept the old shape -- a constant class of static readers, each re-parsing the raw map at every call site. It was the only connector left outside the convention. FlussCatalogProperties replaces FlussConnectorProperties. Five bound keys (fluss.bootstrap.servers, fluss.union_read.mode, fluss.union_read.max_tail_rows and the two engine-wide enable.mapping.* switches) and three derived values (the union-read mode, the type-mapping options, the prefix-stripped fluss client config). The union-read mode was being parsed four separate times per scan and the client config twice; both are now derived once. FlussConnector and FlussScanPlanProvider hold the bound object instead of the map, and the provider's validateProperties is one line -- which, through the SPI default validatePropertiesForUpdate, guards ALTER CATALOG with the same line as CREATE. No FlussConf: that class is for a plugin's own <name>.conf, and fluss ships no template and reads no ConnectorConf key. PaimonSiblingProperties stays as it is -- it holds another plugin's key literals copied across the classloader split, which is the IcebergSiblingProperties precedent, not a properties class of this connector. Every rule stayed in of(). The convention's test is "could an existing catalog violate this and still run?", and here nothing can: FlussConnector's constructor already validated, so all three rules were on the rebuild path before this change. There is no create-time-only set to split out. Two behavior differences, both from adopting the shared binder: - A blank value now counts as absent for every key, so `fluss.union_read.max_tail_rows = ""` reads as the default instead of being rejected. That is the binder's framework-wide rule; a connector that made blank mean something else here would be the one place a user has to remember a local exception. It is also what keeps `fluss.bootstrap.servers = " "` reporting "is missing" as it did before. - The client config now gets the bound (trimmed) bootstrap servers rather than the raw string. They are the same key seen twice -- bound, and forwarded with the prefix stripped -- and letting the raw entry win handed the fluss client an address that differed from the one this connector validated and quotes in its errors. Verified: 194 fluss tests, 0 failures, 0 skipped; checkstyle 0; the plugin zip carries fe-foundation (the binder has to run inside the plugin, since org.apache.doris.foundation is child-first for connectors) and still excludes the parent-first modules; FlussConnectorProperties has zero hits repo-wide. Both new invariants were checked red by mutation: dropping the bound-value-last write, and degrading an unknown union-read mode to auto instead of failing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
3 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: #65185
Problem Summary:
Every connector had grown its own way of dealing with catalog properties. Some kept a
XxxConnectorPropertiesconstant class and parsed values at each read site; some had no propertyclass at all and inlined the key names; iceberg and paimon had two readers for the same keys — a
typed holder used for CREATE-time validation, and a separate raw-map scan with its own copy of the
alias arrays used to actually build the catalog. Nothing kept those two in agreement, so an alias
priority or a blank-value rule could be validated one way and assembled another.
This PR gives every connector the same four-way split, and makes each key have exactly one reader.
No new SPI:
ConnectorPluginSurfaceTestis untouched throughout.Where things live now
A —
<Xxx>CatalogProperties: what a user writes inCREATE CATALOG.One class per connector, in the connector module. Fields carry
@ConnectorPropertywith the aliaslist, so a key name and its aliases are declared exactly once, and
ConnectorPropertiesUtilsbindsthem. This is what the connector, the metadata layer and the scan/write planners read — none of them
touch the raw map for a key this class declares.
The two entry points are deliberately not interchangeable, and this is the part most worth reviewing:
of(Map)binds and derives, and never throws. It runs at CREATE, at ALTER validation, and onevery connector build — including the lazy rebuild after an FE restart. A rule placed here that a
live catalog violates does not fail at review time; it fails months later, as a catalog that stops
coming back after a restart.
checkCreateTimeOnlyRules()carries everything that judges, and only the provider calls it, fromvalidateProperties— one line in most connectors. Unknown keys are never rejected: the same mapcarries engine keys and storage keys, and
ALTER CATALOGcan only overwrite a key, never removeone, so a rejected unknown key could not be repaired.
B —
<Xxx>Conf: deployment-level settings. The keys of the plugin's own<name>.conf, eachfalling back to the
fe.confkey it used to live under. Static accessors (driversDir(context),metastoreClientTimeoutSecond(context), …) rather than a bound object, because these belong to thedeployment and not to any one catalog. Only the connectors that actually have such settings have one.
Per-flavor
*MetaStoreProperties(iceberg / paimon): the keys of one metastore backend.These already existed in
fe-connector-metastore-{iceberg,paimon}but described themselves as"validation only" while the connector re-scanned the same keys to build the catalog. They are now the
single declaration: they gained the fields the assembly needed and the getters it reads, and the
factories consume the bound holder instead of
firstNonBlank(props, ALIASES). The alias arrays thatduplicated them are deleted. The metastore modules stay SDK-free — they expose neutral getters and
maps; engine-SDK option assembly stays in the connector factory.
Splitting by flavor also splits the "annotation count == key count" invariant: the connector-level
holder declares the flavor-independent keys, each backend holder declares its own.
F/G — literals the assembly emits, and mode enums. Option keys and values that a connector
writes (rather than a user setting them) are private to the class that writes them —
IcebergCatalogFactoryfor the iceberg SDK dialect, and so on. Cache key names sit next to the cachethey configure. In several places these had drifted into two spellings of the same string in two
files; folding them removed those duplicates.
Responsibilities, end to end
<Xxx>ConnectorProvidervalidateProperties→of(props).checkCreateTimeOnlyRules()<Xxx>CatalogProperties*MetaStoreProperties<Xxx>CatalogFactory<Xxx>ConfConnectorContextconf + environmentThe raw map is still read in three legitimate shapes, each commented where it happens: copy-all
passthrough into the SDK options, whole-namespace forwarding (
fs./dfs./hadoop.,paimon.,jdbc.), and alias sets that span namespaces and so belong to no single flavor (the S3 regionaliases, the AWS credentials-provider mode).
Verification
Unit tests only — this is a refactor with no intended behavior change, and the guard against
unintended change is a set of whole-map snapshot tests added before each rework: paimon 8 cases and
iceberg 20 cases assert the ENTIRE catalog option map, one per flavor and per emission branch. Both
SDKs silently ignore an option they do not recognize, so a dropped or misspelled key does not throw —
it produces a catalog that connects with different settings than the operator asked for. Those tests
stay in the tree afterwards as the permanent guard that the holder and the assembly agree.
Every touched module, run together at the final commit with
-Dmaven.build.cache.enabled=false(the build cache otherwise reports a stale green):3780 tests, 0 failures, 0 errors, 7 skips — every skip is a pre-existing live-connectivity test
gated on environment variables, and each was already skipped before this PR. checkstyle clean;
ConnectorPluginSurfaceTestgreen.Behavior changes
Small, and all on inputs that are already degenerate. Full per-connector tables are in the commit
messages; the classes of change are:
uriwritten with a trailing space was already validated trimmed while the catalog was builtfrom the untrimmed string. The two now agree. For paimon HMS this removed an existing internal
inconsistency (HiveConf got the trimmed value, the paimon
Optionsgot the raw one).containsKey/getOrDefault. Affects e.g.iceberg.rest.view-enabled = ""(was false, now its default true) andexternal_catalog.name = ""(was an empty namespace level, now absent).the FE parsed a value and then forwarded the unparsed original — the ES
http_ssl_enabledpayloadto BE, the hive
urishorthand, and the trinoconnector.name.Numeric keys were audited per connector and deliberately left as Strings wherever the value is
forwarded verbatim to an engine SDK, so that a catalog created with a value the SDK tolerates keeps
building; the JDBC connection-pool knobs are the one place where the strict/lenient choice is made
per key, with the reasoning in that commit.
Release note
None
Check List (For Author)
Test
Behavior changed:
as unset rather than as an empty string; and three keys the FE interprets are now forwarded in
their interpreted form. Details per connector are in the individual commit messages.
Does this need documentation?