rdblue commented on code in PR #15006:
URL: https://github.com/apache/iceberg/pull/15006#discussion_r2819768511
##########
core/src/main/java/org/apache/iceberg/io/IOUtil.java:
##########
@@ -49,6 +51,24 @@ public static void readFully(InputStream stream, byte[]
bytes, int offset, int l
}
}
+ public static byte[] readBytes(InputFile inputFile, long offset, int length)
{
Review Comment:
I think this ends up being confusing because `RangeReadable#readFully` ends
up calling the method above. It isn't clear which one to use, except that this
one allocates a byte array. That's not related to the difference in behavior,
where this one looks for a specific `FileIO` capability. It would be reasonable
to call `IOUtil.readFully(inputFile.newStream(), buffer, offset,
buffer.length)` to reuse a buffer, even though that would miss the
`RangeReadable` optimization.
To improve this:
- It should also be named `readFully` to capture the semantics: this will
not return after a partial read. (This should also be covered by the method's
Javadoc.)
- It should accept a destination buffer so that the difference doesn't make
people choose one or the other
- It should also use the local implementation of `readFully(InputStream,
byte[])` instead of the Guava one
- It will need a second offset. The `readFully` implementation above uses
`offset` as the location in the destination buffer, not in the source stream
(which is assumed to be in position).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
