Skip to content

feat(csharp/src/Drivers/Databricks): Default catalog + schema support #2806

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
CurtHagenlocher merged 5 commits into from
May 20, 2025
Merged

feat(csharp/src/Drivers/Databricks): Default catalog + schema support #2806

CurtHagenlocher merged 5 commits into from
May 20, 2025

Conversation

@toddmeng-db
Copy link
Contributor

@toddmeng-db toddmeng-db commented May 9, 2025 •
edited
Loading

Allows users to specify an optional default catalog and default schema that will take affect throughout the session.

Includes tests that verifies basic behavior. To test, include in the DATABRICKS_TEST_CONFIG_FILE environment variable:

  "catalog": "system",
  "db_schema": "access",

NOTE: using Namespace to set catalogs is only in DBR 8.4+. We will need to introduce fallbacks (todo in a different PR)

@toddmeng-db toddmeng-db force-pushed the toddmeng-db/default-namespace branch 5 times, most recently from ac168eb to 75256e8 Compare May 9, 2025 20:52
@toddmeng-db toddmeng-db changed the title feat(csharp/src/Drivers/Databricks): Default namespace support feat(csharp/src/Drivers/Databricks): Default catalog + schema support May 9, 2025
@toddmeng-db toddmeng-db force-pushed the toddmeng-db/default-namespace branch from 75256e8 to 1f7a279 Compare May 9, 2025 20:56
@toddmeng-db toddmeng-db marked this pull request as ready for review May 9, 2025 21:28
@github-actions github-actions bot added this to the ADBC Libraries 19 milestone May 9, 2025
@toddmeng-db toddmeng-db marked this pull request as draft May 9, 2025 21:33
jadewang-db
jadewang-db approved these changes May 13, 2025
View reviewed changes
csharp/src/Drivers/Databricks/DatabricksConnection.cs Outdated
Properties.TryGetValue(DatabricksParameters.DefaultCatalog, out defaultCatalog);
Properties.TryGetValue(DatabricksParameters.DefaultSchema, out defaultSchema);

if (!string.IsNullOrEmpty(defaultCatalog))
Copy link
Contributor

@jadewang-db jadewang-db May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use string.IsNullOrWhitespace

@toddmeng-db toddmeng-db marked this pull request as ready for review May 16, 2025 21:38
CurtHagenlocher
CurtHagenlocher requested changes May 19, 2025
View reviewed changes
Copy link
Contributor

@CurtHagenlocher CurtHagenlocher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! New properties should be described in readme.md. One thing that I find non-obvious is the relationship between these properties and the standard ADBC properties adbc.connection.catalog and adbc.connection.db_schema. It looks like those properties aren't supported by the driver at all, whereas these Databricks-specific properties are only supported when creating a new session. Can the current catalog or schema be changed after the session is established? And at that point the initial values don't matter any more?

I think some clarity is required here in the docs even if the code doesn't change.

csharp/test/Drivers/Databricks/DatabricksConnectionTest.cs Outdated
using Thrift.Transport;
using Xunit;
using Xunit.Abstractions;
using System.Reflection;
Copy link
Contributor

@CurtHagenlocher CurtHagenlocher May 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: namespace ordering

csharp/test/Drivers/Databricks/DatabricksConnectionTest.cs Outdated
Assert.NotNull(connection);
Assert.IsType<DatabricksConnection>(connection);

var defaultNamespaceProperty = typeof(DatabricksConnection).GetProperty("DefaultNamespace", BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.Public);
Copy link
Contributor

@CurtHagenlocher CurtHagenlocher May 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With InternalsVisibleToAttribute and an internal property, it shouldn't be necessary to use Reflection to get at this property value.

@CurtHagenlocher
Copy link
Contributor

CurtHagenlocher commented May 19, 2025

I think my expectation as an ADBC user would be that if I set adbc.connection.catalog and adbc.connection.db_schema before the connection is opened, that it would use those values to populate the initial namespace. And if I set them after the connection was opened, I'd get whatever it takes to update the session value (e.g. USE CATALOG catalog_name).

@toddmeng-db
Copy link
Contributor Author

toddmeng-db commented May 19, 2025 •
edited
Loading

I think my expectation as an ADBC user would be that if I set adbc.connection.catalog and adbc.connection.db_schema before the connection is opened, that it would use those values to populate the initial namespace.

Makes sense - sorry I didn't even realize that these parameters were already there 😅

@eric-wang-1990
Copy link
Contributor

eric-wang-1990 commented May 19, 2025

I think my expectation as an ADBC user would be that if I set adbc.connection.catalog and adbc.connection.db_schema before the connection is opened, that it would use those values to populate the initial namespace. And if I set them after the connection was opened, I'd get whatever it takes to update the session value (e.g. USE CATALOG catalog_name).

This initial catalog/schema should only be used when opening session, in powerbi when user signed in and pick the initial catalog/schema there is no way to change that unless user make brand new connection.
The way we setting those parameter is in OpenSession, which is considered faster than a explictly SET catalog=x command. I think until we receive real needs of modifying the initial catalog/schema after connection is established, we can assume it only works for when connection is established.

@toddmeng-db toddmeng-db force-pushed the toddmeng-db/default-namespace branch from 44aee95 to 702f174 Compare May 19, 2025 22:09
@toddmeng-db toddmeng-db force-pushed the toddmeng-db/default-namespace branch from 702f174 to 1a52d88 Compare May 19, 2025 22:19
@toddmeng-db toddmeng-db force-pushed the toddmeng-db/default-namespace branch from 7c25e5c to adda68f Compare May 19, 2025 23:48
@CurtHagenlocher
Copy link
Contributor

CurtHagenlocher commented May 20, 2025

This initial catalog/schema should only be used when opening session, in powerbi when user signed in and pick the initial catalog/schema there is no way to change that unless user make brand new connection. The way we setting those parameter is in OpenSession, which is considered faster than a explictly SET catalog=x command. I think until we receive real needs of modifying the initial catalog/schema after connection is established, we can assume it only works for when connection is established.

Notwithstanding that (at least in principle) this driver should be useful outside of a narrow Power BI context, Power BI does make use of the ability to set the current catalog; it's a prerequisite for being able to use Value.NativeQuery with pooled connections.

@CurtHagenlocher CurtHagenlocher merged commit 80f4404 into apache:main May 20, 2025
7 checks passed
@serramatutu serramatutu mentioned this pull request Aug 12, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@CurtHagenlocher CurtHagenlocher CurtHagenlocher approved these changes

+1 more reviewer

@jadewang-db jadewang-db jadewang-db approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

ADBC Libraries 19

Development

Successfully merging this pull request may close these issues.

4 participants

@toddmeng-db @CurtHagenlocher @eric-wang-1990 @jadewang-db