Skip to content

Conversation

georgesittas
Copy link
Contributor

@georgesittas georgesittas commented Sep 11, 2025

What the title says. We now treat all macro references in SQL as case-insensitive. The rationale is that we already normalize variables and blueprint_variables, so doing it for all variables feels like a natural extension of that. The motivating problem was the following:

MODEL (
    name foo.bar_@{X},
    blueprints ((X := 'bla')),
);

SELECT
  1 AS c

The @{X} variable is not rendered, because we don't do case-insensitive lookups for @{...}-type variables:

>>> from sqlmesh import Context
>>> ctx = Context()
>>> ctx.models
mappingproxy({'"db"."foo"."bar_@{x}"': SqlModel<foo.bar_@{X}: test.sql>})

@georgesittas georgesittas requested a review from a team September 11, 2025 11:55
@georgesittas georgesittas force-pushed the jo/normalize_curly_variable_refs branch 4 times, most recently from dee692e to 6b9a000 Compare September 11, 2025 12:23
@georgesittas georgesittas force-pushed the jo/normalize_curly_variable_refs branch from d5eeeb2 to a96de8b Compare September 11, 2025 15:42
@georgesittas georgesittas marked this pull request as draft September 11, 2025 19:54
@georgesittas
Copy link
Contributor Author

This is more nuanced than I initially thought. I'm planning to revert part of this PR so that we don't lowercase everything in locals. The reason is that Python objects can also be included in it AFAICT and since symbols in Python are case-sensitive, we want to treat them as such.

@georgesittas georgesittas force-pushed the jo/normalize_curly_variable_refs branch from 11842f8 to 32be226 Compare September 12, 2025 14:33
@georgesittas georgesittas marked this pull request as ready for review September 12, 2025 15:04
@georgesittas georgesittas requested a review from a team September 12, 2025 15:24
Comment on lines -259 to +274
if node.name not in self.locals and node.name.lower() not in variables:
# This makes all variables case-insensitive, e.g. @X is the same as @x. We do this
# for consistency, since `variables` and `blueprint_variables` are normalized.
var_name = node.name.lower()

if var_name not in self.locals and var_name not in variables:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change has a subtle side-effect: today, despite being counter-intuitive, one can actually reference a variable defined in Python within a model:

# model.sql
MODEL (name test, kind full);
SELECT @x_plus_one() AS xp1, @X AS x

# macros.py
from sqlmesh import macro

X = 1

@macro()
def x_plus_one(evaluator):
    return X + 1

The above configuration is correct and the model renders into:

SELECT
  2 AS "xp1",
  1 AS "x"

After this PR, the reference @X will be invalid because we'll look up the lowercase "x" in locals, which won't match the Python uppercase key "X", resulting in:

Macro variable 'X' is undefined.

I think this is fine. I doubt anyone's relying on Python variables being exposed to SQL models today. I don't think we've even documented this anywhere, it's just a weird side-effect of populating locals with python_env items.

Comment on lines -316 to +335
mapping = {
k: convert_sql(v, self.dialect)
base_mapping = {
k.lower(): convert_sql(v, self.dialect)
for k, v in chain(self.variables.items(), self.locals.items(), local_variables.items())
}
return MacroStrTemplate(str(text)).safe_substitute(mapping)
return MacroStrTemplate(str(text)).safe_substitute(CaseInsensitiveMapping(base_mapping))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea here is to make variable lookups in SQL case-insensitive; the template method is only used for substituting variables in SQL, as far as I can tell.

Comment on lines -330 to +346
self.locals[node.name] = self.transform(node.expression)
# Make variables defined through `@DEF` case-insensitive
self.locals[node.name.lower()] = self.transform(node.expression)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, this should just affect SQL models that have @DEF– now the defined variables are automatically stored as lowercase.

@georgesittas georgesittas force-pushed the jo/normalize_curly_variable_refs branch from 1894a7e to bc5607c Compare September 15, 2025 09:59
Copy link
Member

@izeigerman izeigerman left a comment

Choose a reason for hiding this comment

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

Do we need to update docs anywhere?

@georgesittas
Copy link
Contributor Author

Do we need to update docs anywhere?

Maybe, I'll take a look and update if needed. 👍

@georgesittas georgesittas merged commit 745db0f into main Sep 15, 2025
36 checks passed
@georgesittas georgesittas deleted the jo/normalize_curly_variable_refs branch September 15, 2025 17:39
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.

3 participants