Change Parquet API interaction for u64 #7252
Conversation
alchemist51
changed the title
|
I'm trying to change the trait I was trying to do check for what traits we could change in for ParquetWriter though was not able to conclude on it. Pretty new to the arrow-rs community and raising my first PR! Any feedback is welcome! @alamb @tustvold |
| impl<T: AsyncFileReader> MetadataFetch for &mut T { | ||
| fn fetch(&mut self, range: Range<usize>) -> BoxFuture<'_, Result<Bytes>> { | ||
| self.get_bytes(range) | ||
| self.get_bytes(range.start as u64..range.end as u64) |
There was a problem hiding this comment.
I think we should also change this trait
tustvold
added
api-change
|
Marking as draft as I think this PR is no longer waiting on feedback. Please mark it as ready for review when it is ready for another look |
Signed-off-by: Arpit Bandejiya <[email protected]>
alchemist51
force-pushed
the
change-parquet-usize
branch
from
48b297d to
11aa2cb
Compare
Signed-off-by: Arpit Bandejiya <[email protected]>
alchemist51
force-pushed
the
change-parquet-usize
branch
from
03469ad to
fef3719
Compare
Is there any annotation or any file where I need to add these traits to mark for breaking change? |
I think we just need to add a github label with "api change" which we have done |
|
@tustvold could you please run the workflows? |
| let start = page.offset as usize; | ||
| let end = start + page.compressed_page_size as usize; | ||
| ranges.push(start..end); | ||
| ranges.push(start as u64..end as u64); |
There was a problem hiding this comment.
| let start = page.offset as usize; | |
| let end = start + page.compressed_page_size as usize; | |
| ranges.push(start..end); | |
| ranges.push(start as u64..end as u64); | |
| let start = page.offset as u64; | |
| let end = start + page.compressed_page_size as u64; | |
| ranges.push(start..end); |
| fn fetch(&mut self, range: Range<usize>) -> BoxFuture<'_, Result<Bytes>> { | ||
| self.get_bytes(range) | ||
| fn fetch(&mut self, range: Range<u64>) -> BoxFuture<'_, Result<Bytes>> { | ||
| self.get_bytes(range.start..range.end) |
There was a problem hiding this comment.
The trait was updated to also use Range<u64>:
| self.get_bytes(range.start..range.end) | |
| self.get_bytes(range) |
| if read != to_read { | ||
| let mut buffer = Vec::with_capacity(to_read as usize); | ||
| let read = self.take(to_read).read_to_end(&mut buffer).await?; | ||
| if read != to_read as usize { |
There was a problem hiding this comment.
| if read != to_read as usize { | |
| if read as u64 != to_read { |
|
Marking as draft as I think this PR is no longer waiting on feedback. Please mark it as ready for review when it is ready for another look |
|
Closing the PR since it's already being worked upon. Thanks @kylebarron @alamb |
…n 4GB in WASM) (#7371) * Change AsyncFileReader trait for u64 Signed-off-by: Arpit Bandejiya <[email protected]> * update metadatafetch trait Signed-off-by: Arpit Bandejiya <[email protected]> * Fix lint issue * fix tests for latest main * Address comments by @mbrobbel from #7252 * Fix compile * Revert suffix length back to usize * Use `u64` for `file_size` * address comments * fix calculation of metadata_start * change file_size type to u64 * change _sized functions to take u64 file_size * clippy * remove some potential panics * use u64 for page index ranges * Revert change to deprecated method --------- Signed-off-by: Arpit Bandejiya <[email protected]> Co-authored-by: Arpit Bandejiya <[email protected]> Co-authored-by: Ed Seidl <[email protected]> Co-authored-by: Andrew Lamb <[email protected]>
5 participants
Which issue does this PR close?