-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GH-34316: [Python] FixedSizeListArray.from_arrays supports mask parameter #39396
Conversation
|
a6be8eb
to
6b11b42
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs one small tweak to be in line with other implementations, but otherwise looks good. Thanks for working on this :)
cpp/src/arrow/array/array_nested.h
Outdated
static Result<std::shared_ptr<Array>> FromArrays( | ||
const std::shared_ptr<Array>& values, | ||
int32_t list_size, | ||
std::shared_ptr<Buffer> null_bitmap = NULLPTR); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other methods that allow passing a null bitmap (such as ListArray::FromArrays()
) support another parameter null_count
to pass the computed null count. Could we support that parameter in C++ as well? We don't need to expose it in Python.
arrow/cpp/src/arrow/array/array_nested.h
Lines 174 to 178 in 6b11b42
static Result<std::shared_ptr<ListArray>> FromArrays( | |
std::shared_ptr<DataType> type, const Array& offsets, const Array& values, | |
MemoryPool* pool = default_memory_pool(), | |
std::shared_ptr<Buffer> null_bitmap = NULLPTR, | |
int64_t null_count = kUnknownNullCount); |
6b11b42
to
31157b5
Compare
31157b5
to
1708fa7
Compare
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 0e597ab. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them. |
… parameter (apache#39396) ### What changes are included in this PR? Add `mask` / `null_bitmap` parameters in corresponding Cython / C++ `FixedSizeListArray` methods, and propagate this bitmap instead of using the current dummy `validity_buf`. ### Are these changes tested? Yes ### Are there any user-facing changes? Yes, `mask` parameter has been added to `FixedSizeListArray.from_arrays` * Closes: apache#34316 Authored-by: LucasG0 <guillermou.lucas@gmail.com> Signed-off-by: Will Jones <willjones127@gmail.com>
… parameter (apache#39396) ### What changes are included in this PR? Add `mask` / `null_bitmap` parameters in corresponding Cython / C++ `FixedSizeListArray` methods, and propagate this bitmap instead of using the current dummy `validity_buf`. ### Are these changes tested? Yes ### Are there any user-facing changes? Yes, `mask` parameter has been added to `FixedSizeListArray.from_arrays` * Closes: apache#34316 Authored-by: LucasG0 <guillermou.lucas@gmail.com> Signed-off-by: Will Jones <willjones127@gmail.com>
… parameter (apache#39396) ### What changes are included in this PR? Add `mask` / `null_bitmap` parameters in corresponding Cython / C++ `FixedSizeListArray` methods, and propagate this bitmap instead of using the current dummy `validity_buf`. ### Are these changes tested? Yes ### Are there any user-facing changes? Yes, `mask` parameter has been added to `FixedSizeListArray.from_arrays` * Closes: apache#34316 Authored-by: LucasG0 <guillermou.lucas@gmail.com> Signed-off-by: Will Jones <willjones127@gmail.com>
What changes are included in this PR?
Add
mask
/null_bitmap
parameters in corresponding Cython / C++FixedSizeListArray
methods, and propagate this bitmap instead of using the current dummyvalidity_buf
.Are these changes tested?
Yes
Are there any user-facing changes?
Yes,
mask
parameter has been added toFixedSizeListArray.from_arrays
mask
inFixedSizeListArray.from_arrays
#34316