[GitHub] [arrow] jorisvandenbossche commented on a diff in pull request #13894: ARROW-15839: [C++][Python] Accept validity bitmap in ListArray.from_arrays

[GitBox <gi...@apache.org>]

jorisvandenbossche commented on code in PR #13894:
URL: https://github.com/apache/arrow/pull/13894#discussion_r946682571


##########
cpp/src/arrow/array/array_nested.cc:
##########
@@ -116,14 +117,20 @@ Result<std::shared_ptr<typename TypeTraits<TYPE>::ArrayType>> ListArrayFromArray
     return Status::TypeError("List offsets must be ", OffsetArrowType::type_name());
   }
 
+  if (null_bitmap != NULLPTR && offsets.null_count() > 0) {

Review Comment:
   ```suggestion
     if (null_bitmap != nullptr && offsets.null_count() > 0) {
   ```
   
   (to be consistent in style)



##########
cpp/src/arrow/array/array_nested.h:
##########
@@ -127,11 +127,15 @@ class ARROW_EXPORT ListArray : public BaseListArray<ListType> {
   /// allocated because of null values
   static Result<std::shared_ptr<ListArray>> FromArrays(
       const Array& offsets, const Array& values,
-      MemoryPool* pool = default_memory_pool());
+      MemoryPool* pool = default_memory_pool(),
+      int64_t null_count = -1,
+      std::shared_ptr<Buffer> null_bitmap = NULLPTR);

Review Comment:
   Small nit: it seems that existing functions here that support passing the bitmap typically have null_bitmap first and null_count second (also, they seem to use `kUnknownNullCount` instead of -1 explicitly, I don't know if that actually makes a difference for the compiler)



##########
cpp/src/arrow/array/array_nested.cc:
##########
@@ -116,14 +117,20 @@ Result<std::shared_ptr<typename TypeTraits<TYPE>::ArrayType>> ListArrayFromArray
     return Status::TypeError("List offsets must be ", OffsetArrowType::type_name());
   }
 
+  if (null_bitmap != NULLPTR && offsets.null_count() > 0) {
+    return Status::Invalid(
+        "Ambiguous to specify both validity map and offsets with nulls");
+  }
+
   std::shared_ptr<Buffer> offset_buf, validity_buf;
   RETURN_NOT_OK(CleanListOffsets<TYPE>(offsets, pool, &offset_buf, &validity_buf));
-  BufferVector buffers = {validity_buf, offset_buf};
+  int64_t null_count_ = null_bitmap ? null_count : offsets.null_count();
 
-  auto internal_data = ArrayData::Make(type, offsets.length() - 1, std::move(buffers),
-                                       offsets.null_count(), offsets.offset());
+  std::shared_ptr<arrow::ArrayData> internal_data = ArrayData::Make(
+      type, offsets.length() - 1,
+      BufferVector{null_bitmap ? std::move(null_bitmap) : validity_buf, offset_buf},

Review Comment:
   It might be more readable to move this if/else to where `buffers` gets defined (which actually no longer is used right now)



##########
python/pyarrow/array.pxi:
##########
@@ -2812,6 +2805,22 @@ cdef dict _array_classes = {
 }
 
 
+cdef inline shared_ptr[CBuffer] c_mask_from_obj(object mask, MemoryPool pool) except *:

Review Comment:
   Maybe use "inverted" in the name of the function, so that when reading the code in `from_arrays`, you still have some notion about the fact that the mask gets inverted



##########
python/pyarrow/array.pxi:
##########
@@ -1890,7 +1890,7 @@ cdef class ListArray(BaseListArray):
     """
 
     @staticmethod
-    def from_arrays(offsets, values, DataType type=None, MemoryPool pool=None):
+    def from_arrays(offsets, values, DataType type=None, MemoryPool pool=None, mask=None):

Review Comment:
   Can you also update the docstring for this?



##########
python/pyarrow/tests/test_array.py:
##########
@@ -3192,3 +3192,41 @@ def test_to_pandas_timezone():
     arr = pa.chunked_array([arr])
     s = arr.to_pandas()
     assert s.dt.tz is not None
+
+
+@pytest.mark.parametrize("arr", (
+    [None, [0]],
+    [None, [0, None], [0]],
+    [[0], [1]],
+)
+)
+def test_list_array_from_arrays(arr):

Review Comment:
   There is already an existing `test_list_from_arrays` test, can you maybe move this one to just after the existing one? (to keep related tests close)



-- 
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-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org