feat(csharp/src/Drivers/Databricks): Make Cloud Fetch options configurable at the connection level#2691
Conversation
…rable at driver level
alexguo-db
changed the title
| if (Properties.TryGetValue(DatabricksParameters.UseCloudFetch, out string? useCloudFetchStr) && | ||
| bool.TryParse(useCloudFetchStr, out bool useCloudFetchValue)) | ||
| { | ||
| _useCloudFetch = useCloudFetchValue; |
There was a problem hiding this comment.
Consider validating the string property. Right now, if someone were to typo as e.g. properties["CloudFetch"] = "flase"; the property value would be silently ignored. (Similarly, properties["CloudFetc"] = "false"; would be ignored but I'm not sure how consistently drivers are producing errors for unsupported properties.)
There was a problem hiding this comment.
It's unfortunate that we're essentially validating these in two places: both here and in DatabricksStatement.
We've also somehow arrived in a weird place where these values are both connection properties (that can only be set before the connection is opened) and statement properties that can be set any time after the statement was created. But that's a preexisting condition.
CurtHagenlocher
left a comment
CurtHagenlocher
left a comment
There was a problem hiding this comment.
Thanks! The change looks fine overall. The preexisting way that properties are handled in the HiveServer2-based drivers seems decidedly non-ideal, but that can't be blamed on this PR. I do think we should validate the boolean parsing by throwing an exception when parsing fails; that would be both a better user experience and more consistent with the rest of the code.
| if (Properties.TryGetValue(DatabricksParameters.UseCloudFetch, out string? useCloudFetchStr) && | ||
| bool.TryParse(useCloudFetchStr, out bool useCloudFetchValue)) | ||
| { | ||
| _useCloudFetch = useCloudFetchValue; |
There was a problem hiding this comment.
It's unfortunate that we're essentially validating these in two places: both here and in DatabricksStatement.
We've also somehow arrived in a weird place where these values are both connection properties (that can only be set before the connection is opened) and statement properties that can be set any time after the statement was created. But that's a preexisting condition.
alexguo-db
force-pushed
the
alex-guo_data/make-cloudfetch-driver-configurable
branch
from
e7488d5 to
44f6853
Compare
CurtHagenlocher
left a comment
CurtHagenlocher
left a comment
There was a problem hiding this comment.
Thanks! Unfortunately, another change appears to have introduced a merge conflict. I'll check this in once that's resolved.
…rable at the connection level (apache#2691) - Currently, cloud fetch enablement is only configurable at the statement level, and this is not exposed to clients outside of arrow-adbc repo - Make these options configurable at the driver/connection level Tested E2E by adding a log for the result set format and whether lz4 compression is enabled - `{ "adbc.databricks.cloudfetch.enabled", "false" }` - `DatabricksConnection.NewReader: resultFormat=ARROW_BASED_SET, isLz4Compressed=True` - `{ "adbc.databricks.cloudfetch.enabled", "false" }, { "adbc.databricks.cloudfetch.lz4.enabled", "false" },` - `DatabricksConnection.NewReader: resultFormat=ARROW_BASED_SET, isLz4Compressed=False` - empty - `DatabricksConnection.NewReader: resultFormat=URL_BASED_SET, isLz4Compressed=True` - `{ "adbc.databricks.cloudfetch.lz4.enabled", "false" },` - `DatabricksConnection.NewReader: resultFormat=URL_BASED_SET, isLz4Compressed=False` Also tested with `dotnet test --filter "FullyQualifiedName~CloudFetchE2ETest"`
3 participants
Tested E2E by adding a log for the result set format and whether lz4 compression is enabled
{ "adbc.databricks.cloudfetch.enabled", "false" }DatabricksConnection.NewReader: resultFormat=ARROW_BASED_SET, isLz4Compressed=True{ "adbc.databricks.cloudfetch.enabled", "false" }, { "adbc.databricks.cloudfetch.lz4.enabled", "false" },DatabricksConnection.NewReader: resultFormat=ARROW_BASED_SET, isLz4Compressed=Falseempty
DatabricksConnection.NewReader: resultFormat=URL_BASED_SET, isLz4Compressed=True{ "adbc.databricks.cloudfetch.lz4.enabled", "false" },DatabricksConnection.NewReader: resultFormat=URL_BASED_SET, isLz4Compressed=FalseAlso tested with
dotnet test --filter "FullyQualifiedName~CloudFetchE2ETest"