Fix client-v2: request the format of internal queries thru settings, not a FORMAT clause - #3069
Conversation
The client sends the requested format of an operation in the X-ClickHouse-Format header of every request. getTableSchema/getTableSchemaFromQuery asked instead for TSKV with a FORMAT clause in the DESCRIBE query, and ping used a FORMAT clause too. A server before 26.8 used the format of the query, but since 26.8 the header wins, so the server answered with RowBinaryWithNamesAndTypes and the TSKV parser failed with "Failed to parse column null defined by type 'null'". The internal queries of the client now carry no FORMAT clause and set their format in the settings of the operation, so the header and the query always agree. Fixes: #3068
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
chernser
left a comment
There was a problem hiding this comment.
Still failing with head version
[ERROR] HttpTransportTests.testMultiPartRequest:1831 » Client Failed to get query response
[ERROR] DataTypeTests.testAllDataTypesKnown:1492 There are some unknown types: [MultiPoint] expected [true] but found [false]
[ERROR] MetadataTests.testCreateTableWithAllDataTypes:194 » Client Failed to get table schema
[ERROR] BinaryReadyReusesBuffersTests>QueryTests.testSettingsNotChanged:2345 expected [JSONEachRow] but found [RowBinaryWithNamesAndTypes]
[ERROR] QueryServerContentCompressionTests>QueryTests.testSettingsNotChanged:2345 expected [JSONEachRow] but found [RowBinaryWithNamesAndTypes]
[ERROR] QueryServerHttpCompressionTests>QueryTests.testSettingsNotChanged:2345 expected [JSONEachRow] but found [RowBinaryWithNamesAndTypes]
[ERROR] QueryTests.testSettingsNotChanged:2345 expected [JSONEachRow] but found [RowBinaryWithNamesAndTypes]
run tests like mvn -DclickhouseVersion=head --projects client-v2 clean install
Documents the new format resolution in CHANGELOG.md and docs/features.md.
|
Thank you for running the suite against Fixed by the new commits
Not caused by this PR
Changes in the new commits: |
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 4ce0695. Configure here.
The server accepts a format name in any case (FORMAT jsoneachrow works), but extractTrailingFormat resolved it with ClickHouseFormat.valueOf, which is case-sensitive. A clause written in a different case gave null, the header kept RowBinaryWithNamesAndTypes, and a 26.8 server preferred that header over the clause - so the caller got the wrong bytes.
TriageCategory: Summary What this impacts
Concerns
Required reviewer action
|
chernser
left a comment
There was a problem hiding this comment.
- remove parsing of SQL for FORMAT clause - this is parser responsibility
- check JDBC and Client if we do queries with FORMAT clause
- make clear in documentation that format should be set via settings
- there is a case when in JDBC where user may send JSONEachRow and we suppose to handle it - create an issue for that.
Reverts the resolution of a FORMAT clause written by a caller (commits f449cf7, 4ce0695 and 838fab7), which reads SQL in the client - a job of the parser. The PR again fixes only the queries the client itself sends: getTableSchema, getTableSchemaFromQuery and ping now request their format in the settings of the operation. Shortens the CHANGELOG entry and states in docs/features.md that the output format is to be set through the query settings.
|
Done — pushed 1. Removed the SQL parsing. Commits 2. Audit of our own queries with a
So after this PR no query the client itself sends can disagree with the header. 3. Documentation. 4. The JDBC case where a user sends On the failures you listed, with
|
|




Description
Fixes #3068.
client-v2sends the requested format of an operation in theX-ClickHouse-Formatheader of every request (HttpAPIClientHelper.addHeaders, from the format of the operation settings, defaulted toRowBinaryWithNamesAndTypesinClient.query).getTableSchema/getTableSchemaFromQueryasked instead forTSKVwith aFORMATclause in theDESCRIBEquery, andping()did the same withTabSeparated. A server before26.8used the format written in the query, so the mismatch was invisible. Since26.8the header wins over the clause, so the server answers aDESCRIBE ... FORMAT TSKVwithRowBinaryWithNamesAndTypes, theTSKVparser reads binary data, and every schema call fails withFailed to parse column null defined by type 'null'.jdbc-v2fails with it too, becauseConnectionImplandPreparedStatementImplread metadata thru those calls.Following the direction on the issue, the internal queries of the client now carry no
FORMATclause and request their format thru the settings of the operation, so the header and the query always agree on every server version.Changes
Client.getTableSchema(table, database)andClient.getTableSchemaFromQuery(sql, params): noFORMAT TSKVclause in theDESCRIBE/DESC (...)statement.Client.getTableSchemaImpl: theQuerySettingsof the call setsTSKV.Client.ping(timeout):SELECT 1withTabSeparatedin the settings instead of aFORMATclause.query(...)overloads: the old text said theFORMATclause of the SQL query always wins, which is not true since server26.8.SpanRecorderTest: the three assertions on the statement text of an internal query follow the new text (SELECT 1,DESCRIBE TABLE <t>,DESC (...)).CHANGELOG.md.Test
New
client-v2/src/test/java/com/clickhouse/client/api/RequestFormatUnitTest.java: aWireMockserver records the requests of the client, and one parametrized test asserts, per operation, the exact statement sent and theX-ClickHouse-Formatheader —getTableSchema->DESCRIBE TABLE test_table+TSKV,getTableSchemaFromQuery->DESC (SELECT id FROM test_table)+TSKV,ping->SELECT 1+TabSeparated. Those three rows fail onmain(the statement still holds the clause). Two more rows pin the unchanged behavior of a caller query: a plain query still asks forRowBinaryWithNamesAndTypesand a query with an explicitJSONEachRowsetting still asks forJSONEachRow; both pass onmainand after the change.Verified against a
26.8.1.1761server (clickhouse/clickhouse-server:head) in a container:MetadataTests.testGetTableSchema,testGetTableSchemaDifferentDbandtestCreateTableWithAllDataTypesfail withFailed to parse column null defined by type 'null'.testCreateTableWithAllDataTypesstill fails, but for an unrelated reason —Unknown data type: MultiPoint, which [client-v2, jdbc-v2] Add MultiPoint type support #3048 adds.client-v2unit tests: 561 pass.SpanRecorderTest: 6 pass.QueryTests: 96 run, see the note below.Compatibility: no public API change and no behavior change for a caller query. A header-only format request works on old servers as well — checked with
24.3.18.7and26.6.2.160, where a request withX-ClickHouse-Format: TSKVand noFORMATclause returnsTSKV.Note — a related problem this PR does not fix
The same server change also affects a caller query that holds a
FORMATclause: the client sends its defaultRowBinaryWithNamesAndTypesheader, which now wins, so the query silently returns other bytes.QueryTests.testSettingsNotChanged(select 1 FORMAT JSONEachRowwith empty settings) fails on26.8for that reason, onmainand with this PR alike; it passes on26.6.jdbc-v2StatementImpl.executeQueryImplsends the SQL of the user verbatim with no format in the settings, soSELECT ... FORMAT JSONEachRowthru JDBC has the same problem, anddocs/client-v2-json-support.mdanddocs/features.mdstill recommend that pattern. Deciding whether the client keeps honouring aFORMATclause (detect it and align the header) or the clause becomes unsupported (then the docs, the examples and that test need to follow) is a separate decision, so it is left out of this fix. Please tell us which way you want it and we will raise it separately.Pre-PR validation gate
26.8.1.1761)SpanRecorderTestassertions on the statement text follow the intentionally changed text)AGENTS.md(CHANGELOG.mdupdated, one logical change,@DataProviderinstead of near-identical test methods, no issue numbers in test code)docs/features.mdneeds no update