Support INSERT ... VALUES in the polyglot SQL dialect - #110321
alexey-milovidov wants to merge 77 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 [e6853bb] Summary: ✅
AI ReviewSummaryThis PR moves polyglot inline Findings❌ Blockers
Tests
Final Verdict
LLVM 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>
| /// When `dialect` is a foreign SQL dialect (e.g. polyglot), the query is transpiled to | ||
| /// ClickHouse SQL up front and the result is kept here for the lifetime of the query, so | ||
| /// that the AST's inline `INSERT ... VALUES`/`FORMAT` data pointers reference a live buffer. | ||
| String transpiled_query; |
There was a problem hiding this comment.
transpiled_query is stored as an ordinary String inside ContextData, but Context::createCopy is a deep copy (src/Interpreters/Context.cpp:1410-1414). That means every child context created on the insert path now clones the entire transpiled inline payload, even though the parsed ASTInsertQuery::data still points into the original query context's buffer.
This is a real memory regression once users raise max_query_size to make this feature usable for larger payloads: AsynchronousInsertQueue::preprocessInsertQuery, StorageDistributed::distributedWrite*, and DistributedSink all createCopy(query_context) for inserts, so a large polyglot INSERT ... VALUES can be duplicated multiple extra times just by entering async/distributed fan-out paths. Those copies do not help the pointer-lifetime problem this field was introduced for.
Please keep the transpiled buffer out of ordinary Context copies, for example by storing it behind shared ownership or by explicitly clearing/excluding it in copied child/query contexts.
There was a problem hiding this comment.
Fixed in 476e871: transpiled_query is now held as std::shared_ptr<const String>, so Context::createCopy copies only the pointer and every child context (async insert queue, distributed writes, DistributedSink) shares the one buffer instead of cloning the inline payload. As a bonus, shared ownership also keeps the buffer alive independently of which context outlives which. getTranspiledQuery keeps its previous contract (returns an empty string when unset).
Verified locally after a rebuild: 04512_polyglot_insert_values.sh and 04039_polyglot_sql_transpiler.sh pass, and a polyglot inline INSERT through the async-insert path (async_insert=1, both with and without wait_for_async_insert) inserts correctly.
There was a problem hiding this comment.
Thanks — I think the underlying lifetime fix is still incomplete in the current code.
transpiled_query moved to std::shared_ptr<const String> in src/Interpreters/Context.h, but ContextData::ContextData(const ContextData &) still never initializes that member (src/Interpreters/Context.cpp:1449). So Context::createCopy produces child query contexts with an empty transpiled_query, while the parsed ASTInsertQuery::data in async/distributed insert paths still points into the original query context's transpiled buffer.
That means the copied context does not actually keep the inline payload alive, and helpers such as hasTranspiledInlineData start returning false there. If the parent query context goes away before the child finishes, the copied path can end up with dangling data pointers again. This still needs transpiled_query(o.transpiled_query) in the copy ctor.
`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 |
|
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. |
|
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 |
…lect
`send_query_verbatim` was enabled for every dialect except `clickhouse` and
`clickhouse_json`, which also caught the `trino` dialect. `ParserTrinoQuery`
hands an INSERT with inline data to the standard parser exactly so that
`ASTInsertQuery::data` keeps pointing into the original query buffer, so the
client must split the query from its data as usual. Sending such a statement
verbatim made the client send the whole text (data included) and then still try
to stream a data block, and the server - which had already executed the INSERT
from the inline data - answered with `Progress`:
Code: 102. DB::NetException: Unexpected packet from server
(expected Data, Exception, Log or TimezoneUpdate, got Progress)
This broke `05019_trino_dialect`, `05023_trino_conformance_tempto` and
`05153_trino_dialect_wrappers_and_set` in `Fast test`. Only the `polyglot`
dialect replaces the query text the client parsed, so restrict the verbatim
path to it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…`clickhouse-local` The `input()` initializer of `LocalConnection` reparses the query it received to learn the `input` structure hint and the data format. It has a branch for every dialect but `polyglot`, so a foreign-dialect `INSERT ... SELECT * FROM input(...)` - the one INSERT shape whose data still travels outside the query text, and which the client therefore sends verbatim - was reparsed with the plain ClickHouse parser and rejected as soon as the original text used non-ClickHouse syntax, even though the server transpiles and accepts it. Capture `polyglot_dialect` and `allow_experimental_polyglot_dialect` together with the other parser-affecting settings and use `ParserPolyglotQuery` here, mirroring `ClientBase::parseQuery`. New test `05227_polyglot_local_input_function`: a PostgreSQL cast over `input()`, a BigQuery statement whose original text the ClickHouse lexer cannot even tokenize (`#comment`), and the same text in the plain dialect, which must still be a `SYNTAX_ERROR`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
🕵 Thanks @groeneai — acknowledging the three follow-ups on the unrelated reds, and reporting what this PR's own push does.
On this PR's own red — |
|
Understood on all three. One thing to add on the gitlink: I am not opening a competing bump, because #120202 already pins Measured cost while master stays on |
|
🕵 @groeneai thanks. Noted that #120202 carries the NATS gitlink fix ( |
Conflict resolution: - `IParser::consumesForeignText` from this PR is dropped in favor of master's `IParser::consumesRawText`, which covers the same need (skip the lexical shortcut and the unmatched-parentheses check for opaque dialect text) while keeping `ErrorMaxQuerySizeExceeded` fatal. `parseQuery.cpp` takes master's version. - `ParserPolyglotQuery`: keeps this PR's up-front `transpilePolyglotToClickHouse` and inline-data handling, plus master's `SET` / `SET ROLE` / `SET TRANSACTION SNAPSHOT` dispatch. That dispatch is extracted into `parsePolyglotNativeStatement` so that the server-side polyglot branch of `executeQuery` (which transpiles before parsing, and so no longer goes through `ParserPolyglotQuery`) classifies these statements exactly like the client, instead of using a bare `ParserSetQuery`. - `ClientBase`: keeps this PR's `ParserPolyglotQuery` arguments and master's new `logsql` dialect branch. - `Settings.cpp`: keeps this PR's documentation of `allow_experimental_polyglot_dialect` and master's settings-history entry. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🕵 Merged master (
|
| /// body to the URL query. | ||
| auto url_query_starts_with_insert = [&query]() | ||
| { | ||
| Lexer lexer(query.data(), query.data() + query.size()); |
There was a problem hiding this comment.
Related to the earlier lexically-foreign query= thread: the non-deferred concatenation path is fine, but the deferred 100 Continue path is still wrong here.
url_query_starts_with_insert() still runs the raw query= text through the ClickHouse lexer, so a valid polyglot prefix like #comment\nINSERT INTO t VALUES returns false and we fall back to the "concatenate body to query" path. That fallback only works when executeQuery is allowed to read the body before parsing; with Expect: 100-continue + X-ClickHouse-100-Continue: defer, executeQuery intentionally parses only the initial buffer, so the server sees just the URL fragment, errors on the incomplete INSERT, and never reaches the usual NOT_IMPLEMENTED body-rejection path.
So the same HTTP request changes meaning when the client only adds deferred 100 Continue. I think this needs either a raw-text INSERT sniff here too, or a conservative fallback that forces the same early body rejection whenever a polyglot query= may still need the body to complete the statement.
| /// below still reflects what the user submitted, not the transpiled SQL. | ||
| bool is_native_statement = false; | ||
| { | ||
| Tokens tokens(begin, end, max_query_size); |
There was a problem hiding this comment.
The new server-side native-SET fast path still tokenizes the original foreign buffer before deciding whether to bypass the transpiler.
That breaks the recovery guarantee in the comment for lexically-foreign prefixes: e.g. #comment\nSET dialect = 'clickhouse' under dialect = 'polyglot' is not recognized as a native SET, so we fall into the transpiler branch and can throw SUPPORT_IS_DISABLED / The \polyglot_dialect` setting must not be emptyinstead of letting the user switch the dialect back. In other words, a session/profile can still get stranded indialect = 'polyglot'` if the client prepends a foreign-only comment or other raw-text prefix.
I think the probe here has to sniff raw text (skip whitespace/comments without going through Tokens) rather than running another ClickHouse-lexer pass up front.
This branch has not been deployed
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]