Support INSERT ... VALUES in the polyglot SQL dialect - #110321
Support INSERT ... VALUES in the polyglot SQL dialect#110321alexey-milovidov wants to merge 73 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 [5d7ad7f] Summary: ❌
AI ReviewSummaryThis PR adds verbatim handling for Findings
Tests
Final VerdictLLVM Coverage Report |
![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 |
…t_values` The case set `Transfer-Encoding: chunked` as a plain header while the body still came from `--data-binary @-`, so `curl` only relabelled a request whose body it sent unencoded. `curl` 7.81 (the version on the CI runners) then never emitted the terminating chunk, the server waited for a body that never arrived, and the request died on the 60-second `--max-time`, failing the test in every stateless job. Uploading from stdin with `-T -` makes `curl` chunk-encode the request itself, which is what the case means to exercise. Reproduced with `curl` 7.81 against a local server: the old form hangs and the new one completes with `200 OK`.
|
🕵 @groeneai, investigate the failure: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=110321&sha=3693dc23ea2791dd76168000106548b5ecf14f20&name_0=PR&name_1=Integration%20tests%20%28amd_tsan%2C%202%2F6%29 and provide a fix in a separate PR. If the fix is already in progress, link it here. Every The stateless reds were ours and are fixed in |
|
The The fix is already in progress: ClickHouse/nats.c#5 The reported race is a symptom of a socket-close ordering violation, which is why the fix is not a lock. It is a submodule PR against On the tracking issue: I treated nats.c#5 as the tracking record, which is why nothing was filed here. Both defects are verbatim in current Frequency of the signature, for triage: three runs across three pull requests since 2026-08-17, none on master. |
… data The client rejected external data (stdin or `INFILE`) for every verbatim foreign-dialect `INSERT` that is not an `INSERT ... SELECT`. That is too broad: when the transpiled statement carries no inline data at all — e.g. `INSERT INTO t FORMAT TSV` with `polyglot_dialect = 'clickhouse'` — the server transpilation produces no inline data either, so the data can travel in the ordinary data packets exactly as for a native `INSERT`. Before this the client threw `NOT_IMPLEMENTED` while the HTTP path accepted the same shape, because `executeQuery` rejects external data only when `hasTranspiledInlineData` is true. `ParserPolyglotQuery` now records `ASTInsertQuery::inline_data_owned_by_transpiled_query` when it clears the inline-data pointers that referenced its transient transpiled buffer, and the client consults that flag: such an `INSERT` still rejects external data, while an `INSERT` without inline data goes through `processInsertQuery` and streams stdin/`INFILE`. The old-server query-parameter substitution guard is narrowed the same way (an `INSERT` without inline data serializes losslessly). Addresses the review comment: #110321 (comment)
`04512_polyglot_insert_values` grew to 45 `clickhouse-client` invocations plus 8 `curl` calls, and process startup dominates its wall time. On a sanitizer build with s3 storage and metadata in Keeper it takes 150-172 s per run against `TEST_MAX_RUN_TIME_IN_SECONDS = 180`, so a single slow run trips "Test runs too long" - one did, at 181 s, in a flaky check that runs several copies at once. Move the HTTP interface, the pinning of parse-time settings for the server-side reparse and the streaming of external data into `05054_polyglot_insert_values_http`, which starts from its own empty table. Every case and every assertion is kept; the expected running totals of the moved half are shifted down by the rows the client-side half had inserted.
`url_query_starts_with_insert` skipped a CTE body only as a parenthesized subquery or a single name, so `WITH cte AS MATERIALIZED (SELECT 1) INSERT ...` fell back to the legacy path that concatenates the request body onto the URL query, letting an HTTP body bypass the `NOT_IMPLEMENTED` rejection of external data for a polyglot `INSERT` with inline data. Consume the optional `MATERIALIZED` keyword (see `ParserWithElement`) before skipping the subquery; a bare `MATERIALIZED` not followed by a parenthesis is still an ordinary alias name, and such a query keeps the legacy body-concatenation path. Both shapes are covered in 05054_polyglot_insert_values_http. Addresses #110321 (comment)
|
🕵 Two CI failures on
|
|
The What the preserved logs show: Two details narrow it:
The bound is not too tight either. Same commit, same tsan build, same |
|
Separate PR is up: #117294 It does not fix the hang. It fixes what the job throws away when the hang happens. Why I stopped short of the hang itself: naming the mechanism needs a thread stack, and none of |
The client-side classifying parse of a `polyglot` query went through `tryParseQuery`, which does a lexer preflight on the original buffer (the invalid-token shortcut) and an unmatched-parentheses check over it. Both operate on the *foreign* SQL, which is not expected to be lexically valid ClickHouse SQL, so `clickhouse-client` and `clickhouse-local` rejected statements that the server - which transpiles before parsing - executes fine. Example: BigQuery spells a single-line comment `#comment`, while the ClickHouse lexer only treats `#` as a comment when followed by a space or `!`. Add `IParser::consumesForeignText`, which `ParserPolyglotQuery` overrides, and skip both original-buffer lexical checks in `tryParseQuery` for such a parser. The text is validated by parsing the *transpiled* ClickHouse SQL instead, exactly like on the server. Test: `05055_polyglot_lexically_foreign_query`.
|
🕵 CI status on
@groeneai, investigate the failure: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=110321&sha=42e6fb428c3380e4904795ba4e6136c94dc0a23c&name_0=PR&name_1=Stress%20test%20%28amd_debug%29 and provide a fix in a separate PR. If the fix is already in progress, link it here. |
| /// after transpiling it — asks for the external data exactly as it does for a native INSERT, so the | ||
| /// client streams stdin or INFILE as usual. Only an INSERT whose data lives inline in the transpiled | ||
| /// query has no room left for a data stream (it was rejected above if any was supplied). | ||
| if (insert && (!insert->select || input_function) && (!is_async_insert_with_inlined_data || input_function) && !is_inline_insert_data |
There was a problem hiding this comment.
This still breaks the new verbatim input() path in clickhouse-local. processParsedSingleQuery now deliberately routes a foreign-dialect INSERT ... SELECT * FROM input(...) into processInsertQuery, but LocalConnection reparses state->query inside its input() initializer with plain ParserQuery and does not capture allow_experimental_polyglot_dialect / polyglot_dialect at all (src/Client/LocalConnection.cpp:231-246, src/Client/LocalConnection.cpp:340-356). So as soon as the original query uses any non-ClickHouse syntax, the local path rejects it before stdin is read even though the initial client-side ParserPolyglotQuery already accepted it.
A concrete shape is PostgreSQL INSERT INTO t SELECT x::Int32 FROM input('x String') with --input-format TSV: the main path transpiles x::Int32, but the local input() setup reparses the original text and fails on ::. If clickhouse-local is meant to support the same verbatim polyglot input() contract as the native client, the input() initializer needs the same polyglot parser branch/settings as ClientBase::parseQuery; otherwise this path should fail close for dialect = 'polyglot' instead of only working for the identity clickhouse source dialect. Please add a focused clickhouse-local regression once the local path is fixed.
|
Investigated. No fix is in flight for this shape, so I am opening a separate PR. Two things up front: the failure is unrelated to this PR, and the master merge in The aborting query, from SELECT a, b, sum(b) FROM test_1.t_grouping_sets_force
GROUP BY GROUPING SETS ((), (b), (a)) ORDER BY a ASC, b ASC
UNION DISTINCT
SELECT a, b, sumOrNull(b) FROM test_1.t_grouping_sets_force
GROUP BY GROUPING SETS ((a), (b)) ORDER BY a ASC, b ASC WITH FILL FROM -26 TO 14It is a mutated variant of the I reproduced it on master CREATE TABLE t (a Int32, b Int32) ENGINE = MergeTree ORDER BY a;
INSERT INTO t VALUES (1, 2), (3, 4), (1, 5);
SET enable_analyzer = 1, group_by_use_nulls = 1;
SELECT a, b FROM t GROUP BY GROUPING SETS ((a), (b)) ORDER BY a ASC, b ASC
UNION DISTINCT
SELECT a, b FROM t GROUP BY GROUPING SETS ((a), (b)) ORDER BY a ASC, b ASC WITH FILL FROM -26 TO 14;That commit already contains #113242, which is the merged fix this family was previously attributed to, so #113242 does not cover this one. Nothing between Root cause: Every ingredient is load-bearing. On the same binary, the abort goes away if any one of these is changed: drop One search note, since you looked for an issue: this assertion message is bucketed across roughly 15 STIDs, so a single-STID query under-reports it. I will post the PR link here. |
|
Separate PR is up: #117832 Root cause of STID SELECT number % 2 ? NULL : toNullable(toInt32(number)) AS x
FROM numbers(2) ORDER BY x ASC WITH FILL FROM 1 TO 3;
-- before: 0, 2, \N, 2 (FROM row dropped, 2 repeated after the NULL)
-- after: 0, 1, 2, \NUnrelated to this PR, as expected, and the master merge in Scoped to a single |
`functional_tests.py` assigns `test_result` only inside the test stage, so a setup failure leaves it `None`. `test_run_failed` is derived from it alone, reads `False`, and `prepare_logs` runs with `all=False`: core dumps, system-table dumps, the object-storage service log, dmesg and the Keeper logs are discarded for exactly the runs that have no test results to explain themselves with. Found while investigating a `Stateless tests (amd_tsan, flaky check)` failure on 2def28a, where `prepare_stateful_data` hit its per-statement bound on `INSERT INTO test.hits_s3 SELECT * FROM test.hits`. That job's `result.json` has three children (`Install ClickHouse`, `Start ClickHouse Server`, `Collect logs`, `Failures: 1/3`), and its `job.log` records `send TRAP signal to generate core file` and `Collect logs` in the same second: the harness asked the kernel for a core with the core, system-table, object-storage-log, dmesg and coordination branches all switched off. Whether a core survived the ten-second wait that follows the `SIGTRAP` cannot be told from what was uploaded, which is itself the problem. Of the 403s an earlier investigation saw, `dmesg.log` is the one fully explained by this defect. In CIDB over 30 days that three-child shape covers 58 job-level rows of `Stateless tests (...)` lanes, across 28 commits and 19 of those lanes, at least 45 of them in lanes that do collect logs. Four changes: - Capture an external all-thread backtrace when a prep statement hits its bound. Both in-process routes are dead for the case that needs them: the `SIGTRAP` in `stop_server` produced no log output at all, and `SYSTEM FLUSH LOGS` returned `Code: 209 SOCKET_TIMEOUT`, so `system.stack_trace` is unreachable. Meanwhile the wedged server stayed alive and unexamined for ten minutes between the prep returning and teardown starting. `STACK_CAPTURE_TIMEOUT_S` is 300s, derived from measurement rather than guessed: on the failing job's own TSan binary (build id 66b21ff2c033) a capture took 16.7s idle over 203 threads, 20.5s and 20.6s loaded over 347 threads, and 13.5s for the literal command emitted here over 381 threads, resolving 1874 `DB::` frames of which 1280 carried `at ./src/...` file and line. `tests/docker_scripts/stress_tests.lib` allows 1800s for the identical command, so 300s is the conservative end. Two consecutive attaches during an insert left the server serving and the insert completed, so the capture does not disturb what it inspects. - Skip that capture on ASan builds, where attaching a debugger disables LeakSanitizer. `tests/clickhouse-test` already applies this policy in `print_c_stacktraces` and its message is reused verbatim so the same wording appears in `job.log`. The test matches `"ubsan"` as well as `"asan"` so that a build name carrying `ubsan` without `asan` (which this repository's defs do not produce, though `functional_tests.py:55` and `src/Core/tests/gtest_protocol_packet_to_string.cpp:23` both refer to private `amd_ubsan` and `arm_ubsan` lanes) is skipped rather than captured. No build type in the tree changes behaviour, since both in-tree `ubsan` names already contain `asan`. `arm_fuzzers` is also built with `SANITIZE=address` and matches neither, but it is consumed only by `libFuzzer tests`, which never runs `functional_tests.py`; a branch for it would be dead code. - Require `timeout`'s own TERM diagnostic before treating exit 124 as an expiry. 124 is also the ClickHouse error code `INCORRECT_ELEMENT_OF_SET`, which the client returns from `main`, so the code alone does not distinguish a bound firing from a server exception. This mirrors the existing treatment of 137. The needle deliberately contains no quotes: the real diagnostic quotes the command name with U+2018/U+2019 under a UTF-8 locale, so a needle written with ASCII quotes would silently never match. The predicate reads the whole prep log rather than the fifteen-line tail the error message uses. `Shell.run` streams stdout and stderr into one file, so the diagnostic and the ERR trap are the last lines written and a tail normally holds them, as it did here; reading the whole log removes the dependence on that ordering. The tail itself is sliced with `io.StringIO(text).readlines()`, which is exactly what the previous `f.readlines()` produced, rather than `str.splitlines`, which also breaks on `\x0b`, `\x0c`, `\x1c`-`\x1e`, U+0085 and U+2028 and would have altered the recorded message. - Produce `dmesg.log` in the full-collection path. Its only other producer is the OOM check, which needs test results to grade, so a setup failure never wrote the file that the collection list then looked for. Where that check has already run, its graded file is left untouched and the current buffer is written beside it as `dmesg.at-collect.log`, because `start` clears the ring buffer for each server generation: declining to capture would upload an earlier generation's buffer, and overwriting would leave the graded `OOM in dmesg` row disagreeing with the uploaded bytes. A setup failure that never reached `start` gets an uncleared buffer, so its dump can include lines from earlier on the host; nothing grades it, because the fatal scan needs test results. A `dmesg` that fails leaves an empty file behind, because the shell creates the redirect target first, so that file is removed and a warning printed rather than uploaded as an empty dump that reads as "no OOM". The capture is written under `log_dir` as `stateful-prep-stacks-<pid>.log`, which is uploaded on every collection and matches none of the `clickhouse-server*.log`, `clickhouse-server*.err.log` or `stderr*.log` globs that `check_fatal_messages_in_logs` greps for `<Fatal>` and sanitizer text, so a backtrace cannot become a false blocker. `gdb` exits 0 even when an attach is refused, so its stderr is redirected into the same file and the artifact explains its own emptiness; `Shell.check` cannot raise, so a missing or unattachable `gdb` leaves the job's outcome unchanged. This does not fix the hang underneath the reported failure. That server logged normally for 35.4s of the insert, then emitted nothing for 62.0 minutes from any of the 186 threads that had ever logged, never raised `max_execution_time`, answered no new query, and ignored SIGTERM for 300s. Naming the mechanism needs a thread stack, and none of the seven occurrences in 90 days produced one, or a core, or a `fatal.log`. An earlier revision of this work instead bracketed the fixture load with `SYSTEM STOP/START THREAD FUZZER`; that was withdrawn after measurement, because the fuzzer accounts for 1.76x against a failure of more than 23.7x, its pthread wrappers are compiled out under TSan, and five arms including one at five times the CI fuzzer strength all completed with continuous logging. Perturbing the timing of the thing being diagnosed could also have made the only occurrences available disappear. Report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=110321&sha=2def28ac1fbdc8166d4e6c37c00ab830b4248148&name_0=PR&name_1=Stateless%20tests%20%28amd_tsan%2C%20flaky%20check%29 Related: ClickHouse#110321 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Separate PR is up: #117997 This reverses what I said here on 2026-08-31. I reported then that I had tried bracketing the
The change stops the fuzzer across |
clickhouse-gh
Bot
added
the
comp-sql-syntax
The comment I added in 0d48000, and that commit's own message, claimed the flaky check is the only lane that arms `ThreadFuzzer`. That is wrong repo-wide, and 0d48000 refutes itself further down by describing the stress runner doing the same thing: `tests/docker_scripts/stress_runner.sh` arms the fuzzer unconditionally at :21-23 and stops it at :96, and `tests/integration/helpers/cluster.py` arms it per instance on request (`DEFAULT_THREAD_FUZZER_SETTINGS` at :113, applied at :640 under `enable_thread_fuzzer`). The narrower claim is true, and it is the one that justifies the gate: of the lanes this job runs, only the flaky check arms the fuzzer, because `enable_thread_fuzzer_config` is appended only under `if is_flaky_check:` (`ci/jobs/functional_tests.py:985-986`) and that is its sole caller. The comment now states that. I also reformatted the `prepare_stateful_data` signature to `black` output, so the parameter added by 0d48000 does not leave a formatting deviation inside this change. Report for the failure this addresses: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=110321&sha=2def28ac1fbdc8166d4e6c37c00ab830b4248148&name_0=PR&name_1=Stateless%20tests%20%28amd_tsan%2C%20flaky%20check%29 ClickHouse#110321
|
The Worth knowing before you pick a merge order: bumping only to |
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
Workflow [PR]
Sync PR [sync-upstream/pr/110321]