Skip to content

fix(bigframes): escape backtick in ordering_clause identifier#17837

Open
Samin061 wants to merge 1 commit into
googleapis:mainfrom
Samin061:bigframes-ordering-escape
Open

fix(bigframes): escape backtick in ordering_clause identifier#17837
Samin061 wants to merge 1 commit into
googleapis:mainfrom
Samin061:bigframes-ordering-escape

Conversation

@Samin061

Copy link
Copy Markdown

ordering_clause drops the column id straight into a backtick-quoted identifier without escaping, so a column name that contains a backtick closes the identifier early and the trailing text is parsed as SQL and run under the reader's BigQuery credentials. The SELECT compile path and this module's own identifier() helper already escape names with escape_chars, so the ORDER BY term now goes through the same escaping.

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

@Samin061
Samin061 requested review from a team as code owners July 23, 2026 08:27
@Samin061
Samin061 requested review from tswast and removed request for a team July 23, 2026 08:27

@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 updates the ordering_clause function in bigframes/core/sql/__init__.py to escape special characters, such as backticks, in column names to prevent SQL injection or syntax errors. It also adds corresponding unit tests in tests/unit/core/test_sql.py to verify the escaping behavior. There are no review comments, so I have no feedback to provide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant