GH-50338: [C++] Add ComputeLogicalNullCount to Datum - #50347
Conversation
|
|
2e82f92 to
30f92df
Compare
github-actions
Bot
added
awaiting committer review
f6dbdc7 to
ecb27c9
Compare
|
@pitrou - the s3fs test times out. Seems unrelated to this change and an CI infra red herring! |
|
@pitrou - This is ready to be reviewed. Thanks. |
pitrou
left a comment
pitrou
left a comment
There was a problem hiding this comment.
Thanks for the update! Here a couple more comments.
| return false; | ||
| } | ||
| auto index_value = DictionaryIndexValue(*value.index); | ||
| if (!index_value.ok() || *index_value < 0 || *index_value >= dict->length()) { |
There was a problem hiding this comment.
Same here: not sure it's worth checking for these invalid cases, and it may actually be better to let them crash rather than return a seemingly valid value.
Datum::null_count() does not account for types that carry logical nulls without a validity bitmap (union, dictionary and run-end encoded types). Add Datum::ComputeLogicalNullCount(), delegating to ArrayData::ComputeLogicalNullCount() and ChunkedArray::ComputeLogicalNullCount() for array-like data; for scalars, is_valid already reflects logical validity.
ecb27c9 to
ec4e1fa
Compare
|
Thank you @goel-skd ! |
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit e1dfbef. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 9 possible false positives for unstable benchmarks that are known to sometimes produce them. |
2 participants
Rationale for this change
Datum::null_count()only counts physical nulls, so it gives the wrong answer for union, run-end encoded and dictionary data.Array,ArrayData,ArraySpanandChunkedArrayalready haveComputeLogicalNullCount()for this, butDatumdoesn't.What changes are included in this PR?
Datum::ComputeLogicalNullCount(), which delegates to the existingArrayData/ChunkedArrayimplementations for array-like data.Scalar::IsLogicalNull(), which is!is_validfor most types.DictionaryScalaroverrides it because a valid index can still refer to a null dictionary value. Ill-formed scalars (e.g. an out-of-bounds index) are not handled defensively — the result is undefined for them, and validation rejects them. Like the array path, it doesn't recurse into nested values.Are these changes tested?
Yes, in datum_test.cc and scalar_test.cc: union (sparse and dense), run-end encoded, dictionary and chunked inputs, scalars obtained from
Array::GetScalar(), and a check that the scalar and array paths agree element by element.Are there any user-facing changes?
The two new APIs above. The new virtual on
Scalarchanges the C++ ABI, so this shouldn't be backported to a patch release.