StringViewArray: enable nullable reference types and optimize encoding with stackalloc/ArrayPool - #400
Conversation
| public class StringViewArray(ArrayData data) : BinaryViewArray(ArrowTypeId.StringView, data), IReadOnlyList<string?> | ||
| { | ||
| public static readonly Encoding DefaultEncoding = Encoding.UTF8; | ||
| public static Encoding DefaultEncoding { get; } = new UTF8Encoding(false); |
There was a problem hiding this comment.
I'm a little concerned that this could be a breaking behaviour change, but I checked that decoding is forgiving of whether or not a BOM is present, regardless of the encoderShouldEmitUTF8Identifier parameter. So I think this is OK.
I notice that we disable testing the StringView type in our C Data Interface Python integration tests:
arrow-dotnet/test/Apache.Arrow.Tests/CDataInterfacePythonTests.cs
Lines 772 to 773 in dc2c566
Did you find that emitting the BOM was causing a problem when integrating with another Arrow implementation?
There was a problem hiding this comment.
@adamreeve Yes, BOM causes problems with DataFusion.
…arger ones - Ensure ArrayPool buffers are returned in finally blocks - Fix variable naming and remove duplicate code
|
In general, I think the idea of temporary buffers is redundant. You can write directly to the ArrowBuffer, which will eliminate unnecessary memory copying and temporary buffer allocation. |
| #if NETCOREAPP | ||
| byte[]? buffer = null; | ||
|
|
||
| Span<byte> span = maxByteCount <= 1024 |
There was a problem hiding this comment.
I've been using 256 as a conservative upper bound for stackalloc in libraries; see VariantValueWriter.StackAllocThreshold.
| namespace Apache.Arrow | ||
| { | ||
| public class StringViewArray : BinaryViewArray, IReadOnlyList<string> | ||
| public class StringViewArray(ArrayData data) : BinaryViewArray(ArrowTypeId.StringView, data), IReadOnlyList<string?> |
There was a problem hiding this comment.
I think a codebase is easier to read when it's consistent in its use of syntax and personally found this new form jarring. I'm curious what other people think.
There was a problem hiding this comment.
I also found it a bit jarring but I haven't been doing as much work in C# lately so maybe I'm just not used to this. I noticed Rider always suggests refactoring to this form.
|
|
||
| encoding ??= DefaultEncoding; | ||
| int maxByteCount = encoding.GetMaxByteCount(value.Length); | ||
| #if NETCOREAPP |
There was a problem hiding this comment.
Please put preprocessor directives in flush-left like the rest of the codebase.
|
In general and for a variety of reasons I think the existing builders kind of suck. I've often been skipping them and building buffers directly for better performance. For your own use cases, do you find that you typically have a known set of .NET |
3 participants
StringViewArray: Enable nullable reference types and improve encoding performance
to avoid per‑string byte array allocation and reduce GC pressure