You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "nealrichardson (via GitHub)" <gi...@apache.org> on 2024/04/25 01:08:14 UTC
[PR] GH-41358: [R] Support join "na_matches" argument [arrow]
nealrichardson opened a new pull request, #41372:
URL: https://github.com/apache/arrow/pull/41372
### Rationale for this change
Noticed in #41350, I made #41358 to implement this in C++, but it turns out the option was there, just buried a bit.
### What changes are included in this PR?
`na_matches` is mapped through to the `key_cmp` field in `HashJoinNodeOptions`. Acero supports having a different value for this for each of the join keys, but dplyr does not, so I kept it constant for all key columns to match the dplyr behavior.
### Are these changes tested?
Yes
### Are there any user-facing changes?
Yes
--
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
Re: [PR] GH-41358: [R] Support join "na_matches" argument [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #41372:
URL: https://github.com/apache/arrow/pull/41372#issuecomment-2076125423
:warning: GitHub issue #41358 **has been automatically assigned in GitHub** to PR creator.
--
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
Re: [PR] GH-41358: [R] Support join "na_matches" argument [arrow]
Posted by "jonkeane (via GitHub)" <gi...@apache.org>.
jonkeane commented on code in PR #41372:
URL: https://github.com/apache/arrow/pull/41372#discussion_r1581582262
##########
r/src/compute-exec.cpp:
##########
@@ -411,10 +411,17 @@ std::shared_ptr<acero::ExecNode> ExecNode_Join(
const std::shared_ptr<acero::ExecNode>& right_data,
std::vector<std::string> left_keys, std::vector<std::string> right_keys,
std::vector<std::string> left_output, std::vector<std::string> right_output,
- std::string output_suffix_for_left, std::string output_suffix_for_right) {
+ std::string output_suffix_for_left, std::string output_suffix_for_right,
+ bool na_matches) {
std::vector<arrow::FieldRef> left_refs, right_refs, left_out_refs, right_out_refs;
+ std::vector<acero::JoinKeyCmp> key_cmps;
for (auto&& name : left_keys) {
left_refs.emplace_back(std::move(name));
+ // Populate key_cmps in this loop, one for each key
+ // Note that Acero supports having different values for each key, but dplyr
+ // only supports one value for all keys, so we're only going to support that
+ // for now.
+ key_cmps.emplace_back(na_matches ? acero::JoinKeyCmp::IS : acero::JoinKeyCmp::EQ);
Review Comment:
So many ways of swapping and passing and evaluating booleans 😂
--
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
Re: [PR] GH-41358: [R] Support join "na_matches" argument [arrow]
Posted by "nealrichardson (via GitHub)" <gi...@apache.org>.
nealrichardson commented on PR #41372:
URL: https://github.com/apache/arrow/pull/41372#issuecomment-2079983661
CI failure appears unrelated: https://github.com/apache/arrow/issues/41398
--
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
Re: [PR] GH-41358: [R] Support join "na_matches" argument [arrow]
Posted by "jonkeane (via GitHub)" <gi...@apache.org>.
jonkeane commented on code in PR #41372:
URL: https://github.com/apache/arrow/pull/41372#discussion_r1581558982
##########
r/src/compute-exec.cpp:
##########
@@ -411,10 +411,17 @@ std::shared_ptr<acero::ExecNode> ExecNode_Join(
const std::shared_ptr<acero::ExecNode>& right_data,
std::vector<std::string> left_keys, std::vector<std::string> right_keys,
std::vector<std::string> left_output, std::vector<std::string> right_output,
- std::string output_suffix_for_left, std::string output_suffix_for_right) {
+ std::string output_suffix_for_left, std::string output_suffix_for_right,
+ bool na_matches) {
std::vector<arrow::FieldRef> left_refs, right_refs, left_out_refs, right_out_refs;
+ std::vector<acero::JoinKeyCmp> key_cmps;
for (auto&& name : left_keys) {
left_refs.emplace_back(std::move(name));
+ // Populate key_cmps in this loop, one for each key
+ // Note that Acero supports having different values for each key, but dplyr
+ // only supports one value for all keys, so we're only going to support that
+ // for now.
+ key_cmps.emplace_back(na_matches ? acero::JoinKeyCmp::IS : acero::JoinKeyCmp::EQ);
Review Comment:
Out of my curiosity to make sure I'm following this all: `acero::JoinKeyCmp::IS` ends up being used when `na_match == "na"`, and therefore `NA`s will match. `acero::JoinKeyCmp::EQ` ends up in all other cases (i.e. `na_match == "never"`) and that uses equality where `NA`s _do not_ match.
--
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
Re: [PR] GH-41358: [R] Support join "na_matches" argument [arrow]
Posted by "nealrichardson (via GitHub)" <gi...@apache.org>.
nealrichardson merged PR #41372:
URL: https://github.com/apache/arrow/pull/41372
--
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
Re: [PR] GH-41358: [R] Support join "na_matches" argument [arrow]
Posted by "nealrichardson (via GitHub)" <gi...@apache.org>.
nealrichardson commented on code in PR #41372:
URL: https://github.com/apache/arrow/pull/41372#discussion_r1581571725
##########
r/src/compute-exec.cpp:
##########
@@ -411,10 +411,17 @@ std::shared_ptr<acero::ExecNode> ExecNode_Join(
const std::shared_ptr<acero::ExecNode>& right_data,
std::vector<std::string> left_keys, std::vector<std::string> right_keys,
std::vector<std::string> left_output, std::vector<std::string> right_output,
- std::string output_suffix_for_left, std::string output_suffix_for_right) {
+ std::string output_suffix_for_left, std::string output_suffix_for_right,
+ bool na_matches) {
std::vector<arrow::FieldRef> left_refs, right_refs, left_out_refs, right_out_refs;
+ std::vector<acero::JoinKeyCmp> key_cmps;
for (auto&& name : left_keys) {
left_refs.emplace_back(std::move(name));
+ // Populate key_cmps in this loop, one for each key
+ // Note that Acero supports having different values for each key, but dplyr
+ // only supports one value for all keys, so we're only going to support that
+ // for now.
+ key_cmps.emplace_back(na_matches ? acero::JoinKeyCmp::IS : acero::JoinKeyCmp::EQ);
Review Comment:
Correct
--
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
Re: [PR] GH-41358: [R] Support join "na_matches" argument [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #41372:
URL: https://github.com/apache/arrow/pull/41372#issuecomment-2080315675
After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit ea314a3f8d9d4446836aa999b66659c07421f7a4.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/24322775693) has more details. It also includes information about 15 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