Skip to content

refactor(bigframes): update SQLGlot compiler to process AI func params better#16757

Merged
sycai merged 3 commits intomainfrom
sycai_sqlglot_ai_refactor
Apr 21, 2026
Merged

refactor(bigframes): update SQLGlot compiler to process AI func params better#16757
sycai merged 3 commits intomainfrom
sycai_sqlglot_ai_refactor

Conversation

@sycai
Copy link
Copy Markdown
Contributor

@sycai sycai commented Apr 21, 2026

No description provided.

@sycai sycai requested review from a team as code owners April 21, 2026 20:57
@sycai sycai requested review from TrevorBergeron, chelsea-lin and tswast and removed request for a team April 21, 2026 20:57
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 refactors the _construct_named_args function to dynamically iterate over operation arguments instead of using hardcoded field checks. The feedback suggests using the local value variable when processing categories for better type safety, ensuring consistent use of double quotes for string literals, and improving the generic argument handler to correctly handle non-string types or raise an error for unsupported ones.

Comment thread packages/bigframes/bigframes/core/compile/sqlglot/expressions/ai_ops.py Outdated
Comment thread packages/bigframes/bigframes/core/compile/sqlglot/expressions/ai_ops.py Outdated
Comment thread packages/bigframes/bigframes/core/compile/sqlglot/expressions/ai_ops.py Outdated
@sycai
Copy link
Copy Markdown
Contributor Author

sycai commented Apr 21, 2026

Test failure is unrelated.

@sycai sycai added the kokoro:run Add this label to force Kokoro to re-run the tests. label Apr 21, 2026
args.append(
sge.Kwarg(this=field, expression=sge.Literal.string(value.upper()))
)
else:
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.

Do we want to check for the field is "output_schema" explicitly? This would allow us to decide how to handle unknown fields - either failing fast, ignoring them, or similarly to "output_schema" - which is better for forward compatibility.

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.

I think we should be good without checking the "output_schema", as the intention of this PR is to make the compiler able to handle all kinds of fields without explicit mentions.

@sycai sycai added kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed kokoro:run Add this label to force Kokoro to re-run the tests. labels Apr 21, 2026
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 21, 2026
@sycai sycai enabled auto-merge (squash) April 21, 2026 22:35
@sycai sycai merged commit 7c8412a into main Apr 21, 2026
30 checks passed
@sycai sycai deleted the sycai_sqlglot_ai_refactor branch April 21, 2026 23:01
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.

3 participants