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!: simplify take row api #2664

Merged
merged 8 commits into from
Jul 31, 2024
Merged

feat!: simplify take row api #2664

merged 8 commits into from
Jul 31, 2024

Conversation

eddyxu
Copy link
Contributor

@eddyxu eddyxu commented Jul 30, 2024

Change take_rows and take APIs to allow directly take Schema again.

@github-actions github-actions bot added enhancement New feature or request breaking-change labels Jul 30, 2024
@eddyxu eddyxu requested review from westonpace and wjones127 and removed request for westonpace July 30, 2024 20:11
@eddyxu
Copy link
Contributor Author

eddyxu commented Jul 30, 2024

Windows failure is not relevant

@codecov-commenter
Copy link

codecov-commenter commented Jul 30, 2024

Codecov Report

Attention: Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 79.78%. Comparing base (c43424c) to head (61b1dfa).

Files Patch % Lines
rust/lance/src/dataset.rs 90.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2664      +/-   ##
==========================================
- Coverage   79.79%   79.78%   -0.01%     
==========================================
  Files         224      224              
  Lines       65871    65871              
  Branches    65871    65871              
==========================================
- Hits        52562    52558       -4     
- Misses      10228    10232       +4     
  Partials     3081     3081              
Flag Coverage Δ
unittests 79.78% <90.00%> (-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.

Comment on lines 272 to 276
impl From<&Schema> for ProjectionRequest {
fn from(schema: &Schema) -> Self {
Self::from(schema.clone())
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather not have this personally. I don't like the idea of there being a hidden clone in the call. The caller can just do take(row_indices, schema.clone()), and then it's clearer the schema is being cloned there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

wjones127 and others added 3 commits July 31, 2024 09:04
Closes #2610

* Supports subschemas in `merge_insert` for updates only
  * Inserts and deletes left as TODO
* Field id `-2` is now reserved as a field "tombstone". These tombstones
are fields that are no longer in the schema, usually because those
fields are now in a different data file.
* Fixed a bug in `Merger` where statistics were reset on each batch.
…rust 1.80 (#2641)

Co-authored-by: Will Jones <willjones127@gmail.com>
Co-authored-by: Lance Release <lance-dev@lancedb.com>
Co-authored-by: rmeng <rob.xu.meng@gmail.com>
@wjones127
Copy link
Contributor

Thanks for doing this. I like this interface a lot better.

@eddyxu eddyxu merged commit b8bda30 into main Jul 31, 2024
23 of 25 checks passed
@eddyxu eddyxu deleted the lei/simplify_take_rows_api branch July 31, 2024 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants