Core: Reject negative ByteBuffer stream seeks

[charliec05]

Negative seeks behaved inconsistently across ByteBufferInputStream implementations: the multi-buffer stream silently reset to position 0, while the single-buffer stream could throw after resetting its state.

Both now reject negative positions before changing state. Shared tests cover rejection at position 0 and after reading, preserve the current position, and verify that subsequent reads still work.

Validation

• ./gradlew :iceberg-core:test --tests org.apache.iceberg.io.TestSingleBufferInputStream --tests org.apache.iceberg.io.TestMultiBufferInputStream
• ./gradlew :iceberg-core:spotlessCheck :iceberg-core:checkstyleTest

---

AI Disclosure

• Model: GPT-5
• Platform/Tool: OpenAI Codex
• Human Oversight: partially reviewed (earlier revision approved by reviewers; test follow-up awaiting review)
• Prompt Summary: Compare seek boundary behavior across implementations, reproduce inconsistent negative-position handling, preserve stream state, and address reviewer requests for initial-position coverage, continued readability, and consistent test naming.

[charliec05]

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!

[singhpk234]

when can this be negative ?

[charliec05]

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.

[uros-b]

+1 here, please address any additional concerns from committers

[laskoviymishka]

Nice catch, the silent seek-to-0 in MultiBufferInputStream (negative position falling through to the reset branch, then skipFully swallowing the negative skip) is exactly the kind of latent bug that's painful to track down later, and guarding before any state mutation is the right fix.

I see uros-b already approved and singhpk234 took a pass inline, so I won't re-tread that ground. Nothing I found blocks merge either.

One thing worth a note rather than a change: within seek(), a negative position throws IllegalArgumentException while past-EOF throws EOFException, so a caller catching IOException around seek() would miss the negative case. I'd keep it — it matches what S3, GCS, ADLS and OSS all do — but it'd be worth documenting on SeekableInputStream.seek() so the unchecked throw isn't a surprise.

The rest is just tightening the new test: cover seek(-1) from position 0 (the case that actually motivated the fix), and drop the fixture-specific first-byte assertion. Left both inline.

Happy to merge once the test's squared away.

[laskoviymishka]

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?

[charliec05]

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.