fix: quote literal values for strings in SQLGlot compiler - #18125
Conversation
There was a problem hiding this comment.
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.
| elif dtype == dtypes.STRING_DTYPE: | ||
| return sge.convert(str(value)) |
There was a problem hiding this comment.
Directly calling str(value) on any input when dtype == dtypes.STRING_DTYPE can lead to incorrect SQL generation and bugs:
- If
valueis 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. - If
valueis a null-like object (such aspa.scalar(None),pd.NA, orNone),str(value)will produce string literals like'None'or'<NA>'instead of SQLNULL.
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))| pytest.param( | ||
| True, | ||
| sql.dtypes.STRING_DTYPE, | ||
| "'True'", | ||
| id="string_from_bool", | ||
| ), |
There was a problem hiding this comment.
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'.
No description provided.