edmondop commented on PR #2610: URL: https://github.com/apache/datafusion-sqlparser-rs/pull/2610#issuecomment-6133742911
I'd like to correct two claims in the closing comment: that this syntax is already handled, and that no dialect needs this hook. I know the second claim is not correct, because I have written — in a real prototype — exactly the workaround that [#2604](https://github.com/apache/datafusion-sqlparser-rs/issues/2604) describes as the only option available today. ## The workaround The dialect I am targeting writes a qualified table name in `FROM` as `table:namespace` — table first, namespace second. Standard SQL has no such production, and `parse_table_factor` offers a dialect no say in how a table name is parsed, so the prototype works around the parser in three steps: 1. Tokenize the statement. 2. Scan the token stream for three adjacent tokens — `Word`, `Colon`, `Word` — and rewrite that triple in place: swap the two words, and replace the colon with a period. The parser then sees the conventional qualified name `namespace.table`. 3. Hand the rewritten stream to the parser with `Parser::with_tokens_with_locations`. ## Why this is not a solution Both of its defects come from doing parser work at token level: - **It has no context.** The scan runs over the whole statement, and a token stream does not record whether a `Word : Word` sequence is a table name or something else — so the rewrite cannot be restricted to table position, the one restriction this syntax actually has. - **It does grammar in the wrong place.** Splitting a name into parts and ordering them as a qualified name is parsing. The rewrite performs it blind, with none of the parser's knowledge. Nor is this the Presto situation in #2611: there the colon is a character inside a single identifier, so a lexer rule settles it; here it separates two parts that must be split and reordered, which no lexer rule can do. If this syntax were already supported, none of this code would exist. The hook is what would let a dialect delete it: the parser already knows it is in table position; the dialect would supply only the name. ## Connecting back to #2604 The special cases in the core parser are not an alternative to this extension point: every dialect-specific table syntax the parser supports today lives in the core as an ad-hoc case, because there is nowhere in a dialect to put it: - BigQuery's hyphenated table names are a `dialect_of!(self is BigQueryDialect)` check inside `parse_object_name_inner`, gated by an `in_table_clause` parameter that exists only for that one case. - Snowflake's `@stage` references are a branch inside `parse_table_factor`, gated by a `supports_stages()` flag: the dialect may flip a switch, but the parsing logic itself had to be added to the core, by a core PR, for that one dialect. - Issue #1430 exists for the same reason: an ongoing effort to move hardcoded `dialect_of!` checks out of the parser and into `Dialect` methods. That is the gap #2604 is about. `Dialect` can already take over prefix, infix, statement and column-option parsing; table names are the remaining place where dialects regularly differ, and the only one with no takeover point. #2604 also lists the other motivations — `{{ ref(...) }}` / `{{ source(...) }}` in `FROM` for tools that parse model SQL, non-standard catalog addressing generally — and offers a follow-up: moving BigQuery's hyphenated names onto a hook of this kind would let `parse_object_name` drop `in_table_clause`, and would prove the hook on syntax already in the tree. ## On the process I do not think closing this PR while leaving #2604 open is a coherent place to land. The limitations raised in the closing comment were flagged by me from the start: the round-trip note is in this PR's own description, and the table-factor-only scope is the scope #2604 proposed. I still believe the hook, limits included, is a base to iterate on in review — and a better one than the token-rewriting workaround every downstream dialect has to write today. Closing it in favour of a refactor that so far exists as a sketch, to "look into in a month or two", leaves #2604 in limbo: the issue open and incremental improvement have been rejected in favor of a bigger improvement that is not currently prioritized -- 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]
