Skip to content

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

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

Closed
HashidaTKS wants to merge 4 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
756e5cf
Fix Values and ValueOffsets start position
Dec 13, 2019
8a8614b
Improve GetValueLength
HashidaTKS Dec 16, 2019
d012ba5
Fix IsValid
Dec 19, 2019
89f2212
Fix GetValueOffset boundary check
Dec 19, 2019
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
2 changes: 1 addition & 1 deletion csharp/src/Apache.Arrow/Arrays/Array.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public virtual void Accept(IArrowArrayVisitor visitor)
}

public bool IsValid(int index) =>
NullCount == 0 || NullBitmapBuffer.IsEmpty || BitUtility.GetBit(NullBitmapBuffer.Span, index);
NullCount == 0 || NullBitmapBuffer.IsEmpty || BitUtility.GetBit(NullBitmapBuffer.Span, index + Offset);

public bool IsNull(int index) => !IsValid(index);

Expand Down
36 changes: 21 additions & 15 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 @@ -21,7 +21,7 @@

namespace Apache.Arrow
{
public class BinaryArray: Array
public class BinaryArray : Array
{
public class Builder : BuilderBase<BinaryArray, Builder>
{
Expand All @@ -42,15 +42,15 @@ public BinaryArray(ArrayData data)
}

public BinaryArray(ArrowTypeId typeId, ArrayData data)
: base (data)
: base(data)
{
data.EnsureDataType(typeId);
data.EnsureBufferCount(3);
}

public abstract class BuilderBase<TArray, TBuilder>: IArrowArrayBuilder<byte, TArray, TBuilder>
where TArray: IArrowArray
where TBuilder: class, IArrowArrayBuilder<byte, TArray, TBuilder>
public abstract class BuilderBase<TArray, TBuilder> : IArrowArrayBuilder<byte, TArray, TBuilder>
where TArray : IArrowArray
where TBuilder : class, IArrowArrayBuilder<byte, TArray, TBuilder>
{
protected IArrowType DataType { get; }
protected TBuilder Instance => this as TBuilder;
Expand All @@ -71,8 +71,8 @@ public TArray Build(MemoryAllocator allocator = default)
{
ValueOffsets.Append(Offset);

var data = new ArrayData(DataType, ValueOffsets.Length - 1, 0, 0,
new [] { ArrowBuffer.Empty, ValueOffsets.Build(allocator), ValueBuffer.Build(allocator) });
var data = new ArrayData(DataType, ValueOffsets.Length - 1, 0, 0,
new[] { ArrowBuffer.Empty, ValueOffsets.Build(allocator), ValueBuffer.Build(allocator) });

return Build(data);
}
Expand Down Expand Up @@ -154,8 +154,8 @@ public BinaryArray(IArrowType dataType, int length,
ArrowBuffer dataBuffer,
ArrowBuffer nullBitmapBuffer,
int nullCount = 0, int offset = 0)
: this(new ArrayData(dataType, length, nullCount, offset,
new [] { nullBitmapBuffer, valueOffsetsBuffer, dataBuffer }))
: this(new ArrayData(dataType, length, nullCount, offset,
new[] { nullBitmapBuffer, valueOffsetsBuffer, dataBuffer }))
{ }

public override void Accept(IArrowArrayVisitor visitor) => Accept(this, visitor);
Expand All @@ -164,30 +164,36 @@ public BinaryArray(IArrowType dataType, int length,

public ArrowBuffer ValueBuffer => Data.Buffers[2];

public ReadOnlySpan<int> ValueOffsets => ValueOffsetsBuffer.Span.CastTo<int>().Slice(0, Length + 1);
public ReadOnlySpan<int> ValueOffsets => ValueOffsetsBuffer.Span.CastTo<int>().Slice(Offset, Length + 1);

public ReadOnlySpan<byte> Values => ValueBuffer.Span.CastTo<byte>();

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public int GetValueOffset(int index)
{
return ValueOffsets[Offset + index];
if (index < 0 || index > Length)
{
throw new ArgumentOutOfRangeException(nameof(index));
}
return ValueOffsets[index];
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public int GetValueLength(int index)
{
if (index < 0 || index >= Length)
{
throw new ArgumentOutOfRangeException(nameof(index));
}
var offsets = ValueOffsets;
Copy link
Member

@kou kou Dec 18, 2019

Choose a reason for hiding this comment

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

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

var offset = Offset + index;

return offsets[offset + 1] - offsets[offset];
return offsets[index + 1] - offsets[index];
}

public ReadOnlySpan<byte> GetBytes(int index)
{
var offset = GetValueOffset(index);
var length = GetValueLength(index);

return ValueBuffer.Span.Slice(offset, length);
}

Expand Down
16 changes: 10 additions & 6 deletions csharp/src/Apache.Arrow/Arrays/Date32Array.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

namespace Apache.Arrow
{
public class Date32Array: PrimitiveArray<int>
public class Date32Array : PrimitiveArray<int>
{
private const int MillisecondsPerDay = 86400000;

Expand All @@ -36,7 +36,7 @@ public Builder() : base(new DateBuilder()) { }

protected override int ConvertTo(DateTimeOffset value)
{
return (int) (value.ToUnixTimeMilliseconds() / MillisecondsPerDay);
return (int)(value.ToUnixTimeMilliseconds() / MillisecondsPerDay);
}
}

Expand All @@ -47,7 +47,7 @@ public Date32Array(
new[] { nullBitmapBuffer, valueBuffer }))
{ }

public Date32Array(ArrayData data)
public Date32Array(ArrayData data)
: base(data)
{
data.EnsureDataType(ArrowTypeId.Date32);
Expand All @@ -57,10 +57,14 @@ public Date32Array(ArrayData data)

public DateTimeOffset? GetDate(int index)
{
var values = ValueBuffer.Span.CastTo<int>().Slice(0, Length);
var value = values[index];
var value = GetValue(index);

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

return DateTimeOffset.FromUnixTimeMilliseconds(value.Value * MillisecondsPerDay);
}
}
}
11 changes: 5 additions & 6 deletions csharp/src/Apache.Arrow/Arrays/Date64Array.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,15 +57,14 @@ public Date64Array(ArrayData data)

public DateTimeOffset? GetDate(int index)
{
if (!IsValid(index))
var value = GetValue(index);

if (!value.HasValue)
{
return null;
return default;
}

var values = ValueBuffer.Span.CastTo<long>().Slice(0, Length);
var value = values[index];

return DateTimeOffset.FromUnixTimeMilliseconds(value);
return DateTimeOffset.FromUnixTimeMilliseconds(value.Value);
}
}
}
4 changes: 2 additions & 2 deletions csharp/src/Apache.Arrow/Arrays/ListArray.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,13 @@ public class ListArray : Array

public ArrowBuffer ValueOffsetsBuffer => Data.Buffers[1];

public ReadOnlySpan<int> ValueOffsets => ValueOffsetsBuffer.Span.CastTo<int>().Slice(0, Length + 1);
public ReadOnlySpan<int> ValueOffsets => ValueOffsetsBuffer.Span.CastTo<int>().Slice(Offset, Length + 1);

public ListArray(IArrowType dataType, int length,
ArrowBuffer valueOffsetsBuffer, IArrowArray values,
ArrowBuffer nullBitmapBuffer, int nullCount = 0, int offset = 0)
: this(new ArrayData(dataType, length, nullCount, offset,
new[] {nullBitmapBuffer, valueOffsetsBuffer}, new[] {values.Data}))
new[] { nullBitmapBuffer, valueOffsetsBuffer }, new[] { values.Data }))
{
Values = values;
}
Expand Down
8 changes: 6 additions & 2 deletions csharp/src/Apache.Arrow/Arrays/PrimitiveArray.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,16 @@ protected PrimitiveArray(ArrayData data)

public ArrowBuffer ValueBuffer => Data.Buffers[1];

public ReadOnlySpan<T> Values => ValueBuffer.Span.CastTo<T>().Slice(0, Length);
public ReadOnlySpan<T> Values => ValueBuffer.Span.CastTo<T>().Slice(Offset, Length);

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public T? GetValue(int index)
{
return IsValid(index) ? Values[index] : (T?) null;
if (index < 0 || index >= Length)
{
throw new ArgumentOutOfRangeException(nameof(index));
}
return IsValid(index) ? Values[index] : (T?)null;
}

public IList<T?> ToList(bool includeNulls = false)
Expand Down
Loading