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: calculate decode priority correctly #2841

Merged

Conversation

westonpace
Copy link
Contributor

@westonpace westonpace commented Sep 7, 2024

When decoding we have to "wait until more than X rows are ready"

There was an issue with how we handled empty lists. For example, if the list offsets are:

# List sizes = [10, 20, 30, 0, 40]
0 10 30 60 60 100

The question is what do we do when we need to wait until more than 3 rows are ready. 3 rows means we have 60 items. The old logic looked for the next offset greater than 60 (i.e. 100) and waited until we had 100 items. This mirrors some logic we do on the reverse side when we calculate how many rows are available based on how many items we have.

However, that is wrong. If we have 60 items we actually have 4 rows available and so we have more than 3 rows available. The logic is much simpler. Just wait until we have at least offsets[desired_rows + 1] items. If that value is equal to offsets[desired_rows] it doesn't matter, we don't need to do any special logic.

If we wait for too many rows it can lead to deadlock in certain situations.

@github-actions github-actions bot added bug Something isn't working python labels Sep 7, 2024
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 64.28571% with 5 lines in your changes missing coverage. Please review.

Project coverage is 78.07%. Comparing base (ef0953d) to head (009fb96).

Files with missing lines Patch % Lines
.../lance-encoding/src/encodings/logical/primitive.rs 61.53% 4 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2841      +/-   ##
==========================================
+ Coverage   78.03%   78.07%   +0.03%     
==========================================
  Files         229      229              
  Lines       70299    70305       +6     
  Branches    70299    70305       +6     
==========================================
+ Hits        54859    54891      +32     
+ Misses      12340    12327      -13     
+ Partials     3100     3087      -13     
Flag Coverage Δ
unittests 78.07% <64.28%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@albertlockett albertlockett left a comment

Choose a reason for hiding this comment

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

Nice logging improvements too

@westonpace westonpace merged commit c8fe1dd into lancedb:main Sep 9, 2024
26 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants