-
Notifications
You must be signed in to change notification settings - Fork 590
[jdbc-v2] Fix statement impl #2471
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes statement implementation in the JDBC v2 driver by improving state management, result set handling, and SQL processing. It addresses missing implementations and consolidates SQL utilities to ensure proper JDBC compliance.
Key Changes
- Comprehensive statement state management with proper result set lifecycle handling
- Moved SQL utilities from
SqlParser
to centralizedSQLUtils
class for better maintainability - Enhanced JDBC specification compliance with missing method implementations
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
StatementImpl.java | Major refactoring with proper state management, result set tracking, and complete JDBC method implementations |
PreparedStatementImpl.java | Updated to use ensureOpen() and SQLUtils for consistent state validation |
WriterStatementImpl.java | Updated to use ensureOpen() for state validation consistency |
SQLUtils.java | New centralized utility class for SQL string processing and identifier handling |
SqlParser.java | Removed duplicate utility methods now centralized in SQLUtils |
DatabaseMetaDataImpl.java | Updated to use SQLUtils for SQL literal escaping |
ParsedStatement.java | Updated to use SQLUtils for identifier processing |
ParsedPreparedStatement.java | Updated to use SQLUtils for identifier processing |
Various test files | Added comprehensive test coverage for new functionality |
throw new SQLFeatureNotSupportedException("Set fetch direction is not supported.", ExceptionUtils.SQL_STATE_FEATURE_NOT_SUPPORTED); | ||
ensureOpen(); | ||
if (direction != ResultSet.FETCH_FORWARD && direction != ResultSet.FETCH_REVERSE && direction != ResultSet.FETCH_UNKNOWN) { | ||
throw new SQLException("Invalid fetch direction: " + direction + ". Should be one of ResultSet.FETCH_FORWARD, ResultSet.FETCH_REVERSE, or ResultSet.FETCH_UNKNOW"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message contains a typo: "FETCH_UNKNOW" should be "FETCH_UNKNOWN"
throw new SQLException("Invalid fetch direction: " + direction + ". Should be one of ResultSet.FETCH_FORWARD, ResultSet.FETCH_REVERSE, or ResultSet.FETCH_UNKNOW"); | |
throw new SQLException("Invalid fetch direction: " + direction + ". Should be one of ResultSet.FETCH_FORWARD, ResultSet.FETCH_REVERSE, or ResultSet.FETCH_UNKNOWN"); |
Copilot uses AI. Check for mistakes.
if (max <= 0) { | ||
throw new SQLException("max should be a positive integer."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validation logic is incorrect. According to JDBC specification, setMaxFieldSize should accept 0 to indicate no limit, but the current check throws an exception for max <= 0. It should only throw an exception for max < 0.
if (max <= 0) { | |
throw new SQLException("max should be a positive integer."); | |
if (max < 0) { | |
throw new SQLException("max should not be negative."); |
Copilot uses AI. Check for mistakes.
@@ -31,7 +31,13 @@ public enum DriverProperties { | |||
*/ | |||
BETA_ROW_BINARY_WRITER("beta.row_binary_for_simple_insert", "false"), | |||
|
|||
/** | |||
* Enables closing result set before |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment is incomplete. It should describe what happens "before" - likely "before executing new statements" or similar.
* Enables closing result set before | |
* Enables closing the result set before executing new statements or when the statement is closed. | |
* This helps to free up resources automatically without requiring explicit result set closure. |
Copilot uses AI. Check for mistakes.
try (Statement stmt = conn.createStatement()) { | ||
stmt.closeOnCompletion(); | ||
try (ResultSet rs = stmt.executeQuery("CREATE TABLE test_empty_table (id String) Engine Memory")) { | ||
}catch (Exception ex){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space after 'catch' keyword. Should be 'catch (Exception ex) {'
}catch (Exception ex){ | |
} catch (Exception ex) { |
Copilot uses AI. Check for mistakes.
* @return true if the identifier needs to be quoted, false otherwise | ||
*/ | ||
private static boolean needsQuoting(String identifier) { | ||
if (identifier.isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we check here if the identifier is null?
We get this 56.4% Coverage on New Code (required ≥ 80%) |
Summary
Closes #2414
Checklist
Delete items not relevant to your PR: