laskoviymishka commented on code in PR #17963:
URL: https://github.com/apache/iceberg/pull/17963#discussion_r3981046553
##########
core/src/test/java/org/apache/iceberg/io/TestByteBufferInputStreams.java:
##########
@@ -362,6 +362,17 @@ public void testSkipFully() throws Exception {
.hasMessageStartingWith("Not enough bytes to skip");
}
+ @Test
+ void seekRejectsNegativePositionWithoutChangingState() throws Exception {
+ ByteBufferInputStream stream = newStream();
+ assertThat(stream.read()).isEqualTo(0);
Review Comment:
`assertThat(stream.read()).isEqualTo(0)` couples this to the fixture's first
byte being 0 — a future `newStream()` returning different data would fail here
for an unrelated reason. Since you only need the position to advance,
`isGreaterThanOrEqualTo(0)` (or calling `read()` without asserting the value)
keeps it robust.
##########
core/src/main/java/org/apache/iceberg/io/MultiBufferInputStream.java:
##########
@@ -68,6 +69,7 @@ public long getPos() {
@Override
public void seek(long newPosition) throws IOException {
+ Preconditions.checkArgument(newPosition >= 0, "Position is negative: %s",
newPosition);
Review Comment:
This throws `IllegalArgumentException` for a negative position, while the
check right below throws `EOFException` for past-EOF — so a caller wrapping
`seek()` in `catch (IOException)` catches one bound but not the other.
I'd keep it as-is, since it matches what S3, GCS, ADLS and OSS all do for
negative seeks — the asymmetry is the established convention across the family,
not something new here. Might be worth documenting the negative-position
contract on `SeekableInputStream.seek()` (`@throws IllegalArgumentException`)
so it isn't a surprise. wdyt?
##########
core/src/test/java/org/apache/iceberg/io/TestByteBufferInputStreams.java:
##########
@@ -362,6 +362,17 @@ public void testSkipFully() throws Exception {
.hasMessageStartingWith("Not enough bytes to skip");
}
+ @Test
+ void seekRejectsNegativePositionWithoutChangingState() throws Exception {
+ ByteBufferInputStream stream = newStream();
+ assertThat(stream.read()).isEqualTo(0);
+
+ assertThatThrownBy(() -> stream.seek(-1))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Position is negative: -1");
+ assertThat(stream.getPos()).isEqualTo(1);
Review Comment:
This covers `seek(-1)` after one read, but the case that actually motivated
the PR is `seek(-1)` from position 0 — that's the one that used to silently
reset `MultiBufferInputStream` to 0. I'd add that directly.
Would also be worth reading a byte after the rejected seek to prove the
stream is still usable, not just that `getPos()` didn't move.
##########
core/src/test/java/org/apache/iceberg/io/TestByteBufferInputStreams.java:
##########
@@ -362,6 +362,17 @@ public void testSkipFully() throws Exception {
.hasMessageStartingWith("Not enough bytes to skip");
}
+ @Test
+ void seekRejectsNegativePositionWithoutChangingState() throws Exception {
Review Comment:
Small consistency thing — this is package-private and named in BDD style,
while every other `@Test` in the file is `public void testXxx`. I'd make it
public and rename along the lines of `testSeekRejectsNegativePosition`.
--
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]
