CurtHagenlocher commented on code in PR #3380:
URL: https://github.com/apache/arrow-adbc/pull/3380#discussion_r2315939420
##########
csharp/src/Drivers/Apache/Spark/SparkConnection.cs:
##########
@@ -62,6 +63,11 @@ public override AdbcStatement CreateStatement()
return statement;
}
+ protected override Task<TGetResultSetMetadataResp>
GetResultSetMetadataAsync(IResponse response, CancellationToken
cancellationToken = default) =>
+ GetResultSetMetadataAsync(response.OperationHandle!, Client,
cancellationToken);
+ protected override Task<TRowSet> GetRowSetAsync(IResponse response,
CancellationToken cancellationToken = default) =>
+ FetchResultsAsync(response.OperationHandle!, cancellationToken:
cancellationToken);
+
Review Comment:
Given that these three members are now implemented concretely in
`SparkConnection`, can the identical definitions in `SparkHttpConnection` be
removed?
##########
csharp/src/Drivers/Apache/Spark/SparkStandardConnection.cs:
##########
@@ -95,19 +99,69 @@ protected override TTransport CreateTransport()
// Assumption: hostName and port have already been validated.
Properties.TryGetValue(SparkParameters.HostName, out string?
hostName);
Properties.TryGetValue(SparkParameters.Port, out string? port);
+ Properties.TryGetValue(SparkParameters.AuthType, out string?
authType);
+
+ if (!SparkAuthTypeParser.TryParse(authType, out SparkAuthType
authTypeValue))
+ {
+ throw new
ArgumentOutOfRangeException(SparkParameters.AuthType, authType, $"Unsupported
{SparkParameters.AuthType} value.");
+ }
// Delay the open connection until later.
bool connectClient = false;
- TSocketTransport transport = new(hostName!, int.Parse(port!),
connectClient, config: new());
- return transport;
+ int portValue = int.Parse(port!);
+
+ // TLS setup
+ TTransport baseTransport;
+ if (TlsOptions.IsTlsEnabled)
+ {
+ X509Certificate2? trustedCert =
!string.IsNullOrEmpty(TlsOptions.TrustedCertificatePath)
+ ? new X509Certificate2(TlsOptions.TrustedCertificatePath!)
+ : null;
+
+ RemoteCertificateValidationCallback certValidator = (sender,
cert, chain, errors) => HiveServer2TlsImpl.ValidateCertificate(cert, errors,
TlsOptions);
+
+ if (IPAddress.TryParse(hostName!, out var ipAddress))
+ {
+ baseTransport = new TTlsSocketTransport(ipAddress,
portValue, config: new(), 0, trustedCert, certValidator);
+ }
+ else
+ {
+ baseTransport = new TTlsSocketTransport(hostName!,
portValue, config: new(), 0, trustedCert, certValidator);
+ }
+ }
+ else
+ {
+ baseTransport = new TSocketTransport(hostName!, portValue,
connectClient, config: new());
+ }
+ baseTransport = new TSocketTransport(hostName!, portValue,
connectClient, config: new());
Review Comment:
This overwrites the value of `baseTransport` assigned above and would
seemingly prevent TLS from working.
##########
csharp/src/Drivers/Apache/Spark/SparkStandardConnection.cs:
##########
@@ -95,19 +99,69 @@ protected override TTransport CreateTransport()
// Assumption: hostName and port have already been validated.
Properties.TryGetValue(SparkParameters.HostName, out string?
hostName);
Properties.TryGetValue(SparkParameters.Port, out string? port);
+ Properties.TryGetValue(SparkParameters.AuthType, out string?
authType);
Review Comment:
There's a comment here that the port has already been validated, but the
implementation of `ValidateConnection` doesn't seem to throw an exception if
the port number is missing or invalid.
##########
csharp/src/Drivers/Apache/Spark/SparkStandardConnection.cs:
##########
@@ -139,10 +197,23 @@ protected override TOpenSessionReq CreateSessionRequest()
return request;
}
+ protected override void ValidateOptions()
+ {
+ Properties.TryGetValue(SparkParameters.DataTypeConv, out string?
dataTypeConv);
+ DataTypeConversion = DataTypeConversionParser.Parse(dataTypeConv);
+ TlsOptions = HiveServer2TlsImpl.GetStandardTlsOptions(Properties);
+ }
+
+ internal override IArrowArrayStream NewReader<T>(T statement, Schema
schema, IResponse response, TGetResultSetMetadataResp? metadataResp = null) =>
+ new HiveServer2Reader(statement, schema, response,
dataTypeConversion: statement.Connection.DataTypeConversion);
+
+
Review Comment:
nit: extra blank line
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
