feat: support complex schemas in append - #2209
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2209 +/- ##
========================================
Coverage 81.10% 81.10%
========================================
Files 184 184
Lines 52469 53102 +633
Branches 52469 53102 +633
========================================
+ Hits 42555 43071 +516
- Misses 7458 7558 +100
- Partials 2456 2473 +17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| let (stream, schema) = reader_to_stream(reader).await?; | ||
|
|
||
| let schema = if matches!(params.mode, WriteMode::Append) { | ||
| // TODO: we need to pass down params somehow. |
westonpace
left a comment
There was a problem hiding this comment.
I'm not sure how holes are handled given we only have min and max? Looks ok otherwise though.
| Err(Error::DatasetNotFound { .. }) => { | ||
| // If the dataset does not exist, we can use the schema from | ||
| // the reader. | ||
| schema | ||
| } |
There was a problem hiding this comment.
If the mode is append and the dataset does not exist isn't this an error?
There was a problem hiding this comment.
In write_dataset it's just a warning. It makes it a lot less complicated if we can make append default.
| let mut builder = Int64Builder::with_capacity((num_columns * num_batches) as usize); | ||
| for col in 0..num_columns { | ||
| let mut builder = | ||
| Int64Builder::with_capacity(field_ids.clone().count() * num_batches as usize); |
There was a problem hiding this comment.
Minor nit but I think you can avoid the clone with field_ids.len()
There was a problem hiding this comment.
ExactSizeIterator isn't implemented for i32, unfortunately. I think because it's possible to construct a range that won't fit into 32-bit usize.
| /// Parameters: | ||
| /// * `position`: The start position in the file where the page table is stored. | ||
| /// * `min_field_id`: The smallest field_id that is present in the schema. | ||
| /// * `max_field_id`: The largest field_id that is present in the schema. | ||
| /// * `num_batches`: The number of batches in the file. | ||
| /// | ||
| /// The page table is stored as an array. The on-disk size is determined based | ||
| /// on the `min_field_id`, `max_field_id`, and `num_batches` parameters. If | ||
| /// these are incorrect, the page table will not be read correctly. |
There was a problem hiding this comment.
Given only min and max how do we handle holes? Or is a "hole" only at the beginning or end (and all fields within a file are contiguous)
There was a problem hiding this comment.
Holes are written to the page table as empty pages, just like struct fields. (This is one of the things that makes it very inefficient. If you have field ids 0, 50, it has to store empty page data for all the non-existent fields 1..49.) I can make this explicit in the docs.
| *Experimental API*. Progress tracking for writing the fragment. Pass | ||
| a custom class that defines hooks to be called when each fragment is | ||
| starting to write and finishing writing. | ||
| mode: str, default "append" |
There was a problem hiding this comment.
Should you mention what other modes are available? Will there ever be a distinction between create and overwrite? If not, should this just be a boolean column?
There was a problem hiding this comment.
Are you thinking something like check_existing_schema: bool=True?
There was a problem hiding this comment.
I feel like "append" vs "overwrite" and "create" communicate how to use this in terms that already exist in our APIs. I don't really foresee any difference between "overwrite" and "create".
There was a problem hiding this comment.
I was thinking something like overwrite_existing_schema with a default of False but I don't feel too strongly if you like append.
There was a problem hiding this comment.
Yeah I think I like this as append for now.
Co-authored-by: Weston Pace <weston.pace@gmail.com>
Fixes two bugs, both associated with schemas that have holes in the field ids:
write_fragments()andLanceFragment.create()assume they can derive the field ids from the Arrow schema. This is not the case if there are holes in the schema. Therefore, when the mode isAppend, we check the existing schema of the dataset and use its field ids. Fixes Makewrite_fragmentssafe to work with on schemas with holes in field ids #2179PageTableassumed that there were no holes in the field ids. It was parametrized asfield_id_offsetandnum_fields, assuming the field ids werefield_id_offset..(field_id_offset + num_fields). This is changed to be parametrized by the min and max field id.