diff --git a/marimo/_ast/sql_visitor.py b/marimo/_ast/sql_visitor.py index 05c36099eec..7437de33eaa 100644 --- a/marimo/_ast/sql_visitor.py +++ b/marimo/_ast/sql_visitor.py @@ -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: @@ -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 diff --git a/tests/_ast/test_sql_visitor.py b/tests/_ast/test_sql_visitor.py index 81e04328c49..1c43f24c677 100644 --- a/tests/_ast/test_sql_visitor.py +++ b/tests/_ast/test_sql_visitor.py @@ -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: diff --git a/tests/_ast/test_visitor.py b/tests/_ast/test_visitor.py index 6b172ad51ee..3c42b7a5f64 100644 --- a/tests/_ast/test_visitor.py +++ b/tests/_ast/test_visitor.py @@ -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") @@ -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( @@ -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