CurtHagenlocher commented on code in PR #1482:
URL: https://github.com/apache/arrow-adbc/pull/1482#discussion_r1463410410
##########
csharp/test/Apache.Arrow.Adbc.Tests/ClientTests.cs:
##########
@@ -212,7 +212,29 @@ public static void
VerifyTypesAndValues(Adbc.Client.AdbcConnection adbcConnectio
if (value != null)
{
- if (!value.GetType().BaseType.Name.Contains("PrimitiveArray"))
+ if (value is IList && !(value is byte[]))
+ {
+ IList list = value as IList;
+ IList expectedList = ctv.ExpectedValue as IList;
+ int i = -1;
+
+ foreach (var actual in list)
+ {
+ i++;
+ int j = -1;
+
+ foreach (var expected in expectedList)
Review Comment:
This seems an odd way to compare two lists. In addition to having two
counters instead of one, it doesn't take the length into account which means an
empty list matches anything. If this is deliberate then it would be good to add
a comment explaining the "why" of the logic.
##########
csharp/src/Drivers/BigQuery/BigQueryConnection.cs:
##########
@@ -70,32 +70,30 @@ public BigQueryConnection(IReadOnlyDictionary<string,
string> properties)
/// <exception cref="ArgumentException"></exception>
internal void Open()
{
- string projectId = string.Empty;
- string clientId = string.Empty;
- string clientSecret = string.Empty;
- string refreshToken = string.Empty;
+ string? projectId = string.Empty;
+ string? clientId = string.Empty;
+ string? clientSecret = string.Empty;
+ string? refreshToken = string.Empty;
Review Comment:
Does it make more sense to initialize these to null now that they're marked
as nullable?
##########
csharp/src/Drivers/BigQuery/BigQueryConnection.cs:
##########
@@ -764,22 +807,44 @@ private Field DescToField(BigQueryRow row)
{
Dictionary<string, string> metaData = new Dictionary<string,
string>();
metaData.Add("PRIMARY_KEY", "");
- metaData.Add("ORDINAL_POSITION",
row["ordinal_position"].ToString());
- metaData.Add("DATA_TYPE", row["data_type"].ToString());
+ metaData.Add("ORDINAL_POSITION",
GetValue(row["ordinal_position"]));
+ metaData.Add("DATA_TYPE", GetValue(row["data_type"]));
- Field.Builder fieldBuilder =
SchemaFieldGenerator(row["column_name"].ToString().ToLower(),
row["data_type"].ToString());
+ Field.Builder fieldBuilder =
SchemaFieldGenerator(GetValue(row["column_name"]).ToLower(),
GetValue(row["data_type"]));
fieldBuilder.Metadata(metaData);
- if (!row["is_nullable"].ToString().Equals("YES",
StringComparison.OrdinalIgnoreCase))
+ if (!GetValue(row["is_nullable"]).Equals("YES",
StringComparison.OrdinalIgnoreCase))
{
fieldBuilder.Nullable(false);
}
- fieldBuilder.Name(row["column_name"].ToString().ToLower());
+ fieldBuilder.Name(GetValue(row["column_name"]).ToLower());
return fieldBuilder.Build();
}
+ private string GetValue(object value)
+ {
+ switch (value)
+ {
+ case string sValue:
+ if (string.IsNullOrEmpty(sValue))
+ return string.Empty;
Review Comment:
We can't reach this line unless sValue is empty
##########
csharp/src/Drivers/BigQuery/BigQueryConnection.cs:
##########
@@ -764,22 +807,44 @@ private Field DescToField(BigQueryRow row)
{
Dictionary<string, string> metaData = new Dictionary<string,
string>();
metaData.Add("PRIMARY_KEY", "");
- metaData.Add("ORDINAL_POSITION",
row["ordinal_position"].ToString());
- metaData.Add("DATA_TYPE", row["data_type"].ToString());
+ metaData.Add("ORDINAL_POSITION",
GetValue(row["ordinal_position"]));
+ metaData.Add("DATA_TYPE", GetValue(row["data_type"]));
- Field.Builder fieldBuilder =
SchemaFieldGenerator(row["column_name"].ToString().ToLower(),
row["data_type"].ToString());
+ Field.Builder fieldBuilder =
SchemaFieldGenerator(GetValue(row["column_name"]).ToLower(),
GetValue(row["data_type"]));
fieldBuilder.Metadata(metaData);
- if (!row["is_nullable"].ToString().Equals("YES",
StringComparison.OrdinalIgnoreCase))
+ if (!GetValue(row["is_nullable"]).Equals("YES",
StringComparison.OrdinalIgnoreCase))
{
fieldBuilder.Nullable(false);
}
- fieldBuilder.Name(row["column_name"].ToString().ToLower());
+ fieldBuilder.Name(GetValue(row["column_name"]).ToLower());
return fieldBuilder.Build();
}
+ private string GetValue(object value)
+ {
+ switch (value)
+ {
+ case string sValue:
+ if (string.IsNullOrEmpty(sValue))
+ return string.Empty;
+ return sValue;
+ default:
+ if (value != null)
+ {
+ string? sValue = value.ToString();
+
+ if (string.IsNullOrEmpty(sValue))
+ return string.Empty;
Review Comment:
could be simplified to "return sValue ?? string.Empty;" though I think we
probably shouldn't call it a success if ToString returns null.
##########
csharp/src/Drivers/BigQuery/BigQueryConnection.cs:
##########
@@ -106,21 +104,43 @@ internal void Open()
if
(!this.properties.TryGetValue(BigQueryParameters.RefreshToken, out
refreshToken))
throw new ArgumentException($"The
{BigQueryParameters.RefreshToken} parameter is not present");
- this.credential =
GoogleCredential.FromAccessToken(GetAccessToken(clientId, clientSecret,
refreshToken, tokenEndpoint));
+ this.credential =
ApplyScopes(GoogleCredential.FromAccessToken(GetAccessToken(clientId,
clientSecret, refreshToken, tokenEndpoint)));
}
else
{
- string json = string.Empty;
+ string? json = string.Empty;
if
(!this.properties.TryGetValue(BigQueryParameters.JsonCredential, out json))
throw new ArgumentException($"The
{BigQueryParameters.JsonCredential} parameter is not present");
- this.credential = GoogleCredential.FromJson(json);
+ this.credential = ApplyScopes(GoogleCredential.FromJson(json));
}
this.client = BigQueryClient.Create(projectId, this.credential);
}
+ /// <summary>
+ /// Apply any additional scopes to the credential.
+ /// </summary>
+ /// <param name="credential"><see cref="GoogleCredential"/></param>
+ /// <returns></returns>
+ private GoogleCredential ApplyScopes(GoogleCredential credential)
+ {
+ if (credential == null) throw new
ArgumentNullException(nameof(credential));
+
+ if (this.properties.TryGetValue(BigQueryParameters.Scopes, out
string? scopes))
+ {
+ if (!string.IsNullOrEmpty(scopes))
+ {
+ IEnumerable<string> parsedScopes =
scopes.Split(',').Where(x => x.Length > 0);
Review Comment:
This doesn't seem to be used anywhere
##########
csharp/test/Drivers/BigQuery/BigQueryTestingUtils.cs:
##########
@@ -54,11 +54,26 @@ BigQueryTestConfiguration testConfiguration
Dictionary<string, string> parameters = new Dictionary<string,
string>
{
{ BigQueryParameters.ProjectId, testConfiguration.ProjectId },
- { BigQueryParameters.ClientId, testConfiguration.ClientId },
- { BigQueryParameters.ClientSecret,
testConfiguration.ClientSecret},
- { BigQueryParameters.RefreshToken,
testConfiguration.RefreshToken}
};
+ if(!string.IsNullOrEmpty(testConfiguration.JsonCredential))
Review Comment:
nit: space before (
--
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]
