ARROW-8917: [C++] Formalize "metafunction" concept. Add Take and Filter metafunctions, port R and Python bindings #7318

wesm · 2020-06-01T20:50:25Z

A "metafunction" is one that dispatches to other functions based on the argument types. It does not contain any kernels.

Other stuff in this PR:

Set up to remove all but two versions of arrow::compute::Take. Other ones are still there but will be deprecated or removed after the GLib bindings are ported to use either CallFunction or the Take with Datum-Datum.
Make "take" and "filter" metafunctions that also deal with RecordBatch, Table arguments
Delete tons of now unnecessary binding code from Python and R. Hence the significant LOC reduction

There is one failing R test that I wasn't able to debug easily.

Connected JIRAs: ARROW-7009

wesm · 2020-06-01T20:51:46Z

@nealrichardson I will need your help on the one failing R test

@kou I removed 6 of the 8 Take APIs -- I think it would be better for GLib to use the Datum/Datum or CallFunction API if possible and perhaps net a code reduction like Python and R have achieved here. What do you think?

github-actions · 2020-06-01T21:02:56Z

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

wesm · 2020-06-01T21:27:05Z

I figured out the R issue, I'm trying to fix. It's this hack here

https://github.com/apache/arrow/blob/master/r/src/compute.cpp#L180

wesm · 2020-06-01T21:44:37Z

Fixed

kou · 2020-06-01T21:59:26Z

I'll take a look this in a few days.

wesm · 2020-06-01T22:21:41Z

@kou thanks. I'll try to add deprecated APIs for the functions that were removed so that merging need not be blocked on the refactoring

wesm · 2020-06-01T22:36:44Z

Done. So GLib can be refactored later.

kou · 2020-06-01T23:40:47Z

OK.
I'll work on the GLib part as a separated pull request and add ARROW_DEPRECATED in the pull request.

wesm · 2020-06-02T02:10:15Z

OK.
I'll work on the GLib part as a separated pull request and add ARROW_DEPRECATED in the pull request.

Perfect

wesm · 2020-06-02T12:03:29Z

The CI is passing except for this s390x issue

src/arrow/CMakeFiles/arrow_objlib.dir/vendored/uriparser/UriShorten.c.o  /usr/lib/s390x-linux-gnu/libcrypto.so  /usr/lib/s390x-linux-gnu/libssl.so  /usr/lib/s390x-linux-gnu/libbrotlienc.so  /usr/lib/s390x-linux-gnu/libbrotlidec.so  /usr/lib/s390x-linux-gnu/libbrotlicommon.so  orc_ep-install/lib/liborc.a  protobuf_ep-install/lib/libprotobuf.a  /usr/lib/s390x-linux-gnu/libglog.so  -ldl  jemalloc_ep-prefix/src/jemalloc_ep/dist//lib/libjemalloc_pic.a  -lrt  /usr/lib/s390x-linux-gnu/libcrypto.so  /usr/lib/s390x-linux-gnu/libssl.so  /usr/lib/s390x-linux-gnu/libbrotlienc.so  /usr/lib/s390x-linux-gnu/libbrotlidec.so  /usr/lib/s390x-linux-gnu/libbrotlicommon.so  /usr/lib/s390x-linux-gnu/libbz2.so  /usr/lib/s390x-linux-gnu/liblz4.so  /usr/lib/s390x-linux-gnu/libsnappy.so.1.1.8  /usr/lib/s390x-linux-gnu/libz.so  /usr/lib/s390x-linux-gnu/libzstd.so  orc_ep-install/lib/liborc.a  protobuf_ep-install/lib/libprotobuf.a  /usr/lib/s390x-linux-gnu/libglog.so  /usr/lib/s390x-linux-gnu/libcrypto.so  -pthread && :
/usr/bin/ld: final link failed: No space left on device
collect2: error: ld returned 1 exit status

jorisvandenbossche

Looks good!
(some nice code removal in the python bindings indeed ;))

Added some small comments.

One more conceptual aspect I am wondering: compute functions on tables that basically just apply the function on each column (like take, but also if eg support for Table would be added to the sum registered function), wouldn't that start to be part of the (higher level) execution plan?

cpp/src/arrow/compute/api_vector.h

cpp/src/arrow/compute/function.h

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

python/pyarrow/array.pxi

wesm · 2020-06-02T13:26:41Z

One more conceptual aspect I am wondering: compute functions on tables that basically just apply the function on each column (like take, but also if eg support for Table would be added to the sum registered function), wouldn't that start to be part of the (higher level) execution plan?

Perhaps, but it's not mutually exclusive. Some applications will just want to do CallFunction(name, args) while others might build an expression pipeline with multiple functions and then request that it be evaluated (once we have this capability, which will be relatively soon)

wesm · 2020-06-02T16:59:57Z

@kiszk the s390x machine on Travis appears to be in bad shape.

It would be good to get this merged sometime today, let me know if there are other comments on the C++ or R side

kiszk · 2020-06-02T17:08:24Z

Oh, I saw https://travis-ci.org/github/apache/arrow/builds one hour ago. And, I got good feeling...

According to my experience, this type of disk full error may disappear when we will run the test after 30 min. or 1 hour.

bkietz

This seems like a clean refactor, thanks for doing this. Would you comment here or on the JIRA about other metafunctions (aside from take and filter)?

cpp/src/arrow/compute/function.h

wesm · 2020-06-02T21:15:37Z

Would you comment here or on the JIRA about other metafunctions (aside from take and filter)?

These are the only ones I can think of aside from cast at the moment

…Port R, Python bindings modulo one failing R test Fix Table__to_dataframe for ChunkedArray with zero chunks Restore deprecated APIs so that GLib will compile and pass tests again CI fixes Try to fix R 4.0 toolchain issue again

wesm · 2020-06-02T22:54:51Z

Will merge this once the build passes

wesm · 2020-06-03T00:26:12Z

@kou this test failure https://github.com/apache/arrow/pull/7318/checks?check_run_id=732734225 seems unrelated to this patch, thoughts? It seems like the test case may be malformed

https://github.com/apache/arrow/blob/master/c_glib/test/test-struct-array.rb#L41

wesm · 2020-06-03T00:27:21Z

Merging as it doesn't seem like the test failure could be related to this patch (the error showed up after rebasing)

kou · 2020-06-03T00:29:05Z

We can ignore the failure.
It's a test code problem. It's a GC related problem. We need to keep reference to the raw buffer data.
I'll fix it later.

r/R/chunked-array.R

r/R/record-batch.R

r/R/table.R

wesm force-pushed the take-filter-metafunctions branch from 743fe38 to 78c3e3e Compare June 1, 2020 22:36

wesm requested review from bkietz and jorisvandenbossche June 1, 2020 22:37

jorisvandenbossche reviewed Jun 2, 2020

View reviewed changes

wesm force-pushed the take-filter-metafunctions branch from 2f518b4 to a7618ea Compare June 2, 2020 15:19

bkietz approved these changes Jun 2, 2020

View reviewed changes

cpp/src/arrow/compute/function.h Outdated Show resolved Hide resolved

wesm and others added 3 commits June 2, 2020 17:52

Address Joris review comments, rebase

5d84b2c

Add pure virtual MetaFunction::ExecuteImpl as suggested by Ben

37753d1

wesm force-pushed the take-filter-metafunctions branch from a7618ea to 37753d1 Compare June 2, 2020 22:52

wesm closed this in 199d089 Jun 3, 2020

wesm deleted the take-filter-metafunctions branch June 3, 2020 00:28

nealrichardson reviewed Jun 3, 2020

View reviewed changes

r/R/chunked-array.R Show resolved Hide resolved

nealrichardson reviewed Jun 3, 2020

View reviewed changes

r/R/record-batch.R Show resolved Hide resolved

nealrichardson reviewed Jun 3, 2020

View reviewed changes

r/R/table.R Show resolved Hide resolved

nealrichardson added a commit to nealrichardson/arrow that referenced this pull request Jun 8, 2020

Remove stale comments from apache#7318

349272c

asfimport mentioned this pull request Jun 6, 2020

[C++][Compute] Formalize "metafunction" concept #25049

Closed

ARROW-8917: [C++] Formalize "metafunction" concept. Add Take and Filter metafunctions, port R and Python bindings #7318

ARROW-8917: [C++] Formalize "metafunction" concept. Add Take and Filter metafunctions, port R and Python bindings #7318

Uh oh!

Conversation

wesm commented Jun 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wesm commented Jun 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jun 1, 2020

Uh oh!

wesm commented Jun 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wesm commented Jun 1, 2020

Uh oh!

kou commented Jun 1, 2020

Uh oh!

wesm commented Jun 1, 2020

Uh oh!

wesm commented Jun 1, 2020

Uh oh!

kou commented Jun 1, 2020

Uh oh!

wesm commented Jun 2, 2020

Uh oh!

wesm commented Jun 2, 2020

Uh oh!

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wesm commented Jun 2, 2020

Uh oh!

wesm commented Jun 2, 2020

Uh oh!

kiszk commented Jun 2, 2020

Uh oh!

bkietz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wesm commented Jun 2, 2020

Uh oh!

wesm commented Jun 2, 2020

Uh oh!

wesm commented Jun 3, 2020

Uh oh!

wesm commented Jun 3, 2020

Uh oh!

kou commented Jun 3, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

wesm commented Jun 1, 2020 •

edited

Loading

wesm commented Jun 1, 2020 •

edited

Loading

wesm commented Jun 1, 2020 •

edited

Loading