charliec05 commented on code in PR #17963:
URL: https://github.com/apache/iceberg/pull/17963#discussion_r3994626846
##########
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:
Addressed in 7510dfb5a. Both negative-seek tests now use `public void
testXxx` names, matching the shared test class.
##########
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:
Addressed in 7510dfb5a. The setup now asserts only that a byte was read
(`isGreaterThanOrEqualTo(0)`), without assuming the fixture starts with 0.
##########
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:
Addressed in 7510dfb5a. Added a separate test for `seek(-1)` from position 0
and retained the test after one read. Both verify that rejection preserves the
position and that a subsequent read succeeds and advances it. The complete
`TestSingleBufferInputStream` and `TestMultiBufferInputStream` classes,
Spotless, and test Checkstyle pass locally.
##########
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:
I agree callers should be aware of the unchecked exception, and kept the
exception behavior as requested. The shared interface documentation needs to
allow for implementation differences: the local file stream delegates to
`RandomAccessFile.seek`, which rejects negative positions with `IOException`. A
blanket negative-position `IllegalArgumentException` contract on
`SeekableInputStream` would therefore need qualification. I have kept this
follow-up focused on the requested tests; a shared Javadoc clarification can
describe both failure modes separately.
--
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]
