ARROW-7386: [C#] Array offset does not work properly #6029

HashidaTKS · 2019-12-13T06:38:59Z

"Array.Values" always starts from first index of "ValueBuffer".
It should start from "Offset".

github-actions · 2019-12-13T06:45:55Z

https://issues.apache.org/jira/browse/ARROW-7386

kou · 2019-12-16T07:52:06Z

csharp/test/Apache.Arrow.Tests/ArrowArrayTests.cs

@@ -0,0 +1,107 @@
+using Apache.Arrow.Tests.Fixtures;


Could you add a license header?

Thank you for your comment!
I added the license header.

kou · 2019-12-16T07:54:48Z

csharp/src/Apache.Arrow/Arrays/BinaryArray.cs

        {
            var offsets = ValueOffsets;
-            var offset = Offset + index;
+            var offset = index;


Can we remove this variable and use index directly?

I modified.

kou · 2019-12-16T07:56:30Z

csharp/test/Apache.Arrow.Tests/ArrowArrayTests.cs

+            //Check all offset and length
+            for (var offset = 0; offset < totalLength; offset++)
+            {
+                for(var length = 1; length + offset <= totalLength; length++)


Could you add a space after for?

I formatted all the changed files.

Add a license header Format codes

HashidaTKS · 2019-12-16T14:30:26Z

csharp/test/Apache.Arrow.Tests/ArrowArrayTests.cs

+    public class ArrowArrayTests
+    {
+        [Fact]
+        public void SliceArray()


The reason for testing Slice is that Slice changes Offset internally.

kou

+1

@eerhardt Do you want to review this too?

kou · 2019-12-18T00:47:50Z

csharp/src/Apache.Arrow/Arrays/BinaryArray.cs

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        public int GetValueLength(int index)
        {
-            var offsets = ValueOffsets;


Can we remove this?
If we remove this, do we call public ReadOnlySpan<int> ValueOffsets twice?

eerhardt

Overall this looks good. Just a few minor things to fix up.

eerhardt · 2019-12-18T02:14:43Z

csharp/src/Apache.Arrow/Arrays/BinaryArray.cs

-            var offset = Offset + index;
-
-            return offsets[offset + 1] - offsets[offset];
+            return ValueOffsets[index + 1] - ValueOffsets[index];


Do we need to check the index for out of range?
Also - no need to call the property twice. Can cache the span in a local variable.

Thank you for your comments.

I added a boundary check to this method and similar methods.

I modified to use local variable.

eerhardt · 2019-12-18T02:17:52Z

csharp/test/Apache.Arrow.Tests/ArrowArrayTests.cs

+                Assert.True(
+                    baseArray.ValueOffsetsBuffer.Span.CastTo<int>().Slice(slicedArray.Offset, slicedArray.Length + 1)
+                        .SequenceEqual(slicedArray.ValueOffsets));
+            }


Should we validate the Values as well?

I added Values check and GetValue check

Fix GetDate Add boundary check to GetValue and so on Improve tests

HashidaTKS · 2019-12-19T02:03:51Z

Looking back at the changes, I noticed Array.IsValid and DateArray.GetDate have the same issue and fixed them.

eerhardt · 2019-12-19T16:27:29Z

csharp/src/Apache.Arrow/Arrays/BinaryArray.cs

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        public int GetValueOffset(int index)
        {
+            if (index < 0 || index >= Length)


Getting the value offset at index == Length is valid, right? That's how you can tell how long the last element is.

Indeed, I fixed it.

Getting the value offset at index == Length is valid, right?

I think that this is invalid. Because it returns the offset of nonexistent value.

That's how you can tell how long the last element is.

I think that users should use GetValueLength() instead of computing value length by GetValueOffset() values. If advanced users want to compute by themselves, using ValueOffsets directly may be better.

The more I look at this method, the less value I see in it. Maybe we should just remove the method all together and people can index into ValueOffsets directly. That’s all this method does.

It used to do the “Offset + index” calculation, but that is no longer necessary since the ValueOffsets does that for them.

I agree with removing this method.

I agree with removing but I'm a little concerned about the impact on current users.
If we don't need to consider it, I'll remove this method.

How about marking this method as "deprecated" in the next release, then remove this method in the future release?
(I'm not sure how to do this in C#. Documentation and warning message?)

In .NET there is an [Obsolete] attribute you can apply to a method. Callers will receive a build time warning that the method is obsolete. We can add that.

Okey. I'll add an [Obsolete] attribute.

eerhardt · 2019-12-19T16:29:56Z

csharp/src/Apache.Arrow/Arrays/Date32Array.cs

-
-            return DateTimeOffset.FromUnixTimeMilliseconds(value * MillisecondsPerDay);
+            var value = GetValue(index);
+            return value.HasValue ?


I wonder if it wouldn't be simpler to just write:

if (!value.HasValue) { return value; // or `return default;` } return DateTimeOffset.FromUnixTimeMilliseconds(value.Value * MillisecondsPerDay);

That way a reader doesn't have to reason about null as DateTimeOffset?

I agree. I fixed it.

Refactor GetDate

kou · 2019-12-25T00:42:43Z

I have a concern that GetValue() for the Length-th index: #6029 (comment)

But we can work on this as a follow-up task if it's needed. I'll merge this for now.

Fix Values and ValueOffsets start position

756e5cf

kou requested changes Dec 16, 2019

View reviewed changes

Improve GetValueLength

8a8614b

Add a license header Format codes

HashidaTKS commented Dec 16, 2019

View reviewed changes

HashidaTKS requested a review from kou December 16, 2019 14:31

kou approved these changes Dec 18, 2019

View reviewed changes

eerhardt approved these changes Dec 18, 2019

View reviewed changes

Fix IsValid

d012ba5

Fix GetDate Add boundary check to GetValue and so on Improve tests

eerhardt reviewed Dec 19, 2019

View reviewed changes

Fix GetValueOffset boundary check

89f2212

Refactor GetDate

kou closed this in fa89d3e Dec 25, 2019

This was referenced Jan 1, 2020

[C#] Array offset does not work properly #23662

Closed

[C#] Make GetValueOffset Obsolete #23779

Closed

ARROW-7386: [C#] Array offset does not work properly #6029

ARROW-7386: [C#] Array offset does not work properly #6029

Uh oh!

Conversation

HashidaTKS commented Dec 13, 2019

Uh oh!

github-actions bot commented Dec 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HashidaTKS commented Dec 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt Dec 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HashidaTKS Dec 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kou Dec 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt Dec 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HashidaTKS Dec 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kou commented Dec 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

eerhardt Dec 26, 2019 •

edited

Loading

HashidaTKS Dec 27, 2019 •

edited

Loading

kou Dec 27, 2019 •

edited

Loading

eerhardt Dec 19, 2019 •

edited

Loading

HashidaTKS Dec 19, 2019 •

edited

Loading