Skip to content

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

Closed
sycai wants to merge 1 commit into
mainfrom
sycai_fix_sqlglot_literal
Closed

fix: quote literal values for strings in SQLGlot compiler#18125
sycai wants to merge 1 commit into
mainfrom
sycai_fix_sqlglot_literal

Conversation

@sycai

@sycai sycai commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@sycai sycai added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 14, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

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.

Code Review

This pull request adds support for converting values to string literals when the data type is dtypes.STRING_DTYPE in the SQL compilation module, along with corresponding unit tests. The review feedback points out that directly calling str(value) on PyArrow scalars or null-like objects (such as pd.NA or None) can lead to incorrect SQL generation (e.g., double quotes or string literals like 'None'). It is recommended to unwrap PyArrow scalars using .as_py() and handle nulls properly, as well as add test cases for these scenarios.

Comment on lines +116 to +117
elif dtype == dtypes.STRING_DTYPE:
return sge.convert(str(value))

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))

Comment on lines +142 to +147
pytest.param(
True,
sql.dtypes.STRING_DTYPE,
"'True'",
id="string_from_bool",
),

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'.

pull Bot pushed a commit to Zezo-Ai/google-cloud-python that referenced this pull request Aug 19, 2026
https://github.com/google/bigframes is the new location for bigframes.

See internal issue b/517198791
🦕

(See below to close bigframes related PRs)
Closes googleapis#17502
Closes googleapis#17837
Closes googleapis#17587
Closes googleapis#17595
Closes googleapis#18095
Closes googleapis#18096
Closes googleapis#18109
Closes googleapis#18110
Closes googleapis#18113
Closes googleapis#18125
Closes googleapis#17587
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do not merge Indicates a pull request not ready for merge, due to either quality or timing.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant