[jdbc-v2] Added test for WITH statement with parameters. #2392
Conversation
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds integration tests for the WITH clause using parameters in prepared statements and cleans up unused overrides in several JDBC classes.
- Added a new test, testWithClauseWithParams, in PreparedStatementTest.java to validate parameterized WITH statements.
- Removed redundant overridden methods from StatementImpl, DataSourceImpl, and ConnectionImpl to streamline their implementations.
Reviewed Changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| jdbc-v2/src/test/java/com/clickhouse/jdbc/PreparedStatementTest.java | Added tests for WITH clause and improved test structure by scoping variable declarations. |
| jdbc-v2/src/main/java/com/clickhouse/jdbc/StatementImpl.java | Removed unused override methods to rely on default implementations. |
| jdbc-v2/src/main/java/com/clickhouse/jdbc/DataSourceImpl.java | Removed unused methods and corresponding imports related to connection building and sharding. |
| jdbc-v2/src/main/java/com/clickhouse/jdbc/ConnectionImpl.java | Revised the array creation implementation and removed deprecated sharding key methods. |
Files not reviewed (2)
- clickhouse-client/pom.xml: Language not supported
- pom.xml: Language not supported
Comments suppressed due to low confidence (2)
jdbc-v2/src/test/java/com/clickhouse/jdbc/PreparedStatementTest.java:332
- Consider using a consistent method (either column labels or column indexes) for accessing result set values to improve clarity and reduce the risk of issues if the column order changes.
Assert.assertEquals(rs.getTimestamp("target_time").toLocalDateTime().truncatedTo(ChronoUnit.SECONDS), target_time.toInstant().atZone(ZoneId.of("UTC")).toLocalDateTime().truncatedTo(ChronoUnit.SECONDS));
jdbc-v2/src/main/java/com/clickhouse/jdbc/ConnectionImpl.java:553
- [nitpick] If there's no specific need for streaming the elements, consider using Arrays.asList(elements) to simplify the code and improve readability, assuming that a direct conversion is acceptable.
List<Object> list = (elements == null || elements.length == 0) ? Collections.emptyList() : Arrays.stream(elements, 0, elements.length).collect(Collectors.toList());
|
/windsurf-review |
|
/windsurf-review |
![windsurf-bot[bot]](https://avatars.githubusercontent.com/in/1066231?s=60&v=4){kind=small}
|
|
||
| ResultSet rs = stmt.executeQuery(); | ||
| int count = 0; | ||
| int expected = 4; // 4,5,6 |
There was a problem hiding this comment.
There's a mismatch between the comment and the assertion. The comment on line 398 says expected is 4 for numbers 4,5,6 (which is actually 3 numbers), but the assertion on line 404 correctly checks for 3. Please update the comment to match the assertion.
| List<Object> list = | ||
| (elements == null || elements.length == 0) ? Collections.emptyList() : Arrays.stream(elements, 0, elements.length).collect(Collectors.toList()); | ||
| return new com.clickhouse.jdbc.types.Array(list, typeName, JdbcUtils.convertToSqlType(ClickHouseDataType.valueOf(typeName)).getVendorTypeNumber()); |
There was a problem hiding this comment.
The Arrays.stream approach for handling array elements could potentially have issues with null elements within the array. Consider using a more robust approach that explicitly handles null elements within the array:
| List<Object> list = | |
| (elements == null || elements.length == 0) ? Collections.emptyList() : Arrays.stream(elements, 0, elements.length).collect(Collectors.toList()); | |
| return new com.clickhouse.jdbc.types.Array(list, typeName, JdbcUtils.convertToSqlType(ClickHouseDataType.valueOf(typeName)).getVendorTypeNumber()); | |
| List<Object> list = Collections.emptyList(); | |
| if (elements != null && elements.length > 0) { | |
| list = Arrays.stream(elements) | |
| .collect(Collectors.toList()); | |
| } |
| " WHERE n > ?)" + | ||
| ") " + | ||
| "SELECT * FROM data WHERE n < ?")) { | ||
| //"WITH data AS ( (SELECT number AS n FROM numbers(?) WHERE n > ?)) SELECT * FROM data WHERE n < ?" |
There was a problem hiding this comment.
This commented-out SQL query on line 391 should be removed as it appears to be for debugging purposes only.
Summary
WITHclause with parameters toPreparedStatementImplTestsCloses #2391
Checklist
Delete items not relevant to your PR: