-
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
fix: seek row for sorted columns #5238
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.
If we're confident we know what's wrong here, can we add a unit test or two to verify behavior?
ClientSupport/src/main/java/io/deephaven/clientsupport/gotorow/SeekRow.java
Show resolved
Hide resolved
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.
Partial comments.
ClientSupport/src/main/java/io/deephaven/clientsupport/gotorow/SeekRow.java
Outdated
Show resolved
Hide resolved
ClientSupport/src/main/java/io/deephaven/clientsupport/gotorow/SeekRow.java
Outdated
Show resolved
Hide resolved
ClientSupport/src/main/java/io/deephaven/clientsupport/gotorow/SeekRow.java
Outdated
Show resolved
Hide resolved
ClientSupport/src/main/java/io/deephaven/clientsupport/gotorow/SeekRow.java
Outdated
Show resolved
Hide resolved
ClientSupport/src/main/java/io/deephaven/clientsupport/gotorow/SeekRow.java
Outdated
Show resolved
Hide resolved
ClientSupport/src/main/java/io/deephaven/clientsupport/gotorow/SeekRow.java
Show resolved
Hide resolved
ClientSupport/src/main/java/io/deephaven/clientsupport/gotorow/SeekRow.java
Outdated
Show resolved
Hide resolved
ClientSupport/src/main/java/io/deephaven/clientsupport/gotorow/SeekRow.java
Outdated
Show resolved
Hide resolved
ClientSupport/src/main/java/io/deephaven/clientsupport/gotorow/SeekRow.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Ryan Caudy <[email protected]>
Co-authored-by: Ryan Caudy <[email protected]>
Co-authored-by: Ryan Caudy <[email protected]>
Co-authored-by: Ryan Caudy <[email protected]>
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'm spending a little time this morning going over the test cases, might propose a patch to add more meat to the tests to ensure we don't need to revisit again.
Also, I will probably propose a change to chunk the non-sorted cases to make them more efficient (so again the tests will help there).
ClientSupport/src/test/java/io/deephaven/clientsupport/gotorow/SeekRowTest.java
Outdated
Show resolved
Hide resolved
ClientSupport/src/main/java/io/deephaven/clientsupport/gotorow/SeekRow.java
Outdated
Show resolved
Hide resolved
ClientSupport/src/main/java/io/deephaven/clientsupport/gotorow/SeekRow.java
Outdated
Show resolved
Hide resolved
ClientSupport/src/main/java/io/deephaven/clientsupport/gotorow/SeekRow.java
Show resolved
Hide resolved
ClientSupport/src/main/java/io/deephaven/clientsupport/gotorow/SeekRow.java
Outdated
Show resolved
Hide resolved
ClientSupport/src/main/java/io/deephaven/clientsupport/gotorow/SeekRow.java
Outdated
Show resolved
Hide resolved
ClientSupport/src/test/java/io/deephaven/clientsupport/gotorow/SeekRowTest.java
Show resolved
Hide resolved
Co-authored-by: Colin Alworth <[email protected]>
SortedColumnsAttribute.getOrderForColumn
instead ofguessSorted
to guarantee[1, 2, 2, 3]
and on the last2
, the algorithm would check the next value (3
) first, then search for first occurrence of1, 2, 2
-1
when not found, instead of the closest valueConcurrentMethod
with snapshots