Skip to content

Conversation

chelsea-lin
Copy link
Contributor

Fixes internal issue 430133370 🦕

@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: bigquery Issues related to the googleapis/python-bigquery-dataframes API. labels Aug 20, 2025
@chelsea-lin chelsea-lin marked this pull request as ready for review August 20, 2025 00:08
@chelsea-lin chelsea-lin requested review from a team as code owners August 20, 2025 00:08
@chelsea-lin chelsea-lin requested a review from sycai August 20, 2025 00:08
@@ -163,3 +175,16 @@ def _(op, left: TypedExpr, right: TypedExpr) -> sge.Expression:
@BINARY_OP_REGISTRATION.register(ops.obj_make_ref_op)
def _(op, left: TypedExpr, right: TypedExpr) -> sge.Expression:
return sge.func("OBJ.MAKE_REF", left.expr, right.expr)


def _coerce_bools(
Copy link
Contributor

Choose a reason for hiding this comment

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

This parameter design choice is a little weird. Suggestion:

let's create a "_coerce_bool_to_int" function that process one value at a time.

Then at callsite, we could

left_expr = _coerce_bool_to_int(left.expr)
right_expr = _coerce_bool_to_int(right.expr)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good points. I added coerce_bool_to_int and coerce_date_to_datetime to the TypedExpr class.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! Could we make them top-level functions instead of class method?

I generally expect methods like TypedExpr.to_abc() to still return a TypedExpr value, which might make our code more verbose. Top-level functions feel like a better choice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense to me. Thanks!

@chelsea-lin chelsea-lin requested a review from sycai August 20, 2025 21:12
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Aug 20, 2025
@chelsea-lin chelsea-lin enabled auto-merge (squash) August 21, 2025 00:06
@chelsea-lin chelsea-lin merged commit b454256 into main Aug 21, 2025
18 of 25 checks passed
@chelsea-lin chelsea-lin deleted the main_chelsealin_eq0 branch August 21, 2025 00:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the googleapis/python-bigquery-dataframes API. size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants