rad-pat commented on code in PR #43964: URL: https://github.com/apache/superset/pull/43964#discussion_r4060574866
########## superset/sql/dialects/databend.py: ########## @@ -0,0 +1,164 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +Databend dialect. + +Databend has no built-in sqlglot dialect, so ``SQLGLOT_DIALECTS`` had no entry for +it and every Databend query fell back to the generic dialect. Superset regenerates +each adhoc column and metric through that dialect (``sanitize_clause``, called from +``_process_sql_expression`` in ``superset/models/helpers.py``), so the fallback's +rendering reached the server on every compile. + +The base is Postgres, which matches ``databend-sqlalchemy``: its +``DatabendCompiler`` and ``DatabendIdentifierPreparer`` derive from ``PGCompiler`` +and ``PGIdentifierPreparer``, so the SQL Superset compiles and the SQL this dialect +regenerates come from the same family. + +ClickHouse looks like the closer fit -- Databend borrows much of its surface +syntax, including ``SETTINGS`` and the ``to_start_of_*`` date helpers -- but it is +not. ClickHouse's generator renames a large number of functions to spellings +Databend does not have. Round-tripping every function name its parser knows +produced 156 renames, 44 of which emitted a name absent from Databend's catalogue: +``argMax``, ``countIf``, ``stddevSamp``, ``splitByString``, ``JSONExtractString``, +``toTypeName``, ``lagInFrame``, ``arrayJoin``, the ``array*`` camelCase family, and +``POSITION(x, y)`` where Databend accepts only ``POSITION(x IN y)``. On a +70-expression battery a ClickHouse base broke 33 where Postgres breaks 17. + +What Postgres still gets wrong is overridden below, and a regression test +enumerates the transforms so the list cannot grow unnoticed on a sqlglot bump. +""" + +from __future__ import annotations + +from typing import Any, Callable + +from sqlglot import exp, generator +from sqlglot.dialects.dialect import rename_func +from sqlglot.dialects.postgres import Postgres +from sqlglot.tokens import TokenType + + +def _date_format(args: list[Any]) -> exp.Expression: + """ + Parse ``formatDateTime(...)`` as Databend's ``DATE_FORMAT(...)``. + + Databend accepts ClickHouse's ``formatDateTime`` spelling nowhere -- "no + function matches the given name: 'formatdatetime', do you mean 'date_format'?" + -- but expressions carrying it turn up in datasets migrated from ClickHouse. + Both take the same ``%``-style format string, so it is passed along untouched. + """ + return exp.Anonymous(this="DATE_FORMAT", expressions=args) + + +def _sha2(self: generator.Generator, expression: exp.SHA2) -> str: + """Databend spells this ``SHA2(x, 256)``; Postgres emits ``SHA256(x)``.""" + return self.func("SHA2", expression.this, expression.args.get("length")) + + +class Databend(Postgres): + class Tokenizer(Postgres.Tokenizer): + # Databend accepts backtick-quoted identifiers as well as the + # double-quoted form Postgres uses. + IDENTIFIERS = ['"', "`"] + + class Parser(Postgres.Parser): + FUNCTIONS: dict[str, Callable[..., exp.Expression]] = { + **Postgres.Parser.FUNCTIONS, + "FORMATDATETIME": _date_format, + } + + # Without this, ``FROM t SETTINGS max_threads = 1`` binds SETTINGS as the + # table's alias and the assignment that follows fails to parse. + TABLE_ALIAS_TOKENS = Postgres.Parser.TABLE_ALIAS_TOKENS - {TokenType.SETTINGS} + + # Databend accepts ClickHouse's trailing ``SETTINGS k = v``. + QUERY_MODIFIER_PARSERS = { + **Postgres.Parser.QUERY_MODIFIER_PARSERS, + TokenType.SETTINGS: lambda self: ( + "settings", + self._advance() or self._parse_csv(self._parse_assignment), + ), + } + + def _parse_statement(self) -> exp.Expression | None: + # Databend also accepts a *leading* ``SETTINGS (...)`` clause before + # the statement -- e.g. + # ``SETTINGS (max_execute_time_in_seconds=300) SELECT ...`` -- which no + # built-in dialect parses. It is absorbed here and re-emitted verbatim + # in ``Generator.generate``. + settings = None + if self._curr and self._curr.token_type == TokenType.SETTINGS: + index = self._index + start = self._curr + self._advance() + if self._curr and self._curr.token_type == TokenType.L_PAREN: + self._parse_wrapped_csv(self._parse_assignment) + settings = self._find_sql(start, self._prev) Review Comment: The observation is factually right and the severity isn't: leading `SETTINGS (...)` values genuinely don't appear in `get_settings()`, but neither consumer of that API can be affected by it. `get_settings()` collects `exp.SetItem` nodes — the `SET foo = 'bar'` **statement** form. It has two callers in `superset/sql/parse.py`, both about session rebinding: - `changes_search_path()` (:1308) looks for a `search_path` key, so that denylist matching against `default_schema` can be treated as unreliable afterwards. - the rebinding check (:1359) looks for `schema` / `current_schema` / `current schema` / `catalog`. Databend's leading `SETTINGS (...)` is a query-scoped wrapper on a single statement — `SETTINGS (max_execute_time_in_seconds=300) SELECT ...`. It cannot rebind resolution for *later* statements, which is the premise of both checks, and the clause is restricted here to constant `key = literal` pairs, so nothing else can ride in on it. The risk that restriction exists for is the one worth naming, because it is adjacent to yours: the clause is re-emitted from source text rather than regenerated from the tree, so anything the parser swallowed without exposing as tree nodes would be invisible to table extraction while still reaching the server. That is closed by `_is_constant_assignment` checking **both** sides of each pair — an earlier revision only checked the value, and `SETTINGS ((SELECT x FROM secret) = 1) SELECT a FROM t` was absorbed with the subquery intact while the tree reported one table. It is now rejected, with `test_leading_settings_rejects_non_constant_values` covering both sides and `test_nothing_absorbed_is_missing_from_the_tree` asserting the property directly: every table named in the re-emitted statement is reachable through the tree. Same conclusion @rusackas reached in the approving review; I have re-derived it from the call sites rather than restating it. Resolving on that basis — happy to reopen if anyone reads those two call sites differently. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
