ARROW-6243: [C++][Dataset] Filter expressions #5157

bkietz · 2019-08-21T19:43:04Z

Adds the Expression class which is used to represent an arbitrarily complex filter expression. Expressions can be constructed using factory functions, for example:

and_(
  equal(field_ref("a"), scalar<int16_t>(5)),   // column 'a' is equal to 5
  greater(field_ref("b"), scalar<double>(0.0)) // column 'b' is greater than 0.0
)

Operator overloads are also provided, so the above could also be written as

"a"_ == int16_t(5) and "b"_ > 0.0

These can be executed against a single record batch (using the arrow::compute:: kernels).

Additionally, expressions may be simplified or even elided given partition information. For example, given a partition where column 'a' is equal to 5 the above query could be simplified to "b"_ > 0.0 (since the condition on column 'a' is satisfied by the entire partition) and given a partition where column 'b' is between -1.0 and 0.0 the query simplifies to false (since no record in the partition will satisfy the condition on column 'b'). This can be used to support arbitrary partitioning schemes and do the least kernel work possible on each record batch.

pitrou

The design looks neat. I haven't looked at the implementation in filter.cc.

One thing that surprises me is the ability for filter to return nulls rather than booleans. Isn't the use case to filter input from a dataset? Why would one want to generate null rows at this point?

cpp/src/arrow/scalar.h

pitrou · 2019-08-27T10:45:24Z

cpp/src/arrow/dataset/filter_test.cc

Is there a particular reason for using "and" rather than "&&"?

Looks more like SQL. I expect somebody will ask me to change it, but I thought it was more readable

Well, I learnt something. I didn't know that these "alternate operator spellings" existed in C++.

Relevant change to cpplint exclusions:
https://github.com/apache/arrow/pull/5157/files#diff-ca3a12ce3f399d977c7dc7c19c893b65R38

pitrou · 2019-08-27T10:48:53Z

cpp/src/arrow/dataset/filter_test.cc

Why can't *equal(fieldRef("b"), null32) be written as "b"_ == null32?

Also, shouldn't it simplify to never instead?

Re filters returning nulls:
If a comparison references a slot which is null, we RefuseToGuess and the result of the comparison is null. Here's a test illustrating this:

// filter expression: "a"_ != 0 and "b"_ > 0.1 // record batch: {"a": 0, "b": -0.1}, // filtered out because "a" == 0, return 0 {"a": 1, "b": 0.2}, // included, return 1 {"a": 2, "b": -0.1}, // filtered out because "b" is not greater than 0.1, return 0 {"a": 0, "b": null} // unknown because "b" is null, return null

RefuseToGuess is also implemented at the level of Expression::Assume. This is necessary when (for example) a filter expression references a column absent from some data fragment (perhaps it is an older file from before the referenced column was added). In that case we can't know whether rows in that fragment are relevant or not and we must yield them, but we can avoid the work of evaluating a kernel there.

If a user needs to query only rows where a field is defined, use a validity expression (which extracts the null bitmask of an array to a boolean expression).

Hmm, I see. I'll let others share their opinions about this. @wesm @fsaintjacques

I am not confident this handles null correctly, and to address this I've created:
https://issues.apache.org/jira/browse/ARROW-6386
Draft PR #5231

cpp/src/arrow/dataset/filter.h

pitrou · 2019-08-27T11:14:31Z

cpp/src/arrow/dataset/filter.h

In which context can it be a scalar? And if it's an array, what is the type?

The most trivial case of evaluating to scalar would be evaluation of a ScalarExpression. I could have just refused the bequest of Evaluate and returned an error in that case, but it seemed more robust to do it this way.

all, any, not and the comparison operators will evaluate to Type::BOOL, whereas a field reference will evaluate to whatever type that column has in the record batch. In general, the evaluated type of an Expression can be examined using Expression::Validate()

I can add a comment describing as much of this as you like.

Thanks for the explanation. We can probably refine this later.

cpp/src/arrow/dataset/filter.h

wesm · 2019-08-27T18:44:39Z

cpp/src/arrow/dataset/filter.h

nit: fieldRef is not compliant with our style guide

Do you mean https://google.github.io/styleguide/cppguide.html#Function_Names ? I was following the factories for Field = field(), StructType = struct_(), etc. What should I have named them?

field_ref should probably be it then.

I'll rename it

wesm · 2019-08-27T18:45:22Z

I'll review in more detail when I can, this week is tough

fsaintjacques · 2019-09-04T18:04:19Z

The MSVC error seems legit.

bkietz · 2019-09-05T13:28:22Z

I'll try to reproduce locally

bkietz · 2019-09-06T21:27:54Z

Travis timed out https://travis-ci.org/apache/arrow/jobs/581750838#L2081

kou · 2019-09-06T22:46:40Z

Restarted.

…pressionType

codecov-io · 2019-09-09T19:36:38Z

Codecov Report

Merging #5157 into master will increase coverage by 0.36%.
The diff coverage is 58.53%.

@@            Coverage Diff             @@
##           master    #5157      +/-   ##
==========================================
+ Coverage   88.75%   89.12%   +0.36%     
==========================================
  Files         946      757     -189     
  Lines      124312   109719   -14593     
  Branches     1437        0    -1437     
==========================================
- Hits       110336    97789   -12547     
+ Misses      13614    11930    -1684     
+ Partials      362        0     -362

Impacted Files	Coverage Δ
cpp/src/arrow/record_batch.h	`100% <ø> (ø)`	⬆️
cpp/src/arrow/compute/kernels/compare.cc	`87.31% <33.33%> (-1.33%)`	⬇️
cpp/src/arrow/scalar.h	`71.42% <33.33%> (-16.08%)`	⬇️
cpp/src/arrow/dataset/filter.cc	`46.53% <46.53%> (ø)`
cpp/src/arrow/scalar.cc	`65.21% <77.77%> (+1.88%)`	⬆️
cpp/src/arrow/record_batch.cc	`94.07% <83.33%> (+5.03%)`	⬆️
cpp/src/arrow/dataset/filter_test.cc	`95.87% <95.87%> (ø)`
cpp/src/arrow/dataset/filter.h	`96.49% <96.49%> (ø)`
cpp/src/arrow/filesystem/s3_internal.h	`90.74% <0%> (-3.71%)`	⬇️
cpp/src/plasma/thirdparty/ae/ae.c	`70.75% <0%> (-0.95%)`	⬇️
... and 203 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 92f16e3...fda4742. Read the comment docs.

fsaintjacques · 2019-09-11T16:48:13Z

This is a +1 for me, since this is still experimental, I say we merge it.

Adds the Expression class which is used to represent an arbitrarily complex filter expression. Expressions can be constructed using factory functions, for example: ```c++ and_( equal(field_ref("a"), scalar<int16_t>(5)), // column 'a' is equal to 5 greater(field_ref("b"), scalar<double>(0.0)) // column 'b' is greater than 0.0 ) ``` Operator overloads are also provided, so the above could also be written as ```c++ "a"_ == int16_t(5) and "b"_ > 0.0 ``` These can be executed against a single record batch (using the `arrow::compute::` kernels). Additionally, expressions may be simplified or even elided given partition information. For example, given a partition where column 'a' is equal to 5 the above query could be simplified to `"b"_ > 0.0` (since the condition on column 'a' is satisfied by the entire partition) and given a partition where column 'b' is between -1.0 and 0.0 the query simplifies to `false` (since no record in the partition will satisfy the condition on column 'b'). This can be used to support arbitrary partitioning schemes and do the least kernel work possible on each record batch. Closes apache#5157 from bkietz/6243-Implement-basic-Filter-ex and squashes the following commits: fda4742 <Benjamin Kietzman> give MSVC a little help to avoid instantiating impossible constructors 539c287 <Benjamin Kietzman> rename fieldRef to field_ref, comments 9494bab <Benjamin Kietzman> refactor And, Or to binary 0e366bb <Benjamin Kietzman> rename all/any to and/or ca155c6 <Benjamin Kietzman> add explicit std::move, msvc doesn't like defining operator and 7c01f76 <Benjamin Kietzman> construct correct scalartype 3ac299b <Benjamin Kietzman> use strongly typed nulls cab17b1 <Benjamin Kietzman> amend doccomments 46fa3fb <Benjamin Kietzman> add Expression::Validate implementations d54ca06 <Benjamin Kietzman> Expressions evaluate to Datums 24323fc <Benjamin Kietzman> implement NotExpression::ToString d7f3ed2 <Benjamin Kietzman> simplify Expression::Equals cb56906 <Benjamin Kietzman> rename FieldRef -> Field, and_ -> all, or_ -> any, add comments to ExpressionType 5bae590 <Benjamin Kietzman> re-enable Invert f9e0c08 <Benjamin Kietzman> remove unused Empty() method e9af935 <Benjamin Kietzman> lint fixes 5899067 <Benjamin Kietzman> break OperatorExpression into multiple classes 02d94a6 <Benjamin Kietzman> use explicit enumeration of comparison results 523186d <Benjamin Kietzman> add support for evaluation of trivial expressions, tests 60d9e08 <Benjamin Kietzman> fix factory linkage, factory fns deal in shared_ptrs exclusively 55aa452 <Benjamin Kietzman> implement more robust null handling 3d355ff <Benjamin Kietzman> break up assumption logic, add function expression factories 3499fd2 <Benjamin Kietzman> add an expression simplification test 06b25be <Benjamin Kietzman> move expression evaluation to a free function a67b330 <Benjamin Kietzman> add comments, more tests, simplify operator overloads 9282284 <Benjamin Kietzman> simplify filter testing 6ccd636 <Benjamin Kietzman> add execution of filter expressions using compute kernels f316578 <Benjamin Kietzman> add basic filter expressions Authored-by: Benjamin Kietzman <bengilgit@gmail.com> Signed-off-by: Neal Richardson <neal.p.richardson@gmail.com>

The condition is an expression guaranteed to evaluate true for all records in a DataSource. This provides some predicate push down funcitonality: DataSources whose condition precludes a filter expression will not yield any fragments (since those fragments would be filtered out anyway). This patch does not implement evaluation of filter expressions against an in memory RecordBatch. It makes a half hearted attempt at API compatibility with #5157 which does implement this. Closes #5221 from bkietz/6244-Implement-Partition-DataS and squashes the following commits: 142cc7b <Benjamin Kietzman> explicit move for Result returning functions 13b5948 <Benjamin Kietzman> add comment on motivation for type erasure approach 42e2ad3 <Benjamin Kietzman> clang-format a9e5d7a <Benjamin Kietzman> bludgeon MSVC linker error with __forceinline e8c8cd6 <Benjamin Kietzman> AssumePartitionExpression's inout argument is confusing 48b349f <Benjamin Kietzman> move overridable GetFragments to protected GetFragmentsImpl 19f26a0 <Benjamin Kietzman> DataSource::assume -> bool, remove partition_expr mutator a651c65 <Benjamin Kietzman> rename DataSource::condition to partition_expression 949fa7a <Benjamin Kietzman> provide basic predicate pushdown to datasources 955cb56 <Benjamin Kietzman> flesh out shim Expression class b1a6c54 <Benjamin Kietzman> remove unused FileSystemBasedDataSource::options_ 4f5a8bc <Benjamin Kietzman> rename partitionner_ d66f159 <Benjamin Kietzman> add an Expression stub Authored-by: Benjamin Kietzman <bengilgit@gmail.com> Signed-off-by: François Saint-Jacques <fsaintjacques@gmail.com>

bkietz force-pushed the 6243-Implement-basic-Filter-ex branch from c214abe to 5541946 Compare August 25, 2019 17:03

bkietz marked this pull request as ready for review August 26, 2019 14:49

bkietz force-pushed the 6243-Implement-basic-Filter-ex branch from 5541946 to 18cae8d Compare August 26, 2019 15:34

pitrou reviewed Aug 27, 2019

View reviewed changes

fsaintjacques changed the title ~~ARROW-6243: [C++] implement basic filter ex~~ ARROW-6243: [C++][Dataset] Filter expressions Aug 27, 2019

wesm reviewed Aug 27, 2019

View reviewed changes

bkietz mentioned this pull request Aug 28, 2019

ARROW-6244: [C++][Dataset] Add partition key to DataSource interface #5221

Closed

bkietz force-pushed the 6243-Implement-basic-Filter-ex branch 2 times, most recently from 1a110b3 to 5416690 Compare September 2, 2019 00:05

bkietz force-pushed the 6243-Implement-basic-Filter-ex branch from bac51ba to 82da536 Compare September 6, 2019 16:10

bkietz added 15 commits September 9, 2019 09:21

add basic filter expressions

f316578

add execution of filter expressions using compute kernels

6ccd636

simplify filter testing

9282284

add comments, more tests, simplify operator overloads

a67b330

move expression evaluation to a free function

06b25be

add an expression simplification test

3499fd2

break up assumption logic, add function expression factories

3d355ff

implement more robust null handling

55aa452

fix factory linkage, factory fns deal in shared_ptrs exclusively

60d9e08

add support for evaluation of trivial expressions, tests

523186d

use explicit enumeration of comparison results

02d94a6

break OperatorExpression into multiple classes

5899067

lint fixes

e9af935

remove unused Empty() method

f9e0c08

re-enable Invert

5bae590

bkietz added 12 commits September 9, 2019 09:21

rename FieldRef -> Field, and_ -> all, or_ -> any, add comments to Ex…

cb56906

…pressionType

simplify Expression::Equals

d7f3ed2

implement NotExpression::ToString

24323fc

Expressions evaluate to Datums

d54ca06

add Expression::Validate implementations

46fa3fb

amend doccomments

cab17b1

use strongly typed nulls

3ac299b

construct correct scalartype

7c01f76

add explicit std::move, msvc doesn't like defining operator and

ca155c6

rename all/any to and/or

0e366bb

refactor And, Or to binary

9494bab

rename fieldRef to field_ref, comments

539c287

bkietz force-pushed the 6243-Implement-basic-Filter-ex branch from 82da536 to 539c287 Compare September 9, 2019 13:25

give MSVC a little help to avoid instantiating impossible constructors

fda4742

fsaintjacques self-assigned this Sep 9, 2019

fsaintjacques approved these changes Sep 11, 2019

View reviewed changes

nealrichardson closed this in d2b6d11 Sep 11, 2019

bkietz deleted the 6243-Implement-basic-Filter-ex branch February 25, 2021 16:45

asfimport mentioned this pull request Aug 6, 2020

[C++] Implement basic Filter expression classes #16847

Closed

ARROW-6243: [C++][Dataset] Filter expressions #5157

ARROW-6243: [C++][Dataset] Filter expressions #5157

Uh oh!

Conversation

bkietz commented Aug 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou Aug 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkietz Aug 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkietz Aug 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wesm commented Aug 27, 2019

Uh oh!

fsaintjacques commented Sep 4, 2019

Uh oh!

bkietz commented Sep 5, 2019

Uh oh!

bkietz commented Sep 6, 2019

Uh oh!

kou commented Sep 6, 2019

Uh oh!

codecov-io commented Sep 9, 2019

Codecov Report

Uh oh!

fsaintjacques commented Sep 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bkietz commented Aug 21, 2019 •

edited

Loading

pitrou Aug 27, 2019 •

edited

Loading

bkietz Aug 29, 2019 •

edited

Loading

bkietz Aug 27, 2019 •

edited

Loading