feat: built-in databricks unity catalog sync by parisni · Pull Request #802 · apache/incubator-xtable

parisni · 2026-02-11T13:44:27Z

Important Read

Please ensure the GitHub issue is mentioned at the beginning of the PR

What is the purpose of the pull request

This pull request adds Databricks Unity Catalog (DATABRICKS_UC) catalog sync support to XTable, with a new xtable-databricks module, UC config wiring, UC sync client
implementation, and documentation updates.

Brief change log

Added new xtable-databricks module + ServiceLoader wiring.
Added DatabricksUnityCatalogConfig and DatabricksUnityCatalogSyncClient (DDL-based UC registration).
Implemented UC sync methods: hasDatabase, createDatabase, getTable, createTable, createOrReplaceTable, dropTable, refreshTable.
Added UC-specific unit tests for all catalog sync methods.
Documented Databricks UC sync flow, auth (OAuth M2M), limitations (Delta-only, unique location), and schema evolution strategy (drop/recreate).

Verify this pull request

This change added tests and can be verified as follows:

Added TestDatabricksUnityCatalogSyncClient in xtable-databricks.
Run:
- mvn -Drat.skip=true -Dspotless.skip=true -Dtest=TestDatabricksUnityCatalogSyncClient -pl xtable-databricks test

not sure if unity catalog refresh by itself the table metadata. will have to investigate for now skip refresh table

- alter column stmt is meant for delta, not uc so it tries to upgrade delta logs - refresh table stmt does not refresh the columns - create or replace stmt is also meant for delta, not uc so tries to upgrade delta logs - drop table + create new table: does the work, also sounds like uc does keep track of the table history so we don't loose associated stuff (top users, related assets, creation date...)

vinishjail97 · 2026-02-23T16:11:51Z

+
+  DatabricksUnityCatalogSyncClient(
+      ExternalCatalogConfig catalogConfig,
+      String tableFormat,


SQL injection risk: fullName is derived from user-provided tableIdentifier and interpolated directly into the SQL string. While escapeSqlString is used for location, the table name itself is not sanitized.

If hierarchicalId contains ; DROP TABLE ..., this would be injected. Consider validating that catalog/schema/table names match [a-zA-Z0-9_]+ before interpolating them into DDL statements.

vinishjail97 · 2026-02-23T16:11:51Z

+    if (table == null) {
+      return null;
+    }
+    return table.getStorageLocation();


MSCK REPAIR TABLE ... SYNC METADATA is a Hive-specific command. Does Databricks UC's SQL Warehouse support this syntax? The standard Databricks approach for refreshing external table metadata is ALTER TABLE <table> SET TBLPROPERTIES or reading from the Delta log directly.

If this doesn't actually work on Databricks, schema drift would silently be ignored. Has this been tested against a real SQL Warehouse?

Has this been tested against a real SQL Warehouse?

yes we are running this code in production. and yes i did validate schema evolution is supported (columns changes, comments changes, properties...). The drawback of the approach is it needs a sql warehouse running (it reads the delta logs to update the UC)

ALso the UC does not support modify tables columns or anything so this is today the best approach i have in mind.

see related doc https://docs.databricks.com/aws/en/sql/language-manual/sql-ref-syntax-ddl-repair-table:

SYNC METADATA Delta Lake only. If the configuration spark.databricks.delta.catalog.update.enabled is set to true, target table metadata updates are automatically synced to the catalog service. Otherwise, a manual sync using REPAIR TABLE table_name SYNC METADATA may be required. Reads the transaction log of the target table and updates the metadata info in the catalog service. To run this command, you must have MODIFY and SELECT privileges on the target table and USE SCHEMA and USE CATALOG privileges on the parent schema and catalog. This argument works with Hive metastore in Databricks Runtime 16.1 and above. For Hive metastore tables, you must have USAGE and MODIFY privileges. If Delta UniForm is enabled (requires Unity Catalog), SYNC METADATA triggers manual conversion of current Delta metadata to Iceberg metadata and syncs the latest Iceberg version for the Unity Catalog Iceberg endpoint. See [Read Delta tables with Iceberg clients](https://docs.databricks.com/aws/en/delta/uniform). You can use REPAIR TABLE table_name SYNC METADATA to apply the Unity Catalog permissions model to shallow clones that you are reading from a foreign catalog that was created using Hive metastore federation. See[ Working with shallow clones](https://docs.databricks.com/aws/en/query-federation/hms-federation-concepts#shallow).

vinishjail97 · 2026-02-23T16:11:51Z

+    String catalog = hierarchical.getCatalogName();
+    if (StringUtils.isBlank(catalog)) {
+      throw new CatalogSyncException(
+          "Databricks UC requires a catalog name (expected catalog.schema.table)");


createOrReplaceTable drops then recreates the table. This is not atomic — if createTable fails after dropTable succeeds, the table is gone. Consider using CREATE OR REPLACE TABLE DDL instead (if supported for external tables), or at minimum catch the create failure and log a clear error that the table was dropped but not recreated.

Well sadly on databricks createOrReplaceTable is meant to modify the table layer (creates a commit) it's not only on UC metadata side. So we can't use this. I assume xtable createOrReplaceTable is meant for catalog only table recreation, not data themself. Or I am missing something ?

Added catch failure

vinishjail97 · 2026-02-23T16:11:51Z

+    } catch (Exception e) {
+      throw new CatalogSyncException("Failed to get schema: " + fullName, e);
+    }
+  }


The init method can create up to 3 separate WorkspaceClient instances (lines 153, 158, 164) if statementExecution, tablesApi, and schemasApi are all null. Create the WorkspaceClient once at the top:

if (this.workspaceClient == null) { this.workspaceClient = new WorkspaceClient(buildConfig(databricksConfig)); } if (this.statementExecution == null) { this.statementExecution = workspaceClient.statementExecution(); } ...

vinishjail97 · 2026-02-23T16:11:51Z

+
+  private ExternalCatalogConfig catalogConfig;
+  private DatabricksUnityCatalogConfig databricksConfig;
+  private Configuration hadoopConf;


All fields (catalogConfig, databricksConfig, hadoopConf, tableFormat, workspaceClient, etc.) are mutable and set via init(). This pattern is fragile — calling any method before init() would NPE.

Consider making these final and removing the no-arg constructor, or at least adding a null-check guard in methods that use these fields. The ServiceLoader no-arg constructor + init() pattern should be documented with a warning.

this class is loaded via ServiceLoader, and that requires a public no-arg constructor. Removing it would break runtime plugin loading.

added warning and guards

vinishjail97 · 2026-02-23T16:11:51Z

+        props.get(CLIENT_SECRET),
+        props.get(TOKEN));
+  }
+}


The token field is defined and parsed but the docs say PAT auth is "intentionally not wired" and "not supported yet". If it's not supported, don't add the field — it creates confusion. Remove TOKEN constant and token field until PAT auth is actually implemented.

vinishjail97 · 2026-02-23T16:11:51Z

+
+    <dependencies>
+        <dependency>
+            <groupId>org.apache.xtable</groupId>


Pin the Databricks SDK version to a property in the parent pom.xml instead of hardcoding 0.85.0 here. Follow the pattern used for other dependency versions in the project.

vinishjail97 · 2026-02-23T16:11:51Z

+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance


Tests have massive boilerplate — every test method repeats the same 15-line client construction block. Extract a createClient(String tableFormat) helper method to reduce duplication and improve readability.

vinishjail97 · 2026-02-23T16:11:51Z

+        .thenReturn(
+            new StatementResponse()
+                .setStatus(new StatementStatus().setState(StatementState.SUCCEEDED)));
+


Missing test: refreshTable with matching schema (no-op path). You test the schema-mismatch path but not the case where schemas match and no MSCK REPAIR is issued. Also missing: refreshTable with null catalogTable and refreshTable with null/empty schema.

vinishjail97 · 2026-02-23T16:11:51Z

                        'hms',
                        'glue-catalog',
-                        'unity-catalog',
+                        'databricks-unity-catalog',


The sidebar reference was changed from unity-catalog to databricks-unity-catalog, but the docs file is still named unity-catalog.md (not renamed). This will break the sidebar link. Either rename the md file or keep the sidebar reference as unity-catalog.

…eplace flow

…client against uninitialized use

…chema paths

…c behavior

parisni · 2026-02-26T10:30:29Z

Hi @vinishjail97 thanks for your detailled review, i did update accordingly

vinishjail97 · 2026-03-06T19:00:18Z

+  String host;
+  String warehouseId;
+  String authType;
+  String clientId;


Lombok @Value generates toString() that includes all fields — clientSecret will be printed in any log statement or exception message that calls .toString() on this object, which is a credential leak.

Add @ToString.Exclude on the clientSecret field:

@ToString.Exclude String clientSecret;

vinishjail97 · 2026-03-06T19:00:18Z

+            .setOnWaitTimeout(ExecuteStatementRequestOnWaitTimeout.CANCEL);
+
+    StatementResponse response = statementExecution.executeStatement(request);
+    if (response.getStatus() != null && response.getStatus().getState() == StatementState.FAILED) {


executeStatement only checks StatementState.FAILED. When the 30s wait timeout fires, the SDK cancels the statement and returns StatementState.CANCELLED — not FAILED. This means a timed-out DDL (CREATE TABLE / DROP / MSCK REPAIR) silently returns without throwing, leaving the caller believing the operation succeeded.

Change the check to cover all non-success states:

StatementState state = response.getStatus() == null ? null : response.getStatus().getState(); if (state != null && state != StatementState.SUCCEEDED) { String errorMsg = (response.getStatus().getError() != null) ? response.getStatus().getError().getMessage() : null; throw new CatalogSyncException("Databricks UC statement " + state + ": " + statement + (StringUtils.isBlank(errorMsg) ? "" : " (" + errorMsg + ")")); }

vinishjail97 · 2026-03-06T19:00:18Z

+  }
+
+  @Override
+  public boolean hasDatabase(CatalogTableIdentifier tableIdentifier) {


The catalog+schema validation block (parse HierarchicalTableIdentifier, blank-check catalog, blank-check schema, call validateIdentifierComponent twice) is duplicated verbatim in hasDatabase (lines 129–141) and createDatabase (lines 157–169), and partially again inside getFullName. Consider extracting a private validateAndGetHierarchical(CatalogTableIdentifier) helper that returns the parsed identifier and throws on any validation failure, then have all three methods delegate to it.

vinishjail97 · 2026-03-06T19:00:18Z

+            .setWarehouseId(databricksConfig.getWarehouseId())
+            .setFormat(Format.JSON_ARRAY)
+            .setDisposition(Disposition.INLINE)
+            .setWaitTimeout("30s")


"30s" is a magic string with no ability to tune for slower warehouses or large DDL operations. Define it as a constant:

private static final String STATEMENT_WAIT_TIMEOUT = "30s";

Ideally, expose it as an optional config key in DatabricksUnityCatalogConfig so users can override it.

vinishjail97 · 2026-03-06T19:00:18Z

+    return result;
+  }
+
+  private static final class ColumnSignature {


ColumnSignature manually implements constructor, equals, and hashCode (~28 lines). Annotate with Lombok @Value to eliminate the boilerplate — consistent with the rest of the codebase.

vinishjail97 · 2026-03-06T19:00:18Z

+    }
+  }
+
+  private static String toStructType(InternalSchema schema) {


Nit: toStructType uses a manual StringBuilder + first flag. Prefer:

return schema.getFields().stream() .map(f -> f.getName() + ":" + toSparkSqlType(f)) .collect(Collectors.joining(",", "struct<", ">"));

vinishjail97 · 2026-03-06T19:00:18Z

+    ThreePartHierarchicalTableIdentifier tableIdentifier =
+        new ThreePartHierarchicalTableIdentifier("main", "default", "people");
+    boolean exists = client.hasDatabase(tableIdentifier);
+    assertEquals(true, exists);


Nit: assertEquals(true, exists) → assertTrue(exists).

vinishjail97 · 2026-03-06T19:00:18Z

+    </properties>
+
+    <dependencies>
+        <dependency>


databricks.sdk.version is declared in <properties> here AND in the root pom.xml's <properties> block. The child module's declaration is redundant since the version is already managed by the parent. Remove the <properties> block from this module's pom.

parisni added 11 commits February 11, 2026 14:04

databricks uc skeleton

757a741

implem/test create table

2c323cb

implem/test hasDatabase/getTable

e590d89

implem/test createDatabase

df63007

implem/test create or replace table

9767c5c

implem/test drop table

c5ef4b5

implem: dummy refresh table

0d117ea

not sure if unity catalog refresh by itself the table metadata. will have to investigate for now skip refresh table

implem/test refresh table

af8cc83

doc

0cef043

fix: use msck repair in place of replacetable for refresh

8e971fb

parisni changed the title ~~feat: databricks unity catalog sync~~ feat: built-in databricks unity catalog sync Feb 11, 2026

vinishjail97 reviewed Feb 23, 2026

View reviewed changes

parisni added 9 commits February 26, 2026 10:22

review: prevent SQL injection

370e417

review: Add explicit recreate-failure warning in Databricks createOrR…

a28958d

…eplace flow

review: Extract workspace client init guard into helper method

c2e46e2

review: Document ServiceLoader init contract and guard Databricks UC …

4188a8e

…client against uninitialized use

review: Remove unsupported PAT token config from Databricks UC client

bbe5594

review: Centralize Databricks SDK version as a Maven property

6ec5c8f

review: Add refreshTable no-op coverage for matching and null/empty s…

0964bdd

…chema paths

review: Refactor Databricks UC tests to use shared client setup helper

d13c9dc

review: Fix Databricks Unity Catalog sidebar doc id

575e2b7

parisni requested a review from vinishjail97 February 26, 2026 10:26

review: Refresh Databricks UC client Javadoc to match implemented syn…

7439f20

…c behavior

rm dead code

7a1ea65

vinishjail97 reviewed May 4, 2026

View reviewed changes

Conversation

parisni commented Feb 11, 2026

Important Read

What is the purpose of the pull request

Brief change log

Verify this pull request

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!

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!

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!

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!

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!

parisni commented Feb 26, 2026

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants