Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 8 additions & 2 deletions marimo/_ast/sql_visitor.py
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,11 @@ def normalize_sql_f_string(node: ast.JoinedStr) -> str:
We add placeholder for {...} expressions in the f-string.
This is so we can create a valid SQL query to be passed to
other utilities.

The placeholder "1" is used instead of "null" because it's valid in more
SQL contexts. For example, interval expressions like `interval '{days} days'`
become `interval '1 days'` which parses correctly, whereas `interval 'null days'`
would cause a parsing error (issue #7717).
"""

def print_part(part: ast.expr) -> str:
Expand All @@ -91,8 +96,9 @@ def print_part(part: ast.expr) -> str:
elif isinstance(part, ast.Constant):
return str(part.value)
else:
# Just add null as a placeholder for {...} expressions
return "null"
# Use "1" as placeholder - it's valid in more SQL contexts than "null"
# (e.g., interval expressions like `interval '1 days'`)
return "1"

result = "".join(print_part(part) for part in node.values)
return result
Expand Down
2 changes: 1 addition & 1 deletion tests/_ast/test_sql_visitor.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ def test_execute_with_f_string() -> None:
tree = ast.parse(source_code)
visitor = SQLVisitor()
visitor.visit(tree)
assert visitor.get_sqls() == ["SELECT * FROM users WHERE name = null"]
assert visitor.get_sqls() == ["SELECT * FROM users WHERE name = 1"]


def test_no_sql_calls() -> None:
Expand Down
90 changes: 88 additions & 2 deletions tests/_ast/test_visitor.py
Original file line number Diff line number Diff line change
Expand Up @@ -1204,6 +1204,7 @@ def fn():


HAS_DEPS = DependencyManager.duckdb.has()
HAS_SQLGLOT = DependencyManager.sqlglot.has()


@pytest.mark.skipif(not HAS_DEPS, reason="Requires duckdb")
Expand Down Expand Up @@ -1285,7 +1286,7 @@ def test_print_f_string() -> None:
assert isinstance(joined_str.body[0].value, ast.JoinedStr) # type: ignore
assert (
normalize_sql_f_string(joined_str.body[0].value) # type: ignore
== "select * from cars where name = null"
== "select * from cars where name = 1"
)

joined_str = ast.parse(
Expand All @@ -1294,10 +1295,95 @@ def test_print_f_string() -> None:
assert isinstance(joined_str.body[0].value, ast.JoinedStr) # type: ignore
assert (
normalize_sql_f_string(joined_str.body[0].value) # type: ignore
== "select * from 'null' where name = null"
== "select * from '1' where name = 1"
)


def test_normalize_sql_f_string_with_interval() -> None:
"""Test that f-string normalization works with SQL interval expressions.

Regression test for issue #7717. Using "1" as placeholder instead of "null"
ensures that interval expressions like `interval '{days} days'` are valid SQL.
"""
import ast

# This would fail with "null" placeholder: interval 'null days' is invalid
joined_str = ast.parse(
"f\"select * from t where ts > current_date - interval '{days_ago} days'\""
)
assert isinstance(joined_str.body[0].value, ast.JoinedStr) # type: ignore
result = normalize_sql_f_string(joined_str.body[0].value) # type: ignore
# With "1" placeholder, we get valid SQL: interval '1 days'
assert "interval '1 days'" in result


@pytest.mark.parametrize(
("expr", "expected", "should_parse"),
[
# WHERE clause value contexts
(
'f"SELECT * FROM t WHERE col = {value}"',
"SELECT * FROM t WHERE col = 1",
True,
),
(
'f"SELECT * FROM t WHERE {lhs} = 10"',
"SELECT * FROM t WHERE 1 = 10",
True,
),
(
'f"SELECT * FROM t WHERE col BETWEEN {a} AND {b}"',
"SELECT * FROM t WHERE col BETWEEN 1 AND 1",
True,
),
(
'f"SELECT * FROM t WHERE col IN ({values})"',
"SELECT * FROM t WHERE col IN (1)",
True,
),
(
"f\"SELECT * FROM t WHERE col LIKE '{pattern}%'\"",
"SELECT * FROM t WHERE col LIKE '1%'",
True,
),
# Misc contexts
(
'f"SELECT {col} FROM t LIMIT {limit} OFFSET {offset}"',
"SELECT 1 FROM t LIMIT 1 OFFSET 1",
True,
),
(
'f"INSERT INTO t VALUES ({x}, {y})"',
"INSERT INTO t VALUES (1, 1)",
True,
),
# Identifier-ish contexts: normalization should still be stable, but
# parseability depends on SQL dialect.
(
'f"SELECT * FROM {table}"',
"SELECT * FROM 1",
False,
),
],
)
def test_normalize_sql_f_string_parameterized_coverage(
expr: str, expected: str, should_parse: bool
) -> None:
joined_str = ast.parse(expr)
assert isinstance(joined_str.body[0].value, ast.JoinedStr) # type: ignore
normalized = normalize_sql_f_string(joined_str.body[0].value) # type: ignore
assert normalized == expected
assert "{" not in normalized
assert "}" not in normalized

if should_parse and HAS_SQLGLOT:
# A best-effort parse check to document what we expect to be valid SQL
# after f-string normalization.
from sqlglot import parse_one

parse_one(normalized, read="duckdb")


def test_normalize_sql_f_string_with_empty_quotes() -> None:
import ast

Expand Down
Loading