westonpace commented on code in PR #34392:
URL: https://github.com/apache/arrow/pull/34392#discussion_r1169677872
##########
cpp/src/arrow/acero/asof_join_node.cc:
##########
@@ -249,12 +250,18 @@ struct MemoStore {
void swap(MemoStore& memo) {
std::swap(no_future_, memo.no_future_);
- std::swap(current_time_, memo.current_time_);
+ current_time_ = memo.current_time_.exchange((OnType)current_time_);
entries_.swap(memo.entries_);
future_entries_.swap(memo.future_entries_);
times_.swap(memo.times_);
}
+ void UpdateTime(OnType ts) {
+ OnType prev_time = current_time_;
+ while (prev_time < ts && current_time_.compare_exchange_weak(prev_time,
ts)) {
Review Comment:
Why use a `while` loop here instead of a single call to
`compare_exchange_strong`?
##########
cpp/src/arrow/acero/asof_join_node.cc:
##########
@@ -569,7 +565,7 @@ class InputState {
// returns the latest time (which is current); otherwise, returns the
current time.
// used when checking whether RHS is up to date with LHS.
OnType GetCurrentTime() const {
- return memo_.no_future_ ? GetLatestTime() : memo_.current_time_;
+ return memo_.no_future_ ? GetLatestTime() : (OnType)memo_.current_time_;
Review Comment:
Prefer `static_cast`.
##########
cpp/src/arrow/acero/asof_join_node.cc:
##########
@@ -707,15 +704,16 @@ class InputState {
updated = memo_.no_future_;
ARROW_ASSIGN_OR_RAISE(advanced, Advance());
} while (advanced);
- if (!memo_.no_future_) { // "updated" was not modified in the loop; set
it here
+ if (!memo_.no_future_ && latest_time >= ts) {
+ // "updated" was not modified in the loop; set it here
updated = memo_.RemoveEntriesWithLesserTime(ts);
}
return updated;
}
void Rehash() {
MemoStore new_memo(memo_.no_future_);
- new_memo.current_time_ = memo_.current_time_;
+ new_memo.current_time_ = (OnType)memo_.current_time_;
Review Comment:
Minor nit: Prefer `static_cast`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
