ARROW-7514: [C#] Make GetValueOffset Obsolete

[HashidaTKS]

• Add an [Obsolete] attribute to BinaryArray.GetValueOffset
  • ListArray.GetValueOffset already has the [Obsolete] attribute, so it is not changed
• Avoid using GetValueOffset in the product source code

As a precaution, I added tests for ValueOffsets and left tests for GetValueOffset.

[github-actions]

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

[emkornfield]

@eerhardt do you have time to review?

[kou]

Could you update test code to suppress obsolete warnings?

[eerhardt]

Could you update test code to suppress obsolete warnings?

👍

[eerhardt]

It's probably best to add a message to the [Obsolete] attribute telling users what they should use instead.

[HashidaTKS]

@kou @eerhardt

Thank you both!
I responded to feedback.

[kou]

How about ValueOffsets[index] instead of ValueOffsets?

[HashidaTKS]

Sounds good.
I fixed it.

[kou]

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);

[HashidaTKS]

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.

[kou]

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?

[eerhardt]

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.

[eerhardt]

I'm going to merge this to move this PR forward. If we want to tweak the GetBytes implementation separately, I think we can do that in a separate PR.