feat(csharp/src/Drivers/Apache): Add support for Hive ADBC Driver with unit tests #2540
Conversation
kushaman
commented
Feb 19, 2025
kushaman
commented
- Added support for the Hive ADBC driver with HTTP transport protocol.
- Added new unit tests for Hive ADBC driver support.
Remove trailing spaces.
| { | ||
| internal enum HiveServer2AuthType | ||
| { | ||
| Invalid = 0, |
There was a problem hiding this comment.
Are there tests for each of these types?
There was a problem hiding this comment.
@davidhcoe - Empty helps the caller differentiate between when no value is associated with the option and Invalid when the option value is unexpected and but not empty. As well, the first item in the enum, 0 (zero) is the 'default' for an enum - again to handle the failed cases.
There was a problem hiding this comment.
This strikes me as somewhat unidiomatic in that it leads to consuming code which ignores the boolean result. For me, at least, a "Try" method that ignores the result value is a big red flag.
There was a problem hiding this comment.
It's equally nonidiomatic to even have the Invalid enum member. Typically, I'd expect the TryParse method to return some arbitrary output value when the return value of the function is false. And if it's necessary to store something like a "Value or perhaps undefined" I'd expect e.g. a Nullable<T>.
That said, this is an internal function and not part of the public API and so it could be cleaned up and/or made more idiomatic later.
CurtHagenlocher
left a comment
CurtHagenlocher
left a comment
There was a problem hiding this comment.
Thanks! I left some comments.
| _ => throw new ArgumentOutOfRangeException(nameof(properties), $"Unsupported or unknown value '{type}' given for property '{HiveServer2Parameters.Type}'. Supported types: {HiveServer2TypeParser.SupportedList}"), | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
nit: extra blank line
There was a problem hiding this comment.
There's still an extra blank line in the latest iteration.
| { | ||
| internal enum HiveServer2AuthType | ||
| { | ||
| Invalid = 0, |
There was a problem hiding this comment.
This strikes me as somewhat unidiomatic in that it leads to consuming code which ignores the boolean result. For me, at least, a "Try" method that ignores the result value is a big red flag.
csharp/src/Drivers/Apache/Hive2/HiveServer2ConnectionFactory.cs
Outdated
Show resolved
Hide resolved
|
@CurtHagenlocher - PR is ready for review. |
CurtHagenlocher
left a comment
CurtHagenlocher
left a comment
There was a problem hiding this comment.
Thanks again! I've left a few comments around formatting and other nitpicky things. They don't need to block checkin if you'd prefer to address them in a followup. Let me know... .
| //SparkServerType.Standard => new SparkStandardConnection(properties), | ||
| SparkServerType.Empty => throw new ArgumentException($"Required property '{SparkParameters.Type}' is missing. Supported types: {ServerTypeParser.SupportedList}", nameof(properties)), | ||
| _ => throw new ArgumentOutOfRangeException(nameof(properties), $"Unsupported or unknown value '{type}' given for property '{SparkParameters.Type}'. Supported types: {ServerTypeParser.SupportedList}"), | ||
|
|
There was a problem hiding this comment.
nit: extra blank line
There was a problem hiding this comment.
Will do this as a follow up. Created ticket for this :#2569.
| statement, | ||
| schema, | ||
| dataTypeConversion: statement.Connection.DataTypeConversion, | ||
| enableBatchSizeStopCondition: false); |
There was a problem hiding this comment.
nit: this block has an extra level of indentation
There was a problem hiding this comment.
Will do this as a follow up. Created ticket for this #2569.
| { | ||
| internal enum HiveServer2AuthType | ||
| { | ||
| Invalid = 0, |
There was a problem hiding this comment.
It's equally nonidiomatic to even have the Invalid enum member. Typically, I'd expect the TryParse method to return some arbitrary output value when the return value of the function is false. And if it's necessary to store something like a "Value or perhaps undefined" I'd expect e.g. a Nullable<T>.
That said, this is an internal function and not part of the public API and so it could be cleaned up and/or made more idiomatic later.
|
@CurtHagenlocher, Created ticket to follow-up on the above comments #2569. Please help to merge the change. |
6 participants
