Skip to content

feat(csharp/src/Drivers/Databricks): Support server side property passthrough #2692

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.

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

Conversation

alexguo-db
Copy link
Contributor

@alexguo-db alexguo-db commented Apr 10, 2025

We want to be able to pass server side properties through the driver, e.g. use_cached_result.
In ODBC driver, the behavior is like this:

  • If ApplySSPWithQueries = 1, set server side properties using "set x=y" commands during open session
  • If ApplySSPWithQueries = 0, set server side properties using thrift configuration

This PR adds support in the ADBC driver for this

Tested E2E by setting

{ "adbc.databricks.apply_ssp_with_queries", "false" },
{ "adbc.databricks.SSP_use_cached_result", "false"},

which disabled result cache for the executed query.

When using { "adbc.databricks.apply_ssp_with_queries", "true" }, I see a set query in the query history
image

@davidhcoe
Copy link
Contributor

Is there some type of test that can be included to demonstrate this is working correctly?

foreach (var property in serverSideProperties)
{
req.Configuration[property.Key] = property.Value;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder what kind of logs we can add here. @davidhcoe Until the logging/telemetry framework is ready do you recommend we log anything?

@alexguo-db alexguo-db marked this pull request as ready for review April 15, 2025 18:55
@alexguo-db
Copy link
Contributor Author

Is there some type of test that can be included to demonstrate this is working correctly?

I added an E2E test for this

@github-actions github-actions bot added this to the ADBC Libraries 18 milestone Apr 15, 2025
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! Out of curiosity, would we still need this feature if it were possible to reliably execute multiple statements in a single batch?

I don't know how to judge the need for escaping and/or the risk of SQL injection. One risk here is that even if injection is not possible today due to (perhaps) the semantics of SET and/or SparkSQL in general, those semantics might change in the future.

@alexguo-db alexguo-db force-pushed the alex-guo_data/add_server_side_property_passthrough branch from c0b83ce to a79a326 Compare April 17, 2025 22:55
@alexguo-db
Copy link
Contributor Author

Thanks! Out of curiosity, would we still need this feature if it were possible to reliably execute multiple statements in a single batch?

Yes, because we still need the ability to set server side properties through Thrift. In Power BI with ODBC driver, we actually set ApplySSPWithQueries to false by default because it's more efficient. Applying SSP with "SET" queries should only be used when there is a Thrift protocol mismatch, where setting options through Thrift configuration is unsupported

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!

@CurtHagenlocher CurtHagenlocher merged commit 6a60a13 into apache:main Apr 18, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants