Repository navigation
Conversation
Session parameters such as TIMEZONE or QUERY_TAG are keywords, not identifiers, so Snowflake rejects them when quoted. sqlglot parses them as identifiers, which means model pre/post statements like ALTER SESSION SET TIMEZONE = 'UTC' were rendered and executed as ALTER SESSION SET "TIMEZONE" = 'UTC', a syntax error. Parse unquoted parameter names in ALTER SESSION SET/UNSET as vars so they are neither normalized nor quoted. Fixes SQLMesh#5666 Signed-off-by: Dhruv Gupta <dg@usesparx.com>
…ession-quoting # Conflicts: # tests/core/engine_adapter/test_snowflake.py
Vamsi-klu
left a comment
Vamsi-klu
left a comment
There was a problem hiding this comment.
Thanks, doing this at parse time is a lot cleaner than the adapter + renderer route from #5670. I double checked the hash claim: a Snowflake model with ALTER SESSION SET TIMEZONE = 'UTC', query_tag = 'x' as a pre statement has the same data and metadata hash on main and on this branch, only the rendered SQL changes. One scoping nit inline, looks good otherwise.
| _override(Parser, _parse_interval_span) | ||
| _override(Parser, _warn_unsupported) | ||
| _override(Snowflake.Parser, _parse_table_parts) | ||
| _override(Parser, _parse_alter_session) |
There was a problem hiding this comment.
On sqlglot 30.8 only the Snowflake parser sends ALTER SESSION to _parse_alter_session, Oracle and Databricks still end up as Command. So it's Snowflake only in practice, but overriding the base Parser makes it read as global. Could this be _override(Snowflake.Parser, _parse_alter_session) like the _parse_table_parts one above? Keeps the scope obvious and won't quietly pick up other dialects if sqlglot routes them here later.
There was a problem hiding this comment.
Good call, done in 637af55: the override is now registered on Snowflake.Parser next to _parse_table_parts, and the function is annotated accordingly. I checked that Databricks and Oracle still come out as Command with the original text untouched.
| if isinstance(name, exp.Column) and not name.table: | ||
| name = name.this | ||
| if isinstance(name, exp.Identifier) and not name.quoted: | ||
| name.replace(exp.var(name.name)) |
There was a problem hiding this comment.
FYI, no change needed: since these are Vars now they don't get normalized, so query_tag stays lowercase where main uppercased it. Snowflake doesn't care about case for parameter names so it's harmless, but maybe worth a line in the comment above so nobody "fixes" it later.
There was a problem hiding this comment.
Added a sentence to the comment in the same commit explaining that vars skip case normalization, that query_tag therefore stays lowercase, and that Snowflake treats parameter names case-insensitively so this is intentional.
|
@cmgoffena13 could you take a look at this one? It fixes #5666: Snowflake session parameters in |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Parse unquoted session parameters sqlmesh/core/dialect.py |
The Snowflake parser converts unquoted identifier parameter names in SET and UNSET statements to vars. The parser override is registered on Snowflake.Parser. |
Verify SQL rendering paths tests/core/test_dialect.py, tests/core/engine_adapter/test_snowflake.py |
Tests check rendering with identifier quoting enabled, preserve explicitly quoted names, and verify engine adapter and model pre-/post-statement output. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix · Severity of issue fixed: Medium
Suggested reviewers: vamsi-klu
Merge Risk: 
99b31
-
The Snowflake parameter-rendering change has no identified issue that needs resolution before merging.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surv… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title "fix(snowflake): do not quote parameter names in ALTER SESSION" states the main change. The changeset adds a Snowflake parser override in sqlmesh/core/dialect.py so unquoted ALTER SESSION SE… Description check The description follows the template. It has a Description section that explains the bug (Snowflake session parameters are keywords, and sqlglot parses ALTER SESSION parameter names as identifiers, so… Linked Issues check Issue [ #5666] requires Snowflake session parameter names to remain unquoted in generatedALTER SESSIONSQL.sqlmesh/core/dialect.pyadds a Snowflake parser override that converts unquotedSETan…Out of Scope Changes check The changes are limited to the Snowflake parser override and focused dialect and Snowflake engine adapter tests. The tests directly validate the issue behavior. No unrelated production changes or unre… Full details: Title check
-
Explanation
The title "fix(snowflake): do not quote parameter names in ALTER SESSION" states the main change. The changeset adds a Snowflake parser override in sqlmesh/core/dialect.py so unquoted ALTER SESSION SET/UNSET parameter names parse as exp.Var and are not quoted on output. The title is short, specific, and follows the conventional-commit style. It is clear enough for a teammate scanning history to understand the primary change.
Full details: Description check
-
Explanation
The description follows the template. It has a Description section that explains the bug (Snowflake session parameters are keywords, and sqlglot parses ALTER SESSION parameter names as identifiers, so they get quoted twice), the fix (a parser override following the existing T-SQL ALTER COLUMN pattern), and the effect on hashes and snapshots. It has a Test Plan section listing the new unit and engine adapter tests, the fail-before and pass-after results, and the commands run with their outcomes. The Checklist is present. One item is left unchecked (make fast-test was not run), and the author explains why. The description also links issue
#5666.
Full details: Linked Issues check
-
Explanation
Issue [
#5666] requires Snowflake session parameter names to remain unquoted in generatedALTER SESSIONSQL.sqlmesh/core/dialect.pyadds a Snowflake parser override that converts unquotedSETandUNSETparameter names toexp.Var. The reported tests cover generated SQL, explicit quoted names, macro values, and rendered model pre/post statements. The engine adapter test verifiesALTER SESSION SET TIMEZONE = 'UTC'andALTER SESSION UNSET TIMEZONE.
Full details: Out of Scope Changes check
-
Explanation
The changes are limited to the Snowflake parser override and focused dialect and Snowflake engine adapter tests. The tests directly validate the issue behavior. No unrelated production changes or unrelated test changes are shown.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 4 functions across 3 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
Description
Fixes #5666.
Snowflake session parameters (
TIMEZONE,QUERY_TAG, ...) are keywords, not identifiers, soALTER SESSION SET "TIMEZONE" = 'UTC'is a syntax error. sqlglot parses the parameter names inALTER SESSION SET/UNSETas identifiers (Column(Identifier)/Identifier), so a model pre/post statement such aswas quoted twice: once by the renderer's normalize-and-quote step and again by
EngineAdapter._to_sql. It reached Snowflake asALTER SESSION SET "TIMEZONE" = 'UTC'.This adds a small parser override in
sqlmesh/core/dialect.py, following the same pattern as the existing T-SQLALTER COLUMNoverride. After sqlglot parsesALTER SESSION, unquoted parameter names becomeexp.Var, so they are neither normalized nor quoted. Values are left alone (literals, macros such as@tag, etc.), and so are parameter names the user quoted explicitly.The SQL produced for the unrendered statement doesn't change (
TIMEZONE = 'UTC'both before and after), so model data and metadata hashes stay the same and existing snapshots aren't affected. I checked this on a sample model.An earlier attempt (#5670) turned quoting off for every
Alter/Setstatement in the Snowflake adapter and also special-cased the renderer. Its author closed it. This change only touchesALTER SESSIONparameter names and lives at the parse level, so the renderer and the engine adapter both pick it up without engine-specific code in either.Test Plan
tests/core/test_dialect.py::test_snowflake_alter_session_parameters_are_not_quoted: parse/generate withidentify=Truefor SET (with a macro value), UNSET, and an explicitly quoted name, plus rendered model pre/post statements.tests/core/engine_adapter/test_snowflake.py::test_alter_session_parameters_are_not_quoted: SQL executed by the mocked Snowflake adapter.main(ALTER SESSION SET "TIMEZONE" = 'UTC') and pass with the fix.pytest -n 8 -m "fast and not web and not github and not dbt and not jupyter and not cicdonly and not isolated": 2436 passed. 6 failures intests/core/test_connection_config.py(pyodbc) are unrelated and come from a missing local unixODBC library; they fail the same way onmain.pytest -m "dbt and fast" tests/dbt: 88 passed.SKIP=prettier,eslint pre-commit run --all-files(ruff, ruff-format, mypy, migrations): passed. The prettier/eslint hooks were skipped because no frontend files changed.Checklist
make styleand fixed any issuesmake fast-test) (I ran the core fast and dbt fast subsets listed above, not the whole target)git commit -s) per the DCOSummary by CodeRabbit
ALTER SESSION SETandUNSETstatements now retain unquoted parameter names when generated, avoiding unexpected quoting.