-
Notifications
You must be signed in to change notification settings - Fork 263
Fix: treat all instances of macro variables as case-insensitive #5352
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
dee692e
to
6b9a000
Compare
d5eeeb2
to
a96de8b
Compare
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 |
11842f8
to
32be226
Compare
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: |
There was a problem hiding this comment.
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.
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)) |
There was a problem hiding this comment.
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.
self.locals[node.name] = self.transform(node.expression) | ||
# Make variables defined through `@DEF` case-insensitive | ||
self.locals[node.name.lower()] = self.transform(node.expression) |
There was a problem hiding this comment.
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.
1894a7e
to
bc5607c
Compare
There was a problem hiding this 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?
Maybe, I'll take a look and update if needed. 👍 |
What the title says. We now treat all macro references in SQL as case-insensitive. The rationale is that we already normalize
variables
andblueprint_variables
, so doing it for all variables feels like a natural extension of that. The motivating problem was the following:The
@{X}
variable is not rendered, because we don't do case-insensitive lookups for@{...}
-type variables: