-
Notifications
You must be signed in to change notification settings - Fork 256
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: ensure that 'block_size' parameter is properly propagated in the ObjectStore #3403
Conversation
ACTION NEEDED The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification. For details on the error please inspect the "PR Title Check" action. |
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.
Good job catching this issue. I have a minor change needed on the unit test, but otherwise this looks great.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3403 +/- ##
==========================================
+ Coverage 78.72% 78.73% +0.01%
==========================================
Files 250 250
Lines 90879 90915 +36
Branches 90879 90915 +36
==========================================
+ Hits 71540 71580 +40
+ Misses 16397 16383 -14
- Partials 2942 2952 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@wjones127 It looks like it is complaining about not being able to update the lock file (https://github.com/lancedb/lance/actions/runs/12918084538/job/36026972853?pr=3403). Should I be including an updated lock file in this change? I didn't have to do it locally for the tests to pass. If so, should I be running cargo update (which is going to update a lot), or something else? |
This happened because you changed the You should NOT run |
Added the Cargo.lock file for the lance-io directory. When I ran |
Previously, setting a block_size parameter did not change the range coalescing being done in FileScheduler's submit_request function. This was because the FileScheduler was using the block_size parameter in the underlying ObjectStore, but that parameter was not being properly propagated in the configure_store method.
This change keeps the same defaults but always uses the parameter value if it is set.
Verified via unit tests and profiling to determine increasing block size now does actually decrease the number of get_range queries.