Support INSERT ... VALUES in the polyglot SQL dialect - #110321
Support INSERT ... VALUES in the polyglot SQL dialect#110321alexey-milovidov wants to merge 55 commits into
Conversation
Inline INSERT data (`INSERT ... VALUES (...)` / `... FORMAT ...`) could not be used with `dialect = 'polyglot'`: it failed with "Multi-statement queries are not supported". The underlying problem is that transpiling inside the parser cannot deliver inline data to the executor - the transpiled buffer is transient, and the executor overwrites `ASTInsertQuery::tail` with the external input stream, so the data pointers (`data`/`end`) must reference a live query buffer. Transpile the query up front instead of inside the parser: - The server (`executeQuery`) transpiles a foreign-dialect query to ClickHouse SQL before parsing, keeps the transpiled text alive on the query context, and parses it with the standard parser. Inline INSERT data then points into a live buffer and is processed by the normal machinery. SET queries are still parsed as-is so `dialect`/`polyglot_dialect` can always be changed back. - The client (`clickhouse-client`/`clickhouse-local`) sends a query written in a non-ClickHouse dialect verbatim (without splitting off inline data) and lets the server transpile and read it, so transpilation happens exactly once, server-side. All changes are gated on the dialect, so ordinary ClickHouse INSERTs are unaffected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Workflow [PR], commit [9455544] Summary: ❌
|
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
…cted cleanly Extend `04512_polyglot_insert_values` to cover the case where a second statement follows inline `INSERT ... VALUES` data in the polyglot dialect (e.g. `INSERT INTO t VALUES (1); SELECT 2`). The whole remaining buffer is transpiled at once and the transpiler rejects the multi-statement input, so the query fails with a clean `SYNTAX_ERROR` rather than being silently mis-executed or reaching the server as unread `Values` tail. The test also asserts that no partial insert happens on the rejected path. This documents the behavior flagged in the PR review, which was verified not to reproduce as a silent regression.
Address review: on the verbatim polyglot path the client sends the query text as-is and never forwards external data, so piped stdin rows were silently dropped: \`printf '(2)\n' | clickhouse-client --dialect polyglot ... -q 'INSERT INTO t VALUES (1)'\` inserted only the inline row while in the \`clickhouse\` dialect both rows are inserted. Now a foreign-dialect \`INSERT\` (without \`SELECT\`) with data on stdin or \`INFILE\` fails with \`NOT_IMPLEMENTED\` before anything is sent, matching the existing checks for \`async_insert\` and inline-insert-data modes, which throw the same way instead of losing data. Add a test case asserting the error and that no partial insert happens. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The Tracked in #109215, and a fix is already in progress: #109114. |
…t dialect `executeQueryImpl` reassigned `begin`/`end` to point at the transpiled ClickHouse SQL when parsing a `polyglot`-dialect query, and those same local variables were then used further down to build `query`/`query_for_logging`/`normalized_query_hash`. As a result, `system.query_log`/processlist showed the transpiled SQL instead of what the user actually submitted, and per-`normalized_query_hash` quotas grouped by the transpiled form. Additionally, when an `ASTInsertQuery::data` pointer (into the transpiled buffer) was used to cut the logged text short, it was sliced against the original (untranspiled) `begin`, mixing pointers from two unrelated buffers. Keep the transpile-and-reparse step local to the polyglot branch instead of reassigning the outer `begin`/`end`, and only use `insert_query->data` to shorten the logged query when it actually falls within `[begin, end)`. Added a regression test asserting `system.query_log.query` for a polyglot `INSERT` matches the original text. Addresses review feedback on #110321
|
The |
The style check requires `SYSTEM FLUSH LOGS log_name` instead of the global `SYSTEM FLUSH LOGS`, and the test only reads `system.query_log`.
For a `polyglot`-dialect inline `INSERT`, the parsed `ASTInsertQuery::data` pointer references the transpiled buffer owned by the query context, not the original query text `[begin, end)`. The previous logging truncation only cut the query short when `data` fell inside `[begin, end)`, so for polyglot inline `INSERT`s it never fired and the full `VALUES`/`FORMAT` payload was written to `system.query_log`, the process list, and `normalized_query_hash`, breaking the "`INSERT` logs omit inserted data" contract and potentially leaking row values. The inline-data boundary cannot be mapped back onto the original text because transpilation rewrites the query, so log the transpiled header up to the data instead: it carries the `INSERT` target and column list but no row values, and reflects what was actually executed. Non-`INSERT` polyglot queries keep logging the original text as before. Updated `04512_polyglot_insert_values` to assert the inline data is absent from `system.query_log` instead of asserting the (leaky) full original text. Addresses review feedback on #110321 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
In the multi-query path, `ClientBase` computed `insert->data - query_to_execute.data()` unconditionally for every `ASTInsertQuery`. For a foreign-dialect (`polyglot`) `INSERT`, the polyglot parser clears `insert->data` (the query is sent verbatim and the server reads the data from its own transpiled buffer), so this became `nullptr - ptr`, which is undefined behavior. The computed length was already unused on the verbatim path (guarded by a later `insert && insert->data && !send_query_verbatim` check), so guard the subtraction on `insert->data` being non-null. Addresses review feedback on #110321 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address review (clickhouse-gh AI verdict): the PR contract claimed "transpilation happens exactly once, on the server", but the native client (`clickhouse-client`/`clickhouse-local`) is fundamentally AST-driven, so for a foreign dialect `ClientBase::parseQuery` transpiles the query locally via `ParserPolyglotQuery` to obtain an AST for client-side handling (statement classification, output format, INSERT detection). That transpiled text is thrown away; the client sends the *original* query verbatim and the server performs the authoritative transpilation whose result is actually executed. No behavior change: this only makes the comments (and the PR description) accurate about the two-stage transpilation and its accepted limitations for this experimental dialect — the client requires the transpiler to be built in (`USE_POLYGLOT`, else `SUPPORT_IS_DISABLED`), and the client and server transpilers are assumed to agree. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address review: `ParserPolyglotQuery::parseImpl` only cleared the dangling `data`/`end` pointers of a top-level `ASTInsertQuery`. The client unwraps an `ASTExplainQuery` and dereferences the nested `ASTInsertQuery::data` the same way when it locates the inline-data boundary (`ClientBase::analyzeMultiQueryText`), so an `EXPLAIN INSERT ... VALUES` in polyglot mode would leave the nested insert's `data`/`end` pointing into the transient `transpiled` string that is freed on return - a use-after-free in `-n`/script mode. Introduce a `findInlineDataInsert` helper that unwraps a single `EXPLAIN` layer (mirroring the client) and clear the pointers of the explained `INSERT` too. This is a defensive correctness fix: no bundled transpiler dialect currently transpiles `EXPLAIN INSERT ... VALUES` (they reject it at the `VALUES` token), so the path is not yet reachable, but the fix future-proofs it against a transpiler that supports the form. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address review: a foreign-dialect `INSERT ... VALUES`/`FORMAT` is transpiled as a whole, so its inline data counts towards `max_query_size` - unlike a native ClickHouse `INSERT`, whose inline data is streamed and is not bounded by `max_query_size`. This explicitly scopes the experimental feature to inline payloads that fit the parser size limit. The oversized case now fails-close with a dedicated, actionable error (added in the previous commit in `transpilePolyglotToClickHouse`) instead of silently changing the `INSERT` size contract; document the limitation on the `allow_experimental_polyglot_dialect` setting so users can discover it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Extend `04512_polyglot_insert_values.sh`: - `EXPLAIN SELECT` transpiles and runs (exercises the new `EXPLAIN`-unwrapping helper, which returns no inline-data `INSERT` for a non-insert). - `EXPLAIN INSERT ... VALUES` is rejected cleanly by the transpiler (no bundled dialect transpiles it) with no partial insert and no use-after-free. - An inline `INSERT` payload larger than `max_query_size` is rejected with the dedicated error and inserts nothing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… mode The client parses scripts with allow_multi_statements enabled, which zeroes the generic per-query length limit, so `ParserPolyglotQuery` was constructed with `max_query_size = 0` and the oversized-query guard never ran on the client: an oversized polyglot inline `INSERT` in `--multiquery` mode was fully transpiled locally before the server rejected it. Construct the polyglot classifier with the real `max_query_size` (it always consumes the whole remaining buffer as a single query sent verbatim, so the per-query limit applies in every mode), and run the transpile - whose size guard rejects oversized input up front - before touching the token stream. Also stop the token-advance loop at `ErrorMaxQuerySizeExceeded`: the lexer emits it on every call once a size-capped stream passes its limit (never reaching `EndOfStream`), so iterating to the end could not terminate. Add a --multiquery regression proving the oversized query is rejected on the client, without a server round trip: its query_id must not appear in `system.query_log` (a server-side rejection would be recorded there as `ExceptionBeforeStart`). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`transpiled_query` was stored as a plain `String` inside `ContextData`, and `Context::createCopy` performs a deep copy of it. Insert paths such as `AsynchronousInsertQueue::preprocessInsertQuery`, `StorageDistributed` distributed writes, and `DistributedSink` clone the query context, so a large polyglot inline `INSERT` payload could be duplicated several extra times in memory. Hold the buffer behind `std::shared_ptr<const String>` so context copies share it instead of cloning it; this also keeps the buffer alive independently of which context outlives which. Addresses the AI review finding on the PR.
|
The It is a The |
|
STID 3262-3b6a. This exact signature (
What the stack does pin down is that the I have not reproduced it yet. Four synthetic shapes (LowCardinality and Array haystacks, array tokenizer, postprocessor, index added after insert, |
|
The type drift does not originate on the text index side.
The text index read is only the consumer that trips first. I checked what happens without it: The fix at the cause is in flight as #113900, which changes exactly that converting prefix so Two notes. It is not only a fuzzer artifact: the text index arm fails at all default settings, |
The style check flags any test file mentioning `system.query_log` without a `current_database = currentDatabase()` condition, including mentions in comments. `04512_polyglot_insert_values.sh` no longer queries the log (that moved to `04843_polyglot_insert_log_redaction`), so reword the cross-reference comment instead of adding a spurious condition. https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=110321&sha=b7ef5e1cb89aa95f9cb5c6d6a489d9a5a971e280&name_0=PR&name_1=Style%20check Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…m polyglot query The client pinned only `dialect`, `allow_experimental_polyglot_dialect` and `polyglot_dialect` before sending a verbatim polyglot query, but the polyglot classifier's parse (and the server-side reparse of the same text) also depends on `allow_settings_after_format_in_insert`, `implicit_select`, `max_query_size`, `max_parser_depth` and `max_parser_backtracks`. A query-local `SETTINGS` clause changing one of those (e.g. `SELECT 3 + 4 SETTINGS max_parser_depth = 1`) made the server reparse the very same text the client already accepted under different parser settings and fail with an exception, even though such settings should only apply to the query's execution. Pin the whole set of parse-time settings instead of the dialect triple. Regression cases added for the parse limits (`max_parser_depth`, `max_query_size`), which are the drift vectors reachable through the bundled transpilers: the flag-shaped knobs cannot drift in practice, because the bundled dialects do not re-emit a `SETTINGS` clause in the positions where those flags govern the parse.
A foreign-dialect query (`dialect = 'polyglot'`, `kusto`, `prql`, `promql`) is sent to the server verbatim, with the parse-time `dialect` pinned, so the server reparses - for `polyglot`: transpiles - exactly the text the client classified. Two client paths break that assumption: they replace the outbound text with the parsed AST serialized back to ClickHouse SQL, and for a foreign dialect that AST is the *transpiled* one. `clickhouse-client --dialect=polyglot --allow_merge_tree_settings --index_granularity=1024 --query "CREATE TABLE t (x int, y text)"` therefore sent a `CREATE TABLE ... ENGINE = MergeTree ORDER BY tuple() SETTINGS index_granularity = 1024` while still asking the server to transpile that text as PostgreSQL. The second transpilation silently dropped the added setting: the table was created with the default `index_granularity = 8192`. The other such path is the query-parameter substitution for a server older than `DBMS_MIN_PROTOCOL_VERSION_WITH_PARAMETERS`. Pin the outbound `dialect` to `clickhouse` when the text being sent is a serialized AST, which is what `pinOutboundDialectForJSONDialect` already did for the `clickhouse_json` dialect - generalized here and renamed to `pinOutboundDialect`, since both dialect families need it for the same reason. Related: #110321 (comment)
The test asserted that `--allow_merge_tree_settings --index_granularity=1024` applies to a foreign-dialect `CREATE TABLE t (x int, y text)`, but the rewrite (`addMergeTreeSettings`) only fires when the parsed AST carries an explicit MergeTree engine — and the transpiled AST of an engine-less foreign `CREATE` has no storage definition at all (only the server fills in the default engine later). A native engine-less `CREATE` ignores the command-line MergeTree settings for the same reason, so polyglot already behaves exactly like the native dialect there, and the test failed in every CI configuration: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=110321&sha=4f0ee56e3a9045e6b7cf3f7cfbd136f3fcf755ee&name_0=PR Only the `clickhouse` source dialect can express an `ENGINE` clause (the other bundled dialects reject it or drop it in transpilation), so exercise the rewrite through it, and prove the pinned outbound dialect via query_log: the rewritten text must be on the wire and must not be sent for a second transpilation. The postgresql part keeps only the type-transpilation assertions. #110321
Per review: drop `redactPolyglotQueryForLogging`. A polyglot query that fails before an AST exists (e.g. the transpiler rejects it) is now logged verbatim, matching the native dialect, which also logs an unparseable `INSERT` with its inline data. The successful-parse paths still log only the statement header: they know the data boundary from the AST (`getInsertAST`). #110321 (comment)
On the verbatim (foreign dialect) path a plain inline `INSERT` goes through `processOrdinaryQuery`; against a server older than `DBMS_MIN_PROTOCOL_VERSION_WITH_PARAMETERS` the client substitutes query parameters and replaces the outbound text with the serialized AST — but `ASTInsertQuery`'s formatter prints only the header, never the inline data, so the payload would be silently dropped. Throw `NOT_IMPLEMENTED` instead. #110321 (comment)
…ced one The guard for external data accompanying a foreign-dialect `INSERT` rejected every deferred HTTP `100 Continue` request, because the body cannot be inspected before that response is sent. But a deferred `100 Continue` without a body is a valid shape (it is exercised by `03353_http_100_continue`), and rejecting it broke a polyglot `INSERT` over HTTP for such clients. Key the guard off actual body presence instead: the new `QueryFlags` field `http_request_has_body` carries what the client announced in the request headers (a non-zero `Content-Length`, or a chunked transfer encoding), which is the only source of truth while the `100 Continue` response is deferred. Without the deferral the buffer stays authoritative. Regression tests: a deferred `100 Continue` without a body inserts, and with a body is still rejected.
|
🕵 Both CI failures on
@groeneai, please investigate the |
|
The 04509 failure is already fixed by #114597 (merged 2026-08-14T00:51:44Z, Root cause: Your run predates the fix. The Post-merge CIDB for the test: 2777 OK and zero FAIL or ERROR since 03:00Z today, against 36 failures across 30 pull requests on 2026-08-13. The only two FAIL rows after the merge timestamp are on branch heads that do not contain the fix. No tracking issue is due. I found none open for this test, and since the defect is already fixed, filing one now would only be closed. Your read on the |
Keep `checkExternalDataAfterDeferredContinue` alive until all possible HTTP 100-Continue callback paths have run.\n\nCI: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=110321&sha=4e453d54f533440fc041cc9dc543c5c9c0588924&name_0=PR
|
🕵 Updated this branch with current |
|
🕵 The current |
2 participants
Enable
INSERT ... VALUESwith inline data in the polyglot SQL dialect (dialect = 'polyglot').Previously, running e.g.
INSERT INTO t VALUES (1), (2), (3)withdialect = 'polyglot'failed withMulti-statement queries are not supported in polyglot dialect mode. The underlying problem is that transpiling inside the parser cannot deliver inline data to the executor: the transpiled buffer is transient, and the executor overwritesASTInsertQuery::tailwith the external input stream, so the inline-data pointers (data/end) must reference a live query buffer.This transpiles the query up front instead of inside the parser:
executeQuery) transpiles a foreign-dialect query to ClickHouse SQL before parsing, keeps the transpiled text alive on the query context, and parses it with the standard parser. Inline INSERT data then points into a live buffer and is processed by the normal machinery.SETqueries are still parsed as-is sodialect/polyglot_dialectcan always be changed back.clickhouse-client/clickhouse-local) parses a non-ClickHouse-dialect query into an AST — which, for a foreign dialect, means transpiling it locally only to drive client-side handling (statement classification, output format, INSERT detection) — but then sends the original query text verbatim, without splitting off inline data. The server performs the authoritative transpilation whose result is actually executed, so inline INSERT data lives in a server-owned buffer and survives parsing. The client-side transpilation is throwaway; note this means the transpiler must also be available on the client (a client built withoutUSE_POLYGLOTfails locally withSUPPORT_IS_DISABLED), and the client and server transpilers are assumed to agree — acceptable for this experimental dialect. Every parse-time setting the query was parsed under (dialect,allow_experimental_polyglot_dialect,polyglot_dialect,allow_settings_after_format_in_insert,implicit_select, and the parse limitsmax_query_size,max_parser_depth,max_parser_backtracks) is pinned in the per-query settings sent along with the verbatim text, so the query's ownSETTINGSclause cannot change how the server reparses that same text (it still applies to the query's execution, and aSETstill takes effect for subsequent queries).All changes are gated on the dialect, so ordinary ClickHouse INSERTs are unaffected. Validated over the HTTP interface, the native client, and
clickhouse-local(multi-row and single-rowVALUES,INSERT ... SELECT, and PostgreSQL literal transpilation such astrue/false);SETpassthrough and multi-statement rejection are preserved. External insert data combined with a foreign-dialectINSERTis rejected withNOT_IMPLEMENTEDinstead of being silently dropped, on both surfaces: the client rejects piped stdin andINFILE(it sends the query verbatim and cannot forward a data tail), and the server rejects a non-empty HTTP request body appended to a streamingINSERT(POST /?query=INSERT ... &dialect=polyglotwith a body). A foreign-dialectINSERTis transpiled as a whole, so the body would go through neither the transpiler nor themax_query_sizeguard, mixing two parsing rules in oneINSERT. An empty body still works, which is the normal way to run a polyglotINSERTover HTTP.Limitations (scoped, experimental): because a foreign-dialect query is transpiled as a whole (the transpiler rewrites the inline data too and cannot know where the SQL header ends without parsing the dialect), the inline
INSERT ... VALUESdata counts towardsmax_query_size— unlike a native ClickHouseINSERT, whose inline data is streamed and is not bounded bymax_query_size. An oversized payload fails-close with a dedicated, actionable error rather than silently changing theINSERTsize contract; increasemax_query_sizeto submit larger inline payloads.Only
INSERT ... VALUESinline data is transpilable by the bundled dialects.INSERT ... FORMAT ...is not:FORMATis a ClickHouse-only extension, so a foreign-dialect parser rejects the query at the inline data that follows (empirically,postgresql/mysql/sqlite/duckdb/snowflake/bigqueryall fail at the first data row afterFORMAT; a hypothetical identity transpiler even drops the rawFORMATpayload rather than re-emitting it). A foreign-dialectINSERT ... FORMATtherefore fails cleanly with a syntax error and inserts nothing — likeEXPLAIN INSERT ... VALUES, which is also not transpilable by the bundled dialects (rejected at theVALUEStoken). The server-owned transpiled buffer that carries the inline data is itself format-agnostic and would handleFORMATdata if a transpiler ever produced such a query; the parser also defensively clears the inline-data pointers of anEXPLAIN-wrappedINSERT— the same way the client unwraps it — so both forms are safe if a future transpiler supports them.Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Support
INSERT ... VALUESwith inline data when using the experimentalpolyglotSQL dialect.Documentation entry for user-facing changes