Copilot commented on code in PR #50430:
URL: https://github.com/apache/arrow/pull/50430#discussion_r3631364375
##########
python/pyarrow/includes/libarrow.pxd:
##########
@@ -941,18 +941,20 @@ cdef extern from "arrow/api.h" namespace "arrow" nogil:
const vector[c_string]& field_names,
const vector[int8_t]& type_codes)
- int32_t value_offset(int i)
+ int32_t value_offset(int64_t i)
shared_ptr[CBuffer] value_offsets()
cdef cppclass CBinaryArray" arrow::BinaryArray"(CArray):
const uint8_t* GetValue(int i, int32_t* length)
Review Comment:
`arrow::BinaryArray::GetValue` takes an `int64_t` index in Arrow C++
(`array_binary.h`), but the Cython declaration uses `int i`, which can truncate
indices on large arrays (and is inconsistent with the new `GetView(int64_t)`
binding right below).
##########
python/pyarrow/array.pxi:
##########
@@ -2444,6 +2454,12 @@ cdef class BooleanArray(Array):
"""
Concrete class for Arrow arrays of boolean data type.
"""
+
+ cdef object _getitem_py(self, int64_t i, object maps_as_pydicts):
+ if self.ap.IsNull(i):
+ return None
+ return (<CBooleanArray*> self.ap).Value(i)
+
Review Comment:
`BooleanArray._getitem_py` and `NumericArray._getitem_py` call C++
`Value(i)` with an `int64_t i`, but the Cython declarations in
`python/pyarrow/includes/libarrow.pxd` still declare most `Value` methods as
`Value(int i)`. Cython will silently truncate indices > INT_MAX, which can
produce incorrect results or crashes for very large arrays now that
`to_pylist()` iterates via `_getitem_py`.
Please update the `Value` method declarations (e.g., `CBooleanArray`,
`CInt*Array`, `CUInt*Array`, `CFloatArray`, `CDoubleArray`, etc.) to accept
`int64_t i` to match the C++ headers (`array_primitive.h`).
##########
python/pyarrow/includes/libarrow.pxd:
##########
@@ -941,18 +941,20 @@ cdef extern from "arrow/api.h" namespace "arrow" nogil:
const vector[c_string]& field_names,
const vector[int8_t]& type_codes)
- int32_t value_offset(int i)
+ int32_t value_offset(int64_t i)
shared_ptr[CBuffer] value_offsets()
cdef cppclass CBinaryArray" arrow::BinaryArray"(CArray):
const uint8_t* GetValue(int i, int32_t* length)
+ cpp_string_view GetView(int64_t i)
shared_ptr[CBuffer] value_data()
int32_t value_offset(int64_t i)
int32_t value_length(int64_t i)
int32_t total_values_length()
cdef cppclass CLargeBinaryArray" arrow::LargeBinaryArray"(CArray):
const uint8_t* GetValue(int i, int64_t* length)
Review Comment:
`arrow::LargeBinaryArray::GetValue` takes an `int64_t` index in Arrow C++
(`array_binary.h`), but the Cython declaration uses `int i`, which can truncate
indices on large arrays (and is inconsistent with the new `GetView(int64_t)`
binding right below).
--
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]
