Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARROW-7514: [C#] Make GetValueOffset Obsolete #6333

Closed
wants to merge 3 commits into from
Closed

ARROW-7514: [C#] Make GetValueOffset Obsolete #6333

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
07d106c
ARROW-7514_make_getvalueoffset_obsolete
Jan 28, 2020
92b14c0
ARROW-7514_make_getvalueoffset_obsolete
Feb 4, 2020
1dbaf39
ARROW-7514_make_getvalueoffset_obsolete
Feb 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions csharp/src/Apache.Arrow/Arrays/BinaryArray.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,7 @@ public BinaryArray(IArrowType dataType, int length,
public ReadOnlySpan<byte> Values => ValueBuffer.Span.CastTo<byte>();

[MethodImpl(MethodImplOptions.AggressiveInlining)]
[Obsolete("This method has been deprecated. Please use ValueOffsets[index] instead.")]
public int GetValueOffset(int index)
{
if (index < 0 || index > Length)
Expand All @@ -193,10 +194,12 @@ public int GetValueLength(int index)

public ReadOnlySpan<byte> GetBytes(int index)
{
var offset = GetValueOffset(index);
var length = GetValueLength(index);
if (index < 0 || index >= Length)
{
throw new ArgumentOutOfRangeException(nameof(index));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check is duplicated of the check in GetValueLength.

How about this?

var offset = ValueOffsets[index];
var length = GetValueLength(index);
return ValueBuffer.Span.Slice(offset, length);

Copy link
Contributor Author

@HashidaTKS HashidaTKS Feb 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially, I implemented as below to avoid duplication of checks.
However, I thought the intention was a little difficult to understand, so it was implemented like current.

  var length = GetValueLength(index);
  return ValueBuffer.Span.Slice(ValueOffsets[index], length);

Also, if we don't care about the type of exception, we can simply remove the check.
In that case, this method throws IndexOutOfRangeException which ValueOffsets[index] throws.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand.
How about adding a new helper private method to validate index:

private void ValidateIndex(int index)
{
  if (index < 0 || index >= Length)
  {
    throw new ArgumentOutOfRangeException(nameof(index));
  }
}

and use it in GetValueLength and GetBytes?

ValidateIndex(index);
var offsets = ValueOffsets;
var offset = offsets[index];
var length = offsets[index + 1] - offset;
return ValueBuffer.Span.Slice(offset, length);

@eerhardt What do you think about this case?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, if we don't care about the type of exception, we can simply remove the check.
In that case, this method throws IndexOutOfRangeException which ValueOffsets[index] throws.

Typically we do care about the type of exception. Bubbling up an IndexOutOfRangeException looks like a bug in our library - similar to if you let something NullReferenceException. See the Design Guidelines for more info about this.

Instead, it is better to throw an ArgumentOutOfRangeException.

I think a helper like ValidateIndex makes the most sense. Also note that as currently written you are validating the index twice - once in GetBytes and then again when GetBytes calls GetValueLength. Not a huge issue, just something I noticed.


return ValueBuffer.Span.Slice(offset, length);
return ValueBuffer.Span.Slice(ValueOffsets[index], GetValueLength(index));
}

}
Expand Down
2 changes: 1 addition & 1 deletion csharp/src/Apache.Arrow/Arrays/ListArray.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ private ListArray(ArrayData data, IArrowArray values) : base(data)
public override void Accept(IArrowArrayVisitor visitor) => Accept(this, visitor);


[Obsolete]
[Obsolete("This method has been deprecated. Please use ValueOffsets[index] instead.")]
public int GetValueOffset(int index)
{
if (index < 0 || index > Length)
Expand Down
8 changes: 8 additions & 0 deletions csharp/test/Apache.Arrow.Tests/ArrowArrayTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,19 @@ public void ThrowsWhenGetValueAndOffsetIndexOutOfBounds()
Assert.Equal(1, array.GetValueLength(1));
Assert.Throws<ArgumentOutOfRangeException>(() => array.GetValueLength(2));

#pragma warning disable 618
Assert.Throws<ArgumentOutOfRangeException>(() => array.GetValueOffset(-1));
Assert.Equal(0, array.GetValueOffset(0));
Assert.Equal(1, array.GetValueOffset(1));
Assert.Equal(2, array.GetValueOffset(2));
Assert.Throws<ArgumentOutOfRangeException>(() => array.GetValueOffset(3));
#pragma warning restore 618

Assert.Throws<IndexOutOfRangeException>(() => array.ValueOffsets[-1]);
Assert.Equal(0, array.ValueOffsets[0]);
Assert.Equal(1, array.ValueOffsets[1]);
Assert.Equal(2, array.ValueOffsets[2]);
Assert.Throws<IndexOutOfRangeException>(() => array.ValueOffsets[3]);

}

Expand Down