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/07 21:59:25 UTC
[GitHub] [arrow] rtpsw opened a new pull request, #14934: ARROW-18427: [C++] Support negative tolerance in `AsofJoinNode`
rtpsw opened a new pull request, #14934:
URL: https://github.com/apache/arrow/pull/14934
See https://issues.apache.org/jira/browse/ARROW-18427
--
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 #14934: ARROW-18427: [C++] Support negative tolerance in `AsofJoinNode`
Posted by GitBox <gi...@apache.org>.
rtpsw commented on PR #14934:
URL: https://github.com/apache/arrow/pull/14934#issuecomment-1369113354
> Generally, yes. There should not be regression on master. Specifically, I think it's probably ok to have a regression here. We haven't really released the asof join node (I don't see it in the 9.0.0 or 10.0.0 release notes) and there is no mention of it in the documentation.
I'll see what I can conveniently do to improve.
--
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 #14934: ARROW-18427: [C++] Support negative tolerance in `AsofJoinNode`
Posted by GitBox <gi...@apache.org>.
rtpsw commented on PR #14934:
URL: https://github.com/apache/arrow/pull/14934#issuecomment-1369115082
> Sorry for the delay looking at this (Christmas vacation). This looks like a solid addition, though it will need a rebase so I am marking "request changes".
Sure.
> Is negative tolerance something that is often used? Wouldn't it be more intuitive to specify an absolute tolerance that works in both directions? (e.g. grab the closest entry within 20ms of the LHS in either direction)?
@icexelloss brought up the negative tolerance, so he should be able to say more about the use cases. Regarding the idea of both-directions, I still can't speak about the use cases, but my guess is the implementation shouldn't be too hard to include in this PR (likely by changing `bool negative` to an enum).
--
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 #14934: ARROW-18427: [C++] Support negative tolerance in `AsofJoinNode`
Posted by GitBox <gi...@apache.org>.
rtpsw commented on PR #14934:
URL: https://github.com/apache/arrow/pull/14934#issuecomment-1349019853
cc @icexelloss @westonpace
My challenge in is this PR was to add support for a negative tolerance without modifying the original code and algorithm beyond recognition. This is not so simple because a negative tolerance requires a future join, which requires per-key entry queues, and the original code was not designed with this in mind. Hopefully, the PR makes sense. The additional and well-covering test cases give me enough confidence in 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] westonpace commented on a diff in pull request #14934: ARROW-18427: [C++] Support negative tolerance in `AsofJoinNode`
Posted by GitBox <gi...@apache.org>.
westonpace commented on code in PR #14934:
URL: https://github.com/apache/arrow/pull/14934#discussion_r1060140237
##########
cpp/src/arrow/compute/exec/asof_join_node.cc:
##########
@@ -140,6 +172,17 @@ struct MemoStore {
// Stores last known values for all the keys
struct Entry {
+ Entry() = default;
+
+ Entry(OnType time, std::shared_ptr<arrow::RecordBatch> batch, row_index_t row)
+ : time(time), batch(batch), row(row) {}
+
+ void swap(Entry& other) {
Review Comment:
We generally discourage mutable references but given this is supposed to be an analogue of `std::swap` it seems 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] westonpace commented on pull request #14934: ARROW-18427: [C++] Support negative tolerance in `AsofJoinNode`
Posted by GitBox <gi...@apache.org>.
westonpace commented on PR #14934:
URL: https://github.com/apache/arrow/pull/14934#issuecomment-1374622011
I don't know why the checks didn't run. Once this is passing I think we can merge it. Can you close and reopen the PR real quick so the tests run?
--
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 #14934: ARROW-18427: [C++] Support negative tolerance in `AsofJoinNode`
Posted by GitBox <gi...@apache.org>.
westonpace commented on PR #14934:
URL: https://github.com/apache/arrow/pull/14934#issuecomment-1369103040
> I should note that there are opportunities to improve performance, in particular around the management of shared pointers to record batches. I could handle this in a separate issue, unless you say as-of-join performance must be ensured in every push to the master branch.
Generally, yes. There should not be regression on master. Specifically, I think it's probably ok to have a regression here. We haven't really released the asof join node (I don't see it in the 9.0.0 or 10.0.0 release notes) and there is no mention of it in the documentation.
--
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 #14934: ARROW-18427: [C++] Support negative tolerance in `AsofJoinNode`
Posted by GitBox <gi...@apache.org>.
rtpsw commented on PR #14934:
URL: https://github.com/apache/arrow/pull/14934#issuecomment-1374617059
@icexelloss confirms the use case of negative tolerance is a recurring one, and says that the use case of a double-sided tolerance is not, at least for the time being. @westonpace, could you clarify the work remaining for this PR?
--
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 #14934: ARROW-18427: [C++] Support negative tolerance in `AsofJoinNode`
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #14934:
URL: https://github.com/apache/arrow/pull/14934#issuecomment-1349003729
https://issues.apache.org/jira/browse/ARROW-18427
--
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 #14934: ARROW-18427: [C++] Support negative tolerance in `AsofJoinNode`
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #14934:
URL: https://github.com/apache/arrow/pull/14934#issuecomment-1374951750
['Python', 'R'] benchmarks have high level of regressions.
[ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/eb77fbe82ed94444858818dd4f7ff2df...4a08b725ca09414fa002145bda87032e/)
--
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 #14934: ARROW-18427: [C++] Support negative tolerance in `AsofJoinNode`
Posted by GitBox <gi...@apache.org>.
rtpsw commented on PR #14934:
URL: https://github.com/apache/arrow/pull/14934#issuecomment-1374743963
@westonpace, looks like this should be ready 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] ursabot commented on pull request #14934: ARROW-18427: [C++] Support negative tolerance in `AsofJoinNode`
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #14934:
URL: https://github.com/apache/arrow/pull/14934#issuecomment-1374951689
Benchmark runs are scheduled for baseline = 686610374ca52b39354df668a66e5fd4103b86d7 and contender = 2acc51a7d5304c3fc6a432f1c09946547ca91d74. 2acc51a7d5304c3fc6a432f1c09946547ca91d74 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/eb79ce6c86294cc0b9d77c4fbcf7c36e...031c870714844d1e83e0a3265f012373/)
[Finished :arrow_down:2.72% :arrow_up:2.13%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/a95472bfd6494a74ac8b3420a9c274c7...de073ad0c1f44b67999f8232e0666c5e/)
[Finished :arrow_down:2.04% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/eb77fbe82ed94444858818dd4f7ff2df...4a08b725ca09414fa002145bda87032e/)
[Finished :arrow_down:1.14% :arrow_up:0.34%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/6d4d8bf5b85d4029af8b2e3734216fa8...2aa25d27ac5e4bbba75821119e3a3420/)
Buildkite builds:
[Finished] [`2acc51a7` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2131)
[Finished] [`2acc51a7` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2154)
[Finished] [`2acc51a7` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2126)
[Finished] [`2acc51a7` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2146)
[Finished] [`68661037` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2130)
[Finished] [`68661037` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2153)
[Finished] [`68661037` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2125)
[Finished] [`68661037` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2145)
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] rtpsw commented on pull request #14934: ARROW-18427: [C++] Support negative tolerance in `AsofJoinNode`
Posted by GitBox <gi...@apache.org>.
rtpsw commented on PR #14934:
URL: https://github.com/apache/arrow/pull/14934#issuecomment-1351345638
I should note that there are opportunities to improve performance, in particular around the management of shared pointers to record batches. I could handle this in a separate issue, unless you say as-of-join performance must be ensured in every push to the master branch.
--
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 #14934: ARROW-18427: [C++] Support negative tolerance in `AsofJoinNode`
Posted by GitBox <gi...@apache.org>.
westonpace merged PR #14934:
URL: https://github.com/apache/arrow/pull/14934
--
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 closed pull request #14934: ARROW-18427: [C++] Support negative tolerance in `AsofJoinNode`
Posted by GitBox <gi...@apache.org>.
rtpsw closed pull request #14934: ARROW-18427: [C++] Support negative tolerance in `AsofJoinNode`
URL: https://github.com/apache/arrow/pull/14934
--
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 #14934: ARROW-18427: [C++] Support negative tolerance in `AsofJoinNode`
Posted by GitBox <gi...@apache.org>.
rtpsw commented on PR #14934:
URL: https://github.com/apache/arrow/pull/14934#issuecomment-1358947677
@westonpace, could you take a look?
--
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