feat(arrow/ipc): implement lazy loading/zero-copy for IPC files #216
Conversation
|
@vtk9 can you give this a try and confirm that it addresses your issue? I added unit tests which confirm that the pointers match and that we're avoiding allocations but it would be great to confirm on your end that this reduces the memory usage you were seeing. |
|
@lidavidm @kou @joellubi would one of you be able to look this over and give a review? I wanna merge this and then kick off an RC as requested from #218 (comment) |
|
I'll try to get to it on Monday |
| func (r *basicReaderImpl) readFooter(f *footerBlock) error { | ||
| var err error | ||
|
|
||
| if f.offset <= int64(len(Magic)*2+4) { |
There was a problem hiding this comment.
Can we add a variable for len(Magic)*2+4 because we use this minimum size multiple times in this file?
| func (r *basicReaderImpl) readFooter(f *footerBlock) error { | ||
| var err error | ||
|
|
||
| if f.offset <= int64(len(Magic)*2+4) { |
There was a problem hiding this comment.
Can we give 4 a name something like footerSizeLen or something?
| return errNotArrowFile | ||
| } | ||
|
|
||
| size := int64(binary.LittleEndian.Uint32(buf[:4])) |
There was a problem hiding this comment.
I think that this should be int32 not uint32: https://arrow.apache.org/docs/format/Columnar.html#ipc-file-format
<FOOTER SIZE: int32>
| return errNotArrowFile | ||
| } | ||
|
|
||
| size := int64(binary.LittleEndian.Uint32(buf[:4])) |
| metaBytes := buf[:blk.meta] | ||
|
|
||
| prefix := 0 | ||
| switch binary.LittleEndian.Uint32(metaBytes) { |
There was a problem hiding this comment.
int32 not uint32?
metaDataLength: int;
|
|
||
| func (r *mappedReaderImpl) getFooterEnd() (int64, error) { return int64(len(r.data)), nil } | ||
|
|
||
| func (r *mappedReaderImpl) readFooter(f *footerBlock) error { |
There was a problem hiding this comment.
Can we unify more codes in this function with basicReaderImple.readFooter()?
It seems that they have many similar codes.
Rationale for this change
closes #207
What changes are included in this PR?
Adding new method
NewMappedFileReaderto ipc package which accepts a byte slice instead of aReaderAtSeeker. UpdatesipcSourceto reference the raw byte slices from the input directly instead of wrapping withbytes.NewReaderwhich forces copies viaRead,ReadFull, etc.Are these changes tested?
Unit tests added to confirm that the pointers match and that we aren't allocating unnecessarily.
Are there any user-facing changes?
Shouldn't be any user-facing changes other than a reduction in memory usage when reading non-compressed IPC data.