Skip to content

fix: quote literal values for strings in SQLGlot compiler #18125

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

Draft
sycai wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix: quote literal values for strings in SQLGlot compiler #18125

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
2 changes: 2 additions & 0 deletions packages/bigframes/bigframes/core/compile/sqlglot/sql/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,8 @@ def literal(value: typing.Any, dtype: dtypes.Dtype | None = None) -> sge.Express
return sge.func("ST_GEOGFROMTEXT", sge.convert(wkt))
elif dtype == dtypes.TIMEDELTA_DTYPE:
return sge.convert(utils.timedelta_to_micros(value))
elif dtype == dtypes.STRING_DTYPE:
return sge.convert(str(value))
Comment on lines +116 to +117

@gemini-code-assist gemini-code-assist Bot Aug 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Directly calling str(value) on any input when dtype == dtypes.STRING_DTYPE can lead to incorrect SQL generation and bugs:

  1. If value is a PyArrow scalar (e.g., pa.scalar("hello")), str(value) returns '"hello"' (with literal double quotes), which results in double quotes being embedded in the SQL string literal.
  2. If value is a null-like object (such as pa.scalar(None), pd.NA, or None), str(value) will produce string literals like 'None' or '<NA>' instead of SQL NULL.

To prevent this, we should first unwrap PyArrow scalars using .as_py() if available, and then handle null/NA values appropriately before converting to string.

    elif dtype == dtypes.STRING_DTYPE:
        if hasattr(value, "as_py"):
            value = value.as_py()
        if value is None or value is pd.NA:
            return sge.convert(None)
        return sge.convert(str(value))

else:
if isinstance(value, np.generic):
value = value.item()
Expand Down
21 changes: 19 additions & 2 deletions packages/bigframes/tests/unit/core/compile/sqlglot/sql/test_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,11 +127,28 @@ def test_literal_for_geo():
"PARSE_JSON('{\\'a\\': 10}')",
id="json",
),
pytest.param(
2019,
sql.dtypes.STRING_DTYPE,
"'2019'",
id="string_from_int",
),
pytest.param(
pa.scalar(2019),
sql.dtypes.STRING_DTYPE,
"'2019'",
id="string_from_pyarrow_scalar",
),
pytest.param(
True,
sql.dtypes.STRING_DTYPE,
"'True'",
id="string_from_bool",
),
Comment on lines +142 to +147

@gemini-code-assist gemini-code-assist Bot Aug 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

It would be beneficial to add test cases for null/NA values (such as pd.NA and pa.scalar(None)) with STRING_DTYPE to ensure they are correctly compiled to NULL instead of string literals like '<NA>' or 'None'.

),
)
def test_literal_explicit_dtype(value, dtype, expected):
got = sql.to_sql(sql.literal(value, dtype=dtype))
assert got == expected
assert sql.to_sql(sql.literal(value, dtype=dtype)) == expected


@pytest.mark.parametrize(
Expand Down
Loading