You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "rtpsw (via GitHub)" <gi...@apache.org> on 2023/07/09 12:43:12 UTC
[GitHub] [arrow] rtpsw opened a new pull request, #36580: GH-36482: [C++][CI] Fix sporadic test failures in AsofJoinBasicTest
rtpsw opened a new pull request, #36580:
URL: https://github.com/apache/arrow/pull/36580
### What changes are included in this PR?
The key hasher is invalidated before the first invocation of `GetKey` (via `GetLatestKey`) after a new batch arrives. In the pre-PR code, this invalidation happens within `Advance`, which is called from `AdvanceAndMemoize` only after `GetLatestKey` is called. The change adds numbering to record batches pushed to the processing thread to enable detecting the pushed record batch in order to then invalidate the key hasher.
### Are these changes tested?
Yes, by existing tests.
### Are there any user-facing changes?
No.
**This PR contains a "Critical Fix".**
* Closes: #36482
--
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 #36580: GH-36482: [C++][CI] Fix sporadic test failures in AsofJoinBasicTest
Posted by "rtpsw (via GitHub)" <gi...@apache.org>.
rtpsw closed pull request #36580: GH-36482: [C++][CI] Fix sporadic test failures in AsofJoinBasicTest
URL: https://github.com/apache/arrow/pull/36580
--
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 #36580: GH-36482: [C++][CI] Fix sporadic test failures in AsofJoinBasicTest
Posted by "rtpsw (via GitHub)" <gi...@apache.org>.
rtpsw commented on code in PR #36580:
URL: https://github.com/apache/arrow/pull/36580#discussion_r1258813028
##########
cpp/src/arrow/acero/asof_join_node.cc:
##########
@@ -812,15 +826,15 @@ class InputState {
}
latest_time_ = next_time;
// If we have an active batch
- if (++latest_ref_row_ >= (row_index_t)queue_.UnsyncFront()->num_rows()) {
+ if (++latest_ref_row_ >= (row_index_t)queue_.UnsyncFront().batch->num_rows()) {
// hit the end of the batch, need to get the next batch if possible.
++batches_processed_;
latest_ref_row_ = 0;
have_active_batch &= !queue_.TryPop();
if (have_active_batch) {
- DCHECK_GT(queue_.UnsyncFront()->num_rows(), 0); // empty batches disallowed
- key_hasher_->Invalidate(); // batch changed - invalidate key hasher's cache
- memo_.UpdateTime(GetTime(queue_.UnsyncFront().get(), 0)); // time changed
+ DCHECK_GT(queue_.UnsyncFront().batch->num_rows(),
+ 0); // empty batches disallowed
+ memo_.UpdateTime(GetTime(queue_.UnsyncFront().batch.get(), 0)); // time changed
Review Comment:
Yes, because only the processing thread, which runs this code, is popping from the (front of the) queue.
--
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 pull request #36580: GH-36482: [C++][CI] Fix sporadic test failures in AsofJoinBasicTest
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #36580:
URL: https://github.com/apache/arrow/pull/36580#issuecomment-1627822011
@github-actions crossbow submit verify-rc-source-*macos*
--
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 a diff in pull request #36580: GH-36482: [C++][CI] Fix sporadic test failures in AsofJoinBasicTest
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #36580:
URL: https://github.com/apache/arrow/pull/36580#discussion_r1258461339
##########
cpp/src/arrow/acero/asof_join_node.cc:
##########
@@ -812,15 +826,15 @@ class InputState {
}
latest_time_ = next_time;
// If we have an active batch
- if (++latest_ref_row_ >= (row_index_t)queue_.UnsyncFront()->num_rows()) {
+ if (++latest_ref_row_ >= (row_index_t)queue_.UnsyncFront().batch->num_rows()) {
// hit the end of the batch, need to get the next batch if possible.
++batches_processed_;
latest_ref_row_ = 0;
have_active_batch &= !queue_.TryPop();
if (have_active_batch) {
- DCHECK_GT(queue_.UnsyncFront()->num_rows(), 0); // empty batches disallowed
- key_hasher_->Invalidate(); // batch changed - invalidate key hasher's cache
- memo_.UpdateTime(GetTime(queue_.UnsyncFront().get(), 0)); // time changed
+ DCHECK_GT(queue_.UnsyncFront().batch->num_rows(),
+ 0); // empty batches disallowed
+ memo_.UpdateTime(GetTime(queue_.UnsyncFront().batch.get(), 0)); // time changed
Review Comment:
Are you sure that `queue_.UnsyncFront()` here is the same as just above?
--
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 #36580: GH-36482: [C++][CI] Fix sporadic test failures in AsofJoinBasicTest
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36580:
URL: https://github.com/apache/arrow/pull/36580#issuecomment-1627822634
Revision: 58a648b4e500750ffa1fe97abd672c3d4280c954
Submitted crossbow builds: [ursacomputing/crossbow @ actions-cdacd2d9b4](https://github.com/ursacomputing/crossbow/branches/all?query=actions-cdacd2d9b4)
|Task|Status|
|----|------|
|verify-rc-source-cpp-macos-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/5501889107/jobs/10025783715)|
|verify-rc-source-cpp-macos-arm64|[](https://github.com/ursacomputing/crossbow/actions/runs/5501889707/jobs/10025784685)|
|verify-rc-source-cpp-macos-conda-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/5501889639/jobs/10025784569)|
|verify-rc-source-csharp-macos-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/5501888777/jobs/10025783190)|
|verify-rc-source-csharp-macos-arm64|[](https://github.com/ursacomputing/crossbow/actions/runs/5501889183/jobs/10025783839)|
|verify-rc-source-go-macos-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/5501889788/jobs/10025784802)|
|verify-rc-source-go-macos-arm64|[](https://github.com/ursacomputing/crossbow/actions/runs/5501888849/jobs/10025783294)|
|verify-rc-source-integration-macos-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/5501889346/jobs/10025784115)|
|verify-rc-source-integration-macos-arm64|[](https://github.com/ursacomputing/crossbow/actions/runs/5501888687/jobs/10025783060)|
|verify-rc-source-integration-macos-conda-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/5501889277/jobs/10025784014)|
|verify-rc-source-java-macos-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/5501889033/jobs/10025783625)|
|verify-rc-source-js-macos-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/5501889932/jobs/10025785042)|
|verify-rc-source-js-macos-arm64|[](https://github.com/ursacomputing/crossbow/actions/runs/5501889862/jobs/10025784917)|
|verify-rc-source-python-macos-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/5501888961/jobs/10025783508)|
|verify-rc-source-python-macos-arm64|[](https://github.com/ursacomputing/crossbow/actions/runs/5501890007/jobs/10025785168)|
|verify-rc-source-python-macos-conda-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/5501889488/jobs/10025784347)|
|verify-rc-source-ruby-macos-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/5501889413/jobs/10025784239)|
|verify-rc-source-ruby-macos-arm64|[](https://github.com/ursacomputing/crossbow/actions/runs/5501888909/jobs/10025783409)|
--
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 #36580: GH-36482: [C++][CI] Fix sporadic test failures in AsofJoinBasicTest
Posted by "rtpsw (via GitHub)" <gi...@apache.org>.
rtpsw commented on PR #36580:
URL: https://github.com/apache/arrow/pull/36580#issuecomment-1629619031
In a discussion with @icexelloss, we decided to close 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