You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2023/01/15 14:19:40 UTC
[GitHub] [arrow] rtpsw opened a new pull request, #33676: GH-33673: [C++] Standardize as-of-join convention for past and future tolerance
rtpsw opened a new pull request, #33676:
URL: https://github.com/apache/arrow/pull/33676
See #33673
--
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] github-actions[bot] commented on pull request #33676: GH-33673: [C++] Standardize as-of-join convention for past and future tolerance
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #33676:
URL: https://github.com/apache/arrow/pull/33676#issuecomment-1383163008
:warning: GitHub issue #33673 **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
[GitHub] [arrow] icexelloss commented on pull request #33676: GH-33673: [C++] Standardize as-of-join convention for past and future tolerance
Posted by GitBox <gi...@apache.org>.
icexelloss commented on PR #33676:
URL: https://github.com/apache/arrow/pull/33676#issuecomment-1387378034
The original ask is to make the direction of "tolerance" here:
https://github.com/apache/arrow/blob/fc53ff8c5e2797c1a5a99db7f3aece80dd0b9f3e/cpp/src/arrow/compute/exec/options.h#L529
To be: negative -> past tolerance, positive -> future tolerance
--
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] rtpsw commented on pull request #33676: GH-33673: [C++] Standardize as-of-join convention for past and future tolerance
Posted by GitBox <gi...@apache.org>.
rtpsw commented on PR #33676:
URL: https://github.com/apache/arrow/pull/33676#issuecomment-1383237187
The CI job failures appear to be unrelated.
--
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] westonpace commented on pull request #33676: GH-33673: [C++] Standardize as-of-join convention for past and future tolerance
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on PR #33676:
URL: https://github.com/apache/arrow/pull/33676#issuecomment-1399305191
Ok, I see @icexelloss 's earlier comment and agree it matches your definition here. So once CI passes this is good to go.
--
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] rtpsw commented on pull request #33676: GH-33673: [C++] Standardize as-of-join convention for past and future tolerance
Posted by GitBox <gi...@apache.org>.
rtpsw commented on PR #33676:
URL: https://github.com/apache/arrow/pull/33676#issuecomment-1398491977
Ping @westonpace - it would be great if this can be reviewed quickly.
--
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] westonpace commented on pull request #33676: GH-33673: [C++] Standardize as-of-join convention for past and future tolerance
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on PR #33676:
URL: https://github.com/apache/arrow/pull/33676#issuecomment-1399304348
Hmm, this TravisCI failure may be relevant:
```
[ RUN ] Substrait.PlanWithAsOfJoinExtension
/arrow/cpp/src/arrow/testing/gtest_util.cc:486: Failure
Failed
Unequal at absolute position 3
@@ -0, +0 @@
-null
-null
-null
+1.2
+1.2
+3.2
Expected:
[
1.2,
1.2,
3.2
]
Actual:
[
null,
null,
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] rtpsw commented on pull request #33676: GH-33673: [C++] Standardize as-of-join convention for past and future tolerance
Posted by "rtpsw (via GitHub)" <gi...@apache.org>.
rtpsw commented on PR #33676:
URL: https://github.com/apache/arrow/pull/33676#issuecomment-1407378621
Something weird seems to be going on with the CI. Some failing jobs, like [this one](https://github.com/apache/arrow/actions/runs/4031369011/jobs/6930622636) and [this one](https://github.com/apache/arrow/actions/runs/4031369011/jobs/6930622746), are mentioning test cases `Substrait.PlanWithExtension` and `Substrait.AsOfJoinDefaultEmit` that [do not exist on the commit](https://github.com/apache/arrow/pull/33676/files#diff-28418456673a1657af039f8233d5b4cd35e0e55b788ee638a5521f81f1795089) nor [the branch](https://github.com/rtpsw/arrow/blob/GH-33673/cpp/src/arrow/engine/substrait/serde_test.cc). @westonpace, any idea?
--
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] rtpsw commented on pull request #33676: GH-33673: [C++] Standardize as-of-join convention for past and future tolerance
Posted by "rtpsw (via GitHub)" <gi...@apache.org>.
rtpsw commented on PR #33676:
URL: https://github.com/apache/arrow/pull/33676#issuecomment-1407398710
CI job failure is unrelated.
--
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] rtpsw commented on pull request #33676: GH-33673: [C++] Standardize as-of-join convention for past and future tolerance
Posted by GitBox <gi...@apache.org>.
rtpsw commented on PR #33676:
URL: https://github.com/apache/arrow/pull/33676#issuecomment-1387193893
cc @icexelloss
--
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] rtpsw commented on pull request #33676: GH-33673: [C++] Standardize as-of-join convention for past and future tolerance
Posted by GitBox <gi...@apache.org>.
rtpsw commented on PR #33676:
URL: https://github.com/apache/arrow/pull/33676#issuecomment-1383564040
cc @westonpace
--
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] rtpsw commented on a diff in pull request #33676: GH-33673: [C++] Standardize as-of-join convention for past and future tolerance
Posted by "rtpsw (via GitHub)" <gi...@apache.org>.
rtpsw commented on code in PR #33676:
URL: https://github.com/apache/arrow/pull/33676#discussion_r1083312503
##########
cpp/src/arrow/compute/exec/options.h:
##########
@@ -523,7 +523,8 @@ class ARROW_EXPORT AsofJoinNodeOptions : public ExecNodeOptions {
///
/// \see `Keys` for details.
std::vector<Keys> input_keys;
- /// \brief Tolerance for inexact "on" key matching. Must be non-negative.
+ /// \brief Tolerance for inexact "on" key matching. Positive (resp. negative) tolerance
+ /// is interpreted as a future (resp. past) as-of-join.
Review Comment:
I wouldn't say the PR is at odds because it takes the tolerance as a single parameter, a positive or negative number, whereas pandas takes the tolerance as two parameters, a positive number and a direction, even though the direction defaults to backwards. We could make this PR take the tolerance as two parameters if we want strict consistency. @icexelloss requested this standardization and should be able to explain the context.
--
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 #33676: GH-33673: [C++] Standardize as-of-join convention for past and future tolerance
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #33676:
URL: https://github.com/apache/arrow/pull/33676#issuecomment-1409481067
Benchmark runs are scheduled for baseline = 36075346750c7f0d0afd098cf48bb3815421ebc3 and contender = 64a48c99934555fe35a69066608328635ed62dad. 64a48c99934555fe35a69066608328635ed62dad is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/0b80ba8673b54e2cb007a448fd97a2b1...543f245aba6843ad8e7d0aa39cfcdf03/)
[Failed] [test-mac-arm](https://conbench.ursa.dev/compare/runs/0e32735645594dbd91845202b1d95031...6f4a6810e3304831914dff733682792c/)
[Failed] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/8ad4d505d3d84a1c993e0510db07cac1...1444d18b169149358cee0ff09cf106c4/)
[Failed] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/0eed8291ddd64bc39810f85c90332dc5...13f677a051f84fe097c8c4a3166526b5/)
Buildkite builds:
[Finished] [`64a48c99` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2290)
[Failed] [`64a48c99` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2316)
[Failed] [`64a48c99` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2287)
[Failed] [`64a48c99` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2308)
[Finished] [`36075346` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2289)
[Failed] [`36075346` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2315)
[Failed] [`36075346` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2286)
[Failed] [`36075346` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2307)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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] icexelloss commented on a diff in pull request #33676: GH-33673: [C++] Standardize as-of-join convention for past and future tolerance
Posted by GitBox <gi...@apache.org>.
icexelloss commented on code in PR #33676:
URL: https://github.com/apache/arrow/pull/33676#discussion_r1073794299
##########
cpp/src/arrow/compute/exec/asof_join_node_test.cc:
##########
@@ -662,19 +662,19 @@ TRACED_TEST_P(AsofJoinBasicTest, TestBasic1, {
runner(basic_test);
})
-BasicTest GetBasicTest1Negative() {
Review Comment:
Let's name these tests "backward" or "forward" similar to how pandas call them:
https://pandas.pydata.org/pandas-docs/version/0.25.0/reference/api/pandas.merge_asof.html#pandas-merge-asof
--
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] westonpace commented on pull request #33676: GH-33673: [C++] Standardize as-of-join convention for past and future tolerance
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on PR #33676:
URL: https://github.com/apache/arrow/pull/33676#issuecomment-1399304546
Same failure on the `C++ / AMD64 Conda C++ (pull_request) ` job
--
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] westonpace merged pull request #33676: GH-33673: [C++] Standardize as-of-join convention for past and future tolerance
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace merged PR #33676:
URL: https://github.com/apache/arrow/pull/33676
--
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] rtpsw commented on a diff in pull request #33676: GH-33673: [C++] Standardize as-of-join convention for past and future tolerance
Posted by "rtpsw (via GitHub)" <gi...@apache.org>.
rtpsw commented on code in PR #33676:
URL: https://github.com/apache/arrow/pull/33676#discussion_r1083261065
##########
cpp/src/arrow/compute/exec/options.h:
##########
@@ -523,7 +523,8 @@ class ARROW_EXPORT AsofJoinNodeOptions : public ExecNodeOptions {
///
/// \see `Keys` for details.
std::vector<Keys> input_keys;
- /// \brief Tolerance for inexact "on" key matching. Must be non-negative.
+ /// \brief Tolerance for inexact "on" key matching. Positive (resp. negative) tolerance
+ /// is interpreted as a future (resp. past) as-of-join.
Review Comment:
The match condition is actually (now) reversed. I committed a more detailed doc.
--
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] westonpace commented on a diff in pull request #33676: GH-33673: [C++] Standardize as-of-join convention for past and future tolerance
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on code in PR #33676:
URL: https://github.com/apache/arrow/pull/33676#discussion_r1083291764
##########
cpp/src/arrow/compute/exec/options.h:
##########
@@ -523,7 +523,8 @@ class ARROW_EXPORT AsofJoinNodeOptions : public ExecNodeOptions {
///
/// \see `Keys` for details.
std::vector<Keys> input_keys;
- /// \brief Tolerance for inexact "on" key matching. Must be non-negative.
+ /// \brief Tolerance for inexact "on" key matching. Positive (resp. negative) tolerance
+ /// is interpreted as a future (resp. past) as-of-join.
Review Comment:
Out of curiosity, why is it reversed? Doesn't this put us at odds with pandas which does backwards by default?
--
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] rtpsw commented on pull request #33676: GH-33673: [C++] Standardize as-of-join convention for past and future tolerance
Posted by "rtpsw (via GitHub)" <gi...@apache.org>.
rtpsw commented on PR #33676:
URL: https://github.com/apache/arrow/pull/33676#issuecomment-1407383636
I merged with master to get the missing test cases and then fixed them similarly.
--
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] westonpace commented on a diff in pull request #33676: GH-33673: [C++] Standardize as-of-join convention for past and future tolerance
Posted by GitBox <gi...@apache.org>.
westonpace commented on code in PR #33676:
URL: https://github.com/apache/arrow/pull/33676#discussion_r1082894510
##########
cpp/src/arrow/compute/exec/options.h:
##########
@@ -523,7 +523,8 @@ class ARROW_EXPORT AsofJoinNodeOptions : public ExecNodeOptions {
///
/// \see `Keys` for details.
std::vector<Keys> input_keys;
- /// \brief Tolerance for inexact "on" key matching. Must be non-negative.
+ /// \brief Tolerance for inexact "on" key matching. Positive (resp. negative) tolerance
+ /// is interpreted as a future (resp. past) as-of-join.
Review Comment:
The `resp.` abbreviation is unknown to me. Perhaps we can be more explicit here.
```suggestion
/// \brief Tolerance for inexact "on" key matching. Two rows are only considered a match
/// if `left.on - right.on < tolerance`. `tolerance` may be negative and, if so, right side
/// rows may even have a greater `on` value than the left side.
```
--
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] rtpsw commented on pull request #33676: GH-33673: [C++] Standardize as-of-join convention for past and future tolerance
Posted by "rtpsw (via GitHub)" <gi...@apache.org>.
rtpsw commented on PR #33676:
URL: https://github.com/apache/arrow/pull/33676#issuecomment-1407368585
Yes, the test was failing because it used the pre-PR tolerance convention. I just fixed 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] github-actions[bot] commented on pull request #33676: GH-33673: [C++] Standardize as-of-join convention for past and future tolerance
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #33676:
URL: https://github.com/apache/arrow/pull/33676#issuecomment-1383162993
* Closes: #33673
--
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