Skip to content

fix(bigframes): improve error message when unescaped { are found in SQL cells#17346

Open
tswast wants to merge 5 commits into
mainfrom
tswast-bigframes-pyformat
Open

fix(bigframes): improve error message when unescaped { are found in SQL cells#17346
tswast wants to merge 5 commits into
mainfrom
tswast-bigframes-pyformat

Conversation

@tswast
Copy link
Copy Markdown
Contributor

@tswast tswast commented Jun 2, 2026

Hints to the user that they may need to escape { and } characters by doubling them, and includes context as to where to correct such errors.

Fixes internal issue b/517909919
🦕

… SQL cells

Hints to the user that they may need to escape `{` and `}` characters by
doubling them, and includes context as to where to correct such errors.

Fixes internal issue b/517909919
@tswast tswast requested review from a team as code owners June 2, 2026 17:13
@tswast tswast requested review from sycai and removed request for a team June 2, 2026 17:13
Copy link
Copy Markdown
Contributor

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

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 enhances the pyformat utility by tracking replacement field positions in SQL templates, allowing it to raise informative ValueError exceptions with visual error context when variables are missing. Unit tests have been added to verify this behavior. The review feedback suggests several robust improvements: adding a bounds check in _consume_literal to prevent potential IndexError exceptions, wrapping _parse_fields in a try-except block to handle parsing errors gracefully, and conditionally appending the error context to avoid trailing newlines.

Comment thread packages/bigframes/bigframes/core/pyformat.py Outdated
Comment thread packages/bigframes/bigframes/core/pyformat.py Outdated
Comment on lines +355 to +360
context = get_error_context_at_pos(sql_template, pos)
raise ValueError(
f"Undetected variable {name!r} in SQL template. "
"Did you mean to escape '{' and '}' by doubling them?\n"
f"{context}"
) from e
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

If get_error_context_at_pos returns an empty string (e.g., when the position is -1 or not found), the error message will end with a trailing newline, which looks untidy. We should only append the context if it is non-empty.

            context = get_error_context_at_pos(sql_template, pos)
            msg = (
                f"Undetected variable {name!r} in SQL template. "
                "Did you mean to escape '{' and '}' by doubling them?"
            )
            if context:
                msg += f"\n{context}"
            raise ValueError(msg) from e

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Since we don't expect this case to happen, I would prefer not to add logic for it.

tswast and others added 4 commits June 2, 2026 18:06
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.

2 participants