-
Notifications
You must be signed in to change notification settings - Fork 215
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
test: test write methods against more varied layouts #2233
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2233 +/- ##
==========================================
+ Coverage 81.23% 81.42% +0.18%
==========================================
Files 187 189 +2
Lines 54698 55176 +478
Branches 54698 55176 +478
==========================================
+ Hits 44434 44925 +491
+ Misses 7771 7741 -30
- Partials 2493 2510 +17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
/// Convert reader to a stream. | ||
/// | ||
/// The reader will be called in a background thread. | ||
pub fn reader_to_stream(batches: Box<dyn RecordBatchReader + Send>) -> SendableRecordBatchStream { |
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.
Found we sometimes didn't need to peek the reader, so separated this into two different functions: peek_reader_schema()
and reader_to_stream()
.
let batches = vec![data.slice(0, 50), data.slice(50, 50)]; | ||
let mut dataset = TestDatasetGenerator::new(batches) | ||
.make_hostile(test_uri) | ||
.await; | ||
|
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.
This is the one test I've integrated this with. In follow up PRs, we can use this in additional tests.
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.
The current Fragment::create()
API didn't allow passing an explicit lance::Schema
, but we need that to generate layouts with specific field ids. So I moved this all over to a nice builder.
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.
This looks pretty cool.
let mut writer = FileWriter::<ManifestDescribing>::try_new( | ||
&object_store, | ||
&full_path, | ||
schema, | ||
&Default::default(), | ||
) | ||
.await?; |
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.
This is no different than before but I suppose we will need to update this path to use the v2 writer as well.
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 point. I've added a TODO to #1929
Adds a test utility to generate a more hostile layout. Adds to one test function. Will add to more in follow up PRs.