CurtHagenlocher commented on code in PR #2540:
URL: https://github.com/apache/arrow-adbc/pull/2540#discussion_r1977773138
##########
csharp/src/Drivers/Apache/Spark/SparkConnectionFactory.cs:
##########
@@ -24,16 +24,23 @@ internal class SparkConnectionFactory
{
public static SparkConnection
NewConnection(IReadOnlyDictionary<string, string> properties)
{
- bool _ = properties.TryGetValue(SparkParameters.Type, out string?
type) && string.IsNullOrEmpty(type);
- bool __ = ServerTypeParser.TryParse(type, out SparkServerType
serverTypeValue);
+ if (!properties.TryGetValue(SparkParameters.Type, out string?
type) && string.IsNullOrEmpty(type))
+ {
+ throw new ArgumentException($"Required property
'{SparkParameters.Type}' is missing. Supported types:
{ServerTypeParser.SupportedList}", nameof(properties));
+ }
+ if (!ServerTypeParser.TryParse(type, out SparkServerType
serverTypeValue))
+ {
+ throw new ArgumentOutOfRangeException(nameof(properties),
$"Unsupported or unknown value '{type}' given for property
'{SparkParameters.Type}'. Supported types: {ServerTypeParser.SupportedList}");
+ }
+
return serverTypeValue switch
{
SparkServerType.Databricks => new
SparkDatabricksConnection(properties),
SparkServerType.Http => new SparkHttpConnection(properties),
// TODO: Re-enable when properly supported
//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}"),
+
Review Comment:
nit: extra blank line
##########
csharp/src/Drivers/Apache/Hive2/HiveServer2HttpConnection.cs:
##########
@@ -0,0 +1,342 @@
+/*
+* Licensed to the Apache Software Foundation (ASF) under one or more
+* contributor license agreements. See the NOTICE file distributed with
+* this work for additional information regarding copyright ownership.
+* The ASF licenses this file to You under the Apache License, Version 2.0
+* (the "License"); you may not use this file except in compliance with
+* the License. You may obtain a copy of the License at
+*
+* http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing, software
+* distributed under the License is distributed on an "AS IS" BASIS,
+* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+* See the License for the specific language governing permissions and
+* limitations under the License.
+*/
+
+using System;
+using System.Collections.Generic;
+using System.Globalization;
+using System.Net;
+using System.Net.Http;
+using System.Net.Http.Headers;
+using System.Net.Security;
+using System.Text;
+using System.Threading;
+using System.Threading.Tasks;
+using Apache.Arrow.Ipc;
+using Apache.Hive.Service.Rpc.Thrift;
+using Thrift;
+using Thrift.Protocol;
+using Thrift.Transport;
+
+namespace Apache.Arrow.Adbc.Drivers.Apache.Hive2
+{
+ internal class HiveServer2HttpConnection : HiveServer2Connection
+ {
+ private const string ProductVersionDefault = "1.0.0";
+ private const string DriverName = "ADBC Hive Driver";
+ private const string ArrowVersion = "1.0.0";
+ private const string BasicAuthenticationScheme = "Basic";
+ private readonly Lazy<string> _productVersion;
+ private static readonly string s_userAgent = $"{DriverName.Replace("
", "")}/{ProductVersionDefault}";
+
+ protected override string GetProductVersionDefault() =>
ProductVersionDefault;
+
+ protected override string ProductVersion => _productVersion.Value;
+
+ public HiveServer2HttpConnection(IReadOnlyDictionary<string, string>
properties) : base(properties)
+ {
+ ValidateProperties();
+ _productVersion = new Lazy<string>(() => GetProductVersion(),
LazyThreadSafetyMode.PublicationOnly);
+ }
+
+ private void ValidateProperties()
+ {
+ ValidateAuthentication();
+ ValidateConnection();
+ ValidateOptions();
+ }
+
+ private void ValidateAuthentication()
+ {
+ // Validate authentication parameters
+ Properties.TryGetValue(AdbcOptions.Username, out string? username);
+ Properties.TryGetValue(AdbcOptions.Password, out string? password);
+ Properties.TryGetValue(HiveServer2Parameters.AuthType, out string?
authType);
+ bool isValidAuthType =
HiveServer2AuthTypeParser.TryParse(authType, out HiveServer2AuthType
authTypeValue);
+ switch (authTypeValue)
+ {
+ case HiveServer2AuthType.Basic:
+ if (string.IsNullOrWhiteSpace(username) ||
string.IsNullOrWhiteSpace(password))
+ throw new ArgumentException(
+ $"Parameter '{HiveServer2Parameters.AuthType}' is
set to '{HiveServer2AuthTypeConstants.Basic}' but parameters
'{AdbcOptions.Username}' or '{AdbcOptions.Password}' are not set. Please
provide a values for these parameters.",
+ nameof(Properties));
+ break;
+ case HiveServer2AuthType.UsernameOnly:
+ if (string.IsNullOrWhiteSpace(username))
+ throw new ArgumentException(
+ $"Parameter '{HiveServer2Parameters.AuthType}' is
set to '{HiveServer2AuthTypeConstants.UsernameOnly}' but parameter
'{AdbcOptions.Username}' is not set. Please provide a values for this
parameter.",
+ nameof(Properties));
+ break;
+ case HiveServer2AuthType.None:
+ break;
+ case HiveServer2AuthType.Empty:
+ if (string.IsNullOrWhiteSpace(username) ||
string.IsNullOrWhiteSpace(password))
+ throw new ArgumentException(
+ $"Parameters must include valid authentication
settings. Please provide '{AdbcOptions.Username}' and
'{AdbcOptions.Password}'.",
+ nameof(Properties));
+ break;
+ default:
+ throw new
ArgumentOutOfRangeException(HiveServer2Parameters.AuthType, authType,
$"Unsupported {HiveServer2Parameters.AuthType} value.");
+ }
+ }
+
+ private void ValidateConnection()
+ {
+ // HostName or Uri is required parameter
+ Properties.TryGetValue(AdbcOptions.Uri, out string? uri);
+ Properties.TryGetValue(HiveServer2Parameters.HostName, out string?
hostName);
+ if ((Uri.CheckHostName(hostName) == UriHostNameType.Unknown)
+ && (string.IsNullOrEmpty(uri) || !Uri.TryCreate(uri,
UriKind.Absolute, out Uri? _)))
+ {
+ throw new ArgumentException(
+ $"Required parameter '{HiveServer2Parameters.HostName}' or
'{AdbcOptions.Uri}' is missing or invalid. Please provide a valid hostname or
URI for the data source.",
+ nameof(Properties));
+ }
+
+ // Validate port range
+ Properties.TryGetValue(HiveServer2Parameters.Port, out string?
port);
+ if (int.TryParse(port, out int portNumber) && (portNumber <=
IPEndPoint.MinPort || portNumber > IPEndPoint.MaxPort))
+ throw new ArgumentOutOfRangeException(
+ nameof(Properties),
+ port,
+ $"Parameter '{HiveServer2Parameters.Port}' value is not in
the valid range of 1 .. {IPEndPoint.MaxPort}.");
+
+ // Ensure the parameters will produce a valid address
+ Properties.TryGetValue(HiveServer2Parameters.Path, out string?
path);
+ _ = new HttpClient()
+ {
+ BaseAddress = GetBaseAddress(uri, hostName, path, port,
HiveServer2Parameters.HostName)
+ };
+ }
+
+ private void ValidateOptions()
+ {
+ Properties.TryGetValue(HiveServer2Parameters.DataTypeConv, out
string? dataTypeConv);
+ DataTypeConversion = DataTypeConversionParser.Parse(dataTypeConv);
+ Properties.TryGetValue(HiveServer2Parameters.TLSOptions, out
string? tlsOptions);
+ TlsOptions = TlsOptionsParser.Parse(tlsOptions);
+
Properties.TryGetValue(HiveServer2Parameters.ConnectTimeoutMilliseconds, out
string? connectTimeoutMs);
+ if (connectTimeoutMs != null)
+ {
+ ConnectTimeoutMilliseconds = int.TryParse(connectTimeoutMs,
NumberStyles.Integer, CultureInfo.InvariantCulture, out int
connectTimeoutMsValue) && (connectTimeoutMsValue >= 0)
+ ? connectTimeoutMsValue
+ : throw new
ArgumentOutOfRangeException(HiveServer2Parameters.ConnectTimeoutMilliseconds,
connectTimeoutMs, $"must be a value of 0 (infinite) or between 1 ..
{int.MaxValue}. default is 30000 milliseconds.");
+ }
+ }
+
+ public override AdbcStatement CreateStatement()
+ {
+ return new HiveServer2Statement(this);
+ }
+
+ internal override IArrowArrayStream NewReader<T>(T statement, Schema
schema) => new HiveServer2Reader(
+ statement,
+ schema,
+ dataTypeConversion: statement.Connection.DataTypeConversion,
+ enableBatchSizeStopCondition: false);
Review Comment:
nit: this block has an extra level of indentation
##########
csharp/src/Drivers/Apache/Hive2/HiveServer2AuthType.cs:
##########
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+namespace Apache.Arrow.Adbc.Drivers.Apache.Hive2
+{
+ internal enum HiveServer2AuthType
+ {
+ Invalid = 0,
Review 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.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
