GH-44446: [C++][Python] Add mask creation helper #44447

amol- · 2024-10-16T21:58:30Z

Rationale for this change

Implement a convenience method to create boolean masks where only some rows are set to true

What changes are included in this PR?

C++ MakeMaskArray factory function and pyarrow.mask Python factory function

Are these changes tested?

Yes both C++ and Python

Are there any user-facing changes?

Yes, new API has been added

GitHub Issue: [C++][Python] Helper to generate boolean masks #44446

github-actions · 2024-10-16T22:14:04Z

⚠️ GitHub issue #44446 has been automatically assigned in GitHub to PR creator.

amol- · 2024-10-17T15:02:20Z

@pitrou Do you have time to review? I think this is more or less done

cpp/src/arrow/array/util.cc

cpp/src/arrow/array/array_test.cc

pitrou · 2024-10-17T15:11:56Z

python/pyarrow/array.pxi

@@ -538,6 +538,51 @@ def repeat(value, size, MemoryPool memory_pool=None):
    return pyarrow_wrap_array(c_array)


+def mask(indices, length, MemoryPool memory_pool=None):


@jorisvandenbossche Does this API look ok?

Late response, and the general API looks good, my one suggestion would be to use a bit more descriptive name. mask could also be interpret in the active sense ("mask those values"), and eg pandas has a method with that name that work in that sense. Some more explicit options: create_mask, make_mask, mask_from_indices, ..

This is essentially the counterpart for indices_nonzero I think? (which converts a mask into indices)
Could maybe mention that in a "See Also" section in the docstring

python/pyarrow/array.pxi

felipecrv · 2024-10-21T21:24:09Z

cpp/src/arrow/array/util.cc

@@ -915,6 +916,29 @@ Result<std::shared_ptr<Array>> MakeEmptyArray(std::shared_ptr<DataType> type,
  return builder->Finish();
 }

+Result<std::shared_ptr<Array>> MakeMaskArray(const std::vector<int64_t>& indices,


This std::vector could be a span<int64_t> for more flexibility. The selection vector could come from anywhere.

python/pyarrow/array.pxi

amol- · 2024-12-02T22:48:44Z

I think the two failures are unrelated to this branch.
For Java it seems to be a memory leak in FlightSQL

Error:    TestFlightSqlStreams.tearDown:238 » IllegalState Memory was leaked by query. Memory leaked: (81920)

And for R it seems related to datetimes

── Failure ('test-dplyr-funcs-datetime.R:302:5'): timestamp round trip correctly via strftime and strptime ──

amol- · 2024-12-03T20:01:14Z

When you have the chance I'd appreciate if you could re-review this one @pitrou 🙂

cpp/src/arrow/array/util.cc

felipecrv · 2024-12-06T16:50:20Z

cpp/src/arrow/array/util.cc

+  ARROW_ASSIGN_OR_RAISE(auto buffer, AllocateBitmap(length, pool));
+  bit_util::SetBitsTo(buffer->mutable_data(), 0, length, false);
+  for (int64_t i = 0; i < indices->length(); ++i) {
+    int64_t index = indices->Value(i);


The value could be null and nulls must be skipped.

The outer MakeMaskArray function already prevents values from being null, that's why it's not done in the Impl again.

felipecrv · 2024-12-06T16:54:03Z

cpp/src/arrow/array/util.cc

+  if (indices->null_count() > 0) {
+    return Status::Invalid("Indices array must not contain null values");
+  }


Hmm. If it takes an Arrow array of indices it should be able to handle nulls. The loop can be specialized based on the result of indices->MayHaveNulls() so the common case doesn't have to check the validity bitmap of every iteration.

I don't see a reason why it would make sense to accept null values, it verifies if there are null values and rejects the array in case there are because there doesn't seem to be a case where it would make sense to have nulls in an array of indices.
If the user has an array containing nulls it can be used by passing it to compute.drop_null before using MakeMaskArray.

felipecrv · 2024-12-06T16:56:17Z

cpp/src/arrow/array/util.h

+/// \param[in] pool the memory pool to allocate memory from
+/// \return the resulting Array
+ARROW_EXPORT
+Result<std::shared_ptr<Array>> MakeMaskArray(const std::vector<int64_t>& indices,


As this function doesn't need to take ownership of indices, it can accept a span<int64_t> instead of a std::vector<int64_t> & so both vectors or any contiguous buffer if int64_t can be passed.

Note that it would be a span<const int64_t> then :)

I thought Arrow was constrained to C++17, isn't span a C++20 addition?

Yes, that's why we have a util::span backport.

cpp/src/arrow/array/util.cc

Co-authored-by: Felipe Oliveira Carvalho <[email protected]>

github-actions bot added Component: C++ Component: Python Component: Documentation awaiting review Awaiting review labels Oct 16, 2024

amol- changed the title ~~[GH-44446] Add mask creation helper~~ GH-44446: Add mask creation helper Oct 16, 2024

apache deleted a comment from github-actions bot Oct 16, 2024

amol- force-pushed the GH-44446 branch 2 times, most recently from f763a31 to d14836f Compare October 16, 2024 22:55

kou changed the title ~~GH-44446: Add mask creation helper~~ GH-44446: [C++][Python] Add mask creation helper Oct 17, 2024

pitrou reviewed Oct 17, 2024

View reviewed changes

cpp/src/arrow/array/util.cc Outdated Show resolved Hide resolved

pitrou reviewed Oct 17, 2024

View reviewed changes

cpp/src/arrow/array/array_test.cc Outdated Show resolved Hide resolved

pitrou reviewed Oct 17, 2024

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Oct 17, 2024

pitrou reviewed Oct 17, 2024

View reviewed changes

python/pyarrow/array.pxi Outdated Show resolved Hide resolved

felipecrv reviewed Oct 21, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Oct 21, 2024

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Nov 27, 2024

amol- added 8 commits December 2, 2024 14:15

Add mask creation helper

2b44595

Add docstring

06fd1d0

Avoid temporary value

9b12391

lint cpp

09249fb

Consistency in naming convention

df2a4f2

fix docstring

b29533e

Fix MakeMaskArray iterating over the end of vector

7e45794

Format C++

3396732

amol- added 6 commits December 2, 2024 14:15

replace use of builder with SetBit

3e886df

Implement MakeMaskArray for Array

e2836ec

Doc for overloaded function and format

117f68b

Update pyarrow to use Arrow based mask helper

2934be7

Lint C++

040e6fe

Catch out of bound indices

1227f6b

amol- force-pushed the GH-44446 branch from fe48db4 to 1227f6b Compare December 2, 2024 14:15

felipecrv reviewed Dec 6, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Dec 6, 2024

Update cpp/src/arrow/array/util.cc

7467b06

Co-authored-by: Felipe Oliveira Carvalho <[email protected]>

github-actions bot removed the awaiting changes Awaiting changes label Dec 9, 2024

Update cpp/src/arrow/array/util.cc

b291114

Co-authored-by: Felipe Oliveira Carvalho <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-44446: [C++][Python] Add mask creation helper #44447

GH-44446: [C++][Python] Add mask creation helper #44447

amol- commented Oct 16, 2024 •

edited by github-actions bot

Loading

github-actions bot commented Oct 16, 2024

amol- commented Oct 17, 2024

pitrou Oct 17, 2024

jorisvandenbossche Dec 9, 2024

felipecrv Oct 21, 2024

amol- commented Dec 2, 2024 •

edited

Loading

amol- commented Dec 3, 2024

felipecrv Dec 6, 2024

amol- Dec 9, 2024

felipecrv Dec 6, 2024

amol- Dec 9, 2024

felipecrv Dec 6, 2024

pitrou Dec 7, 2024

amol- Dec 9, 2024

pitrou Dec 9, 2024

		@@ -538,6 +538,51 @@ def repeat(value, size, MemoryPool memory_pool=None):
		return pyarrow_wrap_array(c_array)


		def mask(indices, length, MemoryPool memory_pool=None):

GH-44446: [C++][Python] Add mask creation helper #44447

Are you sure you want to change the base?

GH-44446: [C++][Python] Add mask creation helper #44447

Conversation

amol- commented Oct 16, 2024 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Oct 16, 2024

amol- commented Oct 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amol- commented Dec 2, 2024 • edited Loading

amol- commented Dec 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amol- commented Oct 16, 2024 •

edited by github-actions bot

Loading

amol- commented Dec 2, 2024 •

edited

Loading