You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "charliec05 (via GitHub)" <gi...@apache.org> on 2026/09/04 23:38:38 UTC
[PR] Core: Reject negative ByteBuffer stream seeks [iceberg]
charliec05 opened a new pull request, #17963:
URL: https://github.com/apache/iceberg/pull/17963
## Summary
- reject negative seek positions in both single- and multi-buffer input streams
- validate before resetting or advancing buffer state
- add shared regression coverage that runs against both implementations
## Motivation
The two `ByteBufferInputStream` implementations handled negative positions inconsistently. `MultiBufferInputStream` silently accepted `seek(-1)` and reset to the beginning, while `SingleBufferInputStream` failed later inside `ByteBuffer.position` after it had already reset its state.
Both implementations now reject the invalid position before changing state, matching the behavior of Iceberg's other seekable streams.
## Testing
- `./gradlew :iceberg-core:spotlessApply :iceberg-core:test`
- `./gradlew :iceberg-core:spotlessCheck :iceberg-core:test --tests org.apache.iceberg.io.TestSingleBufferInputStream --tests org.apache.iceberg.io.TestMultiBufferInputStream`
---
**AI Disclosure**
- Model: GPT-5 (Codex)
- Platform/Tool: OpenAI Codex
- Human Oversight: fully reviewed
- Prompt Summary: Used a detailed, repository-aware prompt to compare seek boundary behavior across implementations, reproduce inconsistent negative-position handling, require state preservation, review the minimal diff, and run both focused and complete Core tests.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Reject negative ByteBuffer stream seeks [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #17963:
URL: https://github.com/apache/iceberg/pull/17963#discussion_r3939015414
##########
core/src/main/java/org/apache/iceberg/io/SingleBufferInputStream.java:
##########
@@ -82,6 +83,7 @@ public int read(byte[] bytes, int off, int len) throws IOException {
@Override
public void seek(long newPosition) throws IOException {
+ Preconditions.checkArgument(newPosition >= 0, "Position is negative: %s", newPosition);
Review Comment:
when can this be negative ?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Reject negative ByteBuffer stream seeks [iceberg]
Posted by "charliec05 (via GitHub)" <gi...@apache.org>.
charliec05 commented on PR #17963:
URL: https://github.com/apache/iceberg/pull/17963#issuecomment-5548132383
Hi @laskoviymishka, would you be willing to review this ByteBufferInputStream boundary fix? It makes both implementations reject negative seeks consistently while preserving stream state, includes shared regression coverage, and all 42 CI checks pass. Thanks!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Reject negative ByteBuffer stream seeks [iceberg]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Reject negative ByteBuffer stream seeks [iceberg]
Posted by "charliec05 (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Reject negative ByteBuffer stream seeks [iceberg]
Posted by "charliec05 (via GitHub)" <gi...@apache.org>.
charliec05 commented on code in PR #17963:
URL: https://github.com/apache/iceberg/pull/17963#discussion_r3939028873
##########
core/src/main/java/org/apache/iceberg/io/SingleBufferInputStream.java:
##########
@@ -82,6 +83,7 @@ public int read(byte[] bytes, int off, int len) throws IOException {
@Override
public void seek(long newPosition) throws IOException {
+ Preconditions.checkArgument(newPosition >= 0, "Position is negative: %s", newPosition);
Review Comment:
`newPosition` is supplied by the caller through the public `ByteBufferInputStream.wrap(...).seek(...)` API. I have not identified a production call path that supplies a negative offset; I found this while testing invalid seek inputs.
On the base revision, after reading the first byte and then calling `seek(-1)`, `SingleBufferInputStream` throws `IllegalArgumentException` but has already reset its position to 0, while `MultiBufferInputStream` accepts the call and also resets to 0. I reproduced both cases: the next read returns the first byte again. With this change, both reject the invalid offset while keeping position 1, and the next read returns the second byte.
The motivation is defensive validation before mutating stream state, consistent with the existing negative-seek guards in `EagerInputStream`, `S3InputStream`, and `GCSInputStream`. The shared regression test checks rejection and position preservation for both implementations.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Reject negative ByteBuffer stream seeks [iceberg]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on PR #17963:
URL: https://github.com/apache/iceberg/pull/17963#issuecomment-5554228779
+1 here, please address any additional concerns from committers
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Reject negative ByteBuffer stream seeks [iceberg]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka merged PR #17963:
URL: https://github.com/apache/iceberg/pull/17963
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Reject negative ByteBuffer stream seeks [iceberg]
Posted by "charliec05 (via GitHub)" <gi...@apache.org>.
charliec05 commented on PR #17963:
URL: https://github.com/apache/iceberg/pull/17963#issuecomment-5840053710
Hi @laskoviymishka, the requested test follow-up is in 7510dfb5a: negative seeks are covered at position 0 and after a read, the fixture-specific byte assertion is removed, and subsequent reads are verified. All 44 checks on the current head are now green. Could you take a final look and merge if the changes look good? The shared seek Javadoc discussion remains open for your confirmation. Thanks!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org