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

feat: add 2.1 read path #2968

Merged
merged 4 commits into from
Oct 25, 2024
Merged

feat: add 2.1 read path #2968

merged 4 commits into from
Oct 25, 2024

Conversation

westonpace
Copy link
Contributor

@westonpace westonpace commented Oct 2, 2024

Unlike the write path we were not able to get away with subtle changes to the existing traits. Most of the read traits needed to be duplicated. On the bright side, there is very little impact to the existing reader code though :)

@github-actions github-actions bot added the enhancement New feature or request label Oct 2, 2024
@westonpace westonpace changed the title feat: add structural decoders to lance feat: add structural encoders / decoders to lance (outline of 2.1 scheme) Oct 2, 2024
@github-actions github-actions bot added the python label Oct 8, 2024
@westonpace westonpace force-pushed the feat/repdef branch 4 times, most recently from 200acf0 to 65b00c1 Compare October 15, 2024 15:31
@westonpace westonpace changed the title feat: add structural encoders / decoders to lance (outline of 2.1 scheme) feat: add 2.1 read path Oct 15, 2024
@westonpace westonpace marked this pull request as ready for review October 18, 2024 22:47
@westonpace westonpace requested review from broccoliSpicy and wjones127 and removed request for broccoliSpicy October 18, 2024 22:47
Copy link
Contributor

@broccoliSpicy broccoliSpicy left a comment

Choose a reason for hiding this comment

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

test issues in lance datafusion, others LGTM

Comment on lines +533 to +534
// TODO: Not that trivial, we might have rep/def that we need to encode / decode still
todo!()
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these be filled in for this PR? Or are we just not writing them yet so it doesn't matter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, there are a fair number of TODO's in this PR that I'm leaving for future PRs.

Cleanup after rebase

Add a few more benchmarks

Fix various bugs in page size / batch size / validity handling

Add copyright header
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 84.11879% with 246 lines in your changes missing coverage. Please review.

Project coverage is 78.30%. Comparing base (f17d88d) to head (a6d2c44).

Files with missing lines Patch % Lines
.../lance-encoding/src/encodings/logical/primitive.rs 85.45% 63 Missing and 25 partials ⚠️
rust/lance-encoding/src/decoder.rs 77.68% 54 Missing and 27 partials ⚠️
...ust/lance-encoding/src/encodings/logical/struct.rs 89.65% 13 Missing and 5 partials ⚠️
rust/lance-file/src/v2/testing.rs 0.00% 11 Missing ⚠️
rust/lance-index/src/vector/ivf/shuffler.rs 16.66% 10 Missing ⚠️
...ust/lance-encoding/src/encodings/logical/binary.rs 0.00% 9 Missing ⚠️
...ust/lance-encoding/src/encodings/physical/value.rs 90.16% 6 Missing ⚠️
rust/lance-file/src/v2/reader.rs 92.00% 1 Missing and 5 partials ⚠️
rust/lance-encoding/src/format.rs 16.66% 5 Missing ⚠️
rust/lance-encoding-datafusion/src/lib.rs 0.00% 4 Missing ⚠️
... and 3 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2968      +/-   ##
==========================================
+ Coverage   78.24%   78.30%   +0.05%     
==========================================
  Files         240      240              
  Lines       77284    78355    +1071     
  Branches    77284    78355    +1071     
==========================================
+ Hits        60470    61354     +884     
- Misses      13696    13902     +206     
+ Partials     3118     3099      -19     
Flag Coverage Δ
unittests 78.30% <84.11%> (+0.05%) ⬆️

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 b1abfff into lancedb:main Oct 25, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants