westonpace commented on code in PR #34408:
URL: https://github.com/apache/arrow/pull/34408#discussion_r1125123670
##########
cpp/src/arrow/array/data.h:
##########
@@ -363,14 +475,73 @@ struct ARROW_EXPORT ArraySpan {
}
}
- /// \brief Return null count, or compute and set it if it's not known
+ /// \brief Return physical null count, or compute and set it if it's not
known
int64_t GetNullCount() const;
+ /// \brief Return true if the array has a validity bitmap and the physical
null
+ /// count is known to be non-zero or not yet known
+ ///
+ /// Note that this is not the same as MayHaveLogicalNulls, which also checks
+ /// for the presence of nulls in child data for types like unions and run-end
+ /// encoded types.
+ ///
+ /// \see HasValidityBitmap
+ /// \see MayHaveLogicalNulls
bool MayHaveNulls() const {
// If an ArrayData is slightly malformed it may have kUnknownNullCount set
// but no buffer
return null_count != 0 && buffers[0].data != NULLPTR;
}
+
+ /// \brief Return true if the array has a validity bitmap
+ bool HasValidityBitmap() const { return buffers[0].data != NULLPTR; }
+
+ /// \brief Return true if the validity bitmap may have 0's in it, or if the
+ /// child arrays (in the case of types without a validity bitmap) may have
+ /// nulls
+ ///
+ /// \see ArrayData::MayHaveLogicalNulls
+ bool MayHaveLogicalNulls() const {
+ if (buffers[0].data != NULLPTR) {
+ return null_count != 0;
+ }
+ const auto t = type->id();
+ if (t == Type::SPARSE_UNION || t == Type::DENSE_UNION) {
+ return UnionMayHaveLogicalNulls();
+ }
+ if (t == Type::RUN_END_ENCODED) {
+ return RunEndEncodedMayHaveLogicalNulls();
+ }
+ return false;
Review Comment:
Do we need to look at `null_count` here? If there is no validity bitmap
(and the type would normally have one) then I think we either are "all null" or
"not all null" depending on `null_count == this->length` right?
##########
cpp/src/arrow/array/array_test.cc:
##########
@@ -482,35 +524,45 @@ void AssertAppendScalar(MemoryPool* pool, const
std::shared_ptr<Scalar>& scalar)
std::unique_ptr<arrow::ArrayBuilder> builder;
auto null_scalar = MakeNullScalar(scalar->type);
ASSERT_OK(MakeBuilderExactIndex(pool, scalar->type, &builder));
- ASSERT_OK(builder->AppendScalar(*scalar));
- ASSERT_OK(builder->AppendScalar(*scalar));
- ASSERT_OK(builder->AppendScalar(*null_scalar));
- ASSERT_OK(builder->AppendScalars({scalar, null_scalar}));
- ASSERT_OK(builder->AppendScalar(*scalar, /*n_repeats=*/2));
- ASSERT_OK(builder->AppendScalar(*null_scalar, /*n_repeats=*/2));
+ ASSERT_OK(builder->AppendScalar(*scalar)); // [0] = scalar
+ ASSERT_OK(builder->AppendScalar(*scalar)); // [1] = scalar
+ ASSERT_OK(builder->AppendScalar(*null_scalar)); // [2] = NULL
+ ASSERT_OK(builder->AppendScalars({scalar, null_scalar})); // [3, 4] =
{scalar, NULL}
+ ASSERT_OK(
+ builder->AppendScalar(*scalar, /*n_repeats=*/2)); // [5, 6] = {scalar,
scalar}
+ ASSERT_OK(
+ builder->AppendScalar(*null_scalar, /*n_repeats=*/2)); // [7, 8] =
{NULL, NULL}
std::shared_ptr<Array> out;
FinishAndCheckPadding(builder.get(), &out);
ASSERT_OK(out->ValidateFull());
AssertTypeEqual(scalar->type, out->type());
ASSERT_EQ(out->length(), 9);
- const bool can_check_nulls = internal::HasValidityBitmap(out->type()->id());
+ auto out_type_id = out->type()->id();
+ const bool has_validity = internal::HasValidityBitmap(out_type_id);
// For a dictionary builder, the output dictionary won't necessarily be the
same
- const bool can_check_values = !is_dictionary(out->type()->id());
+ const bool can_check_values = !is_dictionary(out_type_id);
- if (can_check_nulls) {
+ if (has_validity) {
ASSERT_EQ(out->null_count(), 4);
+ } else {
+ ASSERT_EQ(out->null_count(), 0);
+ }
+ if (scalar->is_valid) {
+ ASSERT_EQ(out->ComputeLogicalNullCount(), 4);
+ } else {
+ ASSERT_EQ(out->ComputeLogicalNullCount(), 9);
}
for (const auto index : {0, 1, 3, 5, 6}) {
- ASSERT_FALSE(out->IsNull(index));
+ ASSERT_FALSE(out->IsNull(index) && scalar->is_valid);
Review Comment:
```suggestion
ASSERT_NE(out->IsNull(index), scalar->is_valid);
```
Minor readability nit
##########
cpp/src/arrow/util/union_util.cc:
##########
@@ -0,0 +1,58 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "arrow/util/union_util.h"
+
+#include <cstdint>
+
+#include "arrow/array/data.h"
+#include "arrow/util/checked_cast.h"
+#include "arrow/util/logging.h"
+
+namespace arrow::union_util {
+
+int64_t LogicalSparseUnionNullCount(const ArraySpan& span) {
+ const auto* sparse_union_type =
+ internal::checked_cast<const SparseUnionType*>(span.type);
+ DCHECK_LE(span.child_data.size(), 128);
+
+ const int8_t* types = span.GetValues<int8_t>(1); // NOLINT
Review Comment:
What lint is being disabled here?
--
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: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
