You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "R-JunmingChen (via GitHub)" <gi...@apache.org> on 2023/07/18 01:17:22 UTC
[GitHub] [arrow] R-JunmingChen opened a new pull request, #36739: GH-36420: [C++] Add an sql-compatible In Function
R-JunmingChen opened a new pull request, #36739:
URL: https://github.com/apache/arrow/pull/36739
### Rationale for this change
We want add an sql-compatible is_in variant, which has a different logic handling Null.
### What changes are included in this PR?
Add a new Function `In` in scalar_set_look_up.
### Are these changes tested?
No, will add ASAP.
### Are there any user-facing changes?
No.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] R-JunmingChen commented on a diff in pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "R-JunmingChen (via GitHub)" <gi...@apache.org>.
R-JunmingChen commented on code in PR #36739:
URL: https://github.com/apache/arrow/pull/36739#discussion_r1309503632
##########
cpp/src/arrow/compute/api_scalar.h:
##########
@@ -268,19 +268,43 @@ class ARROW_EXPORT ExtractRegexOptions : public FunctionOptions {
/// Options for IsIn and IndexIn functions
class ARROW_EXPORT SetLookupOptions : public FunctionOptions {
public:
- explicit SetLookupOptions(Datum value_set, bool skip_nulls = false);
+ enum NullMatchingBehavior { MATCH, SKIP, EMIT_NULL, INCONCLUSIVE };
+
+ explicit SetLookupOptions(Datum value_set, NullMatchingBehavior = MATCH);
+ // DEPRECATED(will be removed after removing of skip_nulls)
Review Comment:
Let's replace
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] conbench-apache-arrow[bot] commented on pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #36739:
URL: https://github.com/apache/arrow/pull/36739#issuecomment-1728578870
Thanks for your patience. Conbench analyzed the 0 benchmarking runs that have been run so far on PR commit 38c1a04817ffa20e3baef1ed6312d7cf6501fc68.
None of the specified runs were found on the Conbench server.
The [full Conbench report](https://github.com/apache/arrow/runs/16984761965) has more details.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] R-JunmingChen commented on pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "R-JunmingChen (via GitHub)" <gi...@apache.org>.
R-JunmingChen commented on PR #36739:
URL: https://github.com/apache/arrow/pull/36739#issuecomment-1728603890
The Python failure looks like #37803.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] R-JunmingChen commented on a diff in pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "R-JunmingChen (via GitHub)" <gi...@apache.org>.
R-JunmingChen commented on code in PR #36739:
URL: https://github.com/apache/arrow/pull/36739#discussion_r1309718406
##########
cpp/src/arrow/compute/api_scalar.cc:
##########
@@ -540,8 +564,29 @@ constexpr char RoundToMultipleOptions::kTypeName[];
SetLookupOptions::SetLookupOptions(Datum value_set, bool skip_nulls)
: FunctionOptions(internal::kSetLookupOptionsType),
value_set(std::move(value_set)),
- skip_nulls(skip_nulls) {}
-SetLookupOptions::SetLookupOptions() : SetLookupOptions({}, false) {}
+ skip_nulls(skip_nulls) {
+ if (skip_nulls) {
+ this->null_matching_behavior = SetLookupOptions::SKIP;
+ } else {
+ this->null_matching_behavior = SetLookupOptions::MATCH;
+ }
Review Comment:
It's necessary that `null_matching_behavior` has a initialized value. Without initialization, `null_matching_behavior` can't be deserialized in the expression using `is_in`/`index_in`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] R-JunmingChen commented on a diff in pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "R-JunmingChen (via GitHub)" <gi...@apache.org>.
R-JunmingChen commented on code in PR #36739:
URL: https://github.com/apache/arrow/pull/36739#discussion_r1309718406
##########
cpp/src/arrow/compute/api_scalar.cc:
##########
@@ -540,8 +564,29 @@ constexpr char RoundToMultipleOptions::kTypeName[];
SetLookupOptions::SetLookupOptions(Datum value_set, bool skip_nulls)
: FunctionOptions(internal::kSetLookupOptionsType),
value_set(std::move(value_set)),
- skip_nulls(skip_nulls) {}
-SetLookupOptions::SetLookupOptions() : SetLookupOptions({}, false) {}
+ skip_nulls(skip_nulls) {
+ if (skip_nulls) {
+ this->null_matching_behavior = SetLookupOptions::SKIP;
+ } else {
+ this->null_matching_behavior = SetLookupOptions::MATCH;
+ }
Review Comment:
After testing, the answer is changed.
It's necessary that `null_matching_behavior` has a initialized value. Without initialization, the value of `null_matching_behavior` is `Invaild`, which can't be deserialized in the expression using `is_in`/`index_in`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a diff in pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #36739:
URL: https://github.com/apache/arrow/pull/36739#discussion_r1307644347
##########
cpp/src/arrow/compute/api_scalar.cc:
##########
@@ -540,8 +564,29 @@ constexpr char RoundToMultipleOptions::kTypeName[];
SetLookupOptions::SetLookupOptions(Datum value_set, bool skip_nulls)
: FunctionOptions(internal::kSetLookupOptionsType),
value_set(std::move(value_set)),
- skip_nulls(skip_nulls) {}
-SetLookupOptions::SetLookupOptions() : SetLookupOptions({}, false) {}
+ skip_nulls(skip_nulls) {
+ if (skip_nulls) {
+ this->null_matching_behavior = SetLookupOptions::SKIP;
+ } else {
+ this->null_matching_behavior = SetLookupOptions::MATCH;
+ }
Review Comment:
Is this coercion necessary since we're using `GetNullMatchingBehavior()`?
##########
cpp/src/arrow/compute/api_scalar.h:
##########
@@ -268,19 +268,43 @@ class ARROW_EXPORT ExtractRegexOptions : public FunctionOptions {
/// Options for IsIn and IndexIn functions
class ARROW_EXPORT SetLookupOptions : public FunctionOptions {
public:
- explicit SetLookupOptions(Datum value_set, bool skip_nulls = false);
+ enum NullMatchingBehavior { MATCH, SKIP, EMIT_NULL, INCONCLUSIVE };
+
+ explicit SetLookupOptions(Datum value_set, NullMatchingBehavior = MATCH);
+ // DEPRECATED(will be removed after removing of skip_nulls)
+ 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;
+
+ /// How to match null values.
+ ///
+ /// Match, any null in `value_set` is successfully matched in
+ /// the input.
+ /// SKIP, any null in `value_set` is ignored and nulls in the input
+ /// produce null (IndexIn) or false (IsIn) values in the output.
+ /// EMIT_NULL, any null in `value_set` is ignored and nulls in the
+ /// input produce null (IndexIn and IsIn) values in the output.
+ /// INCONCLUSIVE, null values are regard as unknown values, which is
+ /// sql-compatible. nulls in the input produce null (IndexIn and IsIn)
+ /// values in the output. Besides, if `value_set` contains a null,
+ /// non-null unmatched values in the input also produce null values
+ /// (IndexIn and IsIn) in the output.
+ NullMatchingBehavior null_matching_behavior;
+
+ // DEPRECATED(will be removed after removing of skip_nulls)
+ NullMatchingBehavior getNullMatchingBehavior() const;
Review Comment:
Nit: this should be named
```suggestion
NullMatchingBehavior GetNullMatchingBehavior() const;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] R-JunmingChen commented on a diff in pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "R-JunmingChen (via GitHub)" <gi...@apache.org>.
R-JunmingChen commented on code in PR #36739:
URL: https://github.com/apache/arrow/pull/36739#discussion_r1330866322
##########
cpp/src/arrow/compute/kernels/scalar_set_lookup.cc:
##########
@@ -274,10 +280,12 @@ struct IndexInVisitor {
const auto& state = checked_cast<const SetLookupState<NullType>&>(*ctx->state());
Review Comment:
Sure, I have modified to that. Besides, for `Status Visit(const DataType& type)`, I add a return `Status::NotImplemented("IndexIn has no implementation with value type ", type)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz merged pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz merged PR #36739:
URL: https://github.com/apache/arrow/pull/36739
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] R-JunmingChen commented on a diff in pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "R-JunmingChen (via GitHub)" <gi...@apache.org>.
R-JunmingChen commented on code in PR #36739:
URL: https://github.com/apache/arrow/pull/36739#discussion_r1309718406
##########
cpp/src/arrow/compute/api_scalar.cc:
##########
@@ -540,8 +564,29 @@ constexpr char RoundToMultipleOptions::kTypeName[];
SetLookupOptions::SetLookupOptions(Datum value_set, bool skip_nulls)
: FunctionOptions(internal::kSetLookupOptionsType),
value_set(std::move(value_set)),
- skip_nulls(skip_nulls) {}
-SetLookupOptions::SetLookupOptions() : SetLookupOptions({}, false) {}
+ skip_nulls(skip_nulls) {
+ if (skip_nulls) {
+ this->null_matching_behavior = SetLookupOptions::SKIP;
+ } else {
+ this->null_matching_behavior = SetLookupOptions::MATCH;
+ }
Review Comment:
After testing, the answer is changed.
It's necessary that `null_matching_behavior` has a initialized value. Without an initialization, the value of `null_matching_behavior` is `Invaild`, which can't be deserialized in the expression using `is_in`/`index_in`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] R-JunmingChen commented on a diff in pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "R-JunmingChen (via GitHub)" <gi...@apache.org>.
R-JunmingChen commented on code in PR #36739:
URL: https://github.com/apache/arrow/pull/36739#discussion_r1308984756
##########
cpp/src/arrow/compute/api_scalar.cc:
##########
@@ -540,8 +564,29 @@ constexpr char RoundToMultipleOptions::kTypeName[];
SetLookupOptions::SetLookupOptions(Datum value_set, bool skip_nulls)
: FunctionOptions(internal::kSetLookupOptionsType),
value_set(std::move(value_set)),
- skip_nulls(skip_nulls) {}
-SetLookupOptions::SetLookupOptions() : SetLookupOptions({}, false) {}
+ skip_nulls(skip_nulls) {
+ if (skip_nulls) {
+ this->null_matching_behavior = SetLookupOptions::SKIP;
+ } else {
+ this->null_matching_behavior = SetLookupOptions::MATCH;
+ }
Review Comment:
Indeed, we can remove it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on pull request #36739: GH-36420: [C++] Add an sql-compatible In Function
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on PR #36739:
URL: https://github.com/apache/arrow/pull/36739#issuecomment-1680796585
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:
```c++
/// 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] R-JunmingChen commented on pull request #36739: GH-36420: [C++] Add an sql-compatible In Function
Posted by "R-JunmingChen (via GitHub)" <gi...@apache.org>.
R-JunmingChen commented on PR #36739:
URL: https://github.com/apache/arrow/pull/36739#issuecomment-1683239205
I will implement this version ASAP. Let me turn this PR to draft temporarily
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] R-JunmingChen commented on a diff in pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "R-JunmingChen (via GitHub)" <gi...@apache.org>.
R-JunmingChen commented on code in PR #36739:
URL: https://github.com/apache/arrow/pull/36739#discussion_r1309595738
##########
cpp/src/arrow/compute/api_scalar.h:
##########
@@ -268,19 +268,43 @@ class ARROW_EXPORT ExtractRegexOptions : public FunctionOptions {
/// Options for IsIn and IndexIn functions
class ARROW_EXPORT SetLookupOptions : public FunctionOptions {
public:
- explicit SetLookupOptions(Datum value_set, bool skip_nulls = false);
+ enum NullMatchingBehavior { MATCH, SKIP, EMIT_NULL, INCONCLUSIVE };
+
+ explicit SetLookupOptions(Datum value_set, NullMatchingBehavior = MATCH);
+ // DEPRECATED(will be removed after removing of skip_nulls)
Review Comment:
I misunderstood, these functions would be removed later indeed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] R-JunmingChen commented on a diff in pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "R-JunmingChen (via GitHub)" <gi...@apache.org>.
R-JunmingChen commented on code in PR #36739:
URL: https://github.com/apache/arrow/pull/36739#discussion_r1309501211
##########
cpp/src/arrow/compute/api_scalar.h:
##########
@@ -268,19 +268,43 @@ class ARROW_EXPORT ExtractRegexOptions : public FunctionOptions {
/// Options for IsIn and IndexIn functions
class ARROW_EXPORT SetLookupOptions : public FunctionOptions {
public:
- explicit SetLookupOptions(Datum value_set, bool skip_nulls = false);
+ enum NullMatchingBehavior { MATCH, SKIP, EMIT_NULL, INCONCLUSIVE };
+
+ explicit SetLookupOptions(Datum value_set, NullMatchingBehavior = MATCH);
+ // DEPRECATED(will be removed after removing of skip_nulls)
+ 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;
+
+ /// How to match null values.
+ ///
+ /// Match, any null in `value_set` is successfully matched in
+ /// the input.
+ /// SKIP, any null in `value_set` is ignored and nulls in the input
+ /// produce null (IndexIn) or false (IsIn) values in the output.
+ /// EMIT_NULL, any null in `value_set` is ignored and nulls in the
+ /// input produce null (IndexIn and IsIn) values in the output.
+ /// INCONCLUSIVE, null values are regard as unknown values, which is
+ /// sql-compatible. nulls in the input produce null (IndexIn and IsIn)
+ /// values in the output. Besides, if `value_set` contains a null,
+ /// non-null unmatched values in the input also produce null values
+ /// (IndexIn and IsIn) in the output.
+ NullMatchingBehavior null_matching_behavior;
+
+ // DEPRECATED(will be removed after removing of skip_nulls)
Review Comment:
No, since `skip_nulls` can be set via its member variable like this.
```
SetLookupOptions option(value_set);
option.skip_nulls = true;
```
The `null_matching_behavior` set in the constructor has a possibility to contradict with `skip_nulls`. To support backward compatibility, we should assign `skip_nulls` with a higher priority.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a diff in pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on code in PR #36739:
URL: https://github.com/apache/arrow/pull/36739#discussion_r1330410134
##########
cpp/src/arrow/compute/api_scalar.h:
##########
@@ -268,19 +268,45 @@ class ARROW_EXPORT ExtractRegexOptions : public FunctionOptions {
/// Options for IsIn and IndexIn functions
class ARROW_EXPORT SetLookupOptions : public FunctionOptions {
public:
- explicit SetLookupOptions(Datum value_set, bool skip_nulls = false);
+ enum NullMatchingBehavior { MATCH, SKIP, EMIT_NULL, INCONCLUSIVE };
Review Comment:
To ensure that this renders cleanly at https://arrow.apache.org/docs/dev/cpp/api/compute.html, I think the descriptions should be moved into comment lines here, inside the enum definition.
Here is an example of this:
https://github.com/apache/arrow/blob/3b646ad4c2b826fe08b31d19e6435f73650bcb5e/cpp/src/arrow/compute/api_scalar.h#L171-L182
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] R-JunmingChen commented on a diff in pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "R-JunmingChen (via GitHub)" <gi...@apache.org>.
R-JunmingChen commented on code in PR #36739:
URL: https://github.com/apache/arrow/pull/36739#discussion_r1331830697
##########
cpp/src/arrow/compute/api_scalar.cc:
##########
@@ -344,7 +367,8 @@ static auto kRoundToMultipleOptionsType = GetFunctionOptionsType<RoundToMultiple
DataMember("round_mode", &RoundToMultipleOptions::round_mode));
static auto kSetLookupOptionsType = GetFunctionOptionsType<SetLookupOptions>(
DataMember("value_set", &SetLookupOptions::value_set),
- DataMember("skip_nulls", &SetLookupOptions::skip_nulls));
+ DataMember("skip_nulls", &SetLookupOptions::skip_nulls),
+ DataMember("null_matching_behavior", &SetLookupOptions::null_matching_behavior));
Review Comment:
It works!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a diff in pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #36739:
URL: https://github.com/apache/arrow/pull/36739#discussion_r1331547237
##########
cpp/src/arrow/compute/api_scalar.cc:
##########
@@ -344,7 +367,8 @@ static auto kRoundToMultipleOptionsType = GetFunctionOptionsType<RoundToMultiple
DataMember("round_mode", &RoundToMultipleOptions::round_mode));
static auto kSetLookupOptionsType = GetFunctionOptionsType<SetLookupOptions>(
DataMember("value_set", &SetLookupOptions::value_set),
- DataMember("skip_nulls", &SetLookupOptions::skip_nulls));
+ DataMember("skip_nulls", &SetLookupOptions::skip_nulls),
+ DataMember("null_matching_behavior", &SetLookupOptions::null_matching_behavior));
Review Comment:
Ah, sorry. I didn't correct the return type of `get()`
```c++
template <typename C, typename T>
struct CoercedDataMemberProperty {
using Class = C;
using Type = T;
constexpr Type get(const Class& obj) const { return (obj.*get_coerced_)(); }
void set(Class* obj, Type value) const { (*obj).*ptr_for_set_ = std::move(value); }
constexpr std::string_view name() const { return name_; }
std::string_view name_;
Type Class::*ptr_for_set_;
Type (Class::*get_coerced_)() const;
};
template <typename Class, typename Type>
constexpr CoercedDataMemberProperty<Class, Type> CoercedDataMember(std::string_view name,
Type Class::*ptr,
Type (Class::*get)()
const) {
return {name, ptr, get};
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] js8544 commented on a diff in pull request #36739: GH-36420: [C++] Add an sql-compatible In Function
Posted by "js8544 (via GitHub)" <gi...@apache.org>.
js8544 commented on code in PR #36739:
URL: https://github.com/apache/arrow/pull/36739#discussion_r1267819204
##########
cpp/src/arrow/compute/kernels/scalar_set_lookup.cc:
##########
@@ -380,6 +380,120 @@ Status ExecIndexIn(KernelContext* ctx, const ExecSpan& batch, ExecResult* out) {
}
// ----------------------------------------------------------------------
+// In writes the results into a preallocated boolean data bitmap
+struct InVisitor {
+ KernelContext* ctx;
+ const ArraySpan& data;
+ ArraySpan* out;
+ uint8_t* out_boolean_bitmap;
+ uint8_t* out_null_bitmap;
+
+ InVisitor(KernelContext* ctx, const ArraySpan& data, ArraySpan* out)
+ : ctx(ctx),
+ data(data),
+ out(out),
+ out_boolean_bitmap(out->buffers[1].data),
+ out_null_bitmap(out->buffers[0].data) {}
+
+ Status Visit(const DataType& type) {
+ DCHECK_EQ(type.id(), Type::NA);
+ // skip_nulls is ignored in sql-compatible In
+ bit_util::SetBitsTo(out_boolean_bitmap, out->offset, out->length, false);
+ bit_util::SetBitsTo(out_null_bitmap, out->offset, out->length, true);
+
+ return Status::OK();
+ }
+
+ template <typename Type>
+ Status ProcessIsIn(const SetLookupState<Type>& state, const ArraySpan& input) {
+ using T = typename GetViewType<Type>::T;
+ FirstTimeBitmapWriter writer_boolean(out_boolean_bitmap, out->offset, out->length);
+ FirstTimeBitmapWriter writer_null(out_null_bitmap, out->offset, out->length);
+ bool value_set_has_null = state.null_index != -1;
+ VisitArraySpanInline<Type>(
+ input,
+ [&](T v) {
+ if (state.lookup_table->Get(v) != -1) {
+ writer_boolean.Set();
+ writer_null.Clear();
Review Comment:
This should probably be `Set()`? null_bitmap[i] = True when the slot is valid, not when it's null.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #36739: GH-36420: [C++] Add an sql-compatible In Function
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #36739:
URL: https://github.com/apache/arrow/pull/36739#issuecomment-1641707024
Why not improve `SetLookupOptions` instead to allow selecting the desired semantics?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mapleFU commented on a diff in pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #36739:
URL: https://github.com/apache/arrow/pull/36739#discussion_r1309597251
##########
cpp/src/arrow/compute/api_scalar.h:
##########
@@ -268,19 +268,43 @@ class ARROW_EXPORT ExtractRegexOptions : public FunctionOptions {
/// Options for IsIn and IndexIn functions
class ARROW_EXPORT SetLookupOptions : public FunctionOptions {
public:
- explicit SetLookupOptions(Datum value_set, bool skip_nulls = false);
+ enum NullMatchingBehavior { MATCH, SKIP, EMIT_NULL, INCONCLUSIVE };
+
+ explicit SetLookupOptions(Datum value_set, NullMatchingBehavior = MATCH);
+ // DEPRECATED(will be removed after removing of skip_nulls)
Review Comment:
Oh I got it, sorry for misleading
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] R-JunmingChen commented on pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "R-JunmingChen (via GitHub)" <gi...@apache.org>.
R-JunmingChen commented on PR #36739:
URL: https://github.com/apache/arrow/pull/36739#issuecomment-1722356797
Hi, @bkietz @ianmcook, please go on the review when you have time
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] R-JunmingChen commented on a diff in pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "R-JunmingChen (via GitHub)" <gi...@apache.org>.
R-JunmingChen commented on code in PR #36739:
URL: https://github.com/apache/arrow/pull/36739#discussion_r1330865673
##########
cpp/src/arrow/compute/api_scalar.h:
##########
@@ -268,19 +268,45 @@ class ARROW_EXPORT ExtractRegexOptions : public FunctionOptions {
/// Options for IsIn and IndexIn functions
class ARROW_EXPORT SetLookupOptions : public FunctionOptions {
public:
- explicit SetLookupOptions(Datum value_set, bool skip_nulls = false);
+ enum NullMatchingBehavior { MATCH, SKIP, EMIT_NULL, INCONCLUSIVE };
Review Comment:
Have moved
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] R-JunmingChen commented on a diff in pull request #36739: GH-36420: [C++] Add an sql-compatible In Function
Posted by "R-JunmingChen (via GitHub)" <gi...@apache.org>.
R-JunmingChen commented on code in PR #36739:
URL: https://github.com/apache/arrow/pull/36739#discussion_r1268311165
##########
cpp/src/arrow/compute/kernels/scalar_set_lookup.cc:
##########
@@ -380,6 +380,120 @@ Status ExecIndexIn(KernelContext* ctx, const ExecSpan& batch, ExecResult* out) {
}
// ----------------------------------------------------------------------
+// In writes the results into a preallocated boolean data bitmap
+struct InVisitor {
+ KernelContext* ctx;
+ const ArraySpan& data;
+ ArraySpan* out;
+ uint8_t* out_boolean_bitmap;
+ uint8_t* out_null_bitmap;
+
+ InVisitor(KernelContext* ctx, const ArraySpan& data, ArraySpan* out)
+ : ctx(ctx),
+ data(data),
+ out(out),
+ out_boolean_bitmap(out->buffers[1].data),
+ out_null_bitmap(out->buffers[0].data) {}
+
+ Status Visit(const DataType& type) {
+ DCHECK_EQ(type.id(), Type::NA);
+ // skip_nulls is ignored in sql-compatible In
+ bit_util::SetBitsTo(out_boolean_bitmap, out->offset, out->length, false);
+ bit_util::SetBitsTo(out_null_bitmap, out->offset, out->length, true);
+
+ return Status::OK();
+ }
+
+ template <typename Type>
+ Status ProcessIsIn(const SetLookupState<Type>& state, const ArraySpan& input) {
+ using T = typename GetViewType<Type>::T;
+ FirstTimeBitmapWriter writer_boolean(out_boolean_bitmap, out->offset, out->length);
+ FirstTimeBitmapWriter writer_null(out_null_bitmap, out->offset, out->length);
+ bool value_set_has_null = state.null_index != -1;
+ VisitArraySpanInline<Type>(
+ input,
+ [&](T v) {
+ if (state.lookup_table->Get(v) != -1) {
+ writer_boolean.Set();
+ writer_null.Clear();
Review Comment:
Sorry for that mistake, have updated
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] R-JunmingChen commented on pull request #36739: GH-36420: [C++] Add an sql-compatible In Function
Posted by "R-JunmingChen (via GitHub)" <gi...@apache.org>.
R-JunmingChen commented on PR #36739:
URL: https://github.com/apache/arrow/pull/36739#issuecomment-1660179473
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] R-JunmingChen commented on pull request #36739: GH-36420: [C++] Add an sql-compatible In Function
Posted by "R-JunmingChen (via GitHub)" <gi...@apache.org>.
R-JunmingChen commented on PR #36739:
URL: https://github.com/apache/arrow/pull/36739#issuecomment-1678311281
>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)
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;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] R-JunmingChen commented on a diff in pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "R-JunmingChen (via GitHub)" <gi...@apache.org>.
R-JunmingChen commented on code in PR #36739:
URL: https://github.com/apache/arrow/pull/36739#discussion_r1309718406
##########
cpp/src/arrow/compute/api_scalar.cc:
##########
@@ -540,8 +564,29 @@ constexpr char RoundToMultipleOptions::kTypeName[];
SetLookupOptions::SetLookupOptions(Datum value_set, bool skip_nulls)
: FunctionOptions(internal::kSetLookupOptionsType),
value_set(std::move(value_set)),
- skip_nulls(skip_nulls) {}
-SetLookupOptions::SetLookupOptions() : SetLookupOptions({}, false) {}
+ skip_nulls(skip_nulls) {
+ if (skip_nulls) {
+ this->null_matching_behavior = SetLookupOptions::SKIP;
+ } else {
+ this->null_matching_behavior = SetLookupOptions::MATCH;
+ }
Review Comment:
After testing, the answer is changed.
It's necessary that `null_matching_behavior` has a initialized value. Without initialization, `null_matching_behavior` can't be deserialized in the expression using `is_in`/`index_in`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] R-JunmingChen commented on a diff in pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "R-JunmingChen (via GitHub)" <gi...@apache.org>.
R-JunmingChen commented on code in PR #36739:
URL: https://github.com/apache/arrow/pull/36739#discussion_r1308986479
##########
cpp/src/arrow/compute/api_scalar.h:
##########
@@ -268,19 +268,43 @@ class ARROW_EXPORT ExtractRegexOptions : public FunctionOptions {
/// Options for IsIn and IndexIn functions
class ARROW_EXPORT SetLookupOptions : public FunctionOptions {
public:
- explicit SetLookupOptions(Datum value_set, bool skip_nulls = false);
+ enum NullMatchingBehavior { MATCH, SKIP, EMIT_NULL, INCONCLUSIVE };
+
+ explicit SetLookupOptions(Datum value_set, NullMatchingBehavior = MATCH);
+ // DEPRECATED(will be removed after removing of skip_nulls)
+ 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;
+
+ /// How to match null values.
+ ///
+ /// Match, any null in `value_set` is successfully matched in
+ /// the input.
+ /// SKIP, any null in `value_set` is ignored and nulls in the input
+ /// produce null (IndexIn) or false (IsIn) values in the output.
+ /// EMIT_NULL, any null in `value_set` is ignored and nulls in the
+ /// input produce null (IndexIn and IsIn) values in the output.
+ /// INCONCLUSIVE, null values are regard as unknown values, which is
+ /// sql-compatible. nulls in the input produce null (IndexIn and IsIn)
+ /// values in the output. Besides, if `value_set` contains a null,
+ /// non-null unmatched values in the input also produce null values
+ /// (IndexIn and IsIn) in the output.
+ NullMatchingBehavior null_matching_behavior;
+
+ // DEPRECATED(will be removed after removing of skip_nulls)
+ NullMatchingBehavior getNullMatchingBehavior() const;
Review Comment:
Sure, have updated
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] R-JunmingChen commented on pull request #36739: GH-36420: [C++] Add an sql-compatible In Function
Posted by "R-JunmingChen (via GitHub)" <gi...@apache.org>.
R-JunmingChen commented on PR #36739:
URL: https://github.com/apache/arrow/pull/36739#issuecomment-1642902140
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mapleFU commented on a diff in pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #36739:
URL: https://github.com/apache/arrow/pull/36739#discussion_r1309086415
##########
cpp/src/arrow/compute/api_scalar.h:
##########
@@ -268,19 +268,43 @@ class ARROW_EXPORT ExtractRegexOptions : public FunctionOptions {
/// Options for IsIn and IndexIn functions
class ARROW_EXPORT SetLookupOptions : public FunctionOptions {
public:
- explicit SetLookupOptions(Datum value_set, bool skip_nulls = false);
+ enum NullMatchingBehavior { MATCH, SKIP, EMIT_NULL, INCONCLUSIVE };
+
+ explicit SetLookupOptions(Datum value_set, NullMatchingBehavior = MATCH);
+ // DEPRECATED(will be removed after removing of skip_nulls)
+ 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;
+
+ /// How to match null values.
+ ///
+ /// Match, any null in `value_set` is successfully matched in
+ /// the input.
+ /// SKIP, any null in `value_set` is ignored and nulls in the input
+ /// produce null (IndexIn) or false (IsIn) values in the output.
+ /// EMIT_NULL, any null in `value_set` is ignored and nulls in the
+ /// input produce null (IndexIn and IsIn) values in the output.
+ /// INCONCLUSIVE, null values are regard as unknown values, which is
+ /// sql-compatible. nulls in the input produce null (IndexIn and IsIn)
+ /// values in the output. Besides, if `value_set` contains a null,
+ /// non-null unmatched values in the input also produce null values
+ /// (IndexIn and IsIn) in the output.
+ NullMatchingBehavior null_matching_behavior;
+
+ // DEPRECATED(will be removed after removing of skip_nulls)
Review Comment:
Hmmm can we set `null_matching_behavior = GetNullMatchingBehavior()` in the constructor `SetLookupOptions(Datum value_set, bool skip_nulls)`, and remove `GetNullMatchingBehavior()`?
##########
cpp/src/arrow/compute/api_scalar.h:
##########
@@ -268,19 +268,43 @@ class ARROW_EXPORT ExtractRegexOptions : public FunctionOptions {
/// Options for IsIn and IndexIn functions
class ARROW_EXPORT SetLookupOptions : public FunctionOptions {
public:
- explicit SetLookupOptions(Datum value_set, bool skip_nulls = false);
+ enum NullMatchingBehavior { MATCH, SKIP, EMIT_NULL, INCONCLUSIVE };
+
+ explicit SetLookupOptions(Datum value_set, NullMatchingBehavior = MATCH);
+ // DEPRECATED(will be removed after removing of skip_nulls)
Review Comment:
Would `ARROW_DEPRECATED("will be removed after removing of skip_nulls")` better? Or this function would be removed later?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] R-JunmingChen commented on a diff in pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "R-JunmingChen (via GitHub)" <gi...@apache.org>.
R-JunmingChen commented on code in PR #36739:
URL: https://github.com/apache/arrow/pull/36739#discussion_r1309501211
##########
cpp/src/arrow/compute/api_scalar.h:
##########
@@ -268,19 +268,43 @@ class ARROW_EXPORT ExtractRegexOptions : public FunctionOptions {
/// Options for IsIn and IndexIn functions
class ARROW_EXPORT SetLookupOptions : public FunctionOptions {
public:
- explicit SetLookupOptions(Datum value_set, bool skip_nulls = false);
+ enum NullMatchingBehavior { MATCH, SKIP, EMIT_NULL, INCONCLUSIVE };
+
+ explicit SetLookupOptions(Datum value_set, NullMatchingBehavior = MATCH);
+ // DEPRECATED(will be removed after removing of skip_nulls)
+ 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;
+
+ /// How to match null values.
+ ///
+ /// Match, any null in `value_set` is successfully matched in
+ /// the input.
+ /// SKIP, any null in `value_set` is ignored and nulls in the input
+ /// produce null (IndexIn) or false (IsIn) values in the output.
+ /// EMIT_NULL, any null in `value_set` is ignored and nulls in the
+ /// input produce null (IndexIn and IsIn) values in the output.
+ /// INCONCLUSIVE, null values are regard as unknown values, which is
+ /// sql-compatible. nulls in the input produce null (IndexIn and IsIn)
+ /// values in the output. Besides, if `value_set` contains a null,
+ /// non-null unmatched values in the input also produce null values
+ /// (IndexIn and IsIn) in the output.
+ NullMatchingBehavior null_matching_behavior;
+
+ // DEPRECATED(will be removed after removing of skip_nulls)
Review Comment:
No, since `skip_nulls` can be set via its member variable like this.
```
SetLookupOptions option(value_set);
option.skip_nulls = true;
```
The `null_matching_behavior` set in the constructor has a possibility to contradict with `skip_nulls`. To support backward compatibility, we assign `skip_nulls` with a higher priority.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mapleFU commented on a diff in pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #36739:
URL: https://github.com/apache/arrow/pull/36739#discussion_r1309537260
##########
cpp/src/arrow/compute/api_scalar.h:
##########
@@ -268,19 +268,43 @@ class ARROW_EXPORT ExtractRegexOptions : public FunctionOptions {
/// Options for IsIn and IndexIn functions
class ARROW_EXPORT SetLookupOptions : public FunctionOptions {
public:
- explicit SetLookupOptions(Datum value_set, bool skip_nulls = false);
+ enum NullMatchingBehavior { MATCH, SKIP, EMIT_NULL, INCONCLUSIVE };
+
+ explicit SetLookupOptions(Datum value_set, NullMatchingBehavior = MATCH);
+ // DEPRECATED(will be removed after removing of skip_nulls)
+ 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;
+
+ /// How to match null values.
+ ///
+ /// Match, any null in `value_set` is successfully matched in
+ /// the input.
+ /// SKIP, any null in `value_set` is ignored and nulls in the input
+ /// produce null (IndexIn) or false (IsIn) values in the output.
+ /// EMIT_NULL, any null in `value_set` is ignored and nulls in the
+ /// input produce null (IndexIn and IsIn) values in the output.
+ /// INCONCLUSIVE, null values are regard as unknown values, which is
+ /// sql-compatible. nulls in the input produce null (IndexIn and IsIn)
+ /// values in the output. Besides, if `value_set` contains a null,
+ /// non-null unmatched values in the input also produce null values
+ /// (IndexIn and IsIn) in the output.
+ NullMatchingBehavior null_matching_behavior;
+
+ // DEPRECATED(will be removed after removing of skip_nulls)
Review Comment:
Got it, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] R-JunmingChen commented on a diff in pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "R-JunmingChen (via GitHub)" <gi...@apache.org>.
R-JunmingChen commented on code in PR #36739:
URL: https://github.com/apache/arrow/pull/36739#discussion_r1330949943
##########
cpp/src/arrow/compute/api_scalar.cc:
##########
@@ -344,7 +367,8 @@ static auto kRoundToMultipleOptionsType = GetFunctionOptionsType<RoundToMultiple
DataMember("round_mode", &RoundToMultipleOptions::round_mode));
static auto kSetLookupOptionsType = GetFunctionOptionsType<SetLookupOptions>(
DataMember("value_set", &SetLookupOptions::value_set),
- DataMember("skip_nulls", &SetLookupOptions::skip_nulls));
+ DataMember("skip_nulls", &SetLookupOptions::skip_nulls),
+ DataMember("null_matching_behavior", &SetLookupOptions::null_matching_behavior));
Review Comment:
Hi, here's an error I have no idea how to handle it when I put it to `reflection_internal.h`. Do you have any solution about it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a diff in pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #36739:
URL: https://github.com/apache/arrow/pull/36739#discussion_r1330188729
##########
cpp/src/arrow/compute/kernels/scalar_set_lookup.cc:
##########
@@ -274,10 +280,12 @@ struct IndexInVisitor {
const auto& state = checked_cast<const SetLookupState<NullType>&>(*ctx->state());
Review Comment:
Would you mind modifying this visit overload to:
```diff
- Status Visit(const DataType& type) {
- DCHECK_EQ(type.id(), Type::NA);
+ Status Visit(const DataType& type) { DCHECK(false) << "IndexIn " << type; }
+ Status Visit(const NullType&) {
```
##########
cpp/src/arrow/compute/kernels/scalar_set_lookup.cc:
##########
@@ -379,49 +388,82 @@ Status ExecIndexIn(KernelContext* ctx, const ExecSpan& batch, ExecResult* out) {
return IndexInVisitor(ctx, batch[0].array, out->array_span_mutable()).Execute();
}
-// ----------------------------------------------------------------------
-
// IsIn writes the results into a preallocated boolean data bitmap
struct IsInVisitor {
KernelContext* ctx;
const ArraySpan& data;
ArraySpan* out;
+ uint8_t* out_boolean_bitmap;
+ uint8_t* out_null_bitmap;
IsInVisitor(KernelContext* ctx, const ArraySpan& data, ArraySpan* out)
- : ctx(ctx), data(data), out(out) {}
+ : ctx(ctx),
+ data(data),
+ out(out),
+ out_boolean_bitmap(out->buffers[1].data),
+ out_null_bitmap(out->buffers[0].data) {}
Status Visit(const DataType& type) {
Review Comment:
Same here
##########
cpp/src/arrow/compute/api_scalar.h:
##########
@@ -268,19 +268,45 @@ class ARROW_EXPORT ExtractRegexOptions : public FunctionOptions {
/// Options for IsIn and IndexIn functions
class ARROW_EXPORT SetLookupOptions : public FunctionOptions {
public:
- explicit SetLookupOptions(Datum value_set, bool skip_nulls = false);
+ enum NullMatchingBehavior { MATCH, SKIP, EMIT_NULL, INCONCLUSIVE };
+
+ explicit SetLookupOptions(Datum value_set, NullMatchingBehavior = MATCH);
SetLookupOptions();
+
+ // DEPRECATED(will be removed after removing of skip_nulls)
+ explicit SetLookupOptions(Datum value_set, bool skip_nulls);
+
static constexpr char const kTypeName[] = "SetLookupOptions";
/// The set of values to look up input values into.
Datum value_set;
+
+ /// How to match null values.
+ ///
+ /// Match, any null in `value_set` is successfully matched in
Review Comment:
```suggestion
/// MATCH, any null in `value_set` is successfully matched in
```
##########
cpp/src/arrow/compute/api_scalar.cc:
##########
@@ -344,7 +367,8 @@ static auto kRoundToMultipleOptionsType = GetFunctionOptionsType<RoundToMultiple
DataMember("round_mode", &RoundToMultipleOptions::round_mode));
static auto kSetLookupOptionsType = GetFunctionOptionsType<SetLookupOptions>(
DataMember("value_set", &SetLookupOptions::value_set),
- DataMember("skip_nulls", &SetLookupOptions::skip_nulls));
+ DataMember("skip_nulls", &SetLookupOptions::skip_nulls),
+ DataMember("null_matching_behavior", &SetLookupOptions::null_matching_behavior));
Review Comment:
I think it'd be worthwhile to change this to only report/accept assignment to null_matching_behavior:
```suggestion
CoercedDataMember("null_matching_behavior", &SetLookupOptions::null_matching_behavior,
&SetLookupOptions::GetNullMatchingBehavior));
```
(This only affects how we deal with SetLookupOptions in reflected contexts like stringification; we'll still be able to assign to `skip_nulls`.) This can be enabled by putting the following into `reflection_internal.h`:
```c++
template <typename C, typename T>
struct CoercedDataMemberProperty {
using Class = C;
using Type = T;
constexpr const Type& get(const Class& obj) const { return obj.*get_coerced_(); }
void set(Class* obj, Type value) const { (*obj).*ptr_for_set_ = std::move(value); }
constexpr std::string_view name() const { return name_; }
std::string_view name_;
Type Class::*ptr_for_set_;
Type (Class::*get_coerced_)() const;
};
template <typename Class, typename Type>
constexpr CoercedDataMemberProperty<Class, Type> CoercedDataMember(std::string_view name,
Type Class::*ptr,
Type (Class::*get)()
const) {
return {name, ptr, get};
}
```
##########
cpp/src/arrow/compute/api_scalar.h:
##########
@@ -268,19 +268,45 @@ class ARROW_EXPORT ExtractRegexOptions : public FunctionOptions {
/// Options for IsIn and IndexIn functions
class ARROW_EXPORT SetLookupOptions : public FunctionOptions {
public:
- explicit SetLookupOptions(Datum value_set, bool skip_nulls = false);
+ enum NullMatchingBehavior { MATCH, SKIP, EMIT_NULL, INCONCLUSIVE };
+
+ explicit SetLookupOptions(Datum value_set, NullMatchingBehavior = MATCH);
SetLookupOptions();
+
+ // DEPRECATED(will be removed after removing of skip_nulls)
+ explicit SetLookupOptions(Datum value_set, bool skip_nulls);
+
static constexpr char const kTypeName[] = "SetLookupOptions";
/// The set of values to look up input values into.
Datum value_set;
+
+ /// How to match null values.
+ ///
+ /// Match, any null in `value_set` is successfully matched in
+ /// the input.
+ /// SKIP, any null in `value_set` is ignored and nulls in the input
+ /// produce null (IndexIn) or false (IsIn) values in the output.
+ /// EMIT_NULL, any null in `value_set` is ignored and nulls in the
+ /// input produce null (IndexIn and IsIn) values in the output.
+ /// INCONCLUSIVE, null values are regard as unknown values, which is
Review Comment:
```suggestion
/// INCONCLUSIVE, null values are regarded as unknown values, which is
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on PR #36739:
URL: https://github.com/apache/arrow/pull/36739#issuecomment-1728617901
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on PR #36739:
URL: https://github.com/apache/arrow/pull/36739#issuecomment-1726093900
Looks great, thanks for your effort and persistence @R-JunmingChen!
I built Arrow from your fork and created a standalone example to test this. Everything seems to be working well. https://gist.github.com/ianmcook/25bb0c82b8af4fc1bde0768863607355.
I checked whether anything needs to change on the `compute.rst` docs page. No changes are required there, I think.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] austin3dickey commented on pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "austin3dickey (via GitHub)" <gi...@apache.org>.
austin3dickey commented on PR #36739:
URL: https://github.com/apache/arrow/pull/36739#issuecomment-1729506479
>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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] conbench-apache-arrow[bot] commented on pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #36739:
URL: https://github.com/apache/arrow/pull/36739#issuecomment-1731946592
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 772a01c080ad57eb11e9323f5347472b769d45de.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/17052092247) has more details. It also includes information about possible false positives for unstable benchmarks that are known to sometimes produce them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on a diff in pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #36739:
URL: https://github.com/apache/arrow/pull/36739#discussion_r1300658624
##########
c_glib/arrow-glib/compute.cpp:
##########
@@ -3346,7 +3346,11 @@ garrow_set_lookup_options_get_property(GObject *object,
g_value_set_object(value, priv->value_set);
break;
case PROP_SET_LOOKUP_OPTIONS_SKIP_NULLS:
- g_value_set_boolean(value, options->skip_nulls);
+ if (!options->skip_nulls.has_value() || !options->skip_nulls.value()) {
+ g_value_set_boolean(value, false);
+ } else {
+ g_value_set_boolean(value, true);
+ }
Review Comment:
```suggestion
g_value_set_boolean(value, options->skip_nulls.has_value() && options->skip_nulls.value());
```
##########
c_glib/arrow-glib/compute.cpp:
##########
@@ -6458,9 +6463,13 @@ garrow_set_lookup_options_new_raw(
arrow_copied_options.get());
auto value_set =
garrow_datum_new_raw(&(arrow_copied_set_lookup_options->value_set));
+ bool skip_nulls = false;
+ if (arrow_options->skip_nulls.has_value() && arrow_options->skip_nulls.value()) {
+ skip_nulls = true;
+ }
Review Comment:
```suggestion
auto skip_nulls = (arrow_options->skip_nulls.has_value() && arrow_options->skip_nulls.value());
```
##########
c_glib/arrow-glib/compute.cpp:
##########
@@ -3398,13 +3402,14 @@ garrow_set_lookup_options_class_init(GArrowSetLookupOptionsClass *klass)
*
* Since: 6.0.0
*/
- spec = g_param_spec_boolean("skip-nulls",
- "Skip NULLs",
- "Whether NULLs are skipped or not",
- options.skip_nulls,
- static_cast<GParamFlags>(G_PARAM_READWRITE));
- g_object_class_install_property(gobject_class,
- PROP_SET_LOOKUP_OPTIONS_SKIP_NULLS,
+ bool skip_nulls = false;
+ if (options.skip_nulls.has_value() && options.skip_nulls.value()) {
+ skip_nulls = true;
+ }
Review Comment:
```suggestion
auto skip_nulls = (options.skip_nulls.has_value() && options.skip_nulls.value());
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] R-JunmingChen commented on pull request #36739: GH-36420: [C++] Add an sql-compatible In Function
Posted by "R-JunmingChen (via GitHub)" <gi...@apache.org>.
R-JunmingChen commented on PR #36739:
URL: https://github.com/apache/arrow/pull/36739#issuecomment-1677107489
Hi, @ianmcook. I think it's a good solution to use an enum option , but I still have one question.
Why do we want to support `EMIT_NULL`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on pull request #36739: GH-36420: [C++] Add an sql-compatible In Function
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on PR #36739:
URL: https://github.com/apache/arrow/pull/36739#issuecomment-1677429633
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: https://github.com/apache/arrow/issues/36420#issuecomment-1662660149
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on pull request #36739: GH-36420: [C++] Add an sql-compatible In Function
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on PR #36739:
URL: https://github.com/apache/arrow/pull/36739#issuecomment-1642116224
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #36739:
URL: https://github.com/apache/arrow/pull/36739#issuecomment-1728445640
Benchmark runs are scheduled for commit 38c1a04817ffa20e3baef1ed6312d7cf6501fc68. Watch https://buildkite.com/apache-arrow and https://conbench.ursa.dev for updates. A comment will be posted here when the runs are complete.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] austin3dickey commented on pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "austin3dickey (via GitHub)" <gi...@apache.org>.
austin3dickey commented on PR #36739:
URL: https://github.com/apache/arrow/pull/36739#issuecomment-1729571442
That should be fixed. If you push an empty commit (https://github.com/voltrondata-labs/arrow-benchmarks-ci/issues/111) and then re-run the command, you can try benchmarks again. Else, yeah, they will be run post-merge.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] R-JunmingChen commented on a diff in pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "R-JunmingChen (via GitHub)" <gi...@apache.org>.
R-JunmingChen commented on code in PR #36739:
URL: https://github.com/apache/arrow/pull/36739#discussion_r1330866568
##########
cpp/src/arrow/compute/kernels/scalar_set_lookup.cc:
##########
@@ -379,49 +388,82 @@ Status ExecIndexIn(KernelContext* ctx, const ExecSpan& batch, ExecResult* out) {
return IndexInVisitor(ctx, batch[0].array, out->array_span_mutable()).Execute();
}
-// ----------------------------------------------------------------------
-
// IsIn writes the results into a preallocated boolean data bitmap
struct IsInVisitor {
KernelContext* ctx;
const ArraySpan& data;
ArraySpan* out;
+ uint8_t* out_boolean_bitmap;
+ uint8_t* out_null_bitmap;
IsInVisitor(KernelContext* ctx, const ArraySpan& data, ArraySpan* out)
- : ctx(ctx), data(data), out(out) {}
+ : ctx(ctx),
+ data(data),
+ out(out),
+ out_boolean_bitmap(out->buffers[1].data),
+ out_null_bitmap(out->buffers[0].data) {}
Status Visit(const DataType& type) {
Review Comment:
Sure
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a diff in pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on code in PR #36739:
URL: https://github.com/apache/arrow/pull/36739#discussion_r1330323751
##########
cpp/src/arrow/compute/kernels/scalar_set_lookup_test.cc:
##########
@@ -185,25 +244,43 @@ TYPED_TEST(TestIsInKernelPrimitive, IsIn) {
/*skip_nulls=*/false);
CheckIsIn(type, "[null, 1, 2, 3, 2]", "[2, 1]", "[false, true, true, false, true]",
/*skip_nulls=*/true);
+ CheckIsIn(type, "[null, 1, 2, 3, 2]", "[2, 1]", "[null, true, true, false, true]",
Review Comment:
For completeness, I think we should:
- repeat each `skip_null=false` check using `null_matching_behavior=SetLookupOptions::NullMatchingBehavior::MATCH`
- repeat each `skip_null=true` check using `null_matching_behavior=SetLookupOptions::NullMatchingBehavior::SKIP`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a diff in pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on code in PR #36739:
URL: https://github.com/apache/arrow/pull/36739#discussion_r1330323751
##########
cpp/src/arrow/compute/kernels/scalar_set_lookup_test.cc:
##########
@@ -185,25 +244,43 @@ TYPED_TEST(TestIsInKernelPrimitive, IsIn) {
/*skip_nulls=*/false);
CheckIsIn(type, "[null, 1, 2, 3, 2]", "[2, 1]", "[false, true, true, false, true]",
/*skip_nulls=*/true);
+ CheckIsIn(type, "[null, 1, 2, 3, 2]", "[2, 1]", "[null, true, true, false, true]",
Review Comment:
For completeness, I think we should:
- repeat each `skip_null=false` check using `null_matching_behavior=SetLookupOptions::MATCH`
- repeat each `skip_null=true` check using `null_matching_behavior=SetLookupOptions::SKIP`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] R-JunmingChen commented on a diff in pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "R-JunmingChen (via GitHub)" <gi...@apache.org>.
R-JunmingChen commented on code in PR #36739:
URL: https://github.com/apache/arrow/pull/36739#discussion_r1330892476
##########
cpp/src/arrow/compute/kernels/scalar_set_lookup_test.cc:
##########
@@ -185,25 +244,43 @@ TYPED_TEST(TestIsInKernelPrimitive, IsIn) {
/*skip_nulls=*/false);
CheckIsIn(type, "[null, 1, 2, 3, 2]", "[2, 1]", "[false, true, true, false, true]",
/*skip_nulls=*/true);
+ CheckIsIn(type, "[null, 1, 2, 3, 2]", "[2, 1]", "[null, true, true, false, true]",
Review Comment:
Sure, have added
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on PR #36739:
URL: https://github.com/apache/arrow/pull/36739#issuecomment-1728445534
@ursabot please benchmark
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on pull request #36739: GH-36420: [C++] Add An Enum Option For SetLookup Options
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on PR #36739:
URL: https://github.com/apache/arrow/pull/36739#issuecomment-1730278689
@bkietz if you want to go ahead and merge when you're comfortable with this, we can just look at the benchmarks post-merge
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on pull request #36739: GH-36420: [C++] Add an sql-compatible In Function
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on PR #36739:
URL: https://github.com/apache/arrow/pull/36739#issuecomment-1680699604
> > 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org