Add the experimental Trino SQL dialect - #115383
Conversation
SET dialect = 'trino' (gated by allow_experimental_trino_dialect) translates Trino SQL into ClickHouse SQL in three stages: token-level syntax rewriting (ARRAY[...] literals, TRY_CAST -> accurateCastOrNull, UNNEST -> ARRAY JOIN, ROW -> Tuple/tuple, VALUES -> SQLStandardValues, OFFSET before LIMIT, FETCH, lambda parenthesization, backslash-literal strings, .5-style numeric literals), then the standard ClickHouse parser, then an AST pass that maps Trino function names and argument conventions to ClickHouse equivalents - including names that resolve in ClickHouse with different semantics (to_unixtime, date_diff, week, rand, transform, repeat, histogram, map, format, and the code-point based varchar functions). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Running the 294 documented examples of the Trino function docs against the
dialect surfaced these gaps, now fixed:
- `x AT LOCAL` failed: the parser desugars it into a zero-argument `timeZone()`
call, which the `timezone` rewriter rejected (exception);
- `TIMESTAMP` literals silently dropped fractional seconds and did not accept
region time zone names (`TIMESTAMP '2024-01-01 12:00:00 Asia/Tokyo'`);
- `DECIMAL '123.45'` typed literals did not parse;
- `current_timestamp(p)` and `localtimestamp(p)` with a precision argument;
- the standard `TRIM('x' FROM s)` and `TRIM(LEADING FROM s)` forms;
- `.06`-style numeric literals without the leading zero (used by TPC-H);
- `bitwise_not` on small literals used a narrow unsigned type instead of the
64-bit two's complement of Trino;
- `translate` was byte-based (now `translateUTF8`).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Workflow [PR], commit [3bb2b92] Summary: ✅
AI ReviewSummaryThis PR adds an experimental Trino dialect by translating Trino syntax into ClickHouse SQL and remapping Trino function semantics on the AST, with broad conformance coverage and follow-up fixes from prior review threads. I found two remaining correctness gaps in the current head: Findings❌ Blockers
Tests
Final VerdictChanges requested. LLVM Coverage ReportMeasured on commit 3bb2b92.
Changed lines: Changed C/C++ lines covered: 1852/2171 (85.31%) · Uncovered code |
- `x BETWEEN SYMMETRIC a AND b` is translated to `x BETWEEN least(a, b) AND greatest(a, b)` (and `BETWEEN ASYMMETRIC`, the explicit default, is accepted); - Trino JSON values are mapped to the ClickHouse `JSON` type, accepting that only objects are supported: `JSON '...'` literals and `json_parse` become casts to `JSON`, `json_format` becomes `toJSONString`, and `json_extract`, `json_extract_scalar`, `json_value`, `json_query`, `json_exists`, `json_size`, `json_array_contains` and `is_json_scalar` are translated to the ClickHouse JSON functions. The string-based JSONPath functions do not accept the `JSON` type, so a cast flowing directly into them is unwrapped back to the JSON text; simple constant paths of `json_extract`/`json_size` become `JSONExtractRaw`/`JSONLength` arguments (returning bare elements like Trino), general paths fall back to `JSON_QUERY` with its SQL-standard array wrapper. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ce test Running 732 self-contained query/expected pairs extracted from Trino's own functional tests (core/trino-main/src/test/java/io/trino/sql/query/Test*.java) plus the convention-based Tempto product tests (tag 455) surfaced these gaps, now fixed: - typed literals: BIGINT '1', VARCHAR 'x', REAL '1.5', UUID '...', ... -> CAST; - set operations default to DISTINCT in Trino (an explicit mode is emitted, so compound set operations no longer depend on `union_default_mode`), and a trailing ORDER BY/LIMIT/OFFSET/FETCH after a set operation applies to the whole operation (wrapped into a subquery) instead of the last SELECT; - VALUES as a set-operation arm, and explicit ROW constructors in VALUES rows; - CAST targets are wrapped in Nullable (all Trino types are nullable), so CAST(NULL AS INTEGER) works; - aggregates with an inline ORDER BY: array_agg(x ORDER BY k [DESC]) and listagg(x, sep) WITHIN GROUP (ORDER BY k) are rewritten through arraySort over (value, key) tuples (preserving FILTER/OVER clauses); for order-insensitive aggregates the clause is dropped; - GROUP BY AUTO -> GROUP BY ALL, the TABLE t query shorthand, standalone UNNEST without column aliases (synthesized names, so SELECT * works); - row field expansion (expr).* -> untuple (previously the expression was silently lost by the parser), other .* forms fail loudly; - window functions were silently dropped when an aggregate rewriter replaced the node: max(x, n) OVER now transfers the window to the inner aggregate, attachParameters keeps the window definition (min(x, n) OVER, listagg OVER), and the remaining composite rewriters reject window usage explicitly. The new test 05021_trino_conformance pins 75 order-deterministic queries derived from the Trino test suite (Apache License 2.0), following the KQL conformance-test precedent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Running 1689 scalar-function assertions scraped from prestodb/presto unit tests, the 130 Tempto product tests that Presto kept after the fork (executed against real TPC-H sf0.01 data plus the datatype/workers fixture tables), and 146 expression tests from apache/impala surfaced these gaps, now fixed: - outer joins padded non-matched columns with type defaults instead of NULL: translated queries now carry `SETTINGS join_use_nulls = 1` (and `use_variant_as_common_type = 0` so set operations use numeric supertypes); - `sum`/`avg`/`min`/`max` over an empty set (or an empty window frame) returned type defaults instead of NULL: mapped to the `-OrNull` variants; - `count`/`count_if`/`count(DISTINCT)` return bigint in Trino: wrapped in `toInt64`, so a UNION of `count()` with signed integers works; - `greatest`/`least` must return NULL when any argument is NULL; - LIKE patterns: Trino has no default escape character, so a backslash in a pattern is a literal character (doubled once more at the LIKE level, unless an explicit ESCAPE clause is present); - minute-precision `TIMESTAMP '2012-08-08 01:00'` literals; - `reduce` with a non-identity output lambda (inlined by parameter substitution), `bit_count` with arbitrary widths (2..64), `width_bucket(x, bins_array)`, and `json_array_get` (negative indexes). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Converts the harvested external test corpora into stateless tests, following the precedent of 05021_trino_conformance (and of the KQL conformance tests): - 05022_trino_conformance_presto_scalar: 1054 scalar-function assertions from the Presto unit tests (the shared Presto/Trino function surface); - 05023_trino_conformance_tempto: 119 queries from the convention-based (Tempto) product tests of Presto and Trino, executed against their fixture tables inlined into the test (the TPC-H `nation`/`region` tables are scale-independent, `datatype`/`workers` ship with the Presto tests); - 05024_trino_conformance_impala: 44 generic-SQL expressions from the Apache Impala functional tests that are also valid Trino SQL. All sources are Apache License 2.0, attributed in the test headers. Only queries with verified matching results were adopted; unordered multi-row queries are wrapped in ORDER BY ALL, and every query was checked to produce identical output under different session time zones, thread counts and block sizes (the references survive the CI randomization). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t` test The fast test builds with `-DENABLE_LIBRARIES=0`, so `base64Encode`/`base64Decode` (simdutf), `normalizeUTF8*` (ICU), and `stem` (libstemmer) are not registered there, which made `05019_trino_dialect` and `05022_trino_conformance_presto_scalar` fail with `UNKNOWN_FUNCTION`. The queries using them are moved into the new `05028_trino_dialect_icu_base64` test tagged `no-fasttest`, so the rest of the conformance coverage keeps running in the fast test.
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
Build profile diff (arm_release)Comparing Binary sizes
Only the stripped binary is compared: the official master build keeps debug symbols while PR builds strip them, so the other binaries differ by construction. Object file sizes
|
| Object file | Master | PR | Δ |
|---|---|---|---|
src/Parsers/CMakeFiles/clickhouse_parsers.dir/Trino/TrinoFunctionMapper.cpp.o |
new | 476.12 KiB | +476.12 KiB |
src/Parsers/CMakeFiles/clickhouse_parsers.dir/Trino/TrinoSyntaxTranslator.cpp.o |
new | 205.07 KiB | +205.07 KiB |
src/Parsers/CMakeFiles/clickhouse_parsers.dir/Trino/ParserTrinoQuery.cpp.o |
new | 71.79 KiB | +71.79 KiB |
716 more object files are built by the master warmup baseline only (it builds every object-file target, a pull request build only clickhouse-bundle) and not compared.
Compile time of recompiled translation units
1889 translation units recompiled, 11056 s compile time in total, 1886 of them have a recent master baseline.
|
Preview deployment for your docs. Learn more about Mintlify Previews.
|
…oined `UNNEST` and the NULL-preserving aggregates - The settings that align the semantics with Trino (`join_use_nulls`, `use_variant_as_common_type`) were injected into the query text and were therefore skipped for a query that already had a `SETTINGS` clause and for wrappers such as `INSERT ... SELECT` or `EXPLAIN SELECT`. They are now applied to the query context, together with `enable_analyzer` - the column alias lists (`AS t (x, y)`) and the type resolution that the translation relies on do not work without the analyzer. - `LEFT JOIN UNNEST(...) ON TRUE` filled empty inputs with the default value of the element type instead of `NULL`; the elements are now made `Nullable`. - A joined `UNNEST` of several arrays required them to be aligned; it now zips them with `arrayZipUnaligned`, as the standalone form already did, so uneven inputs are padded with `NULL`. - The table alias of a joined `UNNEST` (`AS t (x)`) was parsed but dropped, so `t.x` did not resolve after the translation; the qualified references are now rewritten to the emitted `ARRAY JOIN` aliases. - `array_agg` and `array_agg(DISTINCT ...)` resolved to the ClickHouse `groupArray`, which drops `NULL` elements; they are rewritten through a tuple so that the elements are kept. - The `OrNull` rewrite of `sum`/`avg`/`min`/`max` now also applies through the `Distinct` combinator, so `sum(DISTINCT x)` over an empty set returns `NULL`. The new test `05045_trino_dialect_semantics` covers all of the above. CI fixes in the same change: - `05022_trino_conformance_presto_scalar` exceeded the 180 second test limit in the debug and sanitizer builds; it is split into three tests. - The 29-level nested union adopted from the Impala `union.test` exhausted the stack (`TOO_DEEP_RECURSION`) in the debug and TSan builds; the nesting is reduced. - The failures under the old analyzer are fixed by requiring the analyzer.
…ChangesHistory The style check requires new settings to be recorded under the current version block: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=115383&sha=5dc1cec96fa425ef51e6eb18a2a67fd209bf9efe&name_0=PR&name_1=Style%20check Related: #115383 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A top-level bareword `VALUES` or `FORMAT` after `INSERT` delegated the whole statement to the standard parser, so valid `INSERT ... SELECT` queries with the `format` function or a column named `values` in the select list skipped the Trino translation. Now the inline-data tail is recognized only before any top-level `SELECT`/`WITH`, and `FORMAT` must be followed by a bare format name.
…e-based functions
`length`, `substr`, `substring`, `lpad` and `rpad` were unconditionally mapped
to the UTF8 (code point) variants, which broke the VARBINARY overloads:
`length(to_utf8('𐐭'))` must return 4 bytes, not 1 code point. When the first
argument is a syntactically recognizable VARBINARY expression (a call to a
binary-producing function such as `to_utf8`, a `CAST` to `VARBINARY`, or a
byte-preserving function over such an expression), the byte-based ClickHouse
function (`OCTET_LENGTH`, `byteSlice`, `leftPad`, `rightPad`) is emitted instead.
Also bind the column aliases of a joined UNNEST to the query scope that
introduced them: the second-pass `t.x` -> `x` rewrite was global over the whole
statement, so it also rewrote qualified references inside nested subqueries and
CTEs that reuse the same table alias, turning them into ambiguous unqualified
ones. The scope is the innermost enclosing subquery parenthesis.
…ack in the Trino dialect
`JSON_QUERY` wraps scalar matches in an array (the SQL standard ARRAY WRAPPER),
so the implicit fallback silently changed valid Trino `json_extract` results:
`json_extract('{"hello": 2}', '$["hello"]')` returned `[2]` instead of `2`.
The simple-path parser now also understands bracket-quoted member keys
(`$["key"]`, $['key']`), and everything it cannot parse is rejected with a
clear `NOT_IMPLEMENTED` error suggesting `json_query` explicitly.
Covers the inline-data tail of `INSERT ... SELECT`, the VARBINARY overloads of `length`/`substr`/`lpad`/`rpad`, `json_extract` with bracket-quoted paths and the rejection of unsupported ones, and the scoping of qualified references to joined `UNNEST` aliases.
| SELECT '-- ... and a set operation keeps the numeric supertype, not Variant'; | ||
| SELECT toTypeName(x) FROM (SELECT CAST(1 AS INTEGER) AS x UNION ALL SELECT 2.5E0 AS x) AS t LIMIT 1 SETTINGS max_threads = 1; | ||
|
|
||
| SELECT '-- ... and hold for a wrapping INSERT ... SELECT'; |
There was a problem hiding this comment.
The resolved wrapper-settings thread explicitly called out EXPLAIN SELECT, but this regression file never exercises any EXPLAIN wrapper at all. Right now it proves the context-based Trino settings survive SETTINGS ... and INSERT ... SELECT, but if they were still skipped only for EXPLAIN SELECT, the suite would stay green.
Please add one focused EXPLAIN case that would fail without the Trino context shim, so the test matches the behavior claimed in the thread.
| INSERT INTO t_trino_wrap SELECT r.y FROM (VALUES 1) AS l(x) LEFT JOIN (VALUES (2, 10)) AS r(x, y) ON l.x = r.x; | ||
| SELECT * FROM t_trino_wrap; | ||
|
|
||
| SELECT '-- LEFT JOIN UNNEST over an empty array yields NULL, not the default'; |
There was a problem hiding this comment.
The LEFT JOIN UNNEST review reply also claimed that element types whose Trino NULL padding is not representable here (ARRAY, ROW, MAP) are now rejected instead of silently default-padding. This file only covers the happy path on ARRAY(INTEGER), so that contract is still unproven.
Please add one negative case, e.g. an empty ARRAY(ARRAY(INTEGER)) or ARRAY(ROW(...)) under LEFT JOIN UNNEST, and assert the exception.
|
|
||
| -- json_extract with a bracket-quoted path returns the bare JSON value | ||
| -- (previously the JSON_QUERY fallback wrapped it into an array). | ||
| SELECT json_extract('{"hello": 2}', '$["hello"]'); |
There was a problem hiding this comment.
The latest json_extract fix claim mentions both bracket-quoted member spellings, $["key"] and $['key'], but this regression file only exercises the double-quoted form. If the single-quoted variant were still mishandled, all current tests would still pass.
Please add one focused $['key'] assertion here.
| else if (dialect == Dialect::promql) | ||
| parser = std::make_unique<ParserPrometheusQuery>(settings[Setting::promql_database], settings[Setting::promql_table], Field{settings[Setting::promql_evaluation_time]}); | ||
| else if (dialect == Dialect::trino) | ||
| parser = std::make_unique<ParserTrinoQuery>(settings[Setting::max_query_size], settings[Setting::max_parser_depth], settings[Setting::max_parser_backtracks], end, settings[Setting::allow_experimental_trino_dialect]); |
There was a problem hiding this comment.
clickhouse-local's input() initializer reparses state->query after the query's own SETTINGS clause has already mutated context->getSettingsRef(), but this PR only snapshots the JSON case. That means a valid Trino input() query can be accepted by the top-level parse and then fail on the second parse in local mode.
Concrete trace: with session dialect = 'trino', INSERT INTO FUNCTION null('x Int32') SELECT TRY_CAST(s AS INTEGER) FROM input('s String') SETTINGS dialect = 'clickhouse' FORMAT TSV is parsed successfully by executeQuery, but when the input() initializer runs it reaches this branch, sees the mutated clickhouse dialect, and reparses the original Trino text with ParserQuery, which rejects TRY_CAST. SETTINGS allow_experimental_trino_dialect = 0 has the same failure mode via the gate.
The invariant documented a few lines above is that input() must reparse with the dialect/gate the query was originally accepted with. Trino needs the same captured parser state that clickhouse_json now has, otherwise query-local settings change whether an input() insert is executable only in clickhouse-local.
| { | ||
| static const std::unordered_map<String, String> renames = | ||
| { | ||
| /// String functions. Trino character-position semantics are code-point-based, |
There was a problem hiding this comment.
This block still leaves several Trino text functions on byte/ASCII ClickHouse implementations, so valid Unicode input changes results silently.
The comment says the UTF-8 variants are used for Trino's code-point semantics, but lower / upper still map to the ASCII-only lower / upper, hamming_distance maps to byteHammingDistance, and reverse is not rewritten at all. That means queries like upper('München') stay MüNCHEN instead of MÜNCHEN, hamming_distance('Ж', 'あ') becomes a byte-distance on different-width encodings, and reverse('𐐭x') falls through to byte reversal instead of x𐐭.
Please route the text forms to lowerUTF8 / upperUTF8 / reverseUTF8 and either implement a code-point-aware hamming_distance rewrite or reject it explicitly until that exists. As written, the dialect returns wrong answers without an exception.
alexey-milovidov
left a comment
alexey-milovidov
left a comment
There was a problem hiding this comment.
This is good to go.
1 participant
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Added an experimental
trinovalue of thedialectsetting (gated byallow_experimental_trino_dialect): queries written in Trino SQL are translated to ClickHouse SQL, including Trino-specific syntax (ARRAY[...]literals,TRY_CAST,UNNEST,ROWconstructors and types,VALUEStables,OFFSETbeforeLIMIT,FETCH) and a mapping of several hundred Trino function names and argument conventions to their ClickHouse equivalents.Since Trino SQL is close to ClickHouse SQL (which already parses
AT TIME ZONE,FILTER (WHERE ...),IS DISTINCT FROM,DATE/TIMESTAMPliterals,GROUPING SETS,NULLS FIRST/LAST,WITH RECURSIVE, ...), the dialect is not a separate grammar. It works in three stages insrc/Parsers/Trino/:TrinoSyntaxTranslator):ARRAY[1, 2]->[1, 2],TRY_CAST(x AS t)->accurateCastOrNull(x, 't'),ROW(...)->tuple(...)/Tuple(...),UNNESTinFROM->ARRAY JOIN(LEFT JOIN UNNEST ... ON TRUE->LEFT ARRAY JOIN,WITH ORDINALITY->arrayEnumerate, maps ->mapKeys/mapValues),(VALUES ...)-> a subquery overSQLStandardValues,OFFSET n LIMIT m->LIMIT m OFFSET n,FETCH FIRST n ROWS ONLY/WITH TIES->LIMIT,TIMESTAMP(p) [WITH TIME ZONE]type ->DateTime64(p), lambdas are parenthesized (the ClickHouse parser cannot parse a lambda after a literal argument), and backslashes in string literals are escaped (in Trino a backslash is a regular character).INSERTstatements with inline data (VALUES/FORMAT) are always delegated as-is so that the zero-copy data pointers stay valid.TrinoFunctionMapper): ~100 renames plus ~50 structural rewriters built from a review of all ~340 functions in the Trino documentation. This includes argument reordering for every higher-order function (Trino passes lambdas last, ClickHouse first:transform(arr, f)->arrayMap(f, arr)), parametric-aggregate conversion (approx_percentile(x, p)->quantileTDigest(p)(x),listagg(x, sep)->groupConcat(sep)(x)), and — most importantly — the names that resolve in ClickHouse with different semantics and would silently return wrong results if passed through:to_unixtime(an alias ofparseDateTimein ClickHouse),date_diff(boundary crossings vs complete units ->age),week(non-ISO mode 0 ->toISOWeek),from_unixtime(u, x)(format string vs time zone),rand/random->randCanonical,transform,repeat,histogram,map(karr, varr)->mapFromArrays,format->printf, and the varchar family (length,substr,upper,lower,strpos,lpad, ...) which is code-point-based in Trino and byte-based in ClickHouse (->...UTF8variants).The feature gate follows the
polyglotpattern:SETqueries are handled before the gate inside the parser, so a misconfigured profile can always be recovered withSET dialect = 'clickhouse'. ClickHouse functions whose names do not collide with Trino names remain accessible from the dialect. Unsupported constructs (TABLESAMPLE,TRY(...), comparatorarray_sort, ...) throw explicitNOT_IMPLEMENTED/BAD_ARGUMENTSerrors instead of misbehaving, and semantics that cannot be fixed by translation at the query level (integer division/,roundbanker's rounding on Float,greatest/leastNULL handling) are documented.The translation is observable with
EXPLAIN SYNTAX.Note:
polyglot_dialect = 'trino'already exists via thepolyglottranspiler, but it performs no function mapping and passesARRAY[...]through untranslated; this first-class dialect provides a much more complete translation.BETWEEN SYMMETRICis translated toBETWEEN least(a, b) AND greatest(a, b), and the TrinoJSONtype family is mapped to the ClickHouseJSONtype, accepting that only objects are supported:JSON '...'literals andjson_parsebecome casts toJSON,json_formatbecomestoJSONString, andjson_extract,json_extract_scalar,json_value,json_query,json_exists,json_size,json_array_containsandis_json_scalarare translated to the ClickHouse JSON functions (the string-based JSONPath functions do not accept theJSONtype, so casts flowing directly into them are unwrapped back to the JSON text; non-object documents are rejected when materialized asJSONvalues).Validation: the translation was exercised against four corpora derived from Trino's own tests and documentation:
core/trino-main/src/test/java/io/trino/sql/query/Test*.java): 206 produce identical results; of the rest, ~270 need ClickHouse engine features (LATERALjoins alone gate 139, plus correlated-subquery decorrelation shapes and non-integer window frame offsets) and the remainder are unsupported constructs or documented semantic differences. 75 order-deterministic pairs are committed as05021_trino_conformance(following the KQL conformance-test precedent), and three more conformance tests pin the adopted corpora:05022_trino_conformance_presto_scalar(1054 Presto scalar-function assertions),05023_trino_conformance_tempto(119 Tempto product-test queries running against their fixture tables inlined into the test), and05024_trino_conformance_impala(44 generic-SQL Impala expressions) — every adopted query verified to produce the documented result and to be stable under randomized time zones, thread counts and block sizes.testing/trino-product-testsat tag 455,.sql/.resultpairs): 30 of the 34 runnable ones match (using the fixed 25-rownation/5-rowregionTPC-H tables).SELECT-with-expected-result examples from the official function documentation: 142 match.testing/trino-benchmark-queries: all parse and translate.prestodb/prestounit tests (assertFunction("expr", TYPE, value)style) plus the 130 Tempto product tests Presto kept after the fork, executed against real TPC-H sf0.01 data and thedatatype/workersfixture tables: 1059 and 91 match respectively (most of the rest use Presto-only functions that do not exist in current Trino). The 43 Trino-era Tempto tests reach 38/43 with the data available.apache/impalaas a generic-SQL cross-check (Impala is a different dialect; 44 match, and the divergences are Impala-specific by design — e.g. Impala'sLIKEtreats a backslash as an escape character while Trino treats it literally).These corpora drove another round of semantic fixes: outer joins produce NULLs (
join_use_nulls),sum/avg/min/maxover an empty set return NULL (-OrNullvariants),countreturns a signed bigint,greatest/leastpropagate NULL arguments, LIKE backslash literalness, minute-precisionTIMESTAMPliterals,reduceoutput lambdas,bit_countarbitrary widths,width_bucketover an array of bounds, andjson_array_get. Of the 294 examples, 142 produce results matching the documentation (plus a few that differ only by printing the current time or the session time zone); most of the rest use deliberately unsupported constructs (SQL:2016MATCH/UNIQUEpredicates,U&'...'literals, functions with no ClickHouse counterpart, non-objectJSONdocuments per the design above).Workflow [PR]
Sync PR [sync-upstream/pr/115383]