Skip to content

fix!: fix macros with spaces to make them more consistent #4052

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.

Sign up for GitHub

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

Merged
tobymao merged 1 commit into from
Mar 31, 2025
Merged

fix!: fix macros with spaces to make them more consistent #4052

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions sqlmesh/core/macros.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,8 +240,7 @@ def evaluate_macros(
text = self.template(node.this, {})
if node.this != text:
changed = True
node.args["this"] = text
return node
return exp.to_identifier(text, quoted=node.quoted or None)
if node.is_string:
text = node.this
if has_jinja(text):
Expand Down Expand Up @@ -1389,6 +1388,10 @@ def _convert_sql(v: t.Any, dialect: DialectType) -> t.Any:
pass

if isinstance(v, exp.Expression):
if (isinstance(v, exp.Column) and not v.table) or (
isinstance(v, exp.Identifier) or v.is_string
):
return v.name
v = v.sql(dialect=dialect)
return v

Expand Down
21 changes: 21 additions & 0 deletions tests/core/test_macros.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1082,3 +1082,24 @@ def test_resolve_template_table():
evaluator.transform(parsed_sql).sql(identify=True)
== 'SELECT * FROM "test_catalog"."sqlmesh__test"."test__test_model__2517971505$partitions"'
)


def test_macro_with_spaces():
evaluator = MacroEvaluator()
evaluator.evaluate(d.parse_one(""" @DEF(x, "a b") """))
evaluator.evaluate(d.parse_one(""" @DEF(y, 'a b') """))
evaluator.evaluate(d.parse_one(""" @DEF(z, a."b c") """))

for sql, expected in (
Comment on lines +1089 to +1093
Copy link
Contributor

@georgesittas georgesittas Mar 31, 2025

Choose a reason for hiding this comment

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

Consider testing with config variables as well besides @DEF ones. For example:

# config.yaml
variables:
  v1: "a b"
  v2: '"a b"'
  v3: "'a b'"

Copy link
Contributor Author

@tobymao tobymao Mar 31, 2025

Choose a reason for hiding this comment

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

i'm not sure if config variables iare in the scope of these tests, that's probably something to go with your changes

("@x", '"a b"'),
("@{x}", '"a b"'),
("a_@x", '"a_a b"'),
("a.@x", 'a."a b"'),
("@y", "'a b'"),
("@{y}", '"a b"'), # a little tricky here as it's not a string
("a_@y", '"a_a b"'),
("a.@{y}", 'a."a b"'),
("@z", 'a."b c"'),
("d.@z", 'd.a."b c"'),
):
assert evaluator.transform(parse_one(sql)).sql() == expected