[JDBC] ResultImpl fixed #2530
Merged
[JDBC] ResultImpl fixed #2530
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ed methods in result setimpl
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
chernser
requested review from
mzitnik and
Copilot
and removed request for
mzitnik
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
mzitnik
reviewed
Aug 16, 2025
|
|
||
| @Test(groups = {"integration"}) | ||
| public void testGetMetadata() throws SQLException { | ||
| try (Connection conn = getJdbcConnection(); Statement stmt = conn.createStatement()) { |
There was a problem hiding this comment.
Should we expand this to all types
There was a problem hiding this comment.
This test verifies that metadata returns expected types.
We have another issue for Parameter and ResultSetMetadata - so will cover their.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the JDBC ResultSet implementation to properly handle unsupported operations through a new FeatureManager class and improves test coverage.
- Introduces a FeatureManager to centralize handling of unsupported JDBC operations based on configuration
- Implements proper cursor position tracking and fetch size handling in ResultSetImpl
- Adds comprehensive test coverage for unsupported operations, cursor positioning, and fetch functionality
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| FeatureManager.java | New utility class to handle unsupported feature exceptions based on configuration |
| ResultSetImpl.java | Major refactoring to use FeatureManager, implement cursor tracking, and proper fetch size handling |
| StatementImpl.java | Fixes fetch size implementation and adds statement reopening logic |
| StatementTest.java | Updates test assertion to reflect corrected fetch size behavior |
| ResultSetImplTest.java | Extensive new test coverage for unsupported operations, cursor positioning, and metadata |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } | ||
|
|
||
| return false; | ||
| return !reader.hasNext() && rowPos != AFTER_LAST; |
There was a problem hiding this comment.
The isLast() method implementation may be incorrect. It returns true when there are no more rows AND the cursor is not after the last row, but this doesn't properly handle the case where the cursor is on the actual last row. Consider tracking whether the current row is the last row more explicitly.
Suggested change
| return !reader.hasNext() && rowPos != AFTER_LAST; | |
| return rowPos == LAST_ROW; |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Fixed. Added a test.
mshustov
approved these changes
Aug 18, 2025
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Closes #2415
Closes #2300
Checklist
Delete items not relevant to your PR: