[jdbc-v2] Implements java.sql.PreparedStatement#getMetaData
#2311
Conversation
chernser
requested review from
Paultagoras and
mzitnik
and removed request for
Paultagoras
| private static Map<ClickHouseDataType, Class<?>> getDataTypeClassMap() { | ||
| Map<ClickHouseDataType, Class<?>> map = new HashMap<>(); | ||
| for (Map.Entry<ClickHouseDataType, SQLType> e : CLICKHOUSE_TO_SQL_TYPE_MAP.entrySet()) { | ||
| map.put(e.getKey(), SQL_TYPE_TO_CLASS_MAP.get(e.getValue())); |
There was a problem hiding this comment.
Does this actually go the whole way from ClickHouseDataType -> SqlType -> Class?
There was a problem hiding this comment.
yes, but we need to make it more bound to actual code. I mean there should be a single source of truth.
| assertNotNull(metadataRs); | ||
| assertEquals(metadataRs.getColumnCount(), colCountAfterExecution); | ||
| for (int i = 1; i <= metadataRs.getColumnCount(); i++) { | ||
| System.out.println("label=" + metadataRs.getColumnName(i) + " type=" + metadataRs.getColumnType(i)); |
There was a problem hiding this comment.
Pull Request Overview
This PR implements the java.sql.PreparedStatement#getMetaData method to return metadata for SELECT queries (with column details) and an empty metadata object for non-SELECT queries. Key changes include:
- Adding tests in PreparedStatementTest for getMetaData behavior.
- Updating metadata implementations and utility methods across ResultSet, Statement, and PreparedStatement classes.
- Refactoring and cleanup of unused imports and legacy metadata classes.
Reviewed Changes
Copilot reviewed 23 out of 26 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| jdbc-v2/src/test/java/com/clickhouse/jdbc/StatementTest.java | Removed unused imports. |
| jdbc-v2/src/test/java/com/clickhouse/jdbc/PreparedStatementTest.java | Added tests and data provider for PreparedStatement#getMetaData. |
| jdbc-v2/src/test/java/com/clickhouse/jdbc/JdbcIntegrationTest.java | Cleaned up import statements. |
| jdbc-v2/src/test/java/com/clickhouse/jdbc/ConnectionTest.java | Removed unused imports. |
| jdbc-v2/src/main/java/com/clickhouse/jdbc/types/Array.java | Removed unused import. |
| jdbc-v2/src/main/java/com/clickhouse/jdbc/metadata/ResultSetMetaDataImpl.java | Refactored to use a new column list and schema properties. |
| jdbc-v2/src/main/java/com/clickhouse/jdbc/metadata/ParameterMetaDataImpl.java | New implementation providing minimal parameter metadata. |
| jdbc-v2/src/main/java/com/clickhouse/jdbc/metadata/ParameterMetaData.java | Removed legacy metadata implementation. |
| jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/MetadataResultSet.java | Updated to instantiate the new ResultSetMetaDataImpl. |
| jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/JdbcUtils.java | Adopted ImmutableMap for type maps and added a new replaceQuestionMarks utility. |
| jdbc-v2/src/main/java/com/clickhouse/jdbc/StatementImpl.java | Made slight refactoring and ensured proper schema assignment. |
| jdbc-v2/src/main/java/com/clickhouse/jdbc/ResultSetImpl.java | Adjusted metadata initialization and assignment. |
| jdbc-v2/src/main/java/com/clickhouse/jdbc/PreparedStatementImpl.java | Updated getMetaData and getParameterMetaData implementations. |
| client-v2/src/main/java/com/clickhouse/client/api/metadata/TableSchema.java | Removed unused imports. |
| .github/workflows/analysis.yml | Updated sonar command parameters. |
Files not reviewed (3)
- clickhouse-jdbc/pom.xml: Language not supported
- client-v2/pom.xml: Language not supported
- jdbc-v2/pom.xml: Language not supported
Comments suppressed due to low confidence (1)
jdbc-v2/src/main/java/com/clickhouse/jdbc/metadata/ResultSetMetaDataImpl.java:35
- [nitpick] Consider performing an explicit boundary check for 'column' instead of relying on catching IndexOutOfBoundsException. This could improve readability and make the error handling more intentional.
try { return columns.get(column - 1); } catch (IndexOutOfBoundsException e) {
| @@ -303,7 +309,31 @@ public void setArray(int parameterIndex, Array x) throws SQLException { | |||
| @Override | |||
| public ResultSetMetaData getMetaData() throws SQLException { | |||
| checkClosed(); | |||
| return null; | |||
|
|
|||
| if (resultSetMetaData == null && currentResultSet == null) { | |||
There was a problem hiding this comment.
Consider adding inline documentation clarifying that replacing question marks with 'NULL' is solely used for schema inference. This will help future maintainers understand potential limitations when the SQL contains literal question marks.
Copilot uses AI. Check for mistakes.
|
|
||
| public static final String NULL = "NULL"; | ||
|
|
||
| private static final Pattern REPLACE_Q_MARK_PATTERN = Pattern.compile("(\"[^\"]*\"|`[^`]*`)|(\\?)"); |
There was a problem hiding this comment.
The current regex pattern only handles double-quoted and backtick-quoted strings. Since SQL string literals are usually single-quoted, consider extending the pattern to also cover single-quoted strings to avoid unintended replacements.
| private static final Pattern REPLACE_Q_MARK_PATTERN = Pattern.compile("(\"[^\"]*\"|`[^`]*`)|(\\?)"); | |
| private static final Pattern REPLACE_Q_MARK_PATTERN = Pattern.compile("(\"[^\"]*\"|`[^`]*`|'[^']*')|(\\?)"); |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Thank you! That helped a lot!
|
Summary
java.sql.PreparedStatement#getMetaData0columns.Closes #2292
Closes #2311
Checklist
Delete items not relevant to your PR:
java.sql.PreparedStatement#getMetaData#2311