eerhardt commented on a change in pull request #7032:
URL: https://github.com/apache/arrow/pull/7032#discussion_r417711102
##########
File path: csharp/src/Apache.Arrow/Arrays/StringArray.cs
##########
@@ -71,6 +76,15 @@ public string GetString(int index, Encoding encoding =
default)
var bytes = GetBytes(index);
+ if (bytes == Span<byte>.Empty)
+ {
+ return null;
+ }
+ if (bytes.Length == 0)
Review comment:
Do we have unit tests for both of these cases to make sure they act
appropriately? A test for `null` and a test for `string.Empty`. #Closed
##########
File path: csharp/src/Apache.Arrow/Arrays/StringArray.cs
##########
@@ -71,6 +76,15 @@ public string GetString(int index, Encoding encoding =
default)
var bytes = GetBytes(index);
+ if (bytes == Span<byte>.Empty)
Review comment:
We had similar conversations about this in the `DataFrame` type we made
here: https://github.com/dotnet/corefxlab/pull/2710#issuecomment-521799900.
I think it makes more sense to say `if (bytes == default)` here.
Or another idea is to add a `ReadOnlySpan<byte> GetBytes(int index, out bool
isNull)` method to `BinaryArray`, which would allow callers know explicitly if
the value is `null` without having a double `IsValid` check. #Closed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
