Fix client-v2: quote String/temporal elements in container query parameters#2898
Open
polyglotAI-bot wants to merge 2 commits into
Open
Fix client-v2: quote String/temporal elements in container query parameters#2898polyglotAI-bot wants to merge 2 commits into
polyglotAI-bot wants to merge 2 commits into
Conversation
…meters
addStatementParams formatted every HTTP param_<name> value with a blanket
String.valueOf(v). For a container value (List/array/Map) this left inner
elements unquoted (e.g. param_l=[2026-05-13] for an Array(Date) parameter),
which the server rejects with HTTP 400 CANNOT_PARSE_INPUT_ASSERTION_FAILED.
The server's array/map text parser requires String/temporal leaves to be
single-quoted (['2026-05-13']) while numeric leaves must stay unquoted.
Format container parameters type-awarely: frame Collection/array as [..] and
Map as {..}, and render scalar leaves via ClickHouseValues.convertToSqlExpression
(String/temporal single-quoted, numeric/boolean unquoted, null -> NULL). Scalar
parameters are unchanged (still emitted unquoted, as the server requires). Both
transport paths (URL query params and multipart body) funnel through this method.
Fixes: #2897
|
Repository collaborators can run the JMH benchmark suite against this PR by commenting: Optional regression threshold override (Δ% on Time or Alloc/op; defaults to 10%): Only one benchmark run per PR is active at a time — issuing a new |
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
There was a problem hiding this comment.
Pull request overview
Fixes client-v2 HTTP param_<name> formatting for container-valued query parameters so that inner String/temporal leaves are single-quoted (and escaped) while numeric/boolean leaves remain unquoted, enabling ClickHouse to parse Array(...) and Map(...) parameters correctly via the HTTP parameter interface.
Changes:
- Updated
HttpAPIClientHelper.addStatementParamsto use a new type-aware formatter for container parameters. - Added unit tests covering container quoting/escaping and contrast cases where scalars and numeric arrays remain unquoted.
- Added an integration regression test that round-trips
Array(Date),Array(String),Array(Int32), andMap(String,Date)parameters end-to-end.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java | Adds recursive container formatting so nested elements use type-aware SQL literal rendering. |
| client-v2/src/test/java/com/clickhouse/client/api/internal/HttpAPIClientHelperTest.java | Adds unit coverage for the new formatter across scalar/array/map/nested/escaping/null cases. |
| client-v2/src/test/java/com/clickhouse/client/query/QueryTests.java | Adds integration regression ensuring container params parse correctly on a live server. |
chernser
reviewed
Jun 26, 2026
chernser
reviewed
Jun 26, 2026
chernser
reviewed
Jun 26, 2026
chernser
reviewed
Jun 26, 2026
| // Regression for Array/Map parameters whose elements were emitted unquoted (e.g. | ||
| // param_dates=[2026-05-13]) and rejected by the server with HTTP 400. Raw List/array/Map | ||
| // values must now round-trip without the manual DataTypeConverter pre-formatting workaround. | ||
| final Map<String, Object> params = new HashMap<>(); |
Contributor
There was a problem hiding this comment.
there are many other containers.
What about nested types?
- tuples can be passed as arrays or lists
- simple java array should be tested
Collaborator
Author
There was a problem hiding this comment.
Extended the integration test with an object array (LocalDate[]), a primitive array (int[]) and a nested array (Array(Array(Int32))), alongside the existing List/Map cases.
On Tuple specifically: a raw List/array can't be auto-formatted as a Tuple at this layer. The same runtime List is also how an Array is bound — the reported bug binds {l:Array(Date)} from a List<LocalDate> — and there's no declared-type information here to disambiguate the two. I verified against the server that the formats are mutually exclusive:
param_t=('a',1) -> OK
param_t=['a',1] -> Code: 27 ... value ['a',1] cannot be parsed as Tuple(String,Int64) (CANNOT_PARSE_INPUT_ASSERTION_FAILED)
So rendering a List as [...] (required for Array, and for the reported bug) is incompatible with (...) (required for Tuple) unless we parse the {name:Type} placeholder out of the SQL. Tuple was equally unsupported before this PR, so I scoped it out here rather than mis-render it. Happy to follow up with declared-type-aware parameter binding as a separate change if you'd like that. 0972134.
chernser
requested changes
Jun 26, 2026
…rter
Address review feedback on the Array/Map query-parameter quoting fix:
- Move the param formatting out of the HttpAPIClientHelper transport class
into a reusable DataTypeConverter.convertParameterToString instance method,
so the transport class no longer knows about ClickHouse parameter formatting
and there are no static formatting helpers on it.
- Format parameters upstream in Client.query so the transport layer receives
already-formatted text; addStatementParams reverts to a plain passthrough.
- Detect containers via getClass().isArray() and iterate via reflection so
primitive arrays (int[], long[], ...) and nested primitive arrays are
formatted correctly instead of being mis-rendered as a scalar ("[I@..").
- Parametrize the unit tests (DataTypeConverterTest) and extend the
integration test with object array, primitive array and nested array cases.
TriageCategory: Summary What this impacts
Concerns
Required reviewer action
|
Collaborator
Author
|
@chernser thanks for the review — pushed
I left the Tuple thread open: a raw |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0972134. Configure here.
| private boolean isParameterContainer(Object value) { | ||
| return value instanceof Collection || value instanceof Map | ||
| || (value != null && value.getClass().isArray()); | ||
| } |
There was a problem hiding this comment.
Top-level char array mishandled
Medium Severity
convertParameterToString treats every Java array as an Array container via getClass().isArray(), but String.valueOf(char[]) is defined to return the character sequence as plain text, not an array literal. A top-level char[] bound to a scalar placeholder (e.g. {s:String}) is now emitted as numeric codepoints like [104,105] instead of the prior string value, which can break queries that previously worked.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 0972134. Configure here.
Collaborator
Author
There was a problem hiding this comment.
Thanks — I dug into this and it's not a regression; the [104,105] rendering is consistent with this library's existing char[] convention, so I'm leaving it as-is (happy to revisit if you'd prefer char[] treated as text — see end).
The "previously worked / prior string value" premise doesn't hold. Before this PR, params were emitted by HttpAPIClientHelper.addStatementParams via String.valueOf(v), where v is the Map<String,Object> value — statically typed Object. Overload resolution therefore picks String.valueOf(Object), not the String.valueOf(char[]) overload (that one requires a char[] static type). So a top-level char[] was rendered as char[].toString() → [C@<hash> identity junk, never the character sequence:
String.valueOf((Object) new char[]{'h','i'}) => [C@5c18298f
String.valueOf(new char[]{'h','i'}) => hi // array overload — never reached at this call site
Post-PR, Client.query pre-formats via DataTypeConverter.convertParameterToString(Object) — same Object static type — so the char[] overload was never in play on either side. There was no working char[] → 'hi' path to break.
[104,105] matches the library's deliberate char[] convention. ClickHouseValues.convertToString(char[]) (ClickHouseValues.java:630-643) renders a char array as its numeric code points ([104,105]), and convertToSqlExpression maps a Character leaf to its code point (:577-578). The new container formatter delegates leaves to convertToSqlExpression, so it now produces the same [104,105] as the rest of the converter — this makes the param path consistent, not divergent. (Net: for {s:String} both the old [C@hash] and the new [104,105] just store a literal string — neither errors, neither is hi; for an Array(UInt16)-style placeholder the new [104,105] actually parses, where [C@hash] did not.)
This is also outside the reported bug (Array(Date) of boxed temporals) — char[] isn't part of it.
If you'd prefer a char[] parameter be treated as text ('hi') — a deliberate divergence from the existing convertToString(char[]) convention — I'm glad to add that as a small explicit special-case. Leaving this thread open for your call.
|
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
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.
Description
Fixes #2897.
HttpAPIClientHelper.addStatementParamsformatted every HTTPparam_<name>value with a blanketString.valueOf(v). For a container value (aList/array/Map) that callsAbstractCollection.toString(), which renders each element via its owntoString()and leaves innerString/temporal values unquoted — e.g. aList<LocalDate>bound to{l:Array(Date)}is sent asparam_l=[2026-05-13]. ClickHouse's array text parser requires single-quoted dates (['2026-05-13']), so the server rejects the request with HTTP 400CANNOT_PARSE_INPUT_ASSERTION_FAILED. The same affectedArray(String),Array(DateTime),Map(..., Date), and nested containers.The fix formats container parameters type-awarely:
Collection/object-array are framed as[..],Mapas{..}, and scalar leaves are rendered with the existing type-awareClickHouseValues.convertToSqlExpression(single-quotesString/temporal/UUID/Inet, leaves numeric/boolean unquoted,null→NULL). Scalar parameters are untouched — they are still emitted in their bare, unquoted form, which is what the server requires for a scalar placeholder (a quoted scalar'2026-05-13'is itself rejected). Both transport paths (URL query params and the multipart body) funnel through this single method, so both are fixed.Note: the formatter intentionally frames a
Collectionas anArray([..]), not a tuple ((..)) — the server rejects('2026-05-13')forArray(Date)("Array does not start with '['"), andString.valueOfalready used[..]for lists.Changes
client-v2/.../internal/HttpAPIClientHelper.java: replaceString.valueOf(v)inaddStatementParamswith a newformatStatementParam(v); add private recursive helpersformatStatementParamContainer/formatStatementParamElement. Importcom.clickhouse.data.ClickHouseValues.Test
HttpAPIClientHelperTestasserts the exact emitted strings for the full matrix: scalarDate/String/number stay unquoted (contrast);Array(Date)/Array(String)/Array(DateTime), object-array, nestedArray(Array(Date)), andMap(String,Date)single-quote their leaves; embedded-quote escaping (['a\'b']);nullleaf →NULL; and numeric/Decimalarrays stay unquoted ([1,2,3],[1.50]) — contrast cases proving the fix doesn't over-quote. (12 cases, all pass.)QueryTests#testContainerQueryParamsQuoteInnerValuesround-tripsArray(Date),Array(String),Array(Int32)andMap(String,Date)parameters end-to-end viatoString({...})and asserts the parsed values. This fails onmain(server 400) and passes with the fix.['2026-05-13']accepted,[2026-05-13]rejected (Code: 27),['1','2']rejected forArray(Int32)(Code: 130).changes_checklist.md — "String, SQL, or serialized output changed"
String/temporal leaves single-quoted (with backslash-escaping viaconvertToSqlExpression), numeric/Decimalleaves unquoted,null→NULL— each matched against the server's text parser.param_<name>rendering of container values changes; scalar rendering is byte-identical (String.valueOf), so no scalar regression. Existing param tests pass scalar / pre-formatted-string values and are unaffected.Pre-PR validation gate
[2026-05-13])String.valueOfon containers)AGENTS.md/docs/changes_checklist.mdCross-repo context
Same bug class as
clickhouse-connectissue #188 (Python): a server-side parameter formatter not recursively quoting inner temporal/Stringvalues. Root cause is client-side value formatting for the HTTPparam_*path (not the wire protocol), so it is specific to each client's formatter.