Repository navigation
fix(bigquery): use backslash escaping for apostrophes in filter values - #38925
vijaygovindaraja wants to merge 3 commits into
Conversation
BigQuery does not support double-apostrophe escaping for single quotes
in string literals. When a filter value contains an apostrophe (e.g.,
"Armando's"), SQLAlchemy's default literal processor generates
'Armando''s' which causes a BigQuery syntax error: "concatenated
string literals must be separated by whitespace or comments".
Add a BigQueryStringType that overrides the literal_processor to use
backslash escaping ('Armando\'s') instead. Apply it via monkeypatching
the BigQueryDialect.colspecs, following the same pattern used for the
Databricks dialect fix.
Fixes apache#35857
Signed-off-by: V Govindarajan <vijay.govindarajan91@gmail.com>
Code Review Agent Run #0c4e27Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
codeant-ai-for-open-source
Bot
added
the
size:M
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| assert "''" not in result, ( | ||
| f"BigQuery should use backslash escaping, got double-apostrophe: {result}" | ||
| ) | ||
| assert "\\'" in result or "Armando" in result |
There was a problem hiding this comment.
Suggestion: The final assertion in this test is too weak: by allowing "Armando" as an alternative condition, the test will still pass if the generated SQL contains an unescaped apostrophe like Armando's (which is invalid BigQuery SQL), so the test does not actually enforce the stated requirement that BigQuery use backslash escaping for apostrophes. [logic error]
Severity Level: Major ⚠️
- ❌ BigQuery filters with apostrophes can regress without test failing.
- ⚠️ Dashboards using where_in on BigQuery may break silently.
- ⚠️ Regression risk for issue #35857 remains insufficiently covered.| assert "\\'" in result or "Armando" in result | |
| assert "\\'" in result, ( | |
| f"BigQuery should escape apostrophes with backslash: {result}" | |
| ) |
Steps of Reproduction ✅
1. Inspect `test_where_in_bigquery_apostrophe` in `tests/unit_tests/jinja_context_test.py`
lines 510–527, where `result = WhereInMacro(BigQueryDialect())(["Armando's"])` and the
assertions `assert "''" not in result` and `assert "\\'" in result or "Armando" in result`
are evaluated.
2. Note from `superset/jinja_context.py:190–233` that `WhereInMacro.__call__` builds a
parenthesized IN list using the SQLAlchemy dialect's literal rendering, and from
`superset/db_engine_specs/bigquery.py:125–147` that the intended behavior is to escape
apostrophes with backslash for BigQuery.
3. Consider an invalid-but-plausible BigQuery literal output such as `result =
"('Armando's')"` (no double-apostrophe, no backslash escaping), which would cause a
BigQuery syntax error in real queries, but still satisfies the current test conditions: it
contains no `"''"` substring and does contain `"Armando"`.
4. Evaluate the current test logic with this `result` value: the first assertion (`"''"
not in result`) passes because there are no doubled quotes, and the second assertion
(`"\\'" in result or "Armando" in result`) passes because `"Armando"` is present, so
`pytest` would report the test as passing even though the BigQuery requirement
(backslash-escaped apostrophes) is violated.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/jinja_context_test.py
**Line:** 526:526
**Comment:**
*Logic Error: The final assertion in this test is too weak: by allowing `"Armando"` as an alternative condition, the test will still pass if the generated SQL contains an unescaped apostrophe like `Armando's` (which is invalid BigQuery SQL), so the test does not actually enforce the stated requirement that BigQuery use backslash escaping for apostrophes.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.There was a problem hiding this comment.
Good catch — fixed in 2e48b86. Removed the weak fallback and now strictly asserts backslash escaping is present.
Remove the weak `or "Armando" in result` fallback that would allow the test to pass even without proper backslash escaping. Signed-off-by: V Govindarajan <vijay.govindarajan91@gmail.com>
Code Review Agent Run #8a05e4Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
There was a problem hiding this comment.
Pull request overview
This PR addresses BigQuery query failures caused by SQLAlchemy’s default string literal escaping ('') when filter values contain apostrophes, by switching BigQuery string literal escaping to backslash style (\') and adding a regression test.
Changes:
- Added a BigQuery-specific
TypeDecorator(BigQueryStringType) to render string literals with backslash-escaped apostrophes. - Monkeypatched
sqlalchemy_bigquery.BigQueryDialect.colspecsso compiled literals use the custom string type. - Added a unit test asserting
where_inoutput for BigQuery does not contain doubled apostrophes and does contain backslash escaping.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
superset/db_engine_specs/bigquery.py |
Introduces the custom string literal processor and applies a BigQuery dialect monkeypatch at import time. |
tests/unit_tests/jinja_context_test.py |
Adds coverage for BigQuery where_in output when values contain apostrophes. |
| See: https://github.com/apache/superset/issues/35857 | ||
| """ | ||
| try: | ||
| from sqlalchemy_bigquery import BigQueryDialect |
There was a problem hiding this comment.
This test is non-deterministic: the BigQuery dialect monkeypatch is applied when superset/db_engine_specs/bigquery.py is imported, but this test only imports sqlalchemy_bigquery.BigQueryDialect. If the BigQuery engine spec module hasn’t been imported earlier in the test run, WhereInMacro(BigQueryDialect()) will still use the unpatched colspecs and this assertion will fail. Import the Superset BigQuery engine spec (or call the patch helper) in the test before instantiating BigQueryDialect so the test doesn’t depend on global import order.
| from sqlalchemy_bigquery import BigQueryDialect | |
| from sqlalchemy_bigquery import BigQueryDialect | |
| # Import the Superset BigQuery engine spec so its dialect monkeypatch is applied | |
| from superset.db_engine_specs import bigquery as _bigquery_engine_spec # noqa: F401 |
| from datetime import datetime | ||
| from re import Pattern | ||
| from collections.abc import Callable |
There was a problem hiding this comment.
Import ordering: from collections.abc import Callable is inserted after from datetime import datetime/from re import Pattern, which is likely to fail the repo’s import-sorting lint (ruff/isort). Reorder the stdlib from ... import ... lines so they’re sorted consistently.
| from datetime import datetime | |
| from re import Pattern | |
| from collections.abc import Callable | |
| from collections.abc import Callable | |
| from datetime import datetime | |
| from re import Pattern |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #38925 +/- ##
==========================================
- Coverage 64.58% 64.58% -0.01%
==========================================
Files 2536 2536
Lines 130811 130829 +18
Branches 30346 30346
==========================================
+ Hits 84486 84497 +11
- Misses 44856 44863 +7
Partials 1469 1469
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #294b3dActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
anyone available to review this? fixes apostrophe escaping in BigQuery filter values. |
|
Thanks @vijaygovindaraja. Would love to see this close #35857. My one hesitation is the import-time monkeypatch of |
|
Thanks for this @vijaygovindaraja, and nice that you landed on the same backslash-escaping approach. We just merged #38835 for this (same fix for #35857), which had been through a couple of review rounds, so I'm going to close this as a duplicate. Genuinely appreciate the fix though... sorry it overlapped. Please keep 'em coming :) |
User description
SUMMARY
Fixes BigQuery syntax errors when filter values contain apostrophes (e.g., "Armando's").
BigQuery does not support double-apostrophe escaping (
'Armando''s'), which is the default behavior of SQLAlchemy's literal processor. This causes a400 Syntax error: concatenated string literals must be separated by whitespace or commentserror.BEFORE
AFTER
CHANGES
superset/db_engine_specs/bigquery.py:BigQueryStringType— aTypeDecoratorthat overridesliteral_processorto use backslash escaping instead of double-apostrophe escaping_monkeypatch_bigquery_dialect()— patchesBigQueryDialect.colspecsto use the custom string typeDatabricksStringType+monkeypatch_dialect())tests/unit_tests/jinja_context_test.py:test_where_in_bigquery_apostrophe— verifies BigQuery dialect uses backslash escaping for filter values containing apostrophesTESTING INSTRUCTIONS
ADDITIONAL INFORMATION
superset/db_engine_specs/databricks.pylines 57-107CodeAnt-AI Description
Fix BigQuery filters with apostrophes
What Changed
Armando'sno longer trigger a query syntax errorImpact
✅ Fewer BigQuery filter errors✅ More reliable dashboards with apostrophe-containing values✅ Clearer filter behavior for quoted names💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.