CurtHagenlocher commented on code in PR #2587:
URL: https://github.com/apache/arrow-adbc/pull/2587#discussion_r1986088682
##########
csharp/src/Drivers/BigQuery/BigQueryStatement.cs:
##########
@@ -349,15 +374,27 @@ sealed class ReadRowsStream : Stream
ReadOnlyMemory<byte> currentBuffer;
bool first;
int position;
+ bool hasRows = true;
Review Comment:
Consider moving both initialization cases into ctor instead of splitting
between the two.
##########
csharp/test/Apache.Arrow.Adbc.Tests/ClientTests.cs:
##########
@@ -268,7 +277,25 @@ static void AssertTypeAndValue(
}
else
{
- Assert.True(ctv.ExpectedValue.Equals(value),
Utils.FormatMessage($"Expected value [{ctv.ExpectedValue}] does not match
actual value [{value}] for {ctv.Name} for query [{query}]", environmentName));
+ bool areEqual = false;
+
+ if (value is ExpandoObject)
Review Comment:
Is this missing a case for when the result is an array?
##########
csharp/test/Apache.Arrow.Adbc.Tests/ClientTests.cs:
##########
@@ -304,5 +331,49 @@ static void AssertTypeAndValue(
Assert.True(ctv.ExpectedValue == null,
Utils.FormatMessage($"The value for {ctv.Name} is null and but it's expected
value is not null for query [{query}]", environmentName));
}
}
+
+ static bool AreExpandoObjectsEqual(ExpandoObject? obj1, ExpandoObject?
obj2)
+ {
+ if (obj1 == null && obj2 == null)
+ {
+ return true;
+ }
+ else if (obj1 != null && obj2 == null)
+ {
+ return false;
+ }
+ else if (obj1 == null && obj2 != null)
+ {
+ return false;
+ }
+
+ var dict1 = (IDictionary<string, object?>)obj1!;
+ var dict2 = (IDictionary<string, object?>)obj2!;
+
+ if (dict1.Count != dict2.Count)
+ return false;
+
+ foreach (var key in dict1.Keys)
Review Comment:
I know this is test code and therefore non-performance-critical, but I also
think it's good to default to performant code when there's no cost to
readability. (Plus, I feel like I've written this function dozens of times and
so this stands out for me :).)
This can be done more efficiently with
```
foreach (var pair in dict1)
{
if (!dict2.TryGetValue(pair.Key, out object? value2))
return false;
...
}
```
##########
csharp/src/Drivers/BigQuery/BigQueryStatement.cs:
##########
@@ -349,15 +374,27 @@ sealed class ReadRowsStream : Stream
ReadOnlyMemory<byte> currentBuffer;
bool first;
int position;
+ bool hasRows = true;
public ReadRowsStream(IAsyncEnumerator<ReadRowsResponse> response)
{
if (!response.MoveNextAsync().Result) { }
- this.currentBuffer =
response.Current.ArrowSchema.SerializedSchema.Memory;
+
+ if (response.Current != null)
+ {
+ this.currentBuffer =
response.Current.ArrowSchema.SerializedSchema.Memory;
+ }
+ else
+ {
+ this.hasRows = false;
+ }
+
this.response = response;
this.first = true;
}
+ public bool HasRows { get => this.hasRows; }
Review Comment:
```suggestion
public bool HasRows => this.hasRows;
```
##########
csharp/src/Apache.Arrow.Adbc/Extensions/IArrowArrayExtensions.cs:
##########
@@ -273,20 +299,22 @@ public static class IArrowArrayExtensions
/// </summary>
private static string SerializeToJson(StructArray structArray, int
index)
{
- Dictionary<String, object?>? jsonDictionary =
ParseStructArray(structArray, index);
+ ExpandoObject? obj = ParseStructArray(structArray, index);
Review Comment:
What's the advantage of `ExpandoObject` over `Dictionary`? Do we think that
people will want to use these as `dynamic`?
(Also, this is arguably a breaking change though in this case it can be
argued that the previous behavior was broken in that we didn't specify the
"correct" type being returned.)
##########
csharp/src/Apache.Arrow.Adbc/Extensions/IArrowArrayExtensions.cs:
##########
@@ -198,7 +208,23 @@ public static class IArrowArrayExtensions
case ArrowTypeId.Int64:
return (array, index) =>
((Int64Array)array).GetValue(index);
case ArrowTypeId.String:
- return (array, index) =>
((StringArray)array).GetString(index);
+ return (array, index) =>
+ {
+ StringArray? sArray = array as StringArray;
Review Comment:
This gives up some of the performance benefit of the approach. Instead of
having to add a check to each invocation, can we have the caller tell us in
advance what the expected source type is and return one of two different
delegates?
##########
csharp/src/Apache.Arrow.Adbc/Extensions/IArrowArrayExtensions.cs:
##########
@@ -36,7 +43,7 @@ public static class IArrowArrayExtensions
/// <param name="index">
/// The index in the array to get the value from.
/// </param>
- public static object? ValueAt(this IArrowArray arrowArray, int index)
+ public static object? ValueAt(this IArrowArray arrowArray, int index,
StructResultType resultType = StructResultType.JsonString)
Review Comment:
This is a breaking change. Consider adding an overload instead of changing
the signature.
##########
csharp/src/Apache.Arrow.Adbc/Extensions/IArrowArrayExtensions.cs:
##########
@@ -76,7 +83,9 @@ public static class IArrowArrayExtensions
case ArrowTypeId.Int64:
return ((Int64Array)arrowArray).GetValue(index);
case ArrowTypeId.String:
- return ((StringArray)arrowArray).GetString(index);
+ StringArray sArray = (StringArray)arrowArray;
+ if (sArray.Length == 0) { return null; }
Review Comment:
How can we get here? Why is this not an error, and why does it impact only
StringArray and not other arrays?
##########
csharp/src/Apache.Arrow.Adbc/Extensions/IArrowArrayExtensions.cs:
##########
@@ -256,7 +282,7 @@ public static class IArrowArrayExtensions
case ArrowTypeId.List:
return (array, index) =>
((ListArray)array).GetSlicedValues(index);
case ArrowTypeId.Struct:
- return (array, index) =>
SerializeToJson((StructArray)array, index);
+ return (array, index) => resultType ==
StructResultType.JsonString ? SerializeToJson((StructArray)array, index) :
ParseStructArray((StructArray)array, index) ;
Review Comment:
```suggestion
return resultType == StructResultType.JsonString ?
(array, index) => SerializeToJson((StructArray)array, index) : (array, index)
=> ParseStructArray((StructArray)array, index) ;
```
--
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]
