You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/06/14 13:52:33 UTC
[GitHub] [arrow] HashidaTKS opened a new pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
HashidaTKS opened a new pull request #10527:
URL: https://github.com/apache/arrow/pull/10527
This is a implementation of `DictionaryBatch` (de)serialization for the streaming format.
- The (de)serialization for the file format or Flight are not implemented yet
- `isDelta` is not supported yet
--
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:
users@infra.apache.org
[GitHub] [arrow] HashidaTKS commented on pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
HashidaTKS commented on pull request #10527:
URL: https://github.com/apache/arrow/pull/10527#issuecomment-860738406
cc: @eerhardt
Would you please review this when you have time?
--
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:
users@infra.apache.org
[GitHub] [arrow] HashidaTKS commented on a change in pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
HashidaTKS commented on a change in pull request #10527:
URL: https://github.com/apache/arrow/pull/10527#discussion_r663390641
##########
File path: csharp/src/Apache.Arrow/Arrays/DictionaryArray.cs
##########
@@ -0,0 +1,67 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+using System;
+using System.IO;
+using Apache.Arrow.Types;
+
+namespace Apache.Arrow
+{
+ public class DictionaryArray : Array
+ {
+ public IArrowArray Dictionary { get; }
+ public IArrowArray Indices { get; }
+ public ArrowBuffer IndicesBuffer => Data.Buffers[1];
+
+ public DictionaryArray(IArrowType dataType, int length,
+ ArrowBuffer valueOffsetsBuffer, IArrowArray value,
+ ArrowBuffer nullBitmapBuffer, int nullCount = 0, int offset = 0)
+ : this(new ArrayData(dataType, length, nullCount, offset,
+ new[] { nullBitmapBuffer, valueOffsetsBuffer }, new[] { value.Data }, value.Data.Dictionary))
+ {
+ }
+
+ public DictionaryArray(ArrayData data) : base(data)
+ {
+ data.EnsureBufferCount(2);
+ data.EnsureDataType(ArrowTypeId.Dictionary);
+
+ var dicType = data.DataType as DictionaryType;
+ data.Dictionary.EnsureDataType(dicType.ValueType.TypeId);
Review comment:
Please check it again.
I added the null check with the following commit.
https://github.com/apache/arrow/pull/10527/commits/8544064b944745aa757efe23e3c3b3aaac65e89a
https://github.com/apache/arrow/pull/10527/files#diff-f31b887c0ac098b46742c1984588210d7f217a9f546e55255570a1d15d5cc3b3R33
##########
File path: csharp/test/Apache.Arrow.Tests/ArrowStreamReaderTests.cs
##########
@@ -68,7 +68,7 @@ public async Task Ctor_MemoryPool_AllocatesFromPool(bool shouldLeaveOpen)
ArrowStreamReader reader = new ArrowStreamReader(stream, memoryPool, shouldLeaveOpen);
reader.ReadNextRecordBatch();
- Assert.Equal(1, memoryPool.Statistics.Allocations);
+ Assert.Equal(2, memoryPool.Statistics.Allocations);
Review comment:
I fixed it to use `[Theory]`.
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamReaderImplementation.cs
##########
@@ -43,13 +45,48 @@ protected override void Dispose(bool disposing)
}
}
+ protected void ReadInitialDictionaries()
Review comment:
I fixed it.
##########
File path: csharp/src/Apache.Arrow/Ipc/MessageSerializer.cs
##########
@@ -52,14 +53,13 @@ public static Types.NumberType GetNumberType(int bitWidth, bool signed)
$"{(signed ? "signed " : "unsigned")} integer.");
}
- internal static Schema GetSchema(Flatbuf.Schema schema)
+ internal static Schema GetSchema(Flatbuf.Schema schema, LazyCreator<DictionaryMemo> lazyDictionaryMemo)
Review comment:
I fixed it.
##########
File path: csharp/test/Apache.Arrow.Tests/TestData.cs
##########
@@ -21,12 +21,13 @@ namespace Apache.Arrow.Tests
{
public static class TestData
{
- public static RecordBatch CreateSampleRecordBatch(int length)
+ //TODO: Remove the createDictionaryArray argument after all writer/reader supports DictionaryType serialization
Review comment:
I was planning to always create a dictionary array after `ArrowFileWriter/Reader` support `DictionaryType` serialization.
But considering your feedback comments, now I think it is better to leave `createDictionaryArray` even after that.
I will remove this `TODO`.
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamReaderImplementation.cs
##########
@@ -43,13 +45,48 @@ protected override void Dispose(bool disposing)
}
}
+ protected void ReadInitialDictionaries()
+ {
+ if (HasReadInitialDictionary)
+ {
+ return;
+ }
+
+ if (_lazyDictionaryMemo.IsCreated) {
+ int fieldCount = _lazyDictionaryMemo.Instance.GetFieldCount();
+ for (int i = 0; i < fieldCount; ++i)
+ {
+ ReadArrowObject();
+ }
+ }
+ HasReadInitialDictionary = true;
+ }
+
+ protected async ValueTask ReadInitialDictionariesAsync(CancellationToken cancellationToken = default)
+ {
+ if (HasReadInitialDictionary)
+ {
+ return;
+ }
+
+ int fieldCount = _lazyDictionaryMemo.Instance.GetFieldCount();
+ for (int i = 0; i < fieldCount; ++i)
+ {
+ await ReadArrowObjectAsync(cancellationToken).ConfigureAwait(false);
+ }
+
+ HasReadInitialDictionary = true;
+ }
+
public override async ValueTask<RecordBatch> ReadNextRecordBatchAsync(CancellationToken cancellationToken)
{
// TODO: Loop until a record batch is read.
cancellationToken.ThrowIfCancellationRequested();
return await ReadRecordBatchAsync(cancellationToken).ConfigureAwait(false);
}
+
Review comment:
I fixed it.
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamReaderImplementation.cs
##########
@@ -27,8 +27,10 @@ internal class ArrowStreamReaderImplementation : ArrowReaderImplementation
public Stream BaseStream { get; }
private readonly bool _leaveOpen;
private readonly MemoryAllocator _allocator;
+ private protected bool HasReadInitialDictionary { get; set; }
- public ArrowStreamReaderImplementation(Stream stream, MemoryAllocator allocator, bool leaveOpen)
+
+ public ArrowStreamReaderImplementation(Stream stream, MemoryAllocator allocator, bool leaveOpen) : base()
Review comment:
I fixed it.
##########
File path: csharp/src/Apache.Arrow/RecordBatch.cs
##########
@@ -28,6 +28,8 @@ public partial class RecordBatch : IDisposable
public IEnumerable<IArrowArray> Arrays => _arrays;
public int Length { get; }
+ internal IReadOnlyList<IArrowArray> _Arrays => (IReadOnlyList<IArrowArray>)_arrays;
+
private readonly IMemoryOwner<byte> _memoryOwner;
private readonly IList<IArrowArray> _arrays;
Review comment:
I fixed it.
##########
File path: csharp/src/Apache.Arrow/Ipc/MessageSerializer.cs
##########
@@ -73,13 +73,27 @@ internal static Schema GetSchema(Flatbuf.Schema schema)
return new Schema(fields, metadata, copyCollections: false);
}
- private static Field FieldFromFlatbuffer(Flatbuf.Field flatbufField)
+ private static Field FieldFromFlatbuffer(Flatbuf.Field flatbufField, LazyCreator<DictionaryMemo> lazyDictionaryMemo)
{
Field[] childFields = flatbufField.ChildrenLength > 0 ? new Field[flatbufField.ChildrenLength] : null;
for (int i = 0; i < flatbufField.ChildrenLength; i++)
{
Flatbuf.Field? childFlatbufField = flatbufField.Children(i);
- childFields[i] = FieldFromFlatbuffer(childFlatbufField.Value);
+ childFields[i] = FieldFromFlatbuffer(childFlatbufField.Value, lazyDictionaryMemo);
+ }
+
+ Flatbuf.DictionaryEncoding? de = flatbufField.Dictionary;
Review comment:
I fixed it.
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamReaderImplementation.cs
##########
@@ -27,8 +27,10 @@ internal class ArrowStreamReaderImplementation : ArrowReaderImplementation
public Stream BaseStream { get; }
private readonly bool _leaveOpen;
private readonly MemoryAllocator _allocator;
+ private protected bool HasReadInitialDictionary { get; set; }
Review comment:
I fixed it.
##########
File path: csharp/src/Apache.Arrow/Ipc/DictionaryMemo.cs
##########
@@ -0,0 +1,113 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+using System;
+using System.Collections.Generic;
+using Apache.Arrow.Types;
+
+namespace Apache.Arrow.Ipc
+{
+ class DictionaryMemo
+ {
+ private Dictionary<long, IArrowArray> _idToDictionary;
+ private Dictionary<long, IArrowType> _idToValueType;
+ private Dictionary<Field, long> _fieldToId;
+
+ public DictionaryMemo()
+ {
+ _idToDictionary = new Dictionary<long, IArrowArray>();
+ _idToValueType = new Dictionary<long, IArrowType>();
+ _fieldToId = new Dictionary<Field, long>();
+ }
+
+ public IArrowType GetDictionaryType(long id)
+ {
+ if (!_idToValueType.TryGetValue(id, out IArrowType type))
+ {
+ throw new ArgumentException($"Dictionary with id {id} not found");
+ }
+ return type;
+ }
+
+ public IArrowArray GetDictionary(long id)
+ {
+ if (!_idToDictionary.TryGetValue(id, out IArrowArray dictionary))
+ {
+ throw new ArgumentException($"Dictionary with id {id} not found");
+ }
+ return dictionary;
+ }
+
+ public void AddField(long id, Field field)
+ {
+ if (_fieldToId.ContainsKey(field))
+ {
+ throw new ArgumentException($"Field {field.Name} is already in Memo");
+ }
+
+ if (field.DataType.TypeId != ArrowTypeId.Dictionary)
+ {
+ throw new ArgumentException($"Field type is not DictionaryType: Name={field.Name}, {field.DataType.Name}");
+ }
+
+ IArrowType valueType = ((DictionaryType)field.DataType).ValueType;
+
+ if (_idToValueType.TryGetValue(id, out IArrowType valueTypeInDic))
+ {
+ if (valueType != valueTypeInDic)
+ {
+ throw new ArgumentException($"Field type {field.DataType.Name} does not match the existing type {valueTypeInDic})");
+ }
+ }
+
+ _fieldToId.Add(field, id);
+ _idToValueType.Add(id, valueType);
+ }
+
+ public long GetId(Field field)
+ {
+ if (!_fieldToId.TryGetValue(field, out long id))
+ {
+ throw new ArgumentException($"Field with name {field.Name} not found");
+ }
+ return id;
+ }
+
+ public long GetOrAssignId(Field field)
+ {
+ if (!_fieldToId.TryGetValue(field, out long id))
+ {
+ id = _fieldToId.Count + 1;
+ AddField(id, field);
+ }
+ return id;
+ }
+
+ public void AddOrReplaceDictionary(long id, IArrowArray dictionary)
+ {
+ _idToDictionary[id] = dictionary;
+ }
+
+ public void AddDictionaryDelta(long id, IArrowArray dictionary)
Review comment:
Sorry, I didn't commit the change.
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamReaderImplementation.cs
##########
@@ -43,13 +45,48 @@ protected override void Dispose(bool disposing)
}
}
+ protected void ReadInitialDictionaries()
+ {
+ if (HasReadInitialDictionary)
+ {
+ return;
+ }
+
+ if (_lazyDictionaryMemo.IsCreated) {
+ int fieldCount = _lazyDictionaryMemo.Instance.GetFieldCount();
+ for (int i = 0; i < fieldCount; ++i)
+ {
+ ReadArrowObject();
+ }
+ }
+ HasReadInitialDictionary = true;
+ }
+
+ protected async ValueTask ReadInitialDictionariesAsync(CancellationToken cancellationToken = default)
Review comment:
I fixed it.
##########
File path: csharp/test/Apache.Arrow.Tests/ArrowStreamWriterTests.cs
##########
@@ -32,7 +32,7 @@ public class ArrowStreamWriterTests
[Fact]
public void Ctor_LeaveOpenDefault_StreamClosedOnDispose()
{
- RecordBatch originalBatch = TestData.CreateSampleRecordBatch(length: 100);
+ RecordBatch originalBatch = TestData.CreateSampleRecordBatch(length: 100, createDictionaryArray: true);
Review comment:
I fixed it.
##########
File path: csharp/test/Apache.Arrow.Tests/TestData.cs
##########
@@ -21,12 +21,13 @@ namespace Apache.Arrow.Tests
{
public static class TestData
{
- public static RecordBatch CreateSampleRecordBatch(int length)
+ //TODO: Remove the createDictionaryArray argument after all writer/reader supports DictionaryType serialization
Review comment:
I removed the `TODO` comment.
##########
File path: csharp/test/Apache.Arrow.Tests/TestData.cs
##########
@@ -21,12 +21,13 @@ namespace Apache.Arrow.Tests
{
public static class TestData
{
- public static RecordBatch CreateSampleRecordBatch(int length)
+ //TODO: Remove the createDictionaryArray argument after all writer/reader supports DictionaryType serialization
+ public static RecordBatch CreateSampleRecordBatch(int length, bool createDictionaryArray = false)
Review comment:
I understand.
I fixed tests to use `[Theory]`
##########
File path: csharp/src/Apache.Arrow/Ipc/MessageSerializer.cs
##########
@@ -73,13 +73,27 @@ internal static Schema GetSchema(Flatbuf.Schema schema)
return new Schema(fields, metadata, copyCollections: false);
}
- private static Field FieldFromFlatbuffer(Flatbuf.Field flatbufField)
+ private static Field FieldFromFlatbuffer(Flatbuf.Field flatbufField, LazyCreator<DictionaryMemo> lazyDictionaryMemo)
{
Field[] childFields = flatbufField.ChildrenLength > 0 ? new Field[flatbufField.ChildrenLength] : null;
for (int i = 0; i < flatbufField.ChildrenLength; i++)
{
Flatbuf.Field? childFlatbufField = flatbufField.Children(i);
- childFields[i] = FieldFromFlatbuffer(childFlatbufField.Value);
+ childFields[i] = FieldFromFlatbuffer(childFlatbufField.Value, lazyDictionaryMemo);
+ }
+
+ Flatbuf.DictionaryEncoding? de = flatbufField.Dictionary;
+ IArrowType type = GetFieldArrowType(flatbufField, childFields);
+
+ if (de.HasValue)
+ {
+ Flatbuf.Int? indexTypeAsInt = de.Value.IndexType;
+ if (!indexTypeAsInt.HasValue)
+ {
+ throw new InvalidDataException("Dictionary type not defined");
Review comment:
I fixed it.
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamReaderImplementation.cs
##########
@@ -59,6 +96,66 @@ protected async ValueTask<RecordBatch> ReadRecordBatchAsync(CancellationToken ca
{
await ReadSchemaAsync().ConfigureAwait(false);
+ await ReadInitialDictionariesAsync().ConfigureAwait(false);
+
+ return await ReadArrowObjectAsync().ConfigureAwait(false);
Review comment:
I fixed it.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] HashidaTKS commented on pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
HashidaTKS commented on pull request #10527:
URL: https://github.com/apache/arrow/pull/10527#issuecomment-864352773
added some minor changes.
--
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:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on a change in pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
eerhardt commented on a change in pull request #10527:
URL: https://github.com/apache/arrow/pull/10527#discussion_r684687419
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamReaderImplementation.cs
##########
@@ -59,6 +97,75 @@ protected async ValueTask<RecordBatch> ReadRecordBatchAsync(CancellationToken ca
{
await ReadSchemaAsync().ConfigureAwait(false);
+ await ReadInitialDictionariesAsync(cancellationToken).ConfigureAwait(false);
+
+ return await ReadArrowObjectAsync(cancellationToken).ConfigureAwait(false);
+ }
+
+
+ protected RecordBatch ReadRecordBatch()
+ {
+ ReadSchema();
+
+ ReadInitialDictionaries();
+
+ return ReadArrowObject();
Review comment:
> I would like to implement it in another PR because this PR will become too large.
How is that?
That is perfectly reasonable to me. I think the current functionality is sufficient for the first PR.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] HashidaTKS commented on pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
HashidaTKS commented on pull request #10527:
URL: https://github.com/apache/arrow/pull/10527#issuecomment-894907003
@eerhardt
I have addressed commented issues.
I appreciate your review and support!
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] HashidaTKS commented on pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
HashidaTKS commented on pull request #10527:
URL: https://github.com/apache/arrow/pull/10527#issuecomment-863449512
@eerhardt
Thank you for the review!
I have reflected the feedback.
--
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:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on a change in pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
eerhardt commented on a change in pull request #10527:
URL: https://github.com/apache/arrow/pull/10527#discussion_r684688202
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamReaderImplementation.cs
##########
@@ -59,35 +59,41 @@ protected async ValueTask<RecordBatch> ReadRecordBatchAsync(CancellationToken ca
{
await ReadSchemaAsync().ConfigureAwait(false);
- int messageLength = await ReadMessageLengthAsync(throwOnFullRead: false, cancellationToken)
- .ConfigureAwait(false);
+ RecordBatch result = default;
+ Flatbuf.MessageHeader messageHeaderType = Flatbuf.MessageHeader.NONE;
- if (messageLength == 0)
+ do
{
- // reached end
- return null;
- }
-
- RecordBatch result = null;
- await ArrayPool<byte>.Shared.RentReturnAsync(messageLength, async (messageBuff) =>
- {
- int bytesRead = await BaseStream.ReadFullBufferAsync(messageBuff, cancellationToken)
+ int messageLength = await ReadMessageLengthAsync(throwOnFullRead: false, cancellationToken)
.ConfigureAwait(false);
- EnsureFullRead(messageBuff, bytesRead);
- Flatbuf.Message message = Flatbuf.Message.GetRootAsMessage(CreateByteBuffer(messageBuff));
+ if (messageLength == 0)
+ {
+ // reached end
+ return null;
+ }
- int bodyLength = checked((int)message.BodyLength);
+ await ArrayPool<byte>.Shared.RentReturnAsync(messageLength, async (messageBuff) =>
+ {
+ int bytesRead = await BaseStream.ReadFullBufferAsync(messageBuff, cancellationToken)
+ .ConfigureAwait(false);
+ EnsureFullRead(messageBuff, bytesRead);
- IMemoryOwner<byte> bodyBuffOwner = _allocator.Allocate(bodyLength);
- Memory<byte> bodyBuff = bodyBuffOwner.Memory.Slice(0, bodyLength);
- bytesRead = await BaseStream.ReadFullBufferAsync(bodyBuff, cancellationToken)
- .ConfigureAwait(false);
- EnsureFullRead(bodyBuff, bytesRead);
+ Flatbuf.Message message = Flatbuf.Message.GetRootAsMessage(CreateByteBuffer(messageBuff));
- FlatBuffers.ByteBuffer bodybb = CreateByteBuffer(bodyBuff);
- result = CreateArrowObjectFromMessage(message, bodybb, bodyBuffOwner);
- }).ConfigureAwait(false);
+ int bodyLength = checked((int)message.BodyLength);
+ messageHeaderType = message.HeaderType;
+
+ IMemoryOwner<byte> bodyBuffOwner = _allocator.Allocate(bodyLength);
+ Memory<byte> bodyBuff = bodyBuffOwner.Memory.Slice(0, bodyLength);
+ bytesRead = await BaseStream.ReadFullBufferAsync(bodyBuff, cancellationToken)
+ .ConfigureAwait(false);
+ EnsureFullRead(bodyBuff, bytesRead);
+
+ FlatBuffers.ByteBuffer bodybb = CreateByteBuffer(bodyBuff);
+ result = CreateArrowObjectFromMessage(message, bodybb, bodyBuffOwner);
+ }).ConfigureAwait(false);
+ } while (messageHeaderType == Flatbuf.MessageHeader.DictionaryBatch);
Review comment:
Or even
```suggestion
} while (result == null);
```
Note - this also applies to the same loop in `ReadRecordBatch()`.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
eerhardt commented on pull request #10527:
URL: https://github.com/apache/arrow/pull/10527#issuecomment-871421884
Sorry for the delay @HashidaTKS. I didn't forget about this. I will get back to it this week, hopefully sometime today.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] HashidaTKS commented on pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
HashidaTKS commented on pull request #10527:
URL: https://github.com/apache/arrow/pull/10527#issuecomment-864362267
Modified ArrayData constructors.
I'm sorry to commit many times after requesting a review.
--
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:
users@infra.apache.org
[GitHub] [arrow] HashidaTKS commented on a change in pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
HashidaTKS commented on a change in pull request #10527:
URL: https://github.com/apache/arrow/pull/10527#discussion_r654753770
##########
File path: csharp/src/Apache.Arrow/Arrays/ArrayData.cs
##########
@@ -30,10 +30,25 @@ public sealed class ArrayData : IDisposable
public readonly int Offset;
public readonly ArrowBuffer[] Buffers;
public readonly ArrayData[] Children;
+ public readonly ArrayData Dictionary; //Only used for dictionary type
public ArrayData(
IArrowType dataType,
int length, int nullCount = 0, int offset = 0,
+ IEnumerable<ArrowBuffer> buffers = null, IEnumerable<ArrayData> children = null) :
+ this(dataType, null, length, nullCount, offset, buffers, children)
+ { }
+
+ public ArrayData(
+ IArrowType dataType,
+ int length, int nullCount = 0, int offset = 0,
+ ArrowBuffer[] buffers = null, ArrayData[] children = null) :
+ this(dataType, null, length, nullCount, offset, buffers, children)
+ { }
+
+ public ArrayData(
+ IArrowType dataType, ArrayData dictionary,
Review comment:
I noticed that changing original ctor's optional parameters to normal parameters can avoid the ambiguity and keep backward compatibility.
Please excuse my ignorance...
```
public ArrayData(
IArrowType dataType,
int length, int nullCount, int offset,
IEnumerable<ArrowBuffer> buffers, IEnumerable<ArrayData> children) :
this(dataType, length, nullCount, offset, buffers, children, null)
{ }
public ArrayData(
IArrowType dataType,
int length, int nullCount = 0, int offset = 0,
IEnumerable<ArrowBuffer> buffers = null, IEnumerable<ArrayData> children = null, ArrayData dictionary = null)
{
...
}
```
--
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:
users@infra.apache.org
[GitHub] [arrow] HashidaTKS commented on a change in pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
HashidaTKS commented on a change in pull request #10527:
URL: https://github.com/apache/arrow/pull/10527#discussion_r668329358
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamWriter.cs
##########
@@ -248,6 +263,13 @@ private protected void WriteRecordBatchInternal(RecordBatch recordBatch)
HasWrittenSchema = true;
}
+ if (!HasWrittenDictionaryBatch)
+ {
+ DictionaryCollector.Collect(recordBatch, ref _dictionaryMemo);
Review comment:
The benchmark results are below.
I think this is not much impact.
How it that?
before changes:
| Method | Count | Mean | Error | StdDev | Median | Gen 0 | Gen 1 | Gen 2 | Allocated |
|------------------------------------------ |-------- |-----------:|----------:|-----------:|-----------:|------:|------:|------:|------------:|
| ArrowReaderWithMemoryStream | 10000 | 2.869 ms | 0.0561 ms | 0.0982 ms | 2.841 ms | - | - | - | 10.09 KB |
| ArrowReaderWithMemoryStream_ManagedMemory | 10000 | 2.371 ms | 0.0474 ms | 0.1116 ms | 2.335 ms | - | - | - | 1444.86 KB |
| ArrowReaderWithMemory | 10000 | 2.122 ms | 0.1173 ms | 0.3365 ms | 2.160 ms | - | - | - | 7.67 KB |
| ArrowReaderWithMemoryStream | 1000000 | 220.505 ms | 4.3805 ms | 12.5684 ms | 219.305 ms | - | - | - | 8.13 KB |
| ArrowReaderWithMemoryStream_ManagedMemory | 1000000 | 211.168 ms | 4.1733 ms | 11.1395 ms | 206.210 ms | - | - | - | 142425.4 KB |
| ArrowReaderWithMemory | 1000000 | 166.486 ms | 3.9192 ms | 11.5558 ms | 165.832 ms | - | - | - | 7.59 KB |
| Method | BatchLength | ColumnSetCount | Mean | Error | StdDev | Median | Gen 0 | Gen 1 | Gen 2 | Allocated |
|----------- |------------ |--------------- |-----------:|----------:|----------:|-----------:|------:|------:|------:|----------:|
| WriteBatch | 10000 | 10 | 2.844 ms | 0.1597 ms | 0.4633 ms | 2.732 ms | - | - | - | 78.06 KB |
| WriteBatch | 10000 | 14 | 3.913 ms | 0.2181 ms | 0.6362 ms | 3.692 ms | - | - | - | 112.16 KB |
| WriteBatch | 1000000 | 10 | 239.486 ms | 4.7245 ms | 4.4193 ms | 239.822 ms | - | - | - | 79.1 KB |
| WriteBatch | 1000000 | 14 | 331.087 ms | 6.2670 ms | 7.4604 ms | 331.364 ms | - | - | - | 111.48 KB |
after changes:
| Method | Count | Mean | Error | StdDev | Median | Gen 0 | Gen 1 | Gen 2 | Allocated |
|------------------------------------------ |-------- |-----------:|----------:|-----------:|-----------:|----------:|------:|------:|-------------:|
| ArrowReaderWithMemoryStream | 10000 | 2.820 ms | 0.0560 ms | 0.1319 ms | 2.788 ms | 1000.0000 | - | - | 10.27 KB |
| ArrowReaderWithMemoryStream_ManagedMemory | 10000 | 2.376 ms | 0.0475 ms | 0.1252 ms | 2.329 ms | - | - | - | 1445.03 KB |
| ArrowReaderWithMemory | 10000 | 2.233 ms | 0.0305 ms | 0.0255 ms | 2.235 ms | - | - | - | 7.81 KB |
| ArrowReaderWithMemoryStream | 1000000 | 206.608 ms | 4.1289 ms | 10.5095 ms | 201.813 ms | - | - | - | 8.3 KB |
| ArrowReaderWithMemoryStream_ManagedMemory | 1000000 | 226.408 ms | 4.4877 ms | 11.7435 ms | 222.866 ms | - | - | - | 142425.57 KB |
| ArrowReaderWithMemory | 1000000 | 149.248 ms | 2.3519 ms | 5.1625 ms | 147.150 ms | - | - | - | 7.73 KB |
| Method | BatchLength | ColumnSetCount | Mean | Error | StdDev | Median | Gen 0 | Gen 1 | Gen 2 | Allocated |
|----------- |------------ |--------------- |-----------:|----------:|-----------:|-----------:|------:|------:|------:|----------:|
| WriteBatch | 10000 | 10 | 2.794 ms | 0.1591 ms | 0.4566 ms | 2.639 ms | - | - | - | 78.97 KB |
| WriteBatch | 10000 | 14 | 3.991 ms | 0.2347 ms | 0.6772 ms | 3.776 ms | - | - | - | 113.35 KB |
| WriteBatch | 1000000 | 10 | 245.601 ms | 8.2668 ms | 23.8516 ms | 240.295 ms | - | - | - | 77.97 KB |
| WriteBatch | 1000000 | 14 | 317.090 ms | 6.2694 ms | 13.4956 ms | 317.577 ms | - | - | - | 112.35 KB |
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on a change in pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
eerhardt commented on a change in pull request #10527:
URL: https://github.com/apache/arrow/pull/10527#discussion_r662433526
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowReaderImplementation.cs
##########
@@ -29,6 +29,13 @@ internal abstract class ArrowReaderImplementation : IDisposable
{
public Schema Schema { get; protected set; }
protected bool HasReadSchema => Schema != null;
+ protected bool HasReadInitialDictionary { get; set; }
+ protected readonly DictionaryMemo _dictionaryMemo;
+
+ public ArrowReaderImplementation()
+ {
+ _dictionaryMemo = new DictionaryMemo();
Review comment:
1. I wouldn't use create a new class for this, .NET has https://docs.microsoft.com/dotnet/api/system.lazy-1 already for it.
2. Even better is to not use `Lazy<T>` at all, and just check for null before using it. You can make a private property to make this really easy:
```C#
private DictionaryMemo _dictionaryMemo;
private DictionaryMemo DictionaryMemo => _dictionaryMemo ??= new DictionaryMemo();
// later in code that needs to use DictonaryMemo:
DictionaryMemo.GetDictionaryType(id);
```
This way, nothing new is allocated when we don't see a Dictionary batch in the payload.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on a change in pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
eerhardt commented on a change in pull request #10527:
URL: https://github.com/apache/arrow/pull/10527#discussion_r684687558
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamReaderImplementation.cs
##########
@@ -59,6 +97,75 @@ protected async ValueTask<RecordBatch> ReadRecordBatchAsync(CancellationToken ca
{
await ReadSchemaAsync().ConfigureAwait(false);
+ await ReadInitialDictionariesAsync(cancellationToken).ConfigureAwait(false);
+
+ return await ReadArrowObjectAsync(cancellationToken).ConfigureAwait(false);
+ }
+
+
+ protected RecordBatch ReadRecordBatch()
+ {
+ ReadSchema();
+
+ ReadInitialDictionaries();
+
+ return ReadArrowObject();
Review comment:
> I tested reading interleaved dictionaries by creating a test file with python
In my spare time, I've been trying to set up the integration tests, so we can test the C# implementation against the other languages. Hopefully I can get the initial PR for that up soon.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10527:
URL: https://github.com/apache/arrow/pull/10527#issuecomment-860702579
https://issues.apache.org/jira/browse/ARROW-6870
--
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:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on a change in pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
eerhardt commented on a change in pull request #10527:
URL: https://github.com/apache/arrow/pull/10527#discussion_r666503779
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamWriter.cs
##########
@@ -248,6 +263,13 @@ private protected void WriteRecordBatchInternal(RecordBatch recordBatch)
HasWrittenSchema = true;
}
+ if (!HasWrittenDictionaryBatch)
+ {
+ DictionaryCollector.Collect(recordBatch, ref _dictionaryMemo);
Review comment:
Can you run the benchmark tests before and after this change to see what kind of impact this `DictionaryCollector` has when there are no dictionaries in the batch? Hopefully it isn't much, but I'd like to know.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on a change in pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
eerhardt commented on a change in pull request #10527:
URL: https://github.com/apache/arrow/pull/10527#discussion_r684687694
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamWriter.cs
##########
@@ -128,6 +129,15 @@ public void Visit(StructArray array)
}
}
+ public void Visit(DictionaryArray array)
+ {
+ //Dictionary is serialized separately in Dictionary serialization.
+ //We are only interested in indexes at this context.
Review comment:
(nits) comments usually have a space between the slashes and the first character. Also `indexes` => `indices`.
```suggestion
// Dictionary is serialized separately in Dictionary serialization.
// We are only interested in indices at this context.
```
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] HashidaTKS commented on a change in pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
HashidaTKS commented on a change in pull request #10527:
URL: https://github.com/apache/arrow/pull/10527#discussion_r653778602
##########
File path: csharp/src/Apache.Arrow/RecordBatch.cs
##########
@@ -29,7 +29,7 @@ public partial class RecordBatch : IDisposable
public int Length { get; }
private readonly IMemoryOwner<byte> _memoryOwner;
- private readonly IList<IArrowArray> _arrays;
+ internal readonly IReadOnlyList<IArrowArray> _arrays;
Review comment:
Fixed it.
There was already a public property named `Arrays` so I named the internal property `_Arrays`.
##########
File path: csharp/src/Apache.Arrow/Arrays/DictionaryArray.cs
##########
@@ -0,0 +1,67 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+using System;
+using System.IO;
+using Apache.Arrow.Types;
+
+namespace Apache.Arrow
+{
+ public class DictionaryArray : Array
+ {
+ public IArrowArray Dictionary { get; }
+ public IArrowArray Indices { get; }
+ public ArrowBuffer IndicesBuffer => Data.Buffers[1];
+
+ public DictionaryArray(IArrowType dataType, int length,
+ ArrowBuffer valueOffsetsBuffer, IArrowArray value,
+ ArrowBuffer nullBitmapBuffer, int nullCount = 0, int offset = 0)
+ : this(new ArrayData(dataType, length, nullCount, offset,
+ new[] { nullBitmapBuffer, valueOffsetsBuffer }, new[] { value.Data }, value.Data.Dictionary))
+ {
+ }
+
+ public DictionaryArray(ArrayData data) : base(data)
+ {
+ data.EnsureBufferCount(2);
+ data.EnsureDataType(ArrowTypeId.Dictionary);
+
+ var dicType = data.DataType as DictionaryType;
+ data.Dictionary.EnsureDataType(dicType.ValueType.TypeId);
+
+ ArrayData indicesData = new ArrayData(dicType.IndexType, data.Length, data.NullCount, data.Offset, data.Buffers, data.Children);
+
+ Indices = ArrowArrayFactory.BuildArray(indicesData);
+ Dictionary = ArrowArrayFactory.BuildArray(data.Dictionary);
+ }
+
+ public DictionaryArray(IArrowType dataType, IArrowArray indicesArray, IArrowArray dictionary, bool ordered = false) :
Review comment:
Fixed it.
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowReaderImplementation.cs
##########
@@ -29,6 +29,13 @@ internal abstract class ArrowReaderImplementation : IDisposable
{
public Schema Schema { get; protected set; }
protected bool HasReadSchema => Schema != null;
+ protected bool HasReadInitialDictionary { get; set; }
Review comment:
Fixed it.
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowReaderImplementation.cs
##########
@@ -29,6 +29,13 @@ internal abstract class ArrowReaderImplementation : IDisposable
{
public Schema Schema { get; protected set; }
protected bool HasReadSchema => Schema != null;
+ protected bool HasReadInitialDictionary { get; set; }
+ protected readonly DictionaryMemo _dictionaryMemo;
+
+ public ArrowReaderImplementation()
+ {
+ _dictionaryMemo = new DictionaryMemo();
Review comment:
I have created the `LazyCreator` class and changed `DictionaryMemo _dictionaryMemo` to `LazyCreator<DictionaryMemo> _lazyDictionaryMemo`.
However, I am wondering whether this is the best way to lazy load because it need allocate a `LazyCreator` object.
Is there any other good idea?
##########
File path: csharp/src/Apache.Arrow/Arrays/DictionaryArray.cs
##########
@@ -0,0 +1,67 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+using System;
+using System.IO;
+using Apache.Arrow.Types;
+
+namespace Apache.Arrow
+{
+ public class DictionaryArray : Array
+ {
+ public IArrowArray Dictionary { get; }
+ public IArrowArray Indices { get; }
+ public ArrowBuffer IndicesBuffer => Data.Buffers[1];
+
+ public DictionaryArray(IArrowType dataType, int length,
+ ArrowBuffer valueOffsetsBuffer, IArrowArray value,
+ ArrowBuffer nullBitmapBuffer, int nullCount = 0, int offset = 0)
+ : this(new ArrayData(dataType, length, nullCount, offset,
+ new[] { nullBitmapBuffer, valueOffsetsBuffer }, new[] { value.Data }, value.Data.Dictionary))
+ {
+ }
+
+ public DictionaryArray(ArrayData data) : base(data)
+ {
+ data.EnsureBufferCount(2);
+ data.EnsureDataType(ArrowTypeId.Dictionary);
+
+ var dicType = data.DataType as DictionaryType;
Review comment:
Fixed it.
##########
File path: csharp/src/Apache.Arrow/Arrays/ArrayData.cs
##########
@@ -30,31 +30,34 @@ public sealed class ArrayData : IDisposable
public readonly int Offset;
public readonly ArrowBuffer[] Buffers;
public readonly ArrayData[] Children;
+ public readonly ArrayData Dictionary; //Only used for dictionary type
public ArrayData(
IArrowType dataType,
int length, int nullCount = 0, int offset = 0,
- IEnumerable<ArrowBuffer> buffers = null, IEnumerable<ArrayData> children = null)
+ IEnumerable<ArrowBuffer> buffers = null, IEnumerable<ArrayData> children = null, ArrayData dictionary = null)
Review comment:
Fixed it.
##########
File path: csharp/src/Apache.Arrow/Arrays/DictionaryArray.cs
##########
@@ -0,0 +1,67 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+using System;
+using System.IO;
+using Apache.Arrow.Types;
+
+namespace Apache.Arrow
+{
+ public class DictionaryArray : Array
+ {
+ public IArrowArray Dictionary { get; }
+ public IArrowArray Indices { get; }
+ public ArrowBuffer IndicesBuffer => Data.Buffers[1];
+
+ public DictionaryArray(IArrowType dataType, int length,
+ ArrowBuffer valueOffsetsBuffer, IArrowArray value,
+ ArrowBuffer nullBitmapBuffer, int nullCount = 0, int offset = 0)
+ : this(new ArrayData(dataType, length, nullCount, offset,
+ new[] { nullBitmapBuffer, valueOffsetsBuffer }, new[] { value.Data }, value.Data.Dictionary))
+ {
+ }
+
+ public DictionaryArray(ArrayData data) : base(data)
+ {
+ data.EnsureBufferCount(2);
+ data.EnsureDataType(ArrowTypeId.Dictionary);
+
+ var dicType = data.DataType as DictionaryType;
+ data.Dictionary.EnsureDataType(dicType.ValueType.TypeId);
Review comment:
added a null check.
##########
File path: csharp/src/Apache.Arrow/Arrays/DictionaryArray.cs
##########
@@ -0,0 +1,67 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+using System;
+using System.IO;
+using Apache.Arrow.Types;
+
+namespace Apache.Arrow
+{
+ public class DictionaryArray : Array
+ {
+ public IArrowArray Dictionary { get; }
+ public IArrowArray Indices { get; }
+ public ArrowBuffer IndicesBuffer => Data.Buffers[1];
+
+ public DictionaryArray(IArrowType dataType, int length,
Review comment:
removed it.
##########
File path: csharp/src/Apache.Arrow/Arrays/DictionaryArray.cs
##########
@@ -0,0 +1,67 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+using System;
+using System.IO;
+using Apache.Arrow.Types;
+
+namespace Apache.Arrow
+{
+ public class DictionaryArray : Array
+ {
+ public IArrowArray Dictionary { get; }
+ public IArrowArray Indices { get; }
+ public ArrowBuffer IndicesBuffer => Data.Buffers[1];
+
+ public DictionaryArray(IArrowType dataType, int length,
Review comment:
Removed it.
##########
File path: csharp/src/Apache.Arrow/Arrays/DictionaryArray.cs
##########
@@ -0,0 +1,67 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+using System;
+using System.IO;
+using Apache.Arrow.Types;
+
+namespace Apache.Arrow
+{
+ public class DictionaryArray : Array
+ {
+ public IArrowArray Dictionary { get; }
+ public IArrowArray Indices { get; }
+ public ArrowBuffer IndicesBuffer => Data.Buffers[1];
+
+ public DictionaryArray(IArrowType dataType, int length,
+ ArrowBuffer valueOffsetsBuffer, IArrowArray value,
+ ArrowBuffer nullBitmapBuffer, int nullCount = 0, int offset = 0)
+ : this(new ArrayData(dataType, length, nullCount, offset,
+ new[] { nullBitmapBuffer, valueOffsetsBuffer }, new[] { value.Data }, value.Data.Dictionary))
+ {
+ }
+
+ public DictionaryArray(ArrayData data) : base(data)
+ {
+ data.EnsureBufferCount(2);
+ data.EnsureDataType(ArrowTypeId.Dictionary);
+
+ var dicType = data.DataType as DictionaryType;
+ data.Dictionary.EnsureDataType(dicType.ValueType.TypeId);
Review comment:
Added a null check.
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamWriter.cs
##########
@@ -216,10 +231,10 @@ private void CreateSelfAndChildrenFieldNodes(ArrayData data)
Flatbuf.FieldNode.CreateFieldNode(Builder, data.Length, data.NullCount);
}
- private int CountAllNodes()
+ private int CountAllNodes(IReadOnlyDictionary<string, Field> fields)
Review comment:
Fixed it.
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamWriter.cs
##########
@@ -399,6 +439,95 @@ private protected void WriteRecordBatchInternal(RecordBatch recordBatch)
return Tuple.Create(recordBatchBuilder, fieldNodesVectorOffset);
}
+
+ private protected void WriteDictionaries(RecordBatch recordBatch)
+ {
+ foreach (Field field in recordBatch.Schema.Fields.Values)
+ {
+ WriteDictionary(field);
+ }
+ }
+
+ private protected void WriteDictionary(Field field)
+ {
+ if (field.DataType.TypeId != ArrowTypeId.Dictionary)
+ {
+ if (field.DataType is NestedType nestedType)
+ {
+ foreach (Field child in nestedType.Fields)
+ {
+ WriteDictionary(child);
+ }
+ }
+ return;
+ }
+
+ (ArrowRecordBatchFlatBufferBuilder recordBatchBuilder, Offset<Flatbuf.DictionaryBatch> dictionaryBatchOffset) =
+ CreateDictionaryBatchOffset(field);
+
+ WriteMessage(Flatbuf.MessageHeader.DictionaryBatch,
+ dictionaryBatchOffset, recordBatchBuilder.TotalLength);
+
+ WriteBufferData(recordBatchBuilder.Buffers);
+ }
+
+ private protected async Task WriteDictionariesAsync(RecordBatch recordBatch, CancellationToken cancellationToken = default)
+ {
+ foreach (Field field in recordBatch.Schema.Fields.Values)
+ {
+ await WriteDictionaryAsync(field, cancellationToken).ConfigureAwait(false);
+ }
+ }
+
+ private protected async Task WriteDictionaryAsync(Field field, CancellationToken cancellationToken = default)
+ {
+ if (field.DataType.TypeId != ArrowTypeId.Dictionary)
+ {
+ if (field.DataType is NestedType nestedType)
+ {
+ foreach (Field child in nestedType.Fields)
+ {
+ await WriteDictionaryAsync(child, cancellationToken).ConfigureAwait(false);
+ }
+ }
+ return;
+ }
+
+ (ArrowRecordBatchFlatBufferBuilder recordBatchBuilder, Offset<Flatbuf.DictionaryBatch> dictionaryBatchOffset) =
+ CreateDictionaryBatchOffset(field);
+
+ await WriteMessageAsync(Flatbuf.MessageHeader.DictionaryBatch,
+ dictionaryBatchOffset, recordBatchBuilder.TotalLength, cancellationToken).ConfigureAwait(false);
+
+ await WriteBufferDataAsync(recordBatchBuilder.Buffers, cancellationToken).ConfigureAwait(false);
+ }
+
+ private protected Tuple<ArrowRecordBatchFlatBufferBuilder, Offset<Flatbuf.DictionaryBatch>> CreateDictionaryBatchOffset(Field field)
Review comment:
Fixed it.
##########
File path: csharp/src/Apache.Arrow/Ipc/DictionaryMemo.cs
##########
@@ -0,0 +1,113 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+using System;
+using System.Collections.Generic;
+using Apache.Arrow.Types;
+
+namespace Apache.Arrow.Ipc
+{
+ class DictionaryMemo
+ {
+ private Dictionary<long, IArrowArray> _idToDictionary;
+ private Dictionary<long, IArrowType> _idToValueType;
+ private Dictionary<Field, long> _fieldToId;
+
+ public DictionaryMemo()
+ {
+ _idToDictionary = new Dictionary<long, IArrowArray>();
+ _idToValueType = new Dictionary<long, IArrowType>();
+ _fieldToId = new Dictionary<Field, long>();
+ }
+
+ public IArrowType GetDictionaryType(long id)
+ {
+ if (!_idToValueType.TryGetValue(id, out IArrowType type))
+ {
+ throw new ArgumentException($"Dictionary with id {id} not found");
+ }
+ return type;
+ }
+
+ public IArrowArray GetDictionary(long id)
+ {
+ if (!_idToDictionary.TryGetValue(id, out IArrowArray dictionary))
+ {
+ throw new ArgumentException($"Dictionary with id {id} not found");
+ }
+ return dictionary;
+ }
+
+ public void AddField(long id, Field field)
+ {
+ if (_fieldToId.ContainsKey(field))
+ {
+ throw new ArgumentException($"Field {field.Name} is already in Memo");
+ }
+
+ if (field.DataType.TypeId != ArrowTypeId.Dictionary)
+ {
+ throw new ArgumentException($"Field type is not DictionaryType: Name={field.Name}, {field.DataType.Name}");
+ }
+
+ IArrowType valueType = ((DictionaryType)field.DataType).ValueType;
+
+ if (_idToValueType.TryGetValue(id, out IArrowType valueTypeInDic))
+ {
+ if (valueType != valueTypeInDic)
+ {
+ throw new ArgumentException($"Field type {field.DataType.Name} does not match the existing type {valueTypeInDic})");
+ }
+ }
+
+ _fieldToId.Add(field, id);
+ _idToValueType.Add(id, valueType);
+ }
+
+ public long GetId(Field field)
+ {
+ if (!_fieldToId.TryGetValue(field, out long id))
+ {
+ throw new ArgumentException($"Field with name {field.Name} not found");
+ }
+ return id;
+ }
+
+ public long GetOrAssignId(Field field)
+ {
+ if (!_fieldToId.TryGetValue(field, out long id))
+ {
+ id = _fieldToId.Count + 1;
+ AddField(id, field);
+ }
+ return id;
+ }
+
+ public void AddOrReplaceDictionary(long id, IArrowArray dictionary)
+ {
+ _idToDictionary[id] = dictionary;
+ }
+
+ public void AddDictionaryDelta(long id, IArrowArray dictionary)
Review comment:
Fixed it.
##########
File path: csharp/src/Apache.Arrow/Arrays/ArrayData.cs
##########
@@ -30,10 +30,25 @@ public sealed class ArrayData : IDisposable
public readonly int Offset;
public readonly ArrowBuffer[] Buffers;
public readonly ArrayData[] Children;
+ public readonly ArrayData Dictionary; //Only used for dictionary type
public ArrayData(
IArrowType dataType,
int length, int nullCount = 0, int offset = 0,
+ IEnumerable<ArrowBuffer> buffers = null, IEnumerable<ArrayData> children = null) :
+ this(dataType, null, length, nullCount, offset, buffers, children)
+ { }
+
+ public ArrayData(
+ IArrowType dataType,
+ int length, int nullCount = 0, int offset = 0,
+ ArrowBuffer[] buffers = null, ArrayData[] children = null) :
+ this(dataType, null, length, nullCount, offset, buffers, children)
+ { }
+
+ public ArrayData(
+ IArrowType dataType, ArrayData dictionary,
Review comment:
To avoid the ambiguity between ctors, `dictionary` is positioned the second argument...
##########
File path: csharp/test/Apache.Arrow.Tests/ArrowStreamReaderTests.cs
##########
@@ -68,7 +68,7 @@ public async Task Ctor_MemoryPool_AllocatesFromPool(bool shouldLeaveOpen)
ArrowStreamReader reader = new ArrowStreamReader(stream, memoryPool, shouldLeaveOpen);
reader.ReadNextRecordBatch();
- Assert.Equal(1, memoryPool.Statistics.Allocations);
+ Assert.Equal(2, memoryPool.Statistics.Allocations);
Review comment:
Memo:
`_allocator` is called when reading dictionary batches and record batches.
The expected value of `memoryPool.Statistics.Allocations` is `2` at this context.
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowReaderImplementation.cs
##########
@@ -29,6 +29,13 @@ internal abstract class ArrowReaderImplementation : IDisposable
{
public Schema Schema { get; protected set; }
protected bool HasReadSchema => Schema != null;
+ protected bool HasReadInitialDictionary { get; set; }
+ protected readonly DictionaryMemo _dictionaryMemo;
+
+ public ArrowReaderImplementation()
+ {
+ _dictionaryMemo = new DictionaryMemo();
Review comment:
I have created a `LazyCreator` class and changed `DictionaryMemo _dictionaryMemo` to `LazyCreator<DictionaryMemo> _lazyDictionaryMemo`.
However, I am wondering whether this is the best way to lazy load because it need allocate a `LazyCreator` object.
Is there any other good idea?
##########
File path: csharp/src/Apache.Arrow/Arrays/ArrayData.cs
##########
@@ -30,10 +30,25 @@ public sealed class ArrayData : IDisposable
public readonly int Offset;
public readonly ArrowBuffer[] Buffers;
public readonly ArrayData[] Children;
+ public readonly ArrayData Dictionary; //Only used for dictionary type
public ArrayData(
IArrowType dataType,
int length, int nullCount = 0, int offset = 0,
+ IEnumerable<ArrowBuffer> buffers = null, IEnumerable<ArrayData> children = null) :
+ this(dataType, null, length, nullCount, offset, buffers, children)
+ { }
+
+ public ArrayData(
+ IArrowType dataType,
+ int length, int nullCount = 0, int offset = 0,
+ ArrowBuffer[] buffers = null, ArrayData[] children = null) :
+ this(dataType, null, length, nullCount, offset, buffers, children)
+ { }
+
+ public ArrayData(
+ IArrowType dataType, ArrayData dictionary,
Review comment:
To avoid the ambiguity between ctors, `dictionary` is positioned to the second argument...
##########
File path: csharp/src/Apache.Arrow/Arrays/ArrayData.cs
##########
@@ -30,10 +30,25 @@ public sealed class ArrayData : IDisposable
public readonly int Offset;
public readonly ArrowBuffer[] Buffers;
public readonly ArrayData[] Children;
+ public readonly ArrayData Dictionary; //Only used for dictionary type
public ArrayData(
IArrowType dataType,
int length, int nullCount = 0, int offset = 0,
+ IEnumerable<ArrowBuffer> buffers = null, IEnumerable<ArrayData> children = null) :
+ this(dataType, null, length, nullCount, offset, buffers, children)
+ { }
+
+ public ArrayData(
+ IArrowType dataType,
+ int length, int nullCount = 0, int offset = 0,
+ ArrowBuffer[] buffers = null, ArrayData[] children = null) :
+ this(dataType, null, length, nullCount, offset, buffers, children)
+ { }
+
+ public ArrayData(
+ IArrowType dataType, ArrayData dictionary,
Review comment:
To avoid the ambiguity between ctors, `dictionary` is positioned to the second argument.
I don't think it's a very good position in term of the meaning...
--
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:
users@infra.apache.org
[GitHub] [arrow] HashidaTKS commented on a change in pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
HashidaTKS commented on a change in pull request #10527:
URL: https://github.com/apache/arrow/pull/10527#discussion_r663550121
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowReaderImplementation.cs
##########
@@ -29,6 +29,13 @@ internal abstract class ArrowReaderImplementation : IDisposable
{
public Schema Schema { get; protected set; }
protected bool HasReadSchema => Schema != null;
+ protected bool HasReadInitialDictionary { get; set; }
+ protected readonly DictionaryMemo _dictionaryMemo;
+
+ public ArrowReaderImplementation()
+ {
+ _dictionaryMemo = new DictionaryMemo();
Review comment:
Thank you, I reimplemented them with the way 2.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on a change in pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
eerhardt commented on a change in pull request #10527:
URL: https://github.com/apache/arrow/pull/10527#discussion_r662454808
##########
File path: csharp/src/Apache.Arrow/RecordBatch.cs
##########
@@ -28,6 +28,8 @@ public partial class RecordBatch : IDisposable
public IEnumerable<IArrowArray> Arrays => _arrays;
public int Length { get; }
+ internal IReadOnlyList<IArrowArray> _Arrays => (IReadOnlyList<IArrowArray>)_arrays;
+
private readonly IMemoryOwner<byte> _memoryOwner;
private readonly IList<IArrowArray> _arrays;
Review comment:
```suggestion
internal IReadOnlyList<IArrowArray> ArrayList => _arrays;
private readonly IMemoryOwner<byte> _memoryOwner;
private readonly List<IArrowArray> _arrays;
```
1. Using the coding style from https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/coding-style.md, only fields should have `_` prefix.
2. We shouldn't need to cast here. Just have the private field as a `List<IArrowArray>`.
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamReaderImplementation.cs
##########
@@ -27,8 +27,10 @@ internal class ArrowStreamReaderImplementation : ArrowReaderImplementation
public Stream BaseStream { get; }
private readonly bool _leaveOpen;
private readonly MemoryAllocator _allocator;
+ private protected bool HasReadInitialDictionary { get; set; }
- public ArrowStreamReaderImplementation(Stream stream, MemoryAllocator allocator, bool leaveOpen)
+
+ public ArrowStreamReaderImplementation(Stream stream, MemoryAllocator allocator, bool leaveOpen) : base()
Review comment:
```suggestion
public ArrowStreamReaderImplementation(Stream stream, MemoryAllocator allocator, bool leaveOpen)
```
1. Needless extra line.
2. No need for the explicit call to base.
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamReaderImplementation.cs
##########
@@ -43,13 +45,48 @@ protected override void Dispose(bool disposing)
}
}
+ protected void ReadInitialDictionaries()
Review comment:
```suggestion
private void ReadInitialDictionaries()
```
##########
File path: csharp/test/Apache.Arrow.Tests/TestData.cs
##########
@@ -21,12 +21,13 @@ namespace Apache.Arrow.Tests
{
public static class TestData
{
- public static RecordBatch CreateSampleRecordBatch(int length)
+ //TODO: Remove the createDictionaryArray argument after all writer/reader supports DictionaryType serialization
Review comment:
What's the plan for addressing this TODO?
##########
File path: csharp/test/Apache.Arrow.Tests/ArrowStreamReaderTests.cs
##########
@@ -68,7 +68,7 @@ public async Task Ctor_MemoryPool_AllocatesFromPool(bool shouldLeaveOpen)
ArrowStreamReader reader = new ArrowStreamReader(stream, memoryPool, shouldLeaveOpen);
reader.ReadNextRecordBatch();
- Assert.Equal(1, memoryPool.Statistics.Allocations);
+ Assert.Equal(2, memoryPool.Statistics.Allocations);
Review comment:
Why change this test at all? If you want to add a new test for dictionaries - we should add a new one, and leave the existing test.
##########
File path: csharp/src/Apache.Arrow/Arrays/DictionaryArray.cs
##########
@@ -0,0 +1,67 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+using System;
+using System.IO;
+using Apache.Arrow.Types;
+
+namespace Apache.Arrow
+{
+ public class DictionaryArray : Array
+ {
+ public IArrowArray Dictionary { get; }
+ public IArrowArray Indices { get; }
+ public ArrowBuffer IndicesBuffer => Data.Buffers[1];
+
+ public DictionaryArray(IArrowType dataType, int length,
+ ArrowBuffer valueOffsetsBuffer, IArrowArray value,
+ ArrowBuffer nullBitmapBuffer, int nullCount = 0, int offset = 0)
+ : this(new ArrayData(dataType, length, nullCount, offset,
+ new[] { nullBitmapBuffer, valueOffsetsBuffer }, new[] { value.Data }, value.Data.Dictionary))
+ {
+ }
+
+ public DictionaryArray(ArrayData data) : base(data)
+ {
+ data.EnsureBufferCount(2);
+ data.EnsureDataType(ArrowTypeId.Dictionary);
+
+ var dicType = data.DataType as DictionaryType;
+ data.Dictionary.EnsureDataType(dicType.ValueType.TypeId);
Review comment:
I don't see a null check added here.
##########
File path: csharp/src/Apache.Arrow/Ipc/MessageSerializer.cs
##########
@@ -73,13 +73,27 @@ internal static Schema GetSchema(Flatbuf.Schema schema)
return new Schema(fields, metadata, copyCollections: false);
}
- private static Field FieldFromFlatbuffer(Flatbuf.Field flatbufField)
+ private static Field FieldFromFlatbuffer(Flatbuf.Field flatbufField, LazyCreator<DictionaryMemo> lazyDictionaryMemo)
{
Field[] childFields = flatbufField.ChildrenLength > 0 ? new Field[flatbufField.ChildrenLength] : null;
for (int i = 0; i < flatbufField.ChildrenLength; i++)
{
Flatbuf.Field? childFlatbufField = flatbufField.Children(i);
- childFields[i] = FieldFromFlatbuffer(childFlatbufField.Value);
+ childFields[i] = FieldFromFlatbuffer(childFlatbufField.Value, lazyDictionaryMemo);
+ }
+
+ Flatbuf.DictionaryEncoding? de = flatbufField.Dictionary;
Review comment:
(nit) Can you spell out `de`? It doesn't make a great reading experience to figure out what `de` means.
```suggestion
Flatbuf.DictionaryEncoding? dictionaryEncoding = flatbufField.Dictionary;
```
##########
File path: csharp/test/Apache.Arrow.Tests/TestData.cs
##########
@@ -21,12 +21,13 @@ namespace Apache.Arrow.Tests
{
public static class TestData
{
- public static RecordBatch CreateSampleRecordBatch(int length)
+ //TODO: Remove the createDictionaryArray argument after all writer/reader supports DictionaryType serialization
+ public static RecordBatch CreateSampleRecordBatch(int length, bool createDictionaryArray = false)
Review comment:
A general comment about `createDictionaryArray` - I think the existing tests should be left alone. That way we are still testing as much as we can without any dictionaries. Then we create new tests for dictionary support.
An easy way to do this is to change a bunch of tests from `[Fact]` to `[Theory]` and pass in `true` and `false` into the test for `bool createDictoinaryArray`. That way the test runs twice, once with a dictionary, and once without. That should give us the best test coverage.
##########
File path: csharp/src/Apache.Arrow/Ipc/MessageSerializer.cs
##########
@@ -52,14 +53,13 @@ public static Types.NumberType GetNumberType(int bitWidth, bool signed)
$"{(signed ? "signed " : "unsigned")} integer.");
}
- internal static Schema GetSchema(Flatbuf.Schema schema)
+ internal static Schema GetSchema(Flatbuf.Schema schema, LazyCreator<DictionaryMemo> lazyDictionaryMemo)
Review comment:
Along with the comment about removing `LazyCreator`, this could be changed to:
```suggestion
internal static Schema GetSchema(Flatbuf.Schema schema, ref DictionaryMemo dictionaryMemo)
```
And then if this code needs to use the `dictionaryMemo` and it is null, it gets created when it is needed. And the `ref` gets set.
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamReaderImplementation.cs
##########
@@ -43,13 +45,48 @@ protected override void Dispose(bool disposing)
}
}
+ protected void ReadInitialDictionaries()
+ {
+ if (HasReadInitialDictionary)
+ {
+ return;
+ }
+
+ if (_lazyDictionaryMemo.IsCreated) {
+ int fieldCount = _lazyDictionaryMemo.Instance.GetFieldCount();
+ for (int i = 0; i < fieldCount; ++i)
+ {
+ ReadArrowObject();
+ }
+ }
+ HasReadInitialDictionary = true;
+ }
+
+ protected async ValueTask ReadInitialDictionariesAsync(CancellationToken cancellationToken = default)
Review comment:
```suggestion
private async ValueTask ReadInitialDictionariesAsync(CancellationToken cancellationToken)
```
##########
File path: csharp/src/Apache.Arrow/Ipc/MessageSerializer.cs
##########
@@ -73,13 +73,27 @@ internal static Schema GetSchema(Flatbuf.Schema schema)
return new Schema(fields, metadata, copyCollections: false);
}
- private static Field FieldFromFlatbuffer(Flatbuf.Field flatbufField)
+ private static Field FieldFromFlatbuffer(Flatbuf.Field flatbufField, LazyCreator<DictionaryMemo> lazyDictionaryMemo)
{
Field[] childFields = flatbufField.ChildrenLength > 0 ? new Field[flatbufField.ChildrenLength] : null;
for (int i = 0; i < flatbufField.ChildrenLength; i++)
{
Flatbuf.Field? childFlatbufField = flatbufField.Children(i);
- childFields[i] = FieldFromFlatbuffer(childFlatbufField.Value);
+ childFields[i] = FieldFromFlatbuffer(childFlatbufField.Value, lazyDictionaryMemo);
+ }
+
+ Flatbuf.DictionaryEncoding? de = flatbufField.Dictionary;
+ IArrowType type = GetFieldArrowType(flatbufField, childFields);
+
+ if (de.HasValue)
+ {
+ Flatbuf.Int? indexTypeAsInt = de.Value.IndexType;
+ if (!indexTypeAsInt.HasValue)
+ {
+ throw new InvalidDataException("Dictionary type not defined");
Review comment:
Would this be a more correct message?
```suggestion
throw new InvalidDataException("Dictionary IndexType not defined");
```
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamReaderImplementation.cs
##########
@@ -59,6 +96,66 @@ protected async ValueTask<RecordBatch> ReadRecordBatchAsync(CancellationToken ca
{
await ReadSchemaAsync().ConfigureAwait(false);
+ await ReadInitialDictionariesAsync().ConfigureAwait(false);
+
+ return await ReadArrowObjectAsync().ConfigureAwait(false);
Review comment:
```suggestion
await ReadInitialDictionariesAsync(cancellationToken).ConfigureAwait(false);
return await ReadArrowObjectAsync(cancellationToken ).ConfigureAwait(false);
```
##########
File path: csharp/test/Apache.Arrow.Tests/ArrowStreamWriterTests.cs
##########
@@ -32,7 +32,7 @@ public class ArrowStreamWriterTests
[Fact]
public void Ctor_LeaveOpenDefault_StreamClosedOnDispose()
{
- RecordBatch originalBatch = TestData.CreateSampleRecordBatch(length: 100);
+ RecordBatch originalBatch = TestData.CreateSampleRecordBatch(length: 100, createDictionaryArray: true);
Review comment:
I'd prefer if these were left as-is. There is no reason to create dictionary arrays for these dispose tests.
##########
File path: csharp/src/Apache.Arrow/Ipc/DictionaryMemo.cs
##########
@@ -0,0 +1,113 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+using System;
+using System.Collections.Generic;
+using Apache.Arrow.Types;
+
+namespace Apache.Arrow.Ipc
+{
+ class DictionaryMemo
+ {
+ private Dictionary<long, IArrowArray> _idToDictionary;
+ private Dictionary<long, IArrowType> _idToValueType;
+ private Dictionary<Field, long> _fieldToId;
+
+ public DictionaryMemo()
+ {
+ _idToDictionary = new Dictionary<long, IArrowArray>();
+ _idToValueType = new Dictionary<long, IArrowType>();
+ _fieldToId = new Dictionary<Field, long>();
+ }
+
+ public IArrowType GetDictionaryType(long id)
+ {
+ if (!_idToValueType.TryGetValue(id, out IArrowType type))
+ {
+ throw new ArgumentException($"Dictionary with id {id} not found");
+ }
+ return type;
+ }
+
+ public IArrowArray GetDictionary(long id)
+ {
+ if (!_idToDictionary.TryGetValue(id, out IArrowArray dictionary))
+ {
+ throw new ArgumentException($"Dictionary with id {id} not found");
+ }
+ return dictionary;
+ }
+
+ public void AddField(long id, Field field)
+ {
+ if (_fieldToId.ContainsKey(field))
+ {
+ throw new ArgumentException($"Field {field.Name} is already in Memo");
+ }
+
+ if (field.DataType.TypeId != ArrowTypeId.Dictionary)
+ {
+ throw new ArgumentException($"Field type is not DictionaryType: Name={field.Name}, {field.DataType.Name}");
+ }
+
+ IArrowType valueType = ((DictionaryType)field.DataType).ValueType;
+
+ if (_idToValueType.TryGetValue(id, out IArrowType valueTypeInDic))
+ {
+ if (valueType != valueTypeInDic)
+ {
+ throw new ArgumentException($"Field type {field.DataType.Name} does not match the existing type {valueTypeInDic})");
+ }
+ }
+
+ _fieldToId.Add(field, id);
+ _idToValueType.Add(id, valueType);
+ }
+
+ public long GetId(Field field)
+ {
+ if (!_fieldToId.TryGetValue(field, out long id))
+ {
+ throw new ArgumentException($"Field with name {field.Name} not found");
+ }
+ return id;
+ }
+
+ public long GetOrAssignId(Field field)
+ {
+ if (!_fieldToId.TryGetValue(field, out long id))
+ {
+ id = _fieldToId.Count + 1;
+ AddField(id, field);
+ }
+ return id;
+ }
+
+ public void AddOrReplaceDictionary(long id, IArrowArray dictionary)
+ {
+ _idToDictionary[id] = dictionary;
+ }
+
+ public void AddDictionaryDelta(long id, IArrowArray dictionary)
Review comment:
This hasn't been deleted (yet)
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamReaderImplementation.cs
##########
@@ -43,13 +45,48 @@ protected override void Dispose(bool disposing)
}
}
+ protected void ReadInitialDictionaries()
+ {
+ if (HasReadInitialDictionary)
+ {
+ return;
+ }
+
+ if (_lazyDictionaryMemo.IsCreated) {
+ int fieldCount = _lazyDictionaryMemo.Instance.GetFieldCount();
+ for (int i = 0; i < fieldCount; ++i)
+ {
+ ReadArrowObject();
+ }
+ }
+ HasReadInitialDictionary = true;
+ }
+
+ protected async ValueTask ReadInitialDictionariesAsync(CancellationToken cancellationToken = default)
+ {
+ if (HasReadInitialDictionary)
+ {
+ return;
+ }
+
+ int fieldCount = _lazyDictionaryMemo.Instance.GetFieldCount();
+ for (int i = 0; i < fieldCount; ++i)
+ {
+ await ReadArrowObjectAsync(cancellationToken).ConfigureAwait(false);
+ }
+
+ HasReadInitialDictionary = true;
+ }
+
public override async ValueTask<RecordBatch> ReadNextRecordBatchAsync(CancellationToken cancellationToken)
{
// TODO: Loop until a record batch is read.
cancellationToken.ThrowIfCancellationRequested();
return await ReadRecordBatchAsync(cancellationToken).ConfigureAwait(false);
}
+
Review comment:
These empty lines are unnecessary
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamReaderImplementation.cs
##########
@@ -27,8 +27,10 @@ internal class ArrowStreamReaderImplementation : ArrowReaderImplementation
public Stream BaseStream { get; }
private readonly bool _leaveOpen;
private readonly MemoryAllocator _allocator;
+ private protected bool HasReadInitialDictionary { get; set; }
Review comment:
```suggestion
private bool HasReadInitialDictionary { get; set; }
```
Nothing outside this class uses this property.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on a change in pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
eerhardt commented on a change in pull request #10527:
URL: https://github.com/apache/arrow/pull/10527#discussion_r684687886
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamWriter.cs
##########
@@ -248,6 +263,13 @@ private protected void WriteRecordBatchInternal(RecordBatch recordBatch)
HasWrittenSchema = true;
}
+ if (!HasWrittenDictionaryBatch)
+ {
+ DictionaryCollector.Collect(recordBatch, ref _dictionaryMemo);
Review comment:
This is good news. Thanks for running the tests.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] HashidaTKS commented on pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
HashidaTKS commented on pull request #10527:
URL: https://github.com/apache/arrow/pull/10527#issuecomment-893067009
Cc: @eerhardt
Would you please re-review this when you have time?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] HashidaTKS commented on a change in pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
HashidaTKS commented on a change in pull request #10527:
URL: https://github.com/apache/arrow/pull/10527#discussion_r668329358
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamWriter.cs
##########
@@ -248,6 +263,13 @@ private protected void WriteRecordBatchInternal(RecordBatch recordBatch)
HasWrittenSchema = true;
}
+ if (!HasWrittenDictionaryBatch)
+ {
+ DictionaryCollector.Collect(recordBatch, ref _dictionaryMemo);
Review comment:
The benchmark results are below.
I think there is not much impact.
How it that?
before this change:
| Method | Count | Mean | Error | StdDev | Median | Gen 0 | Gen 1 | Gen 2 | Allocated |
|------------------------------------------ |-------- |-----------:|----------:|-----------:|-----------:|------:|------:|------:|------------:|
| ArrowReaderWithMemoryStream | 10000 | 2.869 ms | 0.0561 ms | 0.0982 ms | 2.841 ms | - | - | - | 10.09 KB |
| ArrowReaderWithMemoryStream_ManagedMemory | 10000 | 2.371 ms | 0.0474 ms | 0.1116 ms | 2.335 ms | - | - | - | 1444.86 KB |
| ArrowReaderWithMemory | 10000 | 2.122 ms | 0.1173 ms | 0.3365 ms | 2.160 ms | - | - | - | 7.67 KB |
| ArrowReaderWithMemoryStream | 1000000 | 220.505 ms | 4.3805 ms | 12.5684 ms | 219.305 ms | - | - | - | 8.13 KB |
| ArrowReaderWithMemoryStream_ManagedMemory | 1000000 | 211.168 ms | 4.1733 ms | 11.1395 ms | 206.210 ms | - | - | - | 142425.4 KB |
| ArrowReaderWithMemory | 1000000 | 166.486 ms | 3.9192 ms | 11.5558 ms | 165.832 ms | - | - | - | 7.59 KB |
| Method | BatchLength | ColumnSetCount | Mean | Error | StdDev | Median | Gen 0 | Gen 1 | Gen 2 | Allocated |
|----------- |------------ |--------------- |-----------:|----------:|----------:|-----------:|------:|------:|------:|----------:|
| WriteBatch | 10000 | 10 | 2.844 ms | 0.1597 ms | 0.4633 ms | 2.732 ms | - | - | - | 78.06 KB |
| WriteBatch | 10000 | 14 | 3.913 ms | 0.2181 ms | 0.6362 ms | 3.692 ms | - | - | - | 112.16 KB |
| WriteBatch | 1000000 | 10 | 239.486 ms | 4.7245 ms | 4.4193 ms | 239.822 ms | - | - | - | 79.1 KB |
| WriteBatch | 1000000 | 14 | 331.087 ms | 6.2670 ms | 7.4604 ms | 331.364 ms | - | - | - | 111.48 KB |
after this change:
| Method | Count | Mean | Error | StdDev | Median | Gen 0 | Gen 1 | Gen 2 | Allocated |
|------------------------------------------ |-------- |-----------:|----------:|-----------:|-----------:|----------:|------:|------:|-------------:|
| ArrowReaderWithMemoryStream | 10000 | 2.820 ms | 0.0560 ms | 0.1319 ms | 2.788 ms | 1000.0000 | - | - | 10.27 KB |
| ArrowReaderWithMemoryStream_ManagedMemory | 10000 | 2.376 ms | 0.0475 ms | 0.1252 ms | 2.329 ms | - | - | - | 1445.03 KB |
| ArrowReaderWithMemory | 10000 | 2.233 ms | 0.0305 ms | 0.0255 ms | 2.235 ms | - | - | - | 7.81 KB |
| ArrowReaderWithMemoryStream | 1000000 | 206.608 ms | 4.1289 ms | 10.5095 ms | 201.813 ms | - | - | - | 8.3 KB |
| ArrowReaderWithMemoryStream_ManagedMemory | 1000000 | 226.408 ms | 4.4877 ms | 11.7435 ms | 222.866 ms | - | - | - | 142425.57 KB |
| ArrowReaderWithMemory | 1000000 | 149.248 ms | 2.3519 ms | 5.1625 ms | 147.150 ms | - | - | - | 7.73 KB |
| Method | BatchLength | ColumnSetCount | Mean | Error | StdDev | Median | Gen 0 | Gen 1 | Gen 2 | Allocated |
|----------- |------------ |--------------- |-----------:|----------:|-----------:|-----------:|------:|------:|------:|----------:|
| WriteBatch | 10000 | 10 | 2.794 ms | 0.1591 ms | 0.4566 ms | 2.639 ms | - | - | - | 78.97 KB |
| WriteBatch | 10000 | 14 | 3.991 ms | 0.2347 ms | 0.6772 ms | 3.776 ms | - | - | - | 113.35 KB |
| WriteBatch | 1000000 | 10 | 245.601 ms | 8.2668 ms | 23.8516 ms | 240.295 ms | - | - | - | 77.97 KB |
| WriteBatch | 1000000 | 14 | 317.090 ms | 6.2694 ms | 13.4956 ms | 317.577 ms | - | - | - | 112.35 KB |
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on a change in pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
eerhardt commented on a change in pull request #10527:
URL: https://github.com/apache/arrow/pull/10527#discussion_r666460092
##########
File path: csharp/src/Apache.Arrow/Arrays/DictionaryArray.cs
##########
@@ -0,0 +1,67 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+using System;
+using System.IO;
+using Apache.Arrow.Types;
+
+namespace Apache.Arrow
+{
+ public class DictionaryArray : Array
+ {
+ public IArrowArray Dictionary { get; }
+ public IArrowArray Indices { get; }
+ public ArrowBuffer IndicesBuffer => Data.Buffers[1];
+
+ public DictionaryArray(IArrowType dataType, int length,
+ ArrowBuffer valueOffsetsBuffer, IArrowArray value,
+ ArrowBuffer nullBitmapBuffer, int nullCount = 0, int offset = 0)
+ : this(new ArrayData(dataType, length, nullCount, offset,
+ new[] { nullBitmapBuffer, valueOffsetsBuffer }, new[] { value.Data }, value.Data.Dictionary))
+ {
+ }
+
+ public DictionaryArray(ArrayData data) : base(data)
+ {
+ data.EnsureBufferCount(2);
+ data.EnsureDataType(ArrowTypeId.Dictionary);
+
+ var dicType = data.DataType as DictionaryType;
+ data.Dictionary.EnsureDataType(dicType.ValueType.TypeId);
Review comment:
ah, sorry I missed it. Thanks.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on a change in pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
eerhardt commented on a change in pull request #10527:
URL: https://github.com/apache/arrow/pull/10527#discussion_r666462335
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowReaderImplementation.cs
##########
@@ -30,6 +30,13 @@ internal abstract class ArrowReaderImplementation : IDisposable
public Schema Schema { get; protected set; }
protected bool HasReadSchema => Schema != null;
+ private protected DictionaryMemo _dictionaryMemo;
+ private protected DictionaryMemo DictionaryMemo => _dictionaryMemo ??= new DictionaryMemo();
+
+ public ArrowReaderImplementation()
+ {
+ }
+
Review comment:
This is unnecessary. It can be removed
```suggestion
```
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamReaderImplementation.cs
##########
@@ -43,6 +45,42 @@ protected override void Dispose(bool disposing)
}
}
+ private void ReadInitialDictionaries()
+ {
+ if (HasReadInitialDictionary)
+ {
+ return;
+ }
+
+ if (HasCreatedDictionaryMemo)
+ {
+ int fieldCount = DictionaryMemo.GetFieldCount();
+ for (int i = 0; i < fieldCount; ++i)
+ {
+ ReadArrowObject();
+ }
+ }
+ HasReadInitialDictionary = true;
+ }
+
+ private async ValueTask ReadInitialDictionariesAsync(CancellationToken cancellationToken)
Review comment:
(nit) the other methods in this file have the `Async` method first, and then the synchronous method after it. Can this go above `ReadInitialDictionaries()`?
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamReaderImplementation.cs
##########
@@ -43,6 +45,42 @@ protected override void Dispose(bool disposing)
}
}
+ private void ReadInitialDictionaries()
+ {
+ if (HasReadInitialDictionary)
+ {
+ return;
+ }
+
+ if (HasCreatedDictionaryMemo)
+ {
+ int fieldCount = DictionaryMemo.GetFieldCount();
+ for (int i = 0; i < fieldCount; ++i)
+ {
+ ReadArrowObject();
+ }
+ }
+ HasReadInitialDictionary = true;
+ }
+
+ private async ValueTask ReadInitialDictionariesAsync(CancellationToken cancellationToken)
+ {
+ if (HasReadInitialDictionary)
+ {
+ return;
+ }
+
+ if (HasCreatedDictionaryMemo)
+ {
+ int fieldCount = DictionaryMemo.GetFieldCount();
+ for (int i = 0; i < fieldCount; ++i)
Review comment:
```suggestion
for (int i = 0; i < fieldCount; i++)
```
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamReaderImplementation.cs
##########
@@ -43,6 +45,42 @@ protected override void Dispose(bool disposing)
}
}
+ private void ReadInitialDictionaries()
+ {
+ if (HasReadInitialDictionary)
+ {
+ return;
+ }
+
+ if (HasCreatedDictionaryMemo)
+ {
+ int fieldCount = DictionaryMemo.GetFieldCount();
+ for (int i = 0; i < fieldCount; ++i)
Review comment:
(nit) - the coding style is to use `i++` everywhere. Can we be consistent here and below?
```suggestion
for (int i = 0; i < fieldCount; i++)
```
##########
File path: csharp/src/Apache.Arrow/Ipc/MessageSerializer.cs
##########
@@ -73,13 +73,27 @@ internal static Schema GetSchema(Flatbuf.Schema schema)
return new Schema(fields, metadata, copyCollections: false);
}
- private static Field FieldFromFlatbuffer(Flatbuf.Field flatbufField)
+ private static Field FieldFromFlatbuffer(Flatbuf.Field flatbufField, ref DictionaryMemo dictionaryMemo)
{
Field[] childFields = flatbufField.ChildrenLength > 0 ? new Field[flatbufField.ChildrenLength] : null;
for (int i = 0; i < flatbufField.ChildrenLength; i++)
{
Flatbuf.Field? childFlatbufField = flatbufField.Children(i);
- childFields[i] = FieldFromFlatbuffer(childFlatbufField.Value);
+ childFields[i] = FieldFromFlatbuffer(childFlatbufField.Value, ref dictionaryMemo);
+ }
+
+ Flatbuf.DictionaryEncoding? dictionaryEncoding = flatbufField.Dictionary;
+ IArrowType type = GetFieldArrowType(flatbufField, childFields);
+
+ if (dictionaryEncoding.HasValue)
+ {
+ Flatbuf.Int? indexTypeAsInt = dictionaryEncoding.Value.IndexType;
+ if (!indexTypeAsInt.HasValue)
Review comment:
The format doc says
> If this field is null, the indices must be signed int32.
https://github.com/apache/arrow/blob/0219e9a198b201df852b4219816752b36f116825/format/Schema.fbs#L328-L333
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamReaderImplementation.cs
##########
@@ -59,6 +97,75 @@ protected async ValueTask<RecordBatch> ReadRecordBatchAsync(CancellationToken ca
{
await ReadSchemaAsync().ConfigureAwait(false);
+ await ReadInitialDictionariesAsync(cancellationToken).ConfigureAwait(false);
+
+ return await ReadArrowObjectAsync(cancellationToken).ConfigureAwait(false);
+ }
+
+
+ protected RecordBatch ReadRecordBatch()
+ {
+ ReadSchema();
+
+ ReadInitialDictionaries();
+
+ return ReadArrowObject();
Review comment:
The [spec](https://arrow.apache.org/docs/format/Columnar.html#ipc-streaming-format) says
> DictionaryBatch and RecordBatch messages may be interleaved
This appears to only support DictionaryBatches above RecordBatches. If we read all the "initial" dictionaries, and then read a RecordBatch, the next time `ReadRecordBatch()` is called, if there is a DictionaryBatch, `null` will be returned. Is that intentional?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on a change in pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
eerhardt commented on a change in pull request #10527:
URL: https://github.com/apache/arrow/pull/10527#discussion_r666495112
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamWriter.cs
##########
@@ -723,4 +868,62 @@ public virtual void Dispose()
}
}
}
+
+ internal static class DictionaryCollector
+ {
+ internal static void Collect(RecordBatch recordBatch, ref DictionaryMemo dictionaryMemo)
+ {
+ Schema schema = recordBatch.Schema;
+ for (int i = 0; i < schema.Fields.Count; i++)
+ {
+ {
+ Field field = schema.GetFieldByIndex(i);
+ IArrowArray array = recordBatch.Column(i);
+
+ CollectDictionary(field, array, ref dictionaryMemo);
+ }
+ }
+ }
+
+ private static void CollectDictionary(Field field, IArrowArray array, ref DictionaryMemo dictionaryMemo)
+ {
+ if (field.DataType.TypeId == ArrowTypeId.Dictionary)
+ {
+ IArrowArray dictionary = (array as DictionaryArray).Dictionary;
+ dictionaryMemo ??= new DictionaryMemo();
+ long id = dictionaryMemo.GetOrAssignId(field);
+
+ dictionaryMemo.AddOrReplaceDictionary(id, dictionary);
+ WalkChildren(dictionary, ref dictionaryMemo);
+ }
+ else
+ {
+ WalkChildren(array, ref dictionaryMemo);
+ }
+ }
+
+ private static void WalkChildren(IArrowArray array, ref DictionaryMemo dictionaryMemo)
+ {
+ ArrayData[] children = array.Data.Children;
+
+ if (children == null)
+ {
+ return;
+ }
+
+ if (!(array.Data.DataType is NestedType nestedType))
+ {
+ return;
+ }
+
+ for (int i = 0; i < nestedType.Fields.Count; i++)
+ {
+ Field childField = nestedType.Fields[i];
+ ArrayData child = children[i];
+ IArrowArray childArray = ArrowArrayFactory.BuildArray(child);
Review comment:
Do we really need to build a new IArrowArray here just to walk over the children? That seems wasteful to allocate that object.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on a change in pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
eerhardt commented on a change in pull request #10527:
URL: https://github.com/apache/arrow/pull/10527#discussion_r666493486
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamWriter.cs
##########
@@ -723,4 +868,62 @@ public virtual void Dispose()
}
}
}
+
+ internal static class DictionaryCollector
+ {
+ internal static void Collect(RecordBatch recordBatch, ref DictionaryMemo dictionaryMemo)
+ {
+ Schema schema = recordBatch.Schema;
+ for (int i = 0; i < schema.Fields.Count; i++)
+ {
+ {
+ Field field = schema.GetFieldByIndex(i);
+ IArrowArray array = recordBatch.Column(i);
+
+ CollectDictionary(field, array, ref dictionaryMemo);
+ }
+ }
+ }
+
+ private static void CollectDictionary(Field field, IArrowArray array, ref DictionaryMemo dictionaryMemo)
+ {
+ if (field.DataType.TypeId == ArrowTypeId.Dictionary)
+ {
+ IArrowArray dictionary = (array as DictionaryArray).Dictionary;
+ dictionaryMemo ??= new DictionaryMemo();
+ long id = dictionaryMemo.GetOrAssignId(field);
+
+ dictionaryMemo.AddOrReplaceDictionary(id, dictionary);
+ WalkChildren(dictionary, ref dictionaryMemo);
Review comment:
Do we have a test for a DictionaryArray with children?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on a change in pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
eerhardt commented on a change in pull request #10527:
URL: https://github.com/apache/arrow/pull/10527#discussion_r666494514
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamWriter.cs
##########
@@ -723,4 +868,62 @@ public virtual void Dispose()
}
}
}
+
+ internal static class DictionaryCollector
+ {
+ internal static void Collect(RecordBatch recordBatch, ref DictionaryMemo dictionaryMemo)
+ {
+ Schema schema = recordBatch.Schema;
+ for (int i = 0; i < schema.Fields.Count; i++)
+ {
+ {
+ Field field = schema.GetFieldByIndex(i);
+ IArrowArray array = recordBatch.Column(i);
+
+ CollectDictionary(field, array, ref dictionaryMemo);
+ }
+ }
+ }
+
+ private static void CollectDictionary(Field field, IArrowArray array, ref DictionaryMemo dictionaryMemo)
+ {
+ if (field.DataType.TypeId == ArrowTypeId.Dictionary)
+ {
+ IArrowArray dictionary = (array as DictionaryArray).Dictionary;
+ dictionaryMemo ??= new DictionaryMemo();
+ long id = dictionaryMemo.GetOrAssignId(field);
+
+ dictionaryMemo.AddOrReplaceDictionary(id, dictionary);
+ WalkChildren(dictionary, ref dictionaryMemo);
+ }
+ else
+ {
+ WalkChildren(array, ref dictionaryMemo);
+ }
+ }
+
+ private static void WalkChildren(IArrowArray array, ref DictionaryMemo dictionaryMemo)
+ {
+ ArrayData[] children = array.Data.Children;
+
+ if (children == null)
+ {
+ return;
+ }
+
+ if (!(array.Data.DataType is NestedType nestedType))
+ {
+ return;
+ }
+
+ for (int i = 0; i < nestedType.Fields.Count; i++)
+ {
+ Field childField = nestedType.Fields[i];
+ ArrayData child = children[i];
+ IArrowArray childArray = ArrowArrayFactory.BuildArray(child);
+
+ CollectDictionary(childField, childArray, ref dictionaryMemo);
+ }
+ }
Review comment:
Seems simpler to say
```suggestion
if (array.Data.DataType is NestedType nestedType)
{
for (int i = 0; i < nestedType.Fields.Count; i++)
{
Field childField = nestedType.Fields[i];
ArrayData child = children[i];
IArrowArray childArray = ArrowArrayFactory.BuildArray(child);
CollectDictionary(childField, childArray, ref dictionaryMemo);
}
}
}
```
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] HashidaTKS commented on a change in pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
HashidaTKS commented on a change in pull request #10527:
URL: https://github.com/apache/arrow/pull/10527#discussion_r668346615
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowReaderImplementation.cs
##########
@@ -30,6 +30,13 @@ internal abstract class ArrowReaderImplementation : IDisposable
public Schema Schema { get; protected set; }
protected bool HasReadSchema => Schema != null;
+ private protected DictionaryMemo _dictionaryMemo;
+ private protected DictionaryMemo DictionaryMemo => _dictionaryMemo ??= new DictionaryMemo();
+
+ public ArrowReaderImplementation()
+ {
+ }
+
Review comment:
I fixed it.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] HashidaTKS commented on a change in pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
HashidaTKS commented on a change in pull request #10527:
URL: https://github.com/apache/arrow/pull/10527#discussion_r667522059
##########
File path: csharp/src/Apache.Arrow/Ipc/MessageSerializer.cs
##########
@@ -73,13 +73,27 @@ internal static Schema GetSchema(Flatbuf.Schema schema)
return new Schema(fields, metadata, copyCollections: false);
}
- private static Field FieldFromFlatbuffer(Flatbuf.Field flatbufField)
+ private static Field FieldFromFlatbuffer(Flatbuf.Field flatbufField, ref DictionaryMemo dictionaryMemo)
{
Field[] childFields = flatbufField.ChildrenLength > 0 ? new Field[flatbufField.ChildrenLength] : null;
for (int i = 0; i < flatbufField.ChildrenLength; i++)
{
Flatbuf.Field? childFlatbufField = flatbufField.Children(i);
- childFields[i] = FieldFromFlatbuffer(childFlatbufField.Value);
+ childFields[i] = FieldFromFlatbuffer(childFlatbufField.Value, ref dictionaryMemo);
+ }
+
+ Flatbuf.DictionaryEncoding? dictionaryEncoding = flatbufField.Dictionary;
+ IArrowType type = GetFieldArrowType(flatbufField, childFields);
+
+ if (dictionaryEncoding.HasValue)
+ {
+ Flatbuf.Int? indexTypeAsInt = dictionaryEncoding.Value.IndexType;
+ if (!indexTypeAsInt.HasValue)
Review comment:
I fixed it.
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamWriter.cs
##########
@@ -723,4 +868,62 @@ public virtual void Dispose()
}
}
}
+
+ internal static class DictionaryCollector
+ {
+ internal static void Collect(RecordBatch recordBatch, ref DictionaryMemo dictionaryMemo)
+ {
+ Schema schema = recordBatch.Schema;
+ for (int i = 0; i < schema.Fields.Count; i++)
+ {
+ {
Review comment:
I fixed it.
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamWriter.cs
##########
@@ -723,4 +868,62 @@ public virtual void Dispose()
}
}
}
+
+ internal static class DictionaryCollector
+ {
+ internal static void Collect(RecordBatch recordBatch, ref DictionaryMemo dictionaryMemo)
+ {
+ Schema schema = recordBatch.Schema;
+ for (int i = 0; i < schema.Fields.Count; i++)
+ {
+ {
+ Field field = schema.GetFieldByIndex(i);
+ IArrowArray array = recordBatch.Column(i);
+
+ CollectDictionary(field, array, ref dictionaryMemo);
+ }
+ }
+ }
+
+ private static void CollectDictionary(Field field, IArrowArray array, ref DictionaryMemo dictionaryMemo)
+ {
+ if (field.DataType.TypeId == ArrowTypeId.Dictionary)
+ {
+ IArrowArray dictionary = (array as DictionaryArray).Dictionary;
+ dictionaryMemo ??= new DictionaryMemo();
+ long id = dictionaryMemo.GetOrAssignId(field);
+
+ dictionaryMemo.AddOrReplaceDictionary(id, dictionary);
+ WalkChildren(dictionary, ref dictionaryMemo);
Review comment:
We didn't.
I added tests for a DictionaryArray with children and a ListArray with DictionaryType children.
And I found and fixed some bugs about serialization for those arrays.
Thank you!
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamWriter.cs
##########
@@ -723,4 +868,62 @@ public virtual void Dispose()
}
}
}
+
+ internal static class DictionaryCollector
+ {
+ internal static void Collect(RecordBatch recordBatch, ref DictionaryMemo dictionaryMemo)
+ {
+ Schema schema = recordBatch.Schema;
+ for (int i = 0; i < schema.Fields.Count; i++)
+ {
+ {
+ Field field = schema.GetFieldByIndex(i);
+ IArrowArray array = recordBatch.Column(i);
+
+ CollectDictionary(field, array, ref dictionaryMemo);
+ }
+ }
+ }
+
+ private static void CollectDictionary(Field field, IArrowArray array, ref DictionaryMemo dictionaryMemo)
+ {
+ if (field.DataType.TypeId == ArrowTypeId.Dictionary)
+ {
+ IArrowArray dictionary = (array as DictionaryArray).Dictionary;
+ dictionaryMemo ??= new DictionaryMemo();
+ long id = dictionaryMemo.GetOrAssignId(field);
+
+ dictionaryMemo.AddOrReplaceDictionary(id, dictionary);
+ WalkChildren(dictionary, ref dictionaryMemo);
+ }
+ else
+ {
+ WalkChildren(array, ref dictionaryMemo);
+ }
+ }
+
+ private static void WalkChildren(IArrowArray array, ref DictionaryMemo dictionaryMemo)
+ {
+ ArrayData[] children = array.Data.Children;
+
+ if (children == null)
+ {
+ return;
+ }
+
+ if (!(array.Data.DataType is NestedType nestedType))
+ {
+ return;
+ }
+
+ for (int i = 0; i < nestedType.Fields.Count; i++)
+ {
+ Field childField = nestedType.Fields[i];
+ ArrayData child = children[i];
+ IArrowArray childArray = ArrowArrayFactory.BuildArray(child);
Review comment:
Right, it was wasteful.
I modified to use `ArrayData` to walk over the children and execute `BuildArray()` only when it is needed.
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamReaderImplementation.cs
##########
@@ -59,6 +97,75 @@ protected async ValueTask<RecordBatch> ReadRecordBatchAsync(CancellationToken ca
{
await ReadSchemaAsync().ConfigureAwait(false);
+ await ReadInitialDictionariesAsync(cancellationToken).ConfigureAwait(false);
+
+ return await ReadArrowObjectAsync(cancellationToken).ConfigureAwait(false);
+ }
+
+
+ protected RecordBatch ReadRecordBatch()
+ {
+ ReadSchema();
+
+ ReadInitialDictionaries();
+
+ return ReadArrowObject();
Review comment:
I tested reading interleaved dictionaries by creating a test file with python and reading the test file with C#, and it seemed to work fine.
The code for creating the test file is below, this code is based on the test code of the python implementation.
https://github.com/apache/arrow/blob/master/python/pyarrow/tests/test_ipc.py#L417
```
import pyarrow as pa
ty = pa.dictionary(pa.int8(), pa.utf8())
data = [["foo", "foo", None],
["foo", "bar", "foo"],
["foo", "bar"],
["foo", None, "bar", "quux"],
["bar", "quux"],
]
batches = [
pa.RecordBatch.from_arrays([pa.array(v, type=ty)], names=['dicts'])
for v in data]
schema = batches[0].schema
def write_batches():
with pa.RecordBatchStreamWriter("./dictionary_batch_test.batch", schema = schema) as writer:
for batch in batches:
writer.write_batch(batch)
st = write_batches()
```
We should add C# tests for this after `ArrowStreamWriter` supports writing interleaved dictionaries.
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamReaderImplementation.cs
##########
@@ -59,6 +97,75 @@ protected async ValueTask<RecordBatch> ReadRecordBatchAsync(CancellationToken ca
{
await ReadSchemaAsync().ConfigureAwait(false);
+ await ReadInitialDictionariesAsync(cancellationToken).ConfigureAwait(false);
+
+ return await ReadArrowObjectAsync(cancellationToken).ConfigureAwait(false);
+ }
+
+
+ protected RecordBatch ReadRecordBatch()
+ {
+ ReadSchema();
+
+ ReadInitialDictionaries();
+
+ return ReadArrowObject();
Review comment:
Sorry, it slipped my mind. That is not intentional.
I modified to be able to read interleaved dictionaries.
During the modification, I realized that `ReadInitialDictionariesAsync` and `ReadInitialDictionaries` are needless, so I removed them.
Related to this, `ArrowStreamWriter` does not support writing interleaved dictionaries for now.
I would like to implement it in another PR because this PR will become too large.
How is that?
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamWriter.cs
##########
@@ -723,4 +868,62 @@ public virtual void Dispose()
}
}
}
+
+ internal static class DictionaryCollector
+ {
+ internal static void Collect(RecordBatch recordBatch, ref DictionaryMemo dictionaryMemo)
+ {
+ Schema schema = recordBatch.Schema;
+ for (int i = 0; i < schema.Fields.Count; i++)
+ {
+ {
+ Field field = schema.GetFieldByIndex(i);
+ IArrowArray array = recordBatch.Column(i);
+
+ CollectDictionary(field, array, ref dictionaryMemo);
+ }
+ }
+ }
+
+ private static void CollectDictionary(Field field, IArrowArray array, ref DictionaryMemo dictionaryMemo)
+ {
+ if (field.DataType.TypeId == ArrowTypeId.Dictionary)
+ {
+ IArrowArray dictionary = (array as DictionaryArray).Dictionary;
+ dictionaryMemo ??= new DictionaryMemo();
+ long id = dictionaryMemo.GetOrAssignId(field);
+
+ dictionaryMemo.AddOrReplaceDictionary(id, dictionary);
+ WalkChildren(dictionary, ref dictionaryMemo);
+ }
+ else
+ {
+ WalkChildren(array, ref dictionaryMemo);
+ }
+ }
+
+ private static void WalkChildren(IArrowArray array, ref DictionaryMemo dictionaryMemo)
+ {
+ ArrayData[] children = array.Data.Children;
+
+ if (children == null)
+ {
+ return;
+ }
+
+ if (!(array.Data.DataType is NestedType nestedType))
+ {
+ return;
+ }
+
+ for (int i = 0; i < nestedType.Fields.Count; i++)
+ {
+ Field childField = nestedType.Fields[i];
+ ArrayData child = children[i];
+ IArrowArray childArray = ArrowArrayFactory.BuildArray(child);
+
+ CollectDictionary(childField, childArray, ref dictionaryMemo);
+ }
+ }
Review comment:
I fixed it.
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamWriter.cs
##########
@@ -248,6 +263,13 @@ private protected void WriteRecordBatchInternal(RecordBatch recordBatch)
HasWrittenSchema = true;
}
+ if (!HasWrittenDictionaryBatch)
+ {
+ DictionaryCollector.Collect(recordBatch, ref _dictionaryMemo);
Review comment:
The benchmark results are below.
before changes:
| Method | Count | Mean | Error | StdDev | Median | Gen 0 | Gen 1 | Gen 2 | Allocated |
|------------------------------------------ |-------- |-----------:|----------:|-----------:|-----------:|------:|------:|------:|------------:|
| ArrowReaderWithMemoryStream | 10000 | 2.869 ms | 0.0561 ms | 0.0982 ms | 2.841 ms | - | - | - | 10.09 KB |
| ArrowReaderWithMemoryStream_ManagedMemory | 10000 | 2.371 ms | 0.0474 ms | 0.1116 ms | 2.335 ms | - | - | - | 1444.86 KB |
| ArrowReaderWithMemory | 10000 | 2.122 ms | 0.1173 ms | 0.3365 ms | 2.160 ms | - | - | - | 7.67 KB |
| ArrowReaderWithMemoryStream | 1000000 | 220.505 ms | 4.3805 ms | 12.5684 ms | 219.305 ms | - | - | - | 8.13 KB |
| ArrowReaderWithMemoryStream_ManagedMemory | 1000000 | 211.168 ms | 4.1733 ms | 11.1395 ms | 206.210 ms | - | - | - | 142425.4 KB |
| ArrowReaderWithMemory | 1000000 | 166.486 ms | 3.9192 ms | 11.5558 ms | 165.832 ms | - | - | - | 7.59 KB |
| Method | BatchLength | ColumnSetCount | Mean | Error | StdDev | Median | Gen 0 | Gen 1 | Gen 2 | Allocated |
|----------- |------------ |--------------- |-----------:|----------:|----------:|-----------:|------:|------:|------:|----------:|
| WriteBatch | 10000 | 10 | 2.844 ms | 0.1597 ms | 0.4633 ms | 2.732 ms | - | - | - | 78.06 KB |
| WriteBatch | 10000 | 14 | 3.913 ms | 0.2181 ms | 0.6362 ms | 3.692 ms | - | - | - | 112.16 KB |
| WriteBatch | 1000000 | 10 | 239.486 ms | 4.7245 ms | 4.4193 ms | 239.822 ms | - | - | - | 79.1 KB |
| WriteBatch | 1000000 | 14 | 331.087 ms | 6.2670 ms | 7.4604 ms | 331.364 ms | - | - | - | 111.48 KB |
after changes:
| Method | Count | Mean | Error | StdDev | Median | Gen 0 | Gen 1 | Gen 2 | Allocated |
|------------------------------------------ |-------- |-----------:|----------:|-----------:|-----------:|----------:|------:|------:|-------------:|
| ArrowReaderWithMemoryStream | 10000 | 2.820 ms | 0.0560 ms | 0.1319 ms | 2.788 ms | 1000.0000 | - | - | 10.27 KB |
| ArrowReaderWithMemoryStream_ManagedMemory | 10000 | 2.376 ms | 0.0475 ms | 0.1252 ms | 2.329 ms | - | - | - | 1445.03 KB |
| ArrowReaderWithMemory | 10000 | 2.233 ms | 0.0305 ms | 0.0255 ms | 2.235 ms | - | - | - | 7.81 KB |
| ArrowReaderWithMemoryStream | 1000000 | 206.608 ms | 4.1289 ms | 10.5095 ms | 201.813 ms | - | - | - | 8.3 KB |
| ArrowReaderWithMemoryStream_ManagedMemory | 1000000 | 226.408 ms | 4.4877 ms | 11.7435 ms | 222.866 ms | - | - | - | 142425.57 KB |
| ArrowReaderWithMemory | 1000000 | 149.248 ms | 2.3519 ms | 5.1625 ms | 147.150 ms | - | - | - | 7.73 KB |
| Method | BatchLength | ColumnSetCount | Mean | Error | StdDev | Median | Gen 0 | Gen 1 | Gen 2 | Allocated |
|----------- |------------ |--------------- |-----------:|----------:|-----------:|-----------:|------:|------:|------:|----------:|
| WriteBatch | 10000 | 10 | 2.794 ms | 0.1591 ms | 0.4566 ms | 2.639 ms | - | - | - | 78.97 KB |
| WriteBatch | 10000 | 14 | 3.991 ms | 0.2347 ms | 0.6772 ms | 3.776 ms | - | - | - | 113.35 KB |
| WriteBatch | 1000000 | 10 | 245.601 ms | 8.2668 ms | 23.8516 ms | 240.295 ms | - | - | - | 77.97 KB |
| WriteBatch | 1000000 | 14 | 317.090 ms | 6.2694 ms | 13.4956 ms | 317.577 ms | - | - | - | 112.35 KB |
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] HashidaTKS commented on pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
HashidaTKS commented on pull request #10527:
URL: https://github.com/apache/arrow/pull/10527#issuecomment-872032443
@eerhardt
No problem, I'm in no hurry. Please take your time.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on a change in pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
eerhardt commented on a change in pull request #10527:
URL: https://github.com/apache/arrow/pull/10527#discussion_r666491771
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamWriter.cs
##########
@@ -723,4 +868,62 @@ public virtual void Dispose()
}
}
}
+
+ internal static class DictionaryCollector
+ {
+ internal static void Collect(RecordBatch recordBatch, ref DictionaryMemo dictionaryMemo)
+ {
+ Schema schema = recordBatch.Schema;
+ for (int i = 0; i < schema.Fields.Count; i++)
+ {
+ {
Review comment:
These braces are unnecessary.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] HashidaTKS commented on a change in pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
HashidaTKS commented on a change in pull request #10527:
URL: https://github.com/apache/arrow/pull/10527#discussion_r684866528
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamReaderImplementation.cs
##########
@@ -59,35 +59,41 @@ protected async ValueTask<RecordBatch> ReadRecordBatchAsync(CancellationToken ca
{
await ReadSchemaAsync().ConfigureAwait(false);
- int messageLength = await ReadMessageLengthAsync(throwOnFullRead: false, cancellationToken)
- .ConfigureAwait(false);
+ RecordBatch result = default;
+ Flatbuf.MessageHeader messageHeaderType = Flatbuf.MessageHeader.NONE;
- if (messageLength == 0)
+ do
{
- // reached end
- return null;
- }
-
- RecordBatch result = null;
- await ArrayPool<byte>.Shared.RentReturnAsync(messageLength, async (messageBuff) =>
- {
- int bytesRead = await BaseStream.ReadFullBufferAsync(messageBuff, cancellationToken)
+ int messageLength = await ReadMessageLengthAsync(throwOnFullRead: false, cancellationToken)
.ConfigureAwait(false);
- EnsureFullRead(messageBuff, bytesRead);
- Flatbuf.Message message = Flatbuf.Message.GetRootAsMessage(CreateByteBuffer(messageBuff));
+ if (messageLength == 0)
+ {
+ // reached end
+ return null;
+ }
- int bodyLength = checked((int)message.BodyLength);
+ await ArrayPool<byte>.Shared.RentReturnAsync(messageLength, async (messageBuff) =>
+ {
+ int bytesRead = await BaseStream.ReadFullBufferAsync(messageBuff, cancellationToken)
+ .ConfigureAwait(false);
+ EnsureFullRead(messageBuff, bytesRead);
- IMemoryOwner<byte> bodyBuffOwner = _allocator.Allocate(bodyLength);
- Memory<byte> bodyBuff = bodyBuffOwner.Memory.Slice(0, bodyLength);
- bytesRead = await BaseStream.ReadFullBufferAsync(bodyBuff, cancellationToken)
- .ConfigureAwait(false);
- EnsureFullRead(bodyBuff, bytesRead);
+ Flatbuf.Message message = Flatbuf.Message.GetRootAsMessage(CreateByteBuffer(messageBuff));
- FlatBuffers.ByteBuffer bodybb = CreateByteBuffer(bodyBuff);
- result = CreateArrowObjectFromMessage(message, bodybb, bodyBuffOwner);
- }).ConfigureAwait(false);
+ int bodyLength = checked((int)message.BodyLength);
+ messageHeaderType = message.HeaderType;
+
+ IMemoryOwner<byte> bodyBuffOwner = _allocator.Allocate(bodyLength);
+ Memory<byte> bodyBuff = bodyBuffOwner.Memory.Slice(0, bodyLength);
+ bytesRead = await BaseStream.ReadFullBufferAsync(bodyBuff, cancellationToken)
+ .ConfigureAwait(false);
+ EnsureFullRead(bodyBuff, bytesRead);
+
+ FlatBuffers.ByteBuffer bodybb = CreateByteBuffer(bodyBuff);
+ result = CreateArrowObjectFromMessage(message, bodybb, bodyBuffOwner);
+ }).ConfigureAwait(false);
+ } while (messageHeaderType == Flatbuf.MessageHeader.DictionaryBatch);
Review comment:
I fixed it.
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamWriter.cs
##########
@@ -128,6 +129,15 @@ public void Visit(StructArray array)
}
}
+ public void Visit(DictionaryArray array)
+ {
+ //Dictionary is serialized separately in Dictionary serialization.
+ //We are only interested in indexes at this context.
Review comment:
I fixed it.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] HashidaTKS commented on a change in pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
HashidaTKS commented on a change in pull request #10527:
URL: https://github.com/apache/arrow/pull/10527#discussion_r663390641
##########
File path: csharp/src/Apache.Arrow/Arrays/DictionaryArray.cs
##########
@@ -0,0 +1,67 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+using System;
+using System.IO;
+using Apache.Arrow.Types;
+
+namespace Apache.Arrow
+{
+ public class DictionaryArray : Array
+ {
+ public IArrowArray Dictionary { get; }
+ public IArrowArray Indices { get; }
+ public ArrowBuffer IndicesBuffer => Data.Buffers[1];
+
+ public DictionaryArray(IArrowType dataType, int length,
+ ArrowBuffer valueOffsetsBuffer, IArrowArray value,
+ ArrowBuffer nullBitmapBuffer, int nullCount = 0, int offset = 0)
+ : this(new ArrayData(dataType, length, nullCount, offset,
+ new[] { nullBitmapBuffer, valueOffsetsBuffer }, new[] { value.Data }, value.Data.Dictionary))
+ {
+ }
+
+ public DictionaryArray(ArrayData data) : base(data)
+ {
+ data.EnsureBufferCount(2);
+ data.EnsureDataType(ArrowTypeId.Dictionary);
+
+ var dicType = data.DataType as DictionaryType;
+ data.Dictionary.EnsureDataType(dicType.ValueType.TypeId);
Review comment:
Would you please check it again?
I added the null check with the following commit.
https://github.com/apache/arrow/pull/10527/commits/8544064b944745aa757efe23e3c3b3aaac65e89a
https://github.com/apache/arrow/pull/10527/files#diff-f31b887c0ac098b46742c1984588210d7f217a9f546e55255570a1d15d5cc3b3R33
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] HashidaTKS commented on pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
HashidaTKS commented on pull request #10527:
URL: https://github.com/apache/arrow/pull/10527#issuecomment-893067009
Cc: @eerhardt
Would you please re-review this when you have time?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt closed pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
eerhardt closed pull request #10527:
URL: https://github.com/apache/arrow/pull/10527
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] HashidaTKS commented on a change in pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
HashidaTKS commented on a change in pull request #10527:
URL: https://github.com/apache/arrow/pull/10527#discussion_r654753770
##########
File path: csharp/src/Apache.Arrow/Arrays/ArrayData.cs
##########
@@ -30,10 +30,25 @@ public sealed class ArrayData : IDisposable
public readonly int Offset;
public readonly ArrowBuffer[] Buffers;
public readonly ArrayData[] Children;
+ public readonly ArrayData Dictionary; //Only used for dictionary type
public ArrayData(
IArrowType dataType,
int length, int nullCount = 0, int offset = 0,
+ IEnumerable<ArrowBuffer> buffers = null, IEnumerable<ArrayData> children = null) :
+ this(dataType, null, length, nullCount, offset, buffers, children)
+ { }
+
+ public ArrayData(
+ IArrowType dataType,
+ int length, int nullCount = 0, int offset = 0,
+ ArrowBuffer[] buffers = null, ArrayData[] children = null) :
+ this(dataType, null, length, nullCount, offset, buffers, children)
+ { }
+
+ public ArrayData(
+ IArrowType dataType, ArrayData dictionary,
Review comment:
I noticed that changing original ctor's optional parameters to normal parameters can avoid the ambiguity and keep backward binary compatibility.
Please excuse my ignorance...
```
public ArrayData(
IArrowType dataType,
int length, int nullCount, int offset,
IEnumerable<ArrowBuffer> buffers, IEnumerable<ArrayData> children) :
this(dataType, length, nullCount, offset, buffers, children, null)
{ }
public ArrayData(
IArrowType dataType,
int length, int nullCount = 0, int offset = 0,
IEnumerable<ArrowBuffer> buffers = null, IEnumerable<ArrayData> children = null, ArrayData dictionary = null)
{
...
}
```
--
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:
users@infra.apache.org
[GitHub] [arrow] HashidaTKS edited a comment on pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
HashidaTKS edited a comment on pull request #10527:
URL: https://github.com/apache/arrow/pull/10527#issuecomment-864352773
Added some minor changes.
--
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:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on a change in pull request #10527: ARROW-6870: [C#] Add Support for Dictionary Arrays and Dictionary Encoding
Posted by GitBox <gi...@apache.org>.
eerhardt commented on a change in pull request #10527:
URL: https://github.com/apache/arrow/pull/10527#discussion_r684688085
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamReaderImplementation.cs
##########
@@ -59,35 +59,41 @@ protected async ValueTask<RecordBatch> ReadRecordBatchAsync(CancellationToken ca
{
await ReadSchemaAsync().ConfigureAwait(false);
- int messageLength = await ReadMessageLengthAsync(throwOnFullRead: false, cancellationToken)
- .ConfigureAwait(false);
+ RecordBatch result = default;
+ Flatbuf.MessageHeader messageHeaderType = Flatbuf.MessageHeader.NONE;
- if (messageLength == 0)
+ do
{
- // reached end
- return null;
- }
-
- RecordBatch result = null;
- await ArrayPool<byte>.Shared.RentReturnAsync(messageLength, async (messageBuff) =>
- {
- int bytesRead = await BaseStream.ReadFullBufferAsync(messageBuff, cancellationToken)
+ int messageLength = await ReadMessageLengthAsync(throwOnFullRead: false, cancellationToken)
.ConfigureAwait(false);
- EnsureFullRead(messageBuff, bytesRead);
- Flatbuf.Message message = Flatbuf.Message.GetRootAsMessage(CreateByteBuffer(messageBuff));
+ if (messageLength == 0)
+ {
+ // reached end
+ return null;
+ }
- int bodyLength = checked((int)message.BodyLength);
+ await ArrayPool<byte>.Shared.RentReturnAsync(messageLength, async (messageBuff) =>
+ {
+ int bytesRead = await BaseStream.ReadFullBufferAsync(messageBuff, cancellationToken)
+ .ConfigureAwait(false);
+ EnsureFullRead(messageBuff, bytesRead);
- IMemoryOwner<byte> bodyBuffOwner = _allocator.Allocate(bodyLength);
- Memory<byte> bodyBuff = bodyBuffOwner.Memory.Slice(0, bodyLength);
- bytesRead = await BaseStream.ReadFullBufferAsync(bodyBuff, cancellationToken)
- .ConfigureAwait(false);
- EnsureFullRead(bodyBuff, bytesRead);
+ Flatbuf.Message message = Flatbuf.Message.GetRootAsMessage(CreateByteBuffer(messageBuff));
- FlatBuffers.ByteBuffer bodybb = CreateByteBuffer(bodyBuff);
- result = CreateArrowObjectFromMessage(message, bodybb, bodyBuffOwner);
- }).ConfigureAwait(false);
+ int bodyLength = checked((int)message.BodyLength);
+ messageHeaderType = message.HeaderType;
+
+ IMemoryOwner<byte> bodyBuffOwner = _allocator.Allocate(bodyLength);
+ Memory<byte> bodyBuff = bodyBuffOwner.Memory.Slice(0, bodyLength);
+ bytesRead = await BaseStream.ReadFullBufferAsync(bodyBuff, cancellationToken)
+ .ConfigureAwait(false);
+ EnsureFullRead(bodyBuff, bytesRead);
+
+ FlatBuffers.ByteBuffer bodybb = CreateByteBuffer(bodyBuff);
+ result = CreateArrowObjectFromMessage(message, bodybb, bodyBuffOwner);
+ }).ConfigureAwait(false);
+ } while (messageHeaderType == Flatbuf.MessageHeader.DictionaryBatch);
Review comment:
Would this be better as:
```suggestion
} while (messageHeaderType != Flatbuf.MessageHeader.RecordBatch);
```
?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org