[JDBC] enabled tests. added some more for ConnectionImpl. implementated nati… #2521
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
There was a problem hiding this comment.
Pull Request Overview
This PR enables and extends JDBC tests while refactoring internal class accessibility. It implements the nativeSQL method to handle JDBC escape sequences and adds tests for setSchema functionality.
- Enables previously disabled integration tests
- Implements
nativeSQLmethod to convert JDBC escape sequences to native SQL - Refactors
DriverPropertiesandClientInfoPropertiesclasses from internal package to public
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| jdbc-v2/src/main/java/com/clickhouse/jdbc/DriverProperties.java | Moved from internal package to public API |
| jdbc-v2/src/main/java/com/clickhouse/jdbc/ClientInfoProperties.java | Moved from internal package to public API |
| jdbc-v2/src/main/java/com/clickhouse/jdbc/ConnectionImpl.java | Enhanced connection validation, implemented nativeSQL, made client field private |
| jdbc-v2/src/main/java/com/clickhouse/jdbc/StatementImpl.java | Extracted static method for SQL escape processing, improved error handling |
| jdbc-v2/src/test/java/com/clickhouse/jdbc/ConnectionTest.java | Enabled and expanded tests for connection functionality |
| jdbc-v2/src/test/java/com/clickhouse/jdbc/StatementTest.java | Added schema setting tests and enabled existing tests |
| Various test files | Updated imports to reflect package structure changes |
Comments suppressed due to low confidence (1)
jdbc-v2/src/test/java/com/clickhouse/jdbc/StatementTest.java:1037
- The test checks that the database name remains the same for an existing statement after changing the connection schema, but doesn't verify that new statements use the updated schema. Consider adding a test that creates a new statement after schema change to verify the schema switch works correctly.
assertEquals(getDBName(stmt), db1);
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
Contributor
Author
|
Thank you, @mzitnik ! |
|
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
nativeSQLto return SQL after escaping functions. (Simplest implementation)setSchema(String schema)DriverPropertiesandClientInforPropertiesenums to a public package for better UX.Closes #2411
Checklist
Delete items not relevant to your PR: