[WIP] Variable page size for batch PQ writes #9508
Conversation
andrewvc
changed the title
andrewvc
force-pushed
the
variable-page-size
branch
from
db1ec23 to
9ea962b
Compare
First attempt at variable page sizes that passes all tests
andrewvc
force-pushed
the
variable-page-size
branch
from
9ea962b to
04c0841
Compare
|
@andrewvc sorry for the delay, taking a look now :) |
| this.offsetMap = new IntVector(); | ||
| this.checkSummer = new CRC32(); | ||
| this.file = dirPath.resolve("page." + pageNum).toFile(); | ||
| int existingFileSize = (int) file.length(); |
There was a problem hiding this comment.
@andrewvc this is somewhat optimistic I think. We should do a safe cast here (using Guava's Ints for example) here and hard fail if the length exceeds 2GB because we can't mmap that kind of file.
I know what we won't be able to write that kind of file in the first place, so this shouldn't ever come up but with the amount of weird bugs we ran into I think we should have visibility on that kind of problem if it occurs :)
There was a problem hiding this comment.
+1 to handling the edge cases.
|
@andrewvc this LGTM for a merge to a feature branch for sure btw. Wouldn't wanna block you here if this blocks you with my nitpick on the typecast :) |
|
@original-brownbear I'll do a check against |
|
@andrewvc sure sounds good :) |
Part of #9389 , this implements a variably sized page allowing us to write the entire batch to the given page.
This is in a WIP state, its ready for a pre-review to assess its approach, but needs tests and some code cleanup potentially.
This introduces a new variable and the idea of
pageDataCapacityon top ofpageCapacity. ThepageCapacityis really the page file size,pageDataCapacityis that size - the size of a file header for when we write the batch, since we need to know if it will fit the exact batch size.