refactor(bigframes): Introduce GoogleSqlScalarOp#17037
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces GoogleSqlScalarOp to provide a more direct mapping between BigFrames operations and GoogleSQL functions, refactoring several geography and mathematical operations to use this new infrastructure. It also adds support for omitted optional arguments across the expression and compilation layers for both Ibis and sqlglot. Feedback identifies several critical implementation errors in the Ibis compiler registry, including incorrect list methods, return type mismatches, and logic errors in argument indexing. Additionally, there are issues with missing imports, incorrect type hints, and consistent typos in the 'omitted' property across the codebase.
tswast
left a comment
tswast
left a comment
There was a problem hiding this comment.
I'd like to see some (tested) examples of the PREFIX and INFIX calling modes to know that these work rather than discovering bugs later. Alternatively, we can raise notimplemented for these cases and add those examples in a follow-up.
| return sg.parse_one(f"{op.sql_name} {operands[0].expr.sql(dialect='bigquery')}", dialect="bigquery") | ||
| elif op.calling_convention == CallingConvention.INFIX: | ||
| assert len(operands) == 2, 'infix op expects exactly 2 args' | ||
| return sg.parse_one(f"{operands[0].expr.sql(dialect='bigquery')} {op.sql_name} {operands[1].expr.sql(dialect='bigquery')}", dialect="bigquery") |
There was a problem hiding this comment.
Any way we can avoid the "parse" calls?
There was a problem hiding this comment.
Revert the changes to this file. The typing module is the one exception to the "import modules not classes" rule.
| def googlesql_scalar_op_impl(*operands: ibis_types.Value, op: ops.GoogleSqlScalarOp): | ||
| final_operands = [] | ||
| arg_templates = [] | ||
| if op.calling_convention == CallingConvention.FUNCTION: |
There was a problem hiding this comment.
readability nit: could we define helper methods for each of the calling_convertion branch? The goal is to reduce function length and levels of nesting.
E.g.
if op.calling_convention == CallingConvention.FUNCTION:
_compile_function_sql_scalar_op(...)
else if op.calling_convention == CallingConvention.PREFIX:
_compile_prefix_sql_scalar_op(...)
...
There was a problem hiding this comment.
removed calling_convention entirely - will handle operators in specialized logic later
|
|
||
|
|
||
| @dataclasses.dataclass(frozen=True) | ||
| class Omitted(Expression): |
There was a problem hiding this comment.
readability nit: Maybe use a noun for the class name, such as "OmittedArg"?
|
|
||
|
|
||
| @public | ||
| class Omitted(Value): |
| inputs = tuple( | ||
| TypedExpr(self.compile_expression(sub_expr), sub_expr.output_type) | ||
| if not isinstance(sub_expr, ex.OmittedArg) | ||
| else TypedExpr(sge.Null(), None, is_omitted=True) |
There was a problem hiding this comment.
I'm guessing that this could cause problems at some point, but hopefully we'll catch that when we encounter such a case.
3 participants
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕