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: don't use pushdown scan if a custom batch size has been set #2277

Merged

Conversation

westonpace
Copy link
Contributor

The pushdown scan currently requires fully materializing a batch in memory. As a result, it is not capable of supporting the batch_size parameter in the scanner. This fix is not great, it disables pushdown optimization when a batch size is specified, but if users truly need a batch size (to constrain memory) then its better to give them what they ask for at higher cost.

I would like to do a more correct fix soon but probably not until I work on pushdown for v2 (I'm still not sure if the "late materialization" will be part of the file reader itself or if we still need a dedicated pushdown node.

@github-actions github-actions bot added the bug Something isn't working label Apr 30, 2024
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 96.29630% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 80.88%. Comparing base (098f730) to head (78203f4).

Files Patch % Lines
rust/lance/src/dataset/scanner.rs 96.29% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2277      +/-   ##
==========================================
- Coverage   80.88%   80.88%   -0.01%     
==========================================
  Files         190      190              
  Lines       55866    55869       +3     
  Branches    55866    55869       +3     
==========================================
+ Hits        45190    45192       +2     
- Misses       8158     8159       +1     
  Partials     2518     2518              
Flag Coverage Δ
unittests 80.88% <96.29%> (-0.01%) ⬇️

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.

@westonpace westonpace merged commit 1f06f6e into lancedb:main May 1, 2024
18 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants