pitrou commented on code in PR #50347:
URL: https://github.com/apache/arrow/pull/50347#discussion_r3534609162
##########
cpp/src/arrow/datum.cc:
##########
@@ -142,7 +142,26 @@ int64_t Datum::null_count() const {
const auto& val = *std::get<std::shared_ptr<Scalar>>(this->value);
return val.is_valid ? 0 : 1;
} else {
- DCHECK(false) << "This function only valid for array-like values";
+ DCHECK(false) << "This function only valid for scalar or array-like
values";
+ return 0;
+ }
+}
+
+int64_t Datum::ComputeLogicalNullCount() const {
+ if (this->kind() == Datum::ARRAY) {
+ return
std::get<std::shared_ptr<ArrayData>>(this->value)->ComputeLogicalNullCount();
+ } else if (this->kind() == Datum::CHUNKED_ARRAY) {
+ return std::get<std::shared_ptr<ChunkedArray>>(this->value)
+ ->ComputeLogicalNullCount();
+ } else if (this->kind() == Datum::SCALAR) {
+ // Union and run-end encoded scalars derive is_valid from their underlying
+ // value, so it reflects logical validity. A DictionaryScalar's is_valid
+ // only tracks index validity, so this differs from the array path when a
+ // valid index points to a null dictionary value.
Review Comment:
The dictionary scalar-vs-array discrepancy is going to be confusing and
error-prone.
How about we add a method `bool Scalar::IsLogicalNull` and use it here?
##########
cpp/src/arrow/datum_test.cc:
##########
@@ -139,6 +139,54 @@ TEST(Datum, NullCount) {
ASSERT_EQ(3, val3.null_count());
}
+TEST(Datum, ComputeLogicalNullCount) {
+ // For scalars, is_valid already reflects logical validity.
+ Datum val1(std::make_shared<Int8Scalar>(1));
+ ASSERT_EQ(0, val1.ComputeLogicalNullCount());
+
+ Datum val2(MakeNullScalar(int8()));
+ ASSERT_EQ(1, val2.ComputeLogicalNullCount());
+
+ // For arrays with a validity bitmap, the logical null count matches
+ // null_count().
+ Datum val3(ArrayFromJSON(int8(), "[1, null, null, null]"));
+ ASSERT_EQ(3, val3.null_count());
+ ASSERT_EQ(3, val3.ComputeLogicalNullCount());
+
+ // Union arrays carry logical nulls in their children without a top-level
+ // validity bitmap, so null_count() is 0 while the logical null count is not.
+ auto union_type = sparse_union({field("a", int8()), field("b", boolean())});
+ auto union_arr =
+ ArrayFromJSON(union_type, R"([[0, null], [1, true], [0, 5], [1,
null]])");
+ Datum val4(union_arr);
+ ASSERT_EQ(0, val4.null_count());
+ ASSERT_EQ(2, val4.ComputeLogicalNullCount());
Review Comment:
Can easily add a test for scalars here, for example:
```c++
auto scalar_logical_null_count = [](const Array& arr, int64_t index) ->
Result<int64> {
ARROW_ASSIGN_OR_RAISE(auto scalar, union_arr.GetScalar(index));
return Datum(scalar).ComputeLogicalNullCount();
};
ASSERT_OK_AND_EQ(scalar_logical_null_count(*union_arr, 0), 1);
ASSERT_OK_AND_EQ(scalar_logical_null_count(*union_arr, 1), 0);
```
##########
cpp/src/arrow/datum_test.cc:
##########
@@ -139,6 +139,54 @@ TEST(Datum, NullCount) {
ASSERT_EQ(3, val3.null_count());
}
+TEST(Datum, ComputeLogicalNullCount) {
+ // For scalars, is_valid already reflects logical validity.
+ Datum val1(std::make_shared<Int8Scalar>(1));
+ ASSERT_EQ(0, val1.ComputeLogicalNullCount());
+
+ Datum val2(MakeNullScalar(int8()));
+ ASSERT_EQ(1, val2.ComputeLogicalNullCount());
+
Review Comment:
Perhaps also test an array and a scalar of type null()?
##########
cpp/src/arrow/datum.h:
##########
@@ -276,6 +276,20 @@ struct ARROW_EXPORT Datum {
/// Only valid for scalar and array-like data.
int64_t null_count() const;
+ /// \brief Compute the logical null count.
+ ///
+ /// Only valid for scalar and array-like data. Unlike null_count(), this
+ /// accounts for types whose logical nulls are not captured by the top-level
+ /// validity bitmap, such as union, run-end encoded and dictionary types; for
+ /// those types the count is recomputed on every call. For scalars this
+ /// returns the same value as null_count(); note that a DictionaryScalar
+ /// counts as non-null whenever its index is valid, even if the index points
+ /// to a null dictionary value.
+ ///
+ /// \see ArrayData::ComputeLogicalNullCount
+ /// \see ChunkedArray::ComputeLogicalNullCount
+ int64_t ComputeLogicalNullCount() const;
Review Comment:
How about we add a similar method to the `Scalar` class?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
