GH-36420: [C++] Add An Enum Option For SetLookup Options

[R-JunmingChen]

Rationale for this change

As #36420 says, we want add an sql-compatible is_in variant, which has a different logic handling Null. After a dicussion with @ianmcook and @bkietz, we decide to support an enum option null_matching_behavior for SetLookup, which actually adds two semantics of null handling for is_in and doesn't add an new behavior for index_in.

The enum option null_matching_behavior will replace skip_nulls in the future.

What changes are included in this PR?

Add an enum parameter null_matching_behavior for SetLookupOptions.

Are these changes tested?

Two kinds of tests are implemented

• Replace default parameter with null_matching_behavior instead of skip_nulls for is_in and index_in tests
• Add tests for NullMatchingBehavior::EMIT_NULL and NullMatchingBehavior::INCONCLUSIVE for is_in

Besides, since the skip_nulls is not deprecated now, I still preserve the old tests with skip_nulls. When the skip_nulls is totally deprecated, we can replace the test parameter skip_nulls=false with null_matching_behavior=MATCH and skip_nulls=true with null_matching_behavior=SKIP for these old tests.

Are there any user-facing changes?

No. Currently we support backward compatibility. In the future, we plan to replace skip_nulls with null_matching_behavior completely.

• Closes: [C++] Add an sql-compatible is_in variant #36420

[pitrou]

Why not improve SetLookupOptions instead to allow selecting the desired semantics?

[ianmcook]

Why not improve SetLookupOptions instead to allow selecting the desired semantics?

+1. When possible, we typically prefer to add new options to existing functions instead of adding new functions.

[R-JunmingChen]

Why not improve SetLookupOptions instead to allow selecting the desired semantics?

Hi, here are my two considerations:

1. The null_handling of all the kernels of is_in is NullHandling::OUTPUT_NOT_NULL. While a sql-compatible is_in can have NULL outputs.
2. It's a bit weird to add an option like sql_compatible specific for is_in in SetLookupOptions. Since SetLookupOptions is used by is_in and index_in.

But these two considerations are not strict obstacles for using an option instead. For 1, we can set null_handling to NullHandling::COMPUTED_PREALLOCATE and init the null bitmap to true in exec for un-sql-compatible is_in. And the new option could be named as sql_compatible, which is only used by is_in.

If you think the solution is ok or have a better solution, I will change code to an new option version soon.

[R-JunmingChen]

Since no further feedback, I plan to transfer to an new option version as I have commented before. Hi, @pitrou @ianmcook Do you think it's ok?

[ianmcook]

@R-JunmingChen see my comments at #36420 (comment).

Based on that, here is what I recommend:

In SetLookupOptions, replace the bool option skip_nulls with an enum option:

enum NullMatchingBehavior
How to match null values.
Values:

• enumerator MATCH
  Any null in value_set is successfully matched to null values in the lookup set.
• enumerator SKIP
  Any null in value_set is ignored and produces a null (IndexIn) or false (IsIn) in the output.
• enumerator EMIT_NULL
  Any null in value_set produces a null in the output.
• enumerator INCONCLUSIVE
  Any null in value_set produces a null in the output. Any null in the lookup set is treated as an unknown value that cannot conclusively match values in value_set and any value in value_set that cannot be conclusively matched produces a null in the output. This is consistent with the behavior of IN in SQL.

Alternatively we could name it UNKNOWN instead of INCONCLUSIVE if people think that is clearer.

For backward compatibility:

• Default to MATCH (consistent with the current default skip_nulls = false)
• Continue to accept the existing boolean argument skip_nulls and respect it if the user does not use the new enum-based option. false translates to MATCH and true translates to SKIP.

I believe this should work with both is_in and index_in.

@westonpace does this look OK to you?

[R-JunmingChen]

Hi, @ianmcook. I think it's a good solution to use an enum option, but there are still somethings I think should be discussed.

1. Why do we want to support EMIT_NULL? MATCH, SKIP and INCONCLUSIVE seem sufficient.
2. Since we want support skip_nulls for backward compatibility, we need an enumerator UNSET for NullMatchingBehavior, which means we only use skip_nulls instead of NullMatchingBehavior, and make it default for the enum option. Cause we can't translate when user set the skip_nulls via the way like setLookupOptions.skip_nulls = false.

[ianmcook]

Thanks @R-JunmingChen

1. Why do we want to support EMIT_NULL? MATCH, SKIP and INCONCLUSIVE seem sufficient.

It is nice to have EMIT_NULL for completeness. There are some cases where the user might want that behavior—for example, to match the behavior of DuckDB's list_contains function like I describe here: #36420 (comment)

2. Since we want support skip_nulls for backward compatibility, we need an enumerator UNSET for NullMatchingBehavior, which means we only use skip_nulls instead of NullMatchingBehavior, and make it default for the enum option. Cause we can't translate when user set the skip_nulls via the way like setLookupOptions.skip_nulls = false.

I think it is possible to define 2 explicit public functions of the class SetLookupOptions:

1. SetLookupOptions(Datum value_set, bool skip_nulls = false) (same as currently)
2. SetLookupOptions(Datum value_set, NullMatchingBehavior null_matching_behavior = NullMatchingBehavior::MATCH)

I think that will give backward compatibility.

[R-JunmingChen]

I think it is possible to define 2 explicit public functions of the class SetLookupOptions:

SetLookupOptions(Datum value_set, bool skip_nulls = false) (same as currently)
SetLookupOptions(Datum value_set, NullMatchingBehavior null_matching_behavior = NullMatchingBehavior::MATCH)

Hi @ianmcook, do you mean we remove the member variable skip_nulls and make it a parameter in constructor function and then translate the skip_nulls to NullMatchingBehavior ?

I think this way will affect some users who set skip_nulls directly via member variable. Does it ok?

If we don't remove member variable `skip_nulls, the translation problem still exists. For example:

SetLookupOptions option(value_set);
option.skip_nulls = true;

[ianmcook]

I think it is possible to define 2 explicit public functions of the class SetLookupOptions:
SetLookupOptions(Datum value_set, bool skip_nulls = false) (same as currently)
SetLookupOptions(Datum value_set, NullMatchingBehavior null_matching_behavior = NullMatchingBehavior::MATCH)

Hi @ianmcook, do you mean we remove the member variable skip_nulls and make it a parameter in constructor function and then translate the skip_nulls to NullMatchingBehavior ?

I think this way will affect some users who set skip_nulls directly via member variable. Does it ok?

Yes, I think this is OK. We can mark this as a breaking change if needed, but I think it should not affect most users. @westonpace or @bkietz can you please confirm?

[bkietz]

I would not expect there to be many direct users of the skip_nulls data member, but disentangling the corresponding cython will be annoying. Since it's a breaking change, we could compromise with:

/// Options for IsIn and IndexIn functions
class ARROW_EXPORT SetLookupOptions : public FunctionOptions {
 public:
  enum NullMatchingBehavior { MATCH, SKIP, EMIT_NULL, INCONCLUSIVE };

  explicit SetLookupOptions(Datum value_set, NullMatchingBehavior = MATCH);
  explicit SetLookupOptions(Datum value_set, bool skip_nulls);
  SetLookupOptions();
  static constexpr char const kTypeName[] = "SetLookupOptions";

  /// The set of values to look up input values into.
  Datum value_set;

  // will be overridden by skip_nulls if that is explicitly assigned
  NullMatchingBehavior null_matching_behavior;

  // DEPRECATED(use null_matching_behavior instead)
  std::optional<bool> skip_nulls;
};

with this, opts.skip_nulls = false will still work and operate as expected and (I think) cython changes will be minimal until we're ready fully to accommodate null_matching_behavior in python.

[R-JunmingChen]

I will implement this version ASAP. Let me turn this PR to a draft temporarily

[ianmcook]

This looks good to me in terms of its API and docs. I defer to @bkietz to give a technical approval.

It looks like Python tests are failing. Can you troubleshoot that please? Thanks

[bkietz]

LGTM.

CI failures seem unrelated:

Dev failure looks like #37803

Appveyor failure looks like #37790

I've restarted the python test (which seemed to fail inside pip). If that passes I think we can merge

[ianmcook]

@ursabot please benchmark

[ursabot]

Benchmark runs are scheduled for commit 38c1a04. Watch https://buildkite.com/apache-arrow and https://conbench.ursa.dev for updates. A comment will be posted here when the runs are complete.

[conbench-apache-arrow]

Thanks for your patience. Conbench analyzed the 0 benchmarking runs that have been run so far on PR commit 38c1a04.

None of the specified runs were found on the Conbench server.

The full Conbench report has more details.

[R-JunmingChen]

The Python failure looks like #37803 too. The error in its screenshot is similar with what we fails in our Python CI.

[ianmcook]

I'm not sure why Conbench isn't working as expected here. The benchmarks should run after this merges and we can check for regressions there.

[austin3dickey]

I'm not sure why Conbench isn't working as expected here.

Sorry, this looks to be a bug with the "ursabot please benchmark" workflow. Will fix today.

voltrondata-labs/benchmarks#150

[austin3dickey]

That should be fixed. If you push an empty commit (voltrondata-labs/arrow-benchmarks-ci#111) and then re-run the command, you can try benchmarks again. Else, yeah, they will be run post-merge.

[ianmcook]

@bkietz if you want to go ahead and merge when you're comfortable with this, we can just look at the benchmarks post-merge

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 772a01c.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about possible false positives for unstable benchmarks that are known to sometimes produce them.