[JDBC-V2] Review database metadata #2677
Open
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
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
![windsurf-bot[bot]](https://avatars.githubusercontent.com/in/1066231?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Other comments (1)
- jdbc-v2/src/test/java/com/clickhouse/jdbc/metadata/DatabaseMetaDataTest.java (169-169) There's an indentation issue with the `getExtraNameCharacters()` assertion on line 169. It has extra indentation compared to surrounding lines.
💡 To request another review, post a new comment with "/windsurf-review".
| @Override | ||
| public boolean supportsDifferentTableCorrelationNames() throws SQLException { | ||
| return false; | ||
| return true; // no support |
Contributor
There was a problem hiding this comment.
The method supportsDifferentTableCorrelationNames() returns true with a comment saying "no support", which is contradictory. If ClickHouse doesn't support different table correlation names, this method should return false.
| @Override | ||
| public boolean supportsExpressionsInOrderBy() throws SQLException { | ||
| return true; | ||
| return false; |
Contributor
There was a problem hiding this comment.
The supportsExpressionsInOrderBy() method returns false, but ClickHouse does support expressions in ORDER BY clauses (e.g., ORDER BY column + 1). This should return true.
| @Override | ||
| public boolean nullsAreSortedLow() throws SQLException { | ||
| return !nullsAreSortedHigh(); // opposite of nullsAreSortedHigh | ||
| return true; // opposite of nullsAreSortedHigh |
Contributor
There was a problem hiding this comment.
The comment for nullsAreSortedLow() says it's the "opposite of nullsAreSortedHigh", but the implementation now directly returns true without referencing nullsAreSortedHigh(). Either update the comment or make the implementation reflect this relationship.
|
There was a problem hiding this comment.
Pull request overview
This PR updates the JDBC database metadata implementation to reflect ClickHouse's actual capabilities and limitations. The changes ensure that metadata queries return accurate information about what ClickHouse supports, particularly around SQL grammar, identifiers, transactions, and various database features.
Key changes:
- Updated max value constants to match ClickHouse's actual limitations and guardrails
- Corrected support flags for identifiers, transactions, and SQL grammar features
- Added comprehensive test coverage for metadata support flags and table types
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| jdbc-v2/src/test/java/com/clickhouse/jdbc/metadata/DatabaseMetaDataTest.java | Added comprehensive tests for database metadata support flags and table type verification |
| jdbc-v2/src/main/java/com/clickhouse/jdbc/metadata/DatabaseMetaDataImpl.java | Updated metadata methods to return accurate values for ClickHouse capabilities, added TableType enum, and improved documentation with inline comments |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assertEquals(dbmd.storesUpperCaseQuotedIdentifiers(), false); | ||
| assertEquals(dbmd.storesMixedCaseQuotedIdentifiers(), false); | ||
| assertEquals(dbmd.nullPlusNonNullIsNull(), true); | ||
| assertEquals(dbmd.supportsConvert(), false); |
There was a problem hiding this comment.
This assertion is a duplicate of line 173. Consider removing this redundant check.
Suggested change
| assertEquals(dbmd.supportsConvert(), false); |
|
|
||
| for (int type : new int[] {ResultSet.TYPE_SCROLL_INSENSITIVE, ResultSet.TYPE_SCROLL_SENSITIVE} ) { | ||
| assertFalse(dbmd.supportsResultSetType(type)); | ||
| assertFalse(dbmd.supportsResultSetType(type)); |
There was a problem hiding this comment.
Line 289 is a duplicate of line 288. Remove the redundant assertion.
Suggested change
| assertFalse(dbmd.supportsResultSetType(type)); |
| @Override | ||
| public boolean supportsDifferentTableCorrelationNames() throws SQLException { | ||
| return false; | ||
| return true; // no support |
There was a problem hiding this comment.
The comment 'no support' contradicts the return value true. Based on the method name supportsDifferentTableCorrelationNames, the comment should clarify what is actually supported (e.g., 'supports different table aliases').
Suggested change
| return true; // no support | |
| return true; // supports different table aliases |
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 #2520
Closes #778
Closes #2519
Checklist
Delete items not relevant to your PR: