[GitHub] [arrow] zgramana edited a comment on pull request #6121: ARROW-6603: [C#] - Nullable Array Support

[GitBox <gi...@apache.org>]

zgramana edited a comment on pull request #6121:
URL: https://github.com/apache/arrow/pull/6121#issuecomment-618756547


   @eerhardt I'd like to chime in to say that I have just come across this conversation late--and *after* implementing an alternative approach which much more in-line with other Arrow implementations.
   
   In particular, I really just finished out the previously stubbed `NullBitmapBuffer` already in the codebase.
   
   The most significant change comes with the following addition to `IArrowArrayBuilder<T, out TArray, out TBuilder>`:
   
   ```csharp
   public interface IArrowArrayBuilder<T, out TArray, out TBuilder> : IArrowArrayBuilder<TArray, TBuilder>
       where TArray : IArrowArray
       where TBuilder : IArrowArrayBuilder<TArray>
   {
       TBuilder Append(T value);
       TBuilder Append(ReadOnlySpan<T> span);
       TBuilder AppendRange(IEnumerable<T> values);
       TBuilder AppendNull(); // <- Works with non-nullable types too
       TBuilder Swap(int i, int j);
       TBuilder Set(int index, T value);
   }
   ```
   
   The work includes adding the supporting work for implementation of `AppendNull` to `PrimitiveArrayBuilder` and `Binary.BuilderBase` and then removing the hardcoded `0`'s passed to `nullCount` in `ArrayData` constructors.
   
   I also added two tests to `ArrayBuilderTests`:
   * The first includes a number of `null` scenarios using `TestArrayBuilder<T,U>(...)`.
   * The second includes a `StringArray.Builder` scenario with mixed `null` and `string.Empty` scenarios. For the later, I implemented it such that `string.Empty` is considered a valid value (so it increments `Offset` without adding an bytes to `ValueBuffer`. When a null string is passed to  `Append` it will invoke `AppendNull` internally instead.
   * All 160 (existing + new) tests pass. :-)
   
   I have requested internal approval to be able to submit a PR and get a CCLA signed. If you can hold off on merging this PR, I would like the opportunity to have my alternate approach considered as well.


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