ARROW-10663: [C++] Fix is_in and index_in behaviour #9164

pitrou · 2021-01-11T19:17:43Z

Reject duplicates in SetLookupOptions::value_set, because otherwise the indices returned by index_in would be relative to a deduplicated value_set.
Honour SetLookup::skip_nulls in is_in.
Revamp tests.

github-actions · 2021-01-11T20:50:12Z

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

* Reject duplicates in SetLookupOptions::value_set, because otherwise the indices returned by index_in would be relative to a deduplicated value_set. * Honour SetLookup::skip_nulls in is_in. * Revamp tests.

jorisvandenbossche

Apparently, in the Python bindings we used skip_null instead of skip_nulls, maybe could take the opportunity to fixup that as well

jorisvandenbossche · 2021-01-12T10:01:47Z

cpp/src/arrow/compute/api_scalar.h

@@ -290,17 +290,19 @@ Result<Datum> KleeneAndNot(const Datum& left, const Datum& right,
 /// \brief IsIn returns true for each element of `values` that is contained in
 /// `value_set`
 ///
-/// If null occurs in left, if null count in right is not 0,
-/// it returns true, else returns null.
+/// Behaviour of nulls is governed by SetLookupOptions::skip_nulls.


Could mention here that the default is skip_nulls=False (or in the docstring of SetLookupOptions)?

Also, could maybe add a similar sentence "Behaviour of nulls is governed by SetLookupOptions::skip_nulls" to the is_in_doc ?

jorisvandenbossche · 2021-01-12T13:52:48Z

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

@@ -272,10 +278,9 @@ struct IsInVisitor {

  Status Visit(const DataType&) {
    const auto& state = checked_cast<const SetLookupState<NullType>&>(*ctx->state());
+    // XXX should skip_nulls be taken into account?


It's a bit strange corner case, but for consistency probably yes?

Right now for null array the skip_nulls argument has no effect (using this PR):

In [27]: pc.is_in(pa.array([None]), value_set=pa.array([None]), skip_null=False) Out[27]: <pyarrow.lib.BooleanArray object at 0x7fac1d800880> [ true ] In [28]: pc.is_in(pa.array([None]), value_set=pa.array([None]), skip_null=True) Out[28]: <pyarrow.lib.BooleanArray object at 0x7fac1d800ca0> [ true ]

while if you add a non-null type to the array creation, but use the same values, you get a different result:

In [30]: pc.is_in(pa.array([None], type="int64"), value_set=pa.array([None], type="int64"), skip_null=False) Out[30]: <pyarrow.lib.BooleanArray object at 0x7fac1d82f880> [ true ] In [31]: pc.is_in(pa.array([None], type="int64"), value_set=pa.array([None], type="int64"), skip_null=True) Out[31]: <pyarrow.lib.BooleanArray object at 0x7fac1d7c2040> [ false ]

Good point.

jorisvandenbossche · 2021-01-12T13:56:53Z

Thanks for the PR!
One additional thing I am wondering, while testing it out, would we ever want a behaviour where a null in the input gives a null in the output? Right now it's only possible to get false (if there is no null in the value_set, or if skip_nulls=True) or true (if there is a null in the value_set and skip_nulls=False).

So something like isin([1, 2, null], value_set=[1, 3]) -> [true, false, null] (instead of true, false, false)

If you see "isin" as a shortcut to write multiple equality comparisons (isin(input, value_set=[1, 3, ...] -> (input == 1) | (input == 3) | ...), then you would get such behaviour.
But so it's a bit the question whether for "isin" we use "equality" semantics or "identity/lookup" semantics for nulls (and given it's now called "SetLookup" in the function names, we clearly go for the second, but I am not fully sure which of the two are most useful / expected in practice).

pitrou · 2021-01-12T14:07:36Z

@jorisvandenbossche I have no idea. Perhaps @bkietz or @michalursa can share their opinion.

jorisvandenbossche · 2021-01-12T14:15:56Z

Looking at the behavior of %in% in R (cc @nealrichardson), there NA's also get matched (eg c(1, 2, NA) %in% c(1, 3) gives true,false,false and c(1, 2, NA) %in% c(1, 3, NA) gives true,false,true), so that is consistent with the behaviour we have in Arrow right now.

The SQL IN operator does not seem to match Nulls, because there it is a short-hand for multiple comparisons. But, in practice, you can only use this (as far as I know, only limited SQL knowledge) in a WHERE clause. So whether the Null in the column gives False or Null doesn't matter much, because in both cases the row does not get preserved in a WHERE filter.

jorisvandenbossche · 2021-01-12T14:16:49Z

(BTW, since this is existing behaviour, and this PR doesn't change that, that should certainly not hold up this PR, and should probably move the discussion to a JIRA)

bkietz

LGTM

Thanks for rewriting the tests

bkietz · 2021-01-12T17:38:18Z

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

+      RETURN_NOT_OK(AddArrayValueSet(*options.value_set.array()));
+    } else if (options.value_set.kind() == Datum::CHUNKED_ARRAY) {
+      const ChunkedArray& value_set = *options.value_set.chunked_array();
+      for (const std::shared_ptr<Array>& chunk : value_set.chunks()) {


We have a lot of code like this, maybe later we should add Datum::chunks() so we can write

if (!options.values_set.is_arraylike()) { return Status::Invalid("value_set should be an array or chunked array"); } for (const std::shared_ptr<ArrayData>& chunk : options.value_set.chunks()) { RETURN_NOT_OK(AddArrayValueSet(*chunk->data())); }

If we're not too bothered by the cost of a temporary vector then it may be nice indeed.

pitrou · 2021-01-12T17:49:42Z

The CI failure are spurious. Green Travis-CI build: https://travis-ci.com/github/pitrou/arrow/builds/212852090

pitrou requested a review from bkietz January 11, 2021 19:17

pitrou force-pushed the ARROW-10663-set-lookup-null branch from 2110abd to ba20782 Compare January 11, 2021 19:33

github-actions bot added the Component: C++ label Jan 11, 2021

ARROW-10663: [C++] Fix is_in and index_in behaviour

4351477

* Reject duplicates in SetLookupOptions::value_set, because otherwise the indices returned by index_in would be relative to a deduplicated value_set. * Honour SetLookup::skip_nulls in is_in. * Revamp tests.

jorisvandenbossche reviewed Jan 12, 2021

View reviewed changes

Fix C/GLib and Ruby tests

447c1c3

pitrou force-pushed the ARROW-10663-set-lookup-null branch from ba20782 to 447c1c3 Compare January 12, 2021 14:06

github-actions bot added Component: GLib Component: Ruby labels Jan 12, 2021

pitrou added 2 commits January 12, 2021 16:52

Honor skip_nulls with null type

64fc843

Improve function docs

dd66745

jorisvandenbossche approved these changes Jan 12, 2021

View reviewed changes

github-actions bot added the Component: Python label Jan 12, 2021

bkietz approved these changes Jan 12, 2021

View reviewed changes

pitrou closed this in 75e6060 Jan 12, 2021

pitrou deleted the ARROW-10663-set-lookup-null branch January 12, 2021 17:50

asfimport mentioned this pull request Jan 12, 2021

[C++/Doc] The IsIn kernel ignores the skip_nulls option of SetLookupOptions #26618

Closed

ARROW-10663: [C++] Fix is_in and index_in behaviour #9164

ARROW-10663: [C++] Fix is_in and index_in behaviour #9164

Uh oh!

Conversation

pitrou commented Jan 11, 2021

Uh oh!

github-actions bot commented Jan 11, 2021

Uh oh!

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Uh oh!

jorisvandenbossche Jan 12, 2021

Choose a reason for hiding this comment

Uh oh!

jorisvandenbossche Jan 12, 2021

Choose a reason for hiding this comment

Uh oh!

jorisvandenbossche Jan 12, 2021

Choose a reason for hiding this comment

Uh oh!

pitrou Jan 12, 2021

Choose a reason for hiding this comment

Uh oh!

jorisvandenbossche commented Jan 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pitrou commented Jan 12, 2021

Uh oh!

jorisvandenbossche commented Jan 12, 2021

Uh oh!

jorisvandenbossche commented Jan 12, 2021

Uh oh!

bkietz left a comment

Choose a reason for hiding this comment

Uh oh!

bkietz Jan 12, 2021

Choose a reason for hiding this comment

Uh oh!

pitrou Jan 12, 2021

Choose a reason for hiding this comment

Uh oh!

pitrou commented Jan 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jorisvandenbossche commented Jan 12, 2021 •

edited

Loading