-
Notifications
You must be signed in to change notification settings - Fork 358
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -128,6 +128,17 @@ def _macro_str_replace(text: str) -> str: | |
| return f"self.template({text}, locals())" | ||
|
|
||
|
|
||
| class CaseInsensitiveMapping(t.Dict[str, t.Any]): | ||
| def __init__(self, data: t.Dict[str, t.Any]) -> None: | ||
| super().__init__(data) | ||
|
|
||
| def __getitem__(self, key: str) -> t.Any: | ||
| return super().__getitem__(key.lower()) | ||
|
|
||
| def get(self, key: str, default: t.Any = None, /) -> t.Any: | ||
| return super().get(key.lower(), default) | ||
|
|
||
|
|
||
| class MacroDialect(Python): | ||
| class Generator(Python.Generator): | ||
| TRANSFORMS = { | ||
|
|
@@ -256,14 +267,18 @@ def evaluate_macros( | |
| changed = True | ||
| variables = self.variables | ||
|
|
||
| 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: | ||
| if not isinstance(node.parent, StagedFilePath): | ||
| raise SQLMeshError(f"Macro variable '{node.name}' is undefined.") | ||
|
|
||
| return node | ||
|
|
||
| # Precedence order is locals (e.g. @DEF) > blueprint variables > config variables | ||
| value = self.locals.get(node.name, variables.get(node.name.lower())) | ||
| value = self.locals.get(var_name, variables.get(var_name)) | ||
| if isinstance(value, list): | ||
| return exp.convert( | ||
| tuple( | ||
|
|
@@ -313,11 +328,11 @@ def template(self, text: t.Any, local_variables: t.Dict[str, t.Any]) -> str: | |
| """ | ||
| # We try to convert all variables into sqlglot expressions because they're going to be converted | ||
| # into strings; in sql we don't convert strings because that would result in adding quotes | ||
| 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)) | ||
|
Comment on lines
-316
to
+335
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
|
||
| def evaluate(self, node: MacroFunc) -> exp.Expression | t.List[exp.Expression] | None: | ||
| if isinstance(node, MacroDef): | ||
|
|
@@ -327,7 +342,9 @@ def evaluate(self, node: MacroFunc) -> exp.Expression | t.List[exp.Expression] | | |
| args[0] if len(args) == 1 else exp.Tuple(expressions=list(args)) | ||
| ) | ||
| else: | ||
| self.locals[node.name] = self.transform(node.expression) | ||
| # Make variables defined through `@DEF` case-insensitive | ||
| self.locals[node.name.lower()] = self.transform(node.expression) | ||
|
Comment on lines
-330
to
+346
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, this should just affect SQL models that have |
||
|
|
||
| return node | ||
|
|
||
| if isinstance(node, (MacroSQL, MacroStrReplace)): | ||
|
|
@@ -630,7 +647,7 @@ def substitute( | |
| ) -> exp.Expression | t.List[exp.Expression] | None: | ||
| if isinstance(node, (exp.Identifier, exp.Var)): | ||
| if not isinstance(node.parent, exp.Column): | ||
| name = node.name | ||
| name = node.name.lower() | ||
| if name in args: | ||
| return args[name].copy() | ||
| if name in evaluator.locals: | ||
|
|
@@ -663,7 +680,7 @@ def substitute( | |
| return expressions, lambda args: func.this.transform( | ||
| substitute, | ||
| { | ||
| expression.name: arg | ||
| expression.name.lower(): arg | ||
| for expression, arg in zip( | ||
| func.expressions, args.expressions if isinstance(args, exp.Tuple) else [args] | ||
| ) | ||
|
|
||
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:
The above configuration is correct and the model renders into:
After this PR, the reference
@Xwill be invalid because we'll look up the lowercase"x"inlocals, which won't match the Python uppercase key"X", resulting in: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
localswithpython_envitems.