GH-41136: [C#] Recompute null count for sliced arrays on demand #41144

adamreeve · 2024-04-11T03:47:10Z

Rationale for this change

See #41136. This is required for writing sliced arrays to IPC files and streams. It should also be generally useful too by allowing users to be able to rely on valid null count values for sliced arrays.

What changes are included in this PR?

Adds a new ArrayData.GetNullCount() method which computes the actual null count if it is unknown, and changes most uses of ArrayData.NullCount to GetNullCount().

Are these changes tested?

Yes, I've extended the existing slicing tests to also verify the null count, and added new tests specifically for union arrays, which needed special handling.

Are there any user-facing changes?

Yes, this is a user-facing behaviour change. I don't expect that it should break any existing code as users are unlikely to rely on the NullCount being -1.

GitHub Issue: [C#] NullCount should be recomputed on-demand for sliced arrays #41136

adamreeve · 2024-04-11T03:49:32Z

csharp/src/Apache.Arrow/Arrays/ArrayData.cs

 public readonly int Offset;
 public readonly ArrowBuffer[] Buffers;
 public readonly ArrayData[] Children;
 public readonly ArrayData Dictionary; // Only used for dictionary type

+ public int NullCount


Is this ABI breakage acceptable? I expect most users would access the null count from the Array rather than ArrayData so it might not affect too many users. But as an alternative I could add a new property and mark this one as obsolete, or maybe just always compute the null count in the constructor rather than doing it on demand?

Yeah, this is a tough one. The problem with ABI breakage is that it makes diamond-shaped dependency graphs unfixable without modifying some other dependency. Computing eagerly is a potential performance regression. Marking the original field as obsolete and defining a new property feels like it's probably the best option. I suspect most uses will go through IArrowArray.NullCount anyway, so the impact on existing source is likely to be small. The only painful part is probably coming up with a new name... .

Yeah OK, maybe a GetNullCount method instead would make sense, partly because I can't think of a better property name, but then it's also obvious it might do more work than a plain member access.

I guess we also don't necessarily need to deprecate the existing member because there might be cases where users only want the null count if it can be accessed quickly and don't want to pay the cost of computing it, so we could just document that it may return a negative number. And we could remove the readonly modifier to allow updating it if GetNullCount is called, which isn't a breaking change according to the rules.

I've reverted NullCount back to a normal member but made it non-readonly and added a GetNullCount method instead. I've changed most uses of NullCount to GetNullCount(), except in the C Data interface implementation which explicitly allows -1 values: https://arrow.apache.org/docs/format/CDataInterface.html#c.ArrowArray.null_count

Let me know if you'd prefer to deprecate NullCount instead though, or have any other suggestions for how to handle this.

adamreeve · 2024-04-11T03:52:30Z

csharp/src/Apache.Arrow/Arrays/ArrayData.cs

+ {
+ if (_nullCount == RecalculateNullCount)
+ {
+ _nullCount = ComputeNullCount();


This might result in duplicate work if multiple threads try to access the null count concurrently, but I don't imagine this is a big concern, and the C++ implementation has the same behaviour.

adamreeve · 2024-04-11T03:56:23Z

csharp/src/Apache.Arrow/Arrays/ArrayData.cs

+ // Note: Dictionary arrays may be logically null if there is a null in the dictionary values,
+ // but this isn't accounted for by the IArrowArray.IsNull implementation,
+ // so we maintain consistency with that behaviour here.


The C++ implementation actually has a separate ComputeLogicalNullCount method on ArrayData and Array that handles union arrays, dictionary arrays and run end encoded arrays, and there the null_count for union arrays always returns 0. The C# UnionArray.IsValid method depends on the NullCount value though, so I figured it made sense to compute the actual null count for union arrays.

CurtHagenlocher · 2024-04-11T13:22:01Z

csharp/src/Apache.Arrow/Arrays/ArrayData.cs

@@ -111,7 +123,25 @@ public ArrayData Slice(int offset, int length)
 length = Math.Min(Length - offset, length);
 offset += Offset;

- return new ArrayData(DataType, length, RecalculateNullCount, offset, Buffers, Children, Dictionary);
+ int nullCount;
+ if (NullCount == 0)


This would force calculation of the null count on the original array. Is it worth preserving laziness and saying "if we haven't calculated the original null count then mark the sliced null count as requiring calculation"?

Good catch, yes definitely

This was fixed as a side effect of reverting NullCount back to being a normal int member.

CurtHagenlocher · 2024-04-11T13:24:33Z

csharp/src/Apache.Arrow/Arrays/ArrayData.cs

+ return Length - BitUtility.CountBits(Buffers[0].Span, Offset, Length);
+ }
+
+ private int ComputeSparseUnionNullCount()


Is it worth moving these as statics onto SparseUnionArray and DenseUnionArray respectively so that the type-specific logic remains in one place?

CurtHagenlocher

Thanks, this is great! I think we should preserve binary compatibility by adding a new property.

adamreeve · 2024-04-11T21:54:37Z

csharp/src/Apache.Arrow/Arrays/ArrayDataConcatenator.cs

@@ -71,7 +71,7 @@ public ArrayDataConcatenationVisitor(IReadOnlyList<ArrayData> arrayDataList, Mem
 foreach (ArrayData arrayData in _arrayDataList)
 {
 _totalLength += arrayData.Length;
- _totalNullCount += arrayData.NullCount;
+ _totalNullCount += arrayData.GetNullCount();


It could make sense to check NullCount for -1 here, and then set the final null count to -1 if it was -1 in any array, rather than forcing a computation. But this class has bigger problems as it doesn't account for non-zero offsets anywhere as far as I can see, although it does seem to account for array lengths being less than the full buffer size. Maybe it should throw a NotImplementedException if it encounters a non-zero offset?

Can you file a separate bug for this?

Yep sure, I've made #41164

conbench-apache-arrow · 2024-04-12T08:54:40Z

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit f20c1e8.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them.

…apache#41144) ### Rationale for this change See apache#41136. This is required for writing sliced arrays to IPC files and streams. It should also be generally useful too by allowing users to be able to rely on valid null count values for sliced arrays. ### What changes are included in this PR? Adds a new `ArrayData.GetNullCount()` method which computes the actual null count if it is unknown, and changes most uses of `ArrayData.NullCount` to `GetNullCount()`. ### Are these changes tested? Yes, I've extended the existing slicing tests to also verify the null count, and added new tests specifically for union arrays, which needed special handling. ### Are there any user-facing changes? Yes, this is a user-facing behaviour change. I don't expect that it should break any existing code as users are unlikely to rely on the NullCount being -1. * GitHub Issue: apache#41136 Authored-by: Adam Reeve <adreeve@gmail.com> Signed-off-by: Curt Hagenlocher <curt@hagenlocher.org>

### Rationale for this change See #41136. This is required for writing sliced arrays to IPC files and streams. It should also be generally useful too by allowing users to be able to rely on valid null count values for sliced arrays. ### What changes are included in this PR? Adds a new `ArrayData.GetNullCount()` method which computes the actual null count if it is unknown, and changes most uses of `ArrayData.NullCount` to `GetNullCount()`. ### Are these changes tested? Yes, I've extended the existing slicing tests to also verify the null count, and added new tests specifically for union arrays, which needed special handling. ### Are there any user-facing changes? Yes, this is a user-facing behaviour change. I don't expect that it should break any existing code as users are unlikely to rely on the NullCount being -1. * GitHub Issue: #41136 Authored-by: Adam Reeve <adreeve@gmail.com> Signed-off-by: Curt Hagenlocher <curt@hagenlocher.org>

…apache#41144) ### Rationale for this change See apache#41136. This is required for writing sliced arrays to IPC files and streams. It should also be generally useful too by allowing users to be able to rely on valid null count values for sliced arrays. ### What changes are included in this PR? Adds a new `ArrayData.GetNullCount()` method which computes the actual null count if it is unknown, and changes most uses of `ArrayData.NullCount` to `GetNullCount()`. ### Are these changes tested? Yes, I've extended the existing slicing tests to also verify the null count, and added new tests specifically for union arrays, which needed special handling. ### Are there any user-facing changes? Yes, this is a user-facing behaviour change. I don't expect that it should break any existing code as users are unlikely to rely on the NullCount being -1. * GitHub Issue: apache#41136 Authored-by: Adam Reeve <adreeve@gmail.com> Signed-off-by: Curt Hagenlocher <curt@hagenlocher.org>

adamreeve added 3 commits April 11, 2024 14:40

Handle when slice null count can be computed trivially

55ce751

Implement null count recomputing

9e7bb01

Implement null count calculation for union arrays

3781080

adamreeve requested a review from CurtHagenlocher as a code owner April 11, 2024 03:47

github-actions bot added Component: C# awaiting review Awaiting review labels Apr 11, 2024

adamreeve commented Apr 11, 2024

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Apr 11, 2024

adamreeve commented Apr 11, 2024

View reviewed changes

CurtHagenlocher reviewed Apr 11, 2024

View reviewed changes

CurtHagenlocher requested changes Apr 11, 2024

View reviewed changes

adamreeve added 3 commits April 12, 2024 08:55

Move type specific logic to static methods on the array classes

e18162a

Use new GetNullCount method instead of changing NullCount to a property

3c9d8b8

Add new qualifier to ComputeNullCount methods

8d3e21f

adamreeve commented Apr 11, 2024

View reviewed changes

CurtHagenlocher approved these changes Apr 12, 2024

View reviewed changes

CurtHagenlocher merged commit f20c1e8 into apache:main Apr 12, 2024
12 checks passed

CurtHagenlocher removed the awaiting committer review Awaiting committer review label Apr 12, 2024

CurtHagenlocher mentioned this pull request Apr 12, 2024

[C#] NullCount should be recomputed on-demand for sliced arrays #41136

Closed

github-actions bot added the awaiting committer review Awaiting committer review label Apr 12, 2024

adamreeve deleted the recompute_null_count branch April 12, 2024 03:01

adamreeve mentioned this pull request Jun 6, 2024

[C#] ArrayData.NullCount should be a property #22071

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-41136: [C#] Recompute null count for sliced arrays on demand #41144

GH-41136: [C#] Recompute null count for sliced arrays on demand #41144

adamreeve commented Apr 11, 2024 •

edited

Loading

adamreeve Apr 11, 2024

CurtHagenlocher Apr 11, 2024

adamreeve Apr 11, 2024

adamreeve Apr 11, 2024

adamreeve Apr 11, 2024

adamreeve Apr 11, 2024

CurtHagenlocher Apr 11, 2024

adamreeve Apr 11, 2024

adamreeve Apr 11, 2024

CurtHagenlocher Apr 11, 2024

CurtHagenlocher left a comment

adamreeve Apr 11, 2024

CurtHagenlocher Apr 12, 2024

adamreeve Apr 12, 2024

conbench-apache-arrow bot commented Apr 12, 2024

GH-41136: [C#] Recompute null count for sliced arrays on demand #41144

GH-41136: [C#] Recompute null count for sliced arrays on demand #41144

Conversation

adamreeve commented Apr 11, 2024 • edited Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CurtHagenlocher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conbench-apache-arrow bot commented Apr 12, 2024

adamreeve commented Apr 11, 2024 •

edited

Loading