ARROW-1568: [C++] Implement Drop Null Kernel for Arrays #10802

aocsa · 2021-07-26T15:27:58Z

Implement "drop null" kernels that return array without nulls

Note: This can be implemented as a arrow::compute::VectorFunction because the size of the array is changed, so this function is not valid in a SQL-like context

github-actions · 2021-07-26T15:30:57Z

https://issues.apache.org/jira/browse/ARROW-1568

aocsa · 2021-07-30T03:35:46Z

This PR is ready to review :)

cpp/src/arrow/compute/kernels/vector_selection.cc

nirandaperera

I made some comments on the general approach. Great work @aocsa 👏

cpp/src/arrow/compute/kernels/vector_selection.cc

cpp/src/arrow/compute/kernels/vector_selection_test.cc

westonpace

I agree with Niranda that the record batch implementation probably won't work but I think this is a great start and looks like a very good addition. Thanks for making this!

cpp/src/arrow/compute/kernels/vector_selection.cc

cpp/src/arrow/compute/kernels/vector_selection_test.cc

python/pyarrow/tests/test_compute.py

aocsa · 2021-08-02T14:01:24Z

I updated this PR base on last feedback comments. Main change, the way algorithm drop nulls now are based on filter operation and boolean filter array is obtained directly using the bitmap from input array.

cpp/src/arrow/compute/kernels/vector_selection.cc

cpp/src/arrow/compute/kernels/vector_selection_test.cc

cpp/src/arrow/compute/kernels/vector_selection.cc

jorisvandenbossche

Cool, added a few comments!

How does "drop_null" work for nested types? I suppose it only takes into account the top-level validity? (if so, might be worth to test (if not yet done) and document explicitily)

cpp/src/arrow/compute/kernels/vector_selection.cc

python/pyarrow/compute.py

cpp/src/arrow/compute/kernels/vector_selection.cc

aocsa · 2021-08-05T16:33:11Z

I updated this PR addressing latest feedback comments.
I think this PR is ready for merging. Let me know if anything needs to be taken care of from my end. cc @nirandaperera, @jorisvandenbossche, @westonpace

nirandaperera

Great work @aocsa 👍 I left some minor comments :-)

cpp/src/arrow/compute/kernels/vector_selection.cc

aocsa · 2021-08-06T14:54:57Z

Thanks @nirandaperera. I updated the PR based on latest feedback. Nice to know more about the Arrow BitUtils :)
IMO this PR is ready to be merged :) +1 cc @westonpace

cpp/src/arrow/compute/kernels/vector_selection.cc

cpp/src/arrow/compute/kernels/vector_selection_test.cc

westonpace

Only a few very minor things. Most are fine for a follow up, remove the prints and I'll be good.

cpp/src/arrow/compute/kernels/vector_selection.cc

python/pyarrow/tests/test_compute.py

cpp/src/arrow/compute/kernels/vector_selection_test.cc

minor fix vc++/win minor fix last fix

minor windows compilation fix reuse Bitmap

minor fix

aocsa · 2021-08-18T13:38:31Z

Thanks again @pitrou, just a reminder this PR was updated. Main change related to DropNullTable

... is to get TableBatchReader to get a vector of record batches spanning the table, then call drop_null on each record batch and then Table::FromRecordBatches to get the resulting table.

#10802 (comment)

pitrou · 2021-08-18T15:48:37Z

python/pyarrow/compute.py

    return call_function('take', [data, indices], options, memory_pool)


+def drop_null(data, memory_pool=None):


I'll remove this as the proper definition is auto-generated from the C++ metadata.

pitrou · 2021-08-18T15:48:54Z

cpp/src/arrow/compute/kernels/vector_selection.cc

+  }
+  auto drop_null_filter =
+      std::make_shared<BooleanArray>(batch->num_rows(), dst, nullptr, 0, 0);
+  if (drop_null_filter->null_count() == batch->num_rows()) {


This condition is never satisfied, will fix.

pitrou · 2021-08-18T15:49:20Z

cpp/src/arrow/compute/kernels/vector_selection_test.cc

+    auto schm = schema(fields);
+    std::vector<std::shared_ptr<ChunkedArray>> empty_table(fields.size());
+    auto col_a = std::make_shared<NullArray>(size);
+    auto col_b = std::make_shared<NullArray>(size);


These are the wrong column types. I will fix and add a validation.

pitrou · 2021-08-18T15:59:50Z

Note the auto-generated Python docstring isn't entirely accurate (see the input argument description):

Signature: pc.drop_null(input, *, memory_pool=None)
Docstring:
Drop nulls from the input.

The output is populated with values from the input (Array, ChunkedArray,
RecordBatch, or Table) without the null values.
For the RecordBatch and Table cases, `drop_null` drops the full row if
there is any null.

Parameters
----------
input : Array-like or scalar-like
    Argument to compute function
memory_pool : pyarrow.MemoryPool, optional
    If not passed, will allocate memory from the default memory pool.
File:      ~/arrow/dev/python/pyarrow/compute.py
Type:      function

I'll open a separate JIRA.

aocsa · 2021-08-18T16:11:17Z

Note the auto-generated Python docstring isn't entirely accurate (see the input argument description):

Signature: pc.drop_null(input, *, memory_pool=None)
Docstring:
Drop nulls from the input.

The output is populated with values from the input (Array, ChunkedArray,
RecordBatch, or Table) without the null values.
For the RecordBatch and Table cases, `drop_null` drops the full row if
there is any null.

Parameters
----------
input : Array-like or scalar-like
    Argument to compute function
memory_pool : pyarrow.MemoryPool, optional
    If not passed, will allocate memory from the default memory pool.
File:      ~/arrow/dev/python/pyarrow/compute.py
Type:      function

I'll open a separate JIRA.

Ok, thanks @pitrou let me know in which jira ticket will be this to follow up the updates.

pitrou · 2021-08-18T16:11:59Z

It's in https://issues.apache.org/jira/browse/ARROW-13665

github-actions bot added the Component: C++ label Jul 26, 2021

aocsa changed the title ~~[C++] Implement "drop null" kernels that return array without nulls~~ ARROW-1568: [ComputeKernels] Add Drop Null for Arrays Kernel Jul 26, 2021

aocsa changed the title ~~ARROW-1568: [ComputeKernels] Add Drop Null for Arrays Kernel~~ ARROW-1568: [C++] Add Drop Null for Arrays Kernel Jul 26, 2021

aocsa marked this pull request as draft July 26, 2021 15:34

aocsa changed the title ~~ARROW-1568: [C++] Add Drop Null for Arrays Kernel~~ ARROW-1568: [C++] Implement Drop Null Kernel for Arrays Jul 26, 2021

apache deleted a comment from github-actions bot Jul 27, 2021

github-actions bot added the Component: Python label Jul 30, 2021

aocsa marked this pull request as ready for review July 30, 2021 03:33

bkietz requested changes Jul 30, 2021

View reviewed changes

cpp/src/arrow/compute/kernels/vector_selection.cc Outdated Show resolved Hide resolved

bkietz requested changes Jul 30, 2021

View reviewed changes

cpp/src/arrow/compute/kernels/vector_selection.cc Outdated Show resolved Hide resolved

bkietz requested changes Jul 30, 2021

View reviewed changes

cpp/src/arrow/compute/kernels/vector_selection.cc Outdated Show resolved Hide resolved

nirandaperera reviewed Jul 30, 2021

View reviewed changes

westonpace requested changes Jul 30, 2021

View reviewed changes

nirandaperera suggested changes Aug 2, 2021

View reviewed changes

cpp/src/arrow/compute/kernels/vector_selection.cc Outdated Show resolved Hide resolved

cpp/src/arrow/compute/kernels/vector_selection.cc Outdated Show resolved Hide resolved

cpp/src/arrow/compute/kernels/vector_selection_test.cc Outdated Show resolved Hide resolved

aocsa force-pushed the aocsa/ARROW-1568 branch from bc5fd70 to f0e3318 Compare August 2, 2021 18:13

nirandaperera suggested changes Aug 3, 2021

View reviewed changes

cpp/src/arrow/compute/kernels/vector_selection.cc Outdated Show resolved Hide resolved

jorisvandenbossche reviewed Aug 3, 2021

View reviewed changes

nirandaperera suggested changes Aug 5, 2021

View reviewed changes

aocsa force-pushed the aocsa/ARROW-1568 branch 2 times, most recently from 5b1eedb to eb8f878 Compare August 6, 2021 03:18

nirandaperera suggested changes Aug 6, 2021

View reviewed changes

westonpace requested changes Aug 6, 2021

View reviewed changes

aocsa force-pushed the aocsa/ARROW-1568 branch from eb8f878 to fff75c5 Compare August 9, 2021 16:33

bkietz requested changes Aug 9, 2021

View reviewed changes

cpp/src/arrow/compute/kernels/vector_selection_test.cc Outdated Show resolved Hide resolved

aocsa force-pushed the aocsa/ARROW-1568 branch from 1313d61 to 96b3742 Compare August 10, 2021 06:34

aocsa added 14 commits August 16, 2021 12:36

[python] implement dropnull

41c0527

minor fixes

423cece

lint fixes

1f4c675

fixes based on feedback comments

10e60a5

minor fix vc++/win minor fix last fix

windows test fix

acd8373

update addressing last feedback comments

bb220e7

minor update DropNullRecordBatch and DropNullTable

9072da5

minor windows compilation fix reuse Bitmap

last fixes

2b023f4

last fix

efe921a

new tests cases added

cfbf126

minor fix CI

f2b11a6

minor update to the documentation

92a6099

minor update to the documentation

dc62feb

addressing pitrou review

cb67a9f

aocsa force-pushed the aocsa/ARROW-1568 branch from b4d9285 to c98ac6d Compare August 16, 2021 17:37

minor changes, addressing pitrou review

27c9cb4

minor fix

aocsa force-pushed the aocsa/ARROW-1568 branch from c98ac6d to 27c9cb4 Compare August 17, 2021 04:22

addressing last and ommitted pitrou review

99195ce

pitrou reviewed Aug 18, 2021

View reviewed changes

pitrou added 2 commits August 18, 2021 17:50

Minor improvements and cleanups

3d17ea4

Add Sphinx docs for drop_null

884a5a6

pitrou approved these changes Aug 18, 2021

View reviewed changes

pitrou closed this in 558d83f Aug 18, 2021

asfimport mentioned this pull request Nov 23, 2021

[C++] Implement "drop null" kernels that return array without nulls #15478

Closed

		return call_function('take', [data, indices], options, memory_pool)


		def drop_null(data, memory_pool=None):

ARROW-1568: [C++] Implement Drop Null Kernel for Arrays #10802

ARROW-1568: [C++] Implement Drop Null Kernel for Arrays #10802

Uh oh!

Conversation

aocsa commented Jul 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jul 26, 2021

Uh oh!

aocsa commented Jul 30, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nirandaperera left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

westonpace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aocsa commented Aug 2, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aocsa commented Aug 5, 2021

Uh oh!

nirandaperera left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aocsa commented Aug 6, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

westonpace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aocsa commented Aug 18, 2021

Uh oh!

pitrou Aug 18, 2021

Choose a reason for hiding this comment

Uh oh!

pitrou Aug 18, 2021

Choose a reason for hiding this comment

Uh oh!

aocsa commented Jul 26, 2021 •

edited

Loading