[feature #268890] Fix JdbcTableInputStream large-file buffer handling and add tests - #15
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthrough
ChangesJDBC chunked stream reads
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant JdbcTableInputStream
participant JdbcTableRowFile
participant FileContent
JdbcTableInputStream->>JdbcTableRowFile: Request initial chunk
JdbcTableRowFile->>FileContent: Read JDBC content
JdbcTableInputStream->>JdbcTableRowFile: Request next chunk at absolute position
JdbcTableRowFile->>FileContent: Read subsequent JDBC content
JdbcTableInputStream-->>JdbcTableInputStream: Update buffer and stream position
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
vfs2provider-jdbctable/src/main/java/com/seeburger/vfs2/provider/jdbctable/JdbcTableInputStream.java (1)
33-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor visibility inconsistency:
dataDescriptionisn'tprivatelikechunkLoader.
chunkLoaderisprivate final, butdataDescription(Line 44) has default (package) visibility. Unless package-private access is intentionally needed elsewhere, tighten it for consistency/encapsulation.🔧 Suggested fix
- final DataDescription dataDescription; + private final DataDescription dataDescription;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vfs2provider-jdbctable/src/main/java/com/seeburger/vfs2/provider/jdbctable/JdbcTableInputStream.java` around lines 33 - 48, Change the dataDescription field in JdbcTableInputStream to private final, matching chunkLoader, unless an explicitly required package-level access use exists.vfs2provider-jdbctable/src/test/java/com/seeburger/vfs2/provider/jdbctable/JdbcTableInputStreamTest.java (1)
144-171: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftTests only ever exercise a single chunk boundary; the
MAX_BUFFER_SIZEcap and error path inloadNextChunk()are not verified.
nextReadData's mock computeslen = min(maxLen, fullData.length - desc.pos). Since productionloadNextChunk()passesmaxLen = min(MAX_BUFFER_SIZE, dataLength - nextPos), and these test files are tiny relative to 50 MB,maxLenalways resolves to "all remaining bytes" — so every test after the first (artificially truncated) chunk loads the entire remainder in one shot rather than genuinely chunking further. None of the tests currently exercise: (1) more than one chunk-to-chunk transition, or (2)nextReadDatathrowingIOException(the "content has changed" path), which is exactly where staledataDescription.posstate (see theloadNextChunk()comment inJdbcTableInputStream.java) would surface.Consider adding a test that stubs
nextReadDatato throw once and asserts stream state/behavior on a subsequent call, and/or exposing a package-private way to shrink the effective chunk size beyond the first chunk for a multi-boundary test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vfs2provider-jdbctable/src/test/java/com/seeburger/vfs2/provider/jdbctable/JdbcTableInputStreamTest.java` around lines 144 - 171, Strengthen the JdbcTableInputStream tests around buildStream and loadNextChunk by forcing nextReadData to honor a small effective chunk size, so reads traverse multiple chunk boundaries instead of loading the entire remainder. Add coverage for nextReadData throwing IOException, asserting the documented stream state and behavior on the subsequent call, including correct handling of dataDescription.pos. Use a package-private chunk-size override or equivalent test hook if needed without changing production behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@vfs2provider-jdbctable/src/main/java/com/seeburger/vfs2/provider/jdbctable/JdbcTableInputStream.java`:
- Line 215: Update mark/reset handling in JdbcTableInputStream so reset()
detects when the stream has moved to a different chunk since mark(). Record the
marked chunk’s starting dataDescription.pos in mark(), compare it with the
current chunk in reset(), and throw IOException on mismatch; only restore
bufferPos when both positions refer to the same chunk.
- Around line 62-115: Update the chunk-loading flow in loadNextChunk and the
chunkLoader lambda to preserve dataDescription.pos when file.nextReadData fails.
Save the prior position before assigning the requested pos, restore it if the
load throws, and ensure retries compute the next chunk from the unchanged prior
state.
---
Nitpick comments:
In
`@vfs2provider-jdbctable/src/main/java/com/seeburger/vfs2/provider/jdbctable/JdbcTableInputStream.java`:
- Around line 33-48: Change the dataDescription field in JdbcTableInputStream to
private final, matching chunkLoader, unless an explicitly required package-level
access use exists.
In
`@vfs2provider-jdbctable/src/test/java/com/seeburger/vfs2/provider/jdbctable/JdbcTableInputStreamTest.java`:
- Around line 144-171: Strengthen the JdbcTableInputStream tests around
buildStream and loadNextChunk by forcing nextReadData to honor a small effective
chunk size, so reads traverse multiple chunk boundaries instead of loading the
entire remainder. Add coverage for nextReadData throwing IOException, asserting
the documented stream state and behavior on the subsequent call, including
correct handling of dataDescription.pos. Use a package-private chunk-size
override or equivalent test hook if needed without changing production behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 86a3c89c-e128-49e1-b2e8-6df9b60a8036
📒 Files selected for processing (4)
pom.xmlvfs2provider-jdbctable/pom.xmlvfs2provider-jdbctable/src/main/java/com/seeburger/vfs2/provider/jdbctable/JdbcTableInputStream.javavfs2provider-jdbctable/src/test/java/com/seeburger/vfs2/provider/jdbctable/JdbcTableInputStreamTest.java
2 participants
Summary by CodeRabbit
Bug Fixes
Tests