review comments

Author: bkietz

cpp/src/arrow/array/array_test.cc

@@ -508,7 +508,12 @@ TEST_F(TestArray, TestMakeArrayOfNull) {
     auto req = [](auto type) { return field("", std::move(type), /*nullable=*/false); };
 
     // union with no nullable fields cannot represent a null
-    ASSERT_RAISES(Invalid, MakeArrayOfNull(dense_union({req(int8())}), length));
+    ASSERT_RAISES(TypeError, MakeArrayOfNull(dense_union({req(int8())}), length));
+    // run end encoded with non nullable child cannot represent a null
+    // (not directly constructible, but not invalid per Columnar.rst)
+    auto ree = run_end_encoded(int16(), utf8());
+    const_cast<FieldVector&>(ree->fields())[1] = req(utf8());
+    ASSERT_RAISES(TypeError, MakeArrayOfNull(ree, length));
 
     // struct with no nullable fields has a top level bitmap and can mask them
     ASSERT_OK_AND_ASSIGN(auto s, MakeArrayOfNull(struct_({req(int8())}), length));

cpp/src/arrow/array/util.cc

@@ -387,7 +387,7 @@ class NullArrayFactory {
   // - buffers = []
   // - child_data = [nullptr] * type.num_fields()
   // - dictionary = nullptr
-  bool presizing_zero_buffer_;
+  const bool presizing_zero_buffer_;
 
   NullArrayFactory(const std::shared_ptr<DataType>& type, bool nullable, int64_t length)
       : presizing_zero_buffer_{true},
@@ -534,8 +534,8 @@ class NullArrayFactory {
     for (auto [field, id] : Zip(type.fields(), type.type_codes())) {
       if (field->nullable()) return id;
     }
-    return Status::Invalid("Cannot produce an array of null ", type,
-                           " because no child field is nullable");
+    return Status::TypeError("Cannot produce an array of null ", type,
+                             " because no child field is nullable");
   }
 
   Status Visit(const UnionType& type) {
@@ -576,6 +576,7 @@ class NullArrayFactory {
     }
 
     if (type.mode() == UnionMode::DENSE) {
+      // offsets
       out_->buffers.push_back(*zero_buffer_);
     }
 
@@ -613,8 +614,8 @@ class NullArrayFactory {
 
     out_->buffers = {nullptr};
     if (!type.field(1)->nullable()) {
-      return Status::Invalid("Cannot produce an array of null ", type,
-                             " because the values field is not nullable");
+      return Status::TypeError("Cannot produce an array of null ", type,
+                               " because the values field is not nullable");
     }
 
     std::shared_ptr<Array> run_ends, values;

cpp/src/arrow/array/validate.cc

@@ -581,15 +581,15 @@ struct ValidateArrayImpl {
 
     if (HasValidityBitmap(data.type->id()) && data.buffers[0]) {
       // Do not call GetNullCount() as it would also set the `null_count` member
-      return null_count = data.length -
-                          CountSetBits(data.buffers[0]->data(), data.offset, data.length);
-    }
-
-    if (data.type->storage_id() == Type::NA) {
-      return null_count = data.length;
+      null_count =
+          data.length - CountSetBits(data.buffers[0]->data(), data.offset, data.length);
+    } else if (data.type->storage_id() == Type::NA) {
+      null_count = data.length;
+    } else {
+      null_count = 0;
     }
 
-    return null_count = 0;
+    return null_count;
   }
 
   Status ValidateFixedWidthBuffers() {

cpp/src/arrow/type.h

@@ -183,9 +183,6 @@ class ARROW_EXPORT DataType : public std::enable_shared_from_this<DataType>,
   /// \brief Return the type category of the storage type
   virtual Type::type storage_id() const { return id_; }
 
-  /// \brief Return a reference to the storage type
-  virtual const DataType& storage_type_ref() const { return *this; }
-
   /// \brief Return the storage type
   virtual std::shared_ptr<DataType> storage_type() const { return GetSharedPtr(); }