You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This PR adds the ability to define a target cluster for statements like KILL QUERY when using the jdbc-v2 driver. In environments where ClickHouse runs as a cluster, cancelling a query requires the ON CLUSTER <cluster_name> clause to effectively stop the execution across all nodes.
Changes Made
New Driver Property: Added cluster_name to DriverProperties to allow users to specify their target cluster during connection initialization.
Connection Integration: ConnectionImpl now reads the cluster_name property and activates the internal onCluster state if it is provided.
Test Coverage: Added testCancelOnCluster in StatementTest to verify that Statement.cancel() successfully includes the cluster clause when the cluster_name property is set.
If a user sets cluster_name to a value containing SQL-special characters (e.g., my_cluster; SELECT 1 or a name with embedded double quotes), the generated SQL could be malformed or behave unexpectedly. The codebase already has a suitable helper: SQLUtils.enquoteIdentifier(connection.cluster) wraps the name in double quotes and escapes any embedded double quotes. This should be used here.
Per CLAUDE.md → docs/ai-review.md and docs/features.md: "update docs/features.md when a new user-visible feature is added or when supported behavior changes intentionally."
The PR adds a new driver property (cluster_name) that changes the behavior of cancel(). The relevant entry in docs/features.md currently reads:
Query cancellation and timeout: Supports JDBC query timeout handling and query cancellation through server-side KILL QUERY.
This should be extended to mention that cluster_name enables KILL QUERY ON CLUSTER <name> for clustered environments.
3. cluster field stores untrimmed value — ConnectionImpl.java:77–78 (Low)
onCluster is computed using .trim(), but this.cluster retains the raw (potentially padded) value. In cancel(), connection.cluster is used directly in the SQL, so leading/trailing whitespace in the property value would be passed through. Recommend storing the trimmed value: this.cluster = rawCluster != null ? rawCluster.trim() : null;.
4. Test design issues — StatementTest.java:582–598 (Low)
Several minor issues with testCancelOnCluster:
Resource leak: The ResultSet returned by executeQuery() is never closed. This should be wrapped in a try-with-resources or assigned and closed in finally.
Unnecessary cast: The statement is cast to StatementImpl; cancel() is declared on the Statement interface, so the cast is not needed.
Fragile assertion: The test relies on ClickHouse's error message containing the cluster name. That message format could differ across ClickHouse versions. A unit test that directly inspects the generated SQL string (e.g., mocking the client) would be more robust, though this may be out of scope for an integration test.
No positive test: There is no test that verifies the happy path — i.e., that when a valid, existing cluster name is configured, cancel() executes successfully with the ON CLUSTER clause. This would require a real cluster in the integration environment, which may not be available, but this gap is worth noting.
Summary
What changed
New cluster_name driver property in jdbc-v2 (DriverProperties, ConnectionImpl); when set, cancel() appends ON CLUSTER <name> to KILL QUERY.
Compatibility risk
Low — the new property is opt-in with a null default, and existing behavior (cancel without ON CLUSTER) is fully preserved when not set. No public API signatures changed.
Tests
An integration test is included but has a resource leak and relies on a fragile error-message assertion. No positive path test.
Docs
docs/features.md should be updated to document the new cluster-aware cancellation behavior.
Security
The cluster name should be properly quoted with SQLUtils.enquoteIdentifier() before SQL embedding.
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
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
This PR adds the ability to define a target cluster for statements like
KILL QUERYwhen using thejdbc-v2driver. In environments where ClickHouse runs as a cluster, cancelling a query requires theON CLUSTER <cluster_name>clause to effectively stop the execution across all nodes.Changes Made
cluster_nametoDriverPropertiesto allow users to specify their target cluster during connection initialization.ConnectionImplnow reads thecluster_nameproperty and activates the internalonClusterstate if it is provided.testCancelOnClusterinStatementTestto verify thatStatement.cancel()successfully includes the cluster clause when thecluster_nameproperty is set.Closes #2837
Checklist
Delete items not relevant to your PR: