Skip to content

Add Dialect::parse_table_factor_name hook - #2610

Closed
edmondop wants to merge 2 commits into
apache:mainfrom
edmondop:dialect-table-factor-name
Closed

edmondop wants to merge 2 commits into
apache:mainfrom
edmondop:dialect-table-factor-name

Conversation

@edmondop

@edmondop edmondop commented Oct 3, 2026 •

Copy link
Copy Markdown

Closes #2604.

Dialect can override expression, statement and column-option parsing, but not table names: parse_table_factor always calls parse_object_name(true). A downstream dialect with syntax like FROM events:analytics has to rewrite tokens before parsing.

This adds parse_table_factor_name, which works like parse_prefix/parse_statement: return None to fall back to the default parser. Only the name is replaced; alias, joins, sampling and hints still go through the core parser. Note that ObjectName prints its parts joined with ., so custom separators don't round-trip.

derive_dialect's generated imports gain ObjectName so derived dialects compile.

Dialects: none

Tests: custom_table_factor_name_parser in tests/sqlparser_custom_dialect.rs.

Implemented with AI assistance (Claude Code).

🤖 Generated with Claude Code

https://claude.ai/code/session_01Hb9YMy3Ay2oNWagfTz2KzZ

Lets a dialect parse the name of a table in a table factor, falling back
to parse_object_name when it returns None.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Hb9YMy3Ay2oNWagfTz2KzZ
@github-actions

github-actions Bot commented Oct 3, 2026

Copy link
Copy Markdown

No dialect label was applied because the title does not name a SQL dialect.

If this change targets one or more dialects, edit the title to <Dialect>[, <Dialect>]: <description>, add a Dialects: <Dialect>[, <Dialect>] line to the description, or reply with one. Reply Dialects: none if the change is dialect-agnostic.

@edmondop
edmondop marked this pull request as ready for review October 4, 2026 12:35
@LucaCappelletti94

Copy link
Copy Markdown
Contributor

A downstream dialect with syntax like FROM events:analytics has to rewrite tokens before parsing.

Yeah in my opinion the clean solution will be at some point to refactor the tokenizer dialect-generic, so tokenization can be optimally implemented for each dialect.

@codecov-commenter

codecov-commenter commented Oct 4, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.15%. Comparing base (14cbf75) to head (cc64762).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2610   +/-   ##
=======================================
  Coverage   81.14%   81.15%           
=======================================
  Files          42       42           
  Lines       33736    33748   +12     
  Branches    33736    33748   +12     
=======================================
+ Hits        27376    27388   +12     
  Misses       2797     2797           
  Partials     3563     3563           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@edmondop

edmondop commented Oct 4, 2026

Copy link
Copy Markdown
Author

Agreed that a dialect-specific tokenizer is the right long-term fix for lexing. For Presto, is_identifier_part already covers it (#2611), since its grammar allows : in every identifier. This hook targets a different case: a dialect that only wants special syntax in table position, or wants the name split into parts, which the tokenizer can't decide without parser context.

I'm open to working on the tokenizer side too. If useful, I can open an issue sketching it, for example a Dialect hook that can emit a token before the default tokenizer runs, with the same "return None to fall back" contract as parse_prefix. Would you merge this hook in the meantime, or rather wait for that?

@LucaCappelletti94

Copy link
Copy Markdown
Contributor

No dialect needs this hook. Presto, the one engine named with events:analytics, lexes it as a single identifier, which is_identifier_part already handles. dbt models are Jinja templates, and dbt compile renders {{ ref('model') }} into a plain table name, so a SQL parser should be given the compiled SQL.

The hook also cannot represent what it is for. It returns an ObjectName, which has no : separator, so events:analytics and events.analytics parse to the same AST and both print as events.analytics. It reaches only table factors, so INSERT INTO, CREATE TABLE, DROP TABLE and ALTER TABLE still reject the same name.

Today Tokenizer holds a &dyn Dialect and runs a single next_token for every dialect. Each token goes through dozens of supports_* checks, and byte strings, raw strings, // and # comments and \r comment endings are still hardcoded behind dialect_of!. A dialect-generic tokenizer would be Tokenizer<D: Dialect + ?Sized>, with those lexical rules moved onto Dialect as overridable methods. Each built-in dialect would then get a monomorphised lexer without the branches it never takes, a downstream dialect could add lexemes of its own, and Parser could keep passing &dyn Dialect unchanged. Such a refactoring would be a rather significant challenge, but would fully address this genre of issues more cleanly, I might look into it in a month or two if time allows, especially for a potential no-copy aspect.

I will now close this PR.
cc @alamb @iffyio if you have a different opinion.

@edmondop

Copy link
Copy Markdown
Author

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 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 stop the use of dialect_of and referring to specific dialects in parser #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

@LucaCappelletti94

Copy link
Copy Markdown
Contributor

Please do not be a meat proxy as I did spend my own time reading and trying to make sense of the code.

On the table:namespace syntax, deciding which part is the namespace is name resolution, which this crate leaves to its users. A dialect whose is_identifier_part accepts : parses analytics:events as one identifier in FROM, INSERT INTO, CREATE TABLE, DROP TABLE, ALTER TABLE, UPDATE and DELETE, and prints it back unchanged, so the caller can split it where it resolves table names. The cost is that a::INT then lexes as one identifier. The hook would not replace the token rewrite either, because it only reaches table factors, and INSERT INTO analytics:events and CREATE TABLE analytics:events would still fail.

As I wrote in my previous reply, the long-term answer is a tokenizer generic over the dialect. I will work on it if and when I have the time, as I am interested in improving its performance.

@alamb

alamb commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

@LucaCappelletti94 -- let me know if you need help. I would enjoy helping make sqlparser more efficient as well

@LucaCappelletti94

Copy link
Copy Markdown
Contributor

@LucaCappelletti94 -- let me know if you need help. I would enjoy helping make sqlparser more efficient as well

We will certainly have to chat it over for a while, as I expect it would be a rather large refactoring with a non trivial amount of implications downstream. For the immediate future I prefer sticking to bug squashing so when we finally get to refactoring we will be able to tell whether what we see is a regression or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Let a Dialect override how a table name in FROM is parsed

4 participants