-
Notifications
You must be signed in to change notification settings - Fork 80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf: Embed WindowCheck in TimeSeriesFilter. #6081
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We addressed many concerns inline during the PR.
TestClock should be converged with existing.
TimeSeriesFilter does not need a MergedListener, just a plain old ListenerImpl.
engine/table/src/main/java/io/deephaven/engine/table/impl/WouldMatchOperation.java
Outdated
Show resolved
Hide resolved
} else if (refilterUnmatchedRequested) { | ||
// things that are added or removed are already reflected in source.getRowSet | ||
final WritableRowSet unmatchedRows = source.getRowSet().minus(getRowSet()); | ||
// we must check rows that have been modified instead of just preserving them | ||
if (upstream != null) { | ||
unmatchedRows.insert(upstream.modified()); | ||
} | ||
if (refilterRequestedRowset != null) { | ||
unmatchedRows.insert(refilterRequestedRowset); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We copy()
unmatchedRows below, and I think that is redundant. As-is, we're still never closing it.
engine/table/src/main/java/io/deephaven/engine/table/impl/select/WhereFilter.java
Outdated
Show resolved
Hide resolved
|
||
// Note that removed must be propagated to listeners in pre-shift keyspace. | ||
if (upstream != null) { | ||
upstream.shifted().unapply(postShiftRemovals); | ||
} | ||
update.removed.writableCast().insert(postShiftRemovals); | ||
update.removed = postShiftRemovals; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should close removed
, or just never assign it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a comment related to this in the other part of the code where removed is created originally. Because the shift aware updates support added + removed, we need to include the original removed from the update, plus our new removals to account for rows that are added/removed as a modify w/o lighting up the entire MCS.
Fixes #6062.