Skip to content
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

Merged
merged 21 commits into from
Apr 11, 2024
Merged

Conversation

wusteven815
Copy link
Contributor

@wusteven815 wusteven815 commented Mar 8, 2024

  • Change algorithm for sorted columns
    • Use SortedColumnsAttribute.getOrderForColumn instead of guessSorted to guarantee
    • Add a binary search algorithm to find first/last occurrence in a sorted list
    • Check next/prev row first, then loop around to end/start and get last/first occurrence
      • e.g. for [1, 2, 2, 3] and on the last 2, the algorithm would check the next value (3) first, then search for first occurrence of 1, 2, 2
  • Change algorithm for unsorted columns to return -1 when not found, instead of the closest value
  • Replace ConcurrentMethod with snapshots

@wusteven815 wusteven815 self-assigned this Mar 8, 2024
@wusteven815 wusteven815 marked this pull request as ready for review March 8, 2024 20:41
@wusteven815 wusteven815 added the bug Something isn't working label Mar 8, 2024
Copy link
Member

@niloc132 niloc132 left a 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?

Copy link
Member

@rcaudy rcaudy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial comments.

Copy link
Member

@niloc132 niloc132 left a 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).

@wusteven815 wusteven815 merged commit b1f657b into deephaven:main Apr 11, 2024
15 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working NoDocumentationNeeded NoReleaseNotesNeeded No release notes are needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants