[Spark] Simplify UCClientFactory to single createUCClient(ucConfig) interface by openinx · Pull Request #6792 · delta-io/delta

openinx · 2026-05-15T04:59:33Z

Which Delta project/connector is this regarding?

Description

Simplify UCClientFactory from a multi-method interface (createUCClient(uri, authConfig), createUCClientWithVersions(...), plus Java overloads) to a single unified method:

def createUCClient(ucConfig: Map[String, String]): UCClient

Why? The previous design required callers to assemble separate uri, authConfig, and appVersions parameters. It also had a compile-time dependency on UCDeltaTokenBasedRestClient. This change:

Passes a single flat ucConfig map containing all keys (uri, auth.*, deltaRestApi.enabled, ucclient.impl, appVersions.*).
Uses reflection (Utils.classForName) to load the UCClient implementation at runtime, removing the compile-time dependency on UCDeltaTokenBasedRestClient.
Selects the implementation via ucclient.impl key, or deltaRestApi.enabled flag, defaulting to UCTokenBasedRestClient.

Changes by file

UCCommitCoordinatorBuilder.scala
- UCClientFactory trait: createUCClient(ucConfig: Map[String, String]).
- UCTokenBasedRestClientFactory: single createUCClient that extracts URI, auth, impl class, and app versions from ucConfig. Reflection-based instantiation.
- getCatalogConfigs: simplified from (name, uri, authConfigMap) to (name, ucConfig) — collects all sub-keys under spark.sql.catalog.<name>.* into a flat map.
- UCCatalogConfig: changed from (catalogName, uri, authConfig) to (catalogName, ucConfig) with computed uri and authConfig accessors.
CreateTableBuilder.java / SnapshotManagerFactory.java: use UCTableInfo.toUcConfig() to build the flat config map.
UCTableInfo.java: added toUcConfig() to centralize the uri + auth config assembly.

How was this patch tested?

UCCommitCoordinatorBuilderSuite — 14 tests pass (7 pre-existing Mockito/Java 21 ByteBuddy failures unrelated to this change).
UCCommitCoordinatorClientSuite — 11/11 pass.
New unit tests added for extractAuthConfig (auth.* preference over legacy token, legacy fallback) and extractAppVersions (merges caller-supplied entries with defaults).
scalastyle, javafmt, checkstyle all clean.

Does this PR introduce any user-facing changes?

No. All changed APIs (UCClientFactory, UCTokenBasedRestClientFactory, UCCatalogConfig, getCatalogConfigs) are internal to the Delta coordinated commits module. No user-facing configs or behaviors are changed.

…sedRestClientFactory Replace the hard-coded `new UCTokenBasedRestClient(...)` call with reflection-based class loading so that alternative UCClient implementations (e.g. UCDeltaTokenBasedRestClient) can be selected at runtime via the `ucclient.impl` config key, without introducing a compile-time dependency.

- Consolidate the UCClientFactory trait to a single method: `createUCClient(ucConfig: Map[String, String])` - UCTokenBasedRestClientFactory now parses uri, auth, and deltaRestApi.enabled from the unified ucConfig map to select between UCTokenBasedRestClient and UCDeltaTokenBasedRestClient (loaded via reflection for compile-time decoupling). - Simplify getCatalogConfigs to collect all sub-keys under spark.sql.catalog.<name>.* into a flat config map, removing the separate URI / auth parsing logic. - Update UCCatalogConfig with convenience accessors (uri, authConfig) that extract from the unified ucConfig map. - Update all callers in spark/v2 Java modules.

Signed-off-by: openinx <openinx@gmail.com>

openinx · 2026-05-15T06:22:37Z

+  // scalastyle:off line.size.limit
+  private def noMatchingCatalogException(
+      metastoreId: String) = {


this change is unneccessary, I think we can revert it.

Done. Reverted -- back on a single line, scalastyle:off guard removed.

openinx · 2026-05-15T06:22:54Z

+  private def multipleMatchingCatalogs(
+      metastoreId: String,
+      uris: List[String]) = {


unrelated change, we should revert it.

Done. Reverted -- back on a single line matching the original.

openinx · 2026-05-15T06:25:39Z

+ * Recognised ucConfig keys:
+ *  - `uri` (required) -- the UC server endpoint.
+ *  - `auth.*` / `token` (legacy) -- authentication parameters for [[TokenProvider]].
+ *  - `ucclient.impl` -- fully-qualified class name of the [[UCClient]] to instantiate.


As we already have the deltaRestApi.enabled switch, then I think we no need this any more. Remove it.

Done. Removed ucclient.impl key and UC_CLIENT_IMPL_KEY constant. The deltaRestApi.enabled switch is now the only mechanism for selecting the UCClient implementation.

openinx · 2026-05-15T06:28:07Z

+        DEFAULT_UC_CLIENT_CLASS
+      })
+
+    val appVersions = extractAppVersions(ucConfig)


Pls simply this, we don't need to use the ucConfig to specific the appVersions. in this case, we should always use the default app versions from here: https://github.com/delta-io/delta/pull/6792/changes#diff-e713dd4904f1e14bc5a90fc8517d818c6aa12655799e935e6f7a020d950c47ceR335

Done. createUCClient now merges defaultAppVersions with any caller-supplied appVersions.* entries from ucConfig via extractAppVersions. Callers like SnapshotManagerFactory add custom versions (e.g. Kernel) through ucConfig keys.

openinx · 2026-05-15T06:30:43Z

-   * The provided `appVersions` map is used as-is; callers are responsible for
-   * including all desired version entries.
+   * Extracts authentication configuration from ucConfig.
+   * Prefers `auth.*` keys; falls back to legacy `token` key.


In this doc, pls give some example for reviewer to understand the context. Just like what we did in the javadoc before this PR: https://github.com/delta-io/delta/pull/6792/changes#diff-e713dd4904f1e14bc5a90fc8517d818c6aa12655799e935e6f7a020d950c47ceL167-L185

Done. Added configuration examples showing both the new auth.* format and the legacy token format, similar to the old getCatalogConfigs javadoc.

openinx · 2026-05-15T06:32:27Z

+  /**
+   * Merges default app versions with any `appVersions.*` entries from ucConfig.
+   * Caller-supplied entries override defaults with the same key.
+   */
+  private[coordinatedcommits] def extractAppVersions(
+      ucConfig: Map[String, String]): Map[String, String] = {
+    val extra = ucConfig
+      .filterKeys(_.startsWith(APP_VERSIONS_PREFIX))
+      .map { case (k, v) => (k.stripPrefix(APP_VERSIONS_PREFIX), v) }
+      .toMap
+    defaultAppVersions ++ extra


totally remove this.

Done. Removed extractAppVersions, APP_VERSIONS_PREFIX, and all appVersions.* handling. createUCClient now uses defaultAppVersions directly. SnapshotManagerFactory reverted to a simple createUCClient(tableInfo.toUcConfig()) call.

…es, remove appVersions - Revert unnecessary signature splits for noMatchingCatalogException and multipleMatchingCatalogs (back to single-line, matching original style). - Remove stray scalastyle:off guard. - Add configuration examples to UCTokenBasedRestClientFactory Scaladoc. - Remove extractAppVersions and appVersions.* mechanism entirely. - Revert SnapshotManagerFactory to simple createUCClient call.

- Remove UC_CLIENT_IMPL_KEY / ucclient.impl config key; deltaRestApi.enabled is sufficient to select the UCClient implementation. - Re-add extractAppVersions so callers can supply custom version entries via appVersions.* keys in ucConfig (merged with defaults). - Restore SnapshotManagerFactory to pass Kernel and Delta V2 connector versions through ucConfig appVersions.* entries. - Add extractAppVersions unit test back to BuilderSuite.

timothyw553 · 2026-05-15T07:32:35Z

-      .filter { case (uri, authConfig) => getMetastoreId(uri, authConfig).contains(metastoreId) }
+    val matchingConfigs: List[Map[String, String]] = getCatalogConfigs(spark)
+      .map(_._2)
+      .distinct


why doing .distinct on the whole ucconfig? why not on (uri, authConfig) like before

Because as we adding more parameters to initialize the UCClient instance, this getMatchingUCClient will become more and more complicated, and have to parse all the specific catalog configs in this method. that's hard to maintain.

So a simpler approach here, is: we just parse the spark.<catalog>.* from the session configs, and put all the * as the ucConfigs, and let the specific UCClient to parse the configs, and get the corresponding keys to finish the initialization. that will be much simpler for us the maintain.

You can see in this PR, actually it reduces lots of code (rather than adding more code to parse the UCDeltaClient's configs ) , but can still finish the functionality to initialize the UCDeltaClient.

Yeah, i get that this is for creating the client but here with .distinct, we're deciding if two catalogs are the same and deduping them.

but now in your code you may treat these two:

catalog1 ucConfig = Map( "uri" -> "https://uc", "token" -> "t" ) catalog2 ucConfig = Map( "uri" -> "https://uc", "token" -> "t", "some.spark.catalog.option" -> "x" )

as different even though they have the same URI and authConfig (token), just because the second one has an extra option.

In this case, I think there are indeed two different catalogs, because:

people explicitly specify the two catalog instances for the same catalog entity, that represent they should have the difference in the catalog calling logic.

the two catalog1 and catalog2 indeed have the difference, as catalog2 has a new more config key for the catalog instance. and if the people use the catalog2, the table access via this catalog should have difference with the catalog1, for example the credentialRenewal.eanbled switch.

Restore the URI syntax check that was present before this PR: catalogs with a malformed URI are now skipped with a warning instead of flowing through and failing deep inside the client.

timothyw553 · 2026-05-18T05:44:14Z

+   */
+  public Map<String, String> toUcConfig() {
+    Map<String, String> ucConfig = new HashMap<>();
+    ucConfig.put("uri", ucUri);


just wondering why we dont plumb config.ucConfig() around in UCTableInfo? it seems useful as it carries "deltaRestApi.enabled" -> "true"

V2 path factory selection will depend on {uri, authConfig, deltaRestApi.enabled} right?

openinx force-pushed the fix/ucclient-reflection-factory branch from 510832a to 877d8f9 Compare May 15, 2026 05:26

openinx added 2 commits May 14, 2026 22:37

openinx force-pushed the fix/ucclient-reflection-factory branch from 877d8f9 to 80c4e89 Compare May 15, 2026 05:51

openinx requested review from huan233usc, murali-db, raveeram-db, tdas and timothyw553 as code owners May 15, 2026 05:51

openinx added 2 commits May 14, 2026 23:07

Update

a7dcc0d

Signed-off-by: openinx <openinx@gmail.com>

Update

829cc50

Signed-off-by: openinx <openinx@gmail.com>

openinx changed the title ~~[Storage] Use reflection to load UCClient implementation in UCTokenBasedRestClientFactory~~ [Storage] Simplify UCClientFactory to single createUCClient(ucConfig) interface May 15, 2026

openinx changed the title ~~[Storage] Simplify UCClientFactory to single createUCClient(ucConfig) interface~~ [Spark] Simplify UCClientFactory to single createUCClient(ucConfig) interface May 15, 2026

openinx commented May 15, 2026

View reviewed changes

Comment thread spark/v2/src/main/java/io/delta/spark/internal/v2/snapshot/SnapshotManagerFactory.java

timothyw553 reviewed May 15, 2026

View reviewed changes

Comment thread ...rc/main/scala/org/apache/spark/sql/delta/coordinatedcommits/UCCommitCoordinatorBuilder.scala

Merge branch 'master' into fix/ucclient-reflection-factory

d3c9335

This was referenced May 15, 2026

[Spark] Route Delta loadTable through the Delta REST API #6796

Merged

Enable the UCClientFactory to create a UCDeltaClient instance #6806

Open

Add URI validation back to getCatalogConfigs

7003559

Restore the URI syntax check that was present before this PR: catalogs with a malformed URI are now skipped with a warning instead of flowing through and failing deep inside the client.

timothyw553 reviewed May 18, 2026

View reviewed changes

timothyw553 approved these changes May 19, 2026

View reviewed changes

openinx merged commit 70f3b6b into delta-io:master May 19, 2026
31 checks passed

Uh oh!

Conversation

openinx commented May 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which Delta project/connector is this regarding?

Description

Changes by file

How was this patch tested?

Does this PR introduce any user-facing changes?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openinx May 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openinx May 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openinx May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

timothyw553 May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

openinx commented May 15, 2026 •

edited

Loading

openinx May 15, 2026 •

edited

Loading

openinx May 15, 2026 •

edited

Loading

openinx May 18, 2026 •

edited

Loading

timothyw553 May 18, 2026 •

edited

Loading