Skip to content

fix: Swap parser override order for macro IF under T-SQL (Issue #5823)#5828

Open
jagannalla wants to merge 1 commit into
SQLMesh:mainfrom
jagannalla:fix/tsql-macro-if-5823
Open

fix: Swap parser override order for macro IF under T-SQL (Issue #5823)#5828
jagannalla wants to merge 1 commit into
SQLMesh:mainfrom
jagannalla:fix/tsql-macro-if-5823

Conversation

@jagannalla

@jagannalla jagannalla commented Jun 4, 2026

Copy link
Copy Markdown

Summary

This PR fixes a bug where conditional macro statements (e.g. @IF(cond, statement)) using the T-SQL / MSSQL dialect would fail validation with:
Required keyword: 'true' missing for <class 'sqlglot.expressions.functions.If'>

Root Cause

In extend_sqlglot() within sqlmesh/core/dialect.py, the overrides were registered in the wrong order:

_override(TSQL.Parser, Parser._parse_if)
_override(Parser, _parse_if)

Solution

  1. Swapped the registration lines in sqlmesh/core/dialect.py so the custom parser wrapper is registered on the base Parser first.
  2. Added unit tests under tests/core/test_dialect.py for T-SQL-specific statements parsed within @IF.

Link to Issue

Closes #5823

@StuffbyYuki StuffbyYuki added the Improvement Improves existing functionality label Jun 7, 2026

@noezhiya-dot noezhiya-dot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix is a simple line swap but the issue it addresses is important. Two comments:

  1. The core change (swapping lines 1125-1126) is correct — registering the base Parser override before the TSQL-specific one ensures the custom _parse_if wrapper is applied in the right order.

  2. The test for PRINT strips the quotes from 'hello' — the expected output shows PRINT hello without quotes. Is this intentional? It seems like the T-SQL parser might be stripping string literal quotes from PRINT statements. If this is expected behavior, a comment in the test would help future readers understand why.

  3. Consider adding a test that verifies the original error case from issue #5823 (the Required keyword: 'true' missing error) no longer occurs, to make the regression test more explicit.

@noezhiya-dot

Copy link
Copy Markdown

Clean fix — the root cause explanation makes sense, and swapping the override order is the minimal correct change. Tests cover the specific T-SQL case well. LGTM.

@jagannalla jagannalla force-pushed the fix/tsql-macro-if-5823 branch from 90abae3 to 2d07241 Compare June 10, 2026 21:37
@jagannalla

Copy link
Copy Markdown
Author

Thanks for the review, @noezhiya-dot!

I have updated the PR to address all comments and successfully force-pushed:

  • Test Case: Added an explicit regression test to verify that the original error case from issue Required keyword: 'true' missing for <class 'sqlglot.expressions.functions.If'> #5823 (the Required keyword: 'true' missing error) no longer occurs. It now correctly parses @IF into an Anonymous expression rather than exp.If.
  • Comments: Added an explanatory comment in the test suite regarding SQLGlot's fallback Command parser stripping string literal quotes from unparsed commands like PRINT.

Ready for review/merge!

@jagannalla jagannalla force-pushed the fix/tsql-macro-if-5823 branch from 2d07241 to 304dfc5 Compare June 10, 2026 21:52
…sh#5823)

Signed-off-by: Jagan Nalla <jagannalla1@gmail.com>
@jagannalla jagannalla force-pushed the fix/tsql-macro-if-5823 branch from 304dfc5 to 97e9db2 Compare June 11, 2026 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Improvement Improves existing functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Required keyword: 'true' missing for <class 'sqlglot.expressions.functions.If'>

3 participants