Skip to content
This repository was archived by the owner on May 7, 2026. It is now read-only.

chore: migrate 10 scalar operators to SQLGlot#1922

Merged
chelsea-lin merged 4 commits into
mainfrom
main_chelsealin_migratescalar
Jul 21, 2025
Merged

chore: migrate 10 scalar operators to SQLGlot#1922
chelsea-lin merged 4 commits into
mainfrom
main_chelsealin_migratescalar

Conversation

@chelsea-lin

@chelsea-lin chelsea-lin commented Jul 18, 2025

Copy link
Copy Markdown
Contributor

This change contains three commits generated by Gemini CLI tool:

  • Migrated cos, hash, isnull, notnull, and sin operators.
  • Migrated tan_op, arcsin_op, arccos_op, arctan_op, and sinh_op scalar operators to SQLGlot.

Fixes internal issue 430133370 🦕

Migrated cos, hash, isnull, notnull, and sin operators.
Migrated tan_op, arcsin_op, arccos_op, arctan_op, and sinh_op scalar operators to SQLGlot.
@product-auto-label product-auto-label Bot added size: l Pull request size is large. api: bigquery Issues related to the googleapis/python-bigquery-dataframes API. labels Jul 18, 2025
@chelsea-lin chelsea-lin changed the title chore: Migrate up to 15 scalar operators to SQLGlot chore: Migrate up to 10 scalar operators to SQLGlot Jul 18, 2025
@chelsea-lin
chelsea-lin marked this pull request as ready for review July 18, 2025 19:25
@chelsea-lin
chelsea-lin requested review from a team and sycai July 18, 2025 19:25
sycai
sycai previously approved these changes Jul 18, 2025
Comment on lines +37 to +41
sge.If(
this=sge.func("ABS", expr.expr) > sge.convert(1),
true=sge.func("IEEE_DIVIDE", sge.convert(0), sge.convert(0)),
)
],

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.

We might want to add some comments on the necessity of this check.

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.

Using _NAN instead for better readability.

Comment on lines +49 to +54
ifs=[
sge.If(
this=sge.func("ABS", expr.expr) > sge.convert(1),
true=sge.func("IEEE_DIVIDE", sge.convert(0), sge.convert(0)),
)
],

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.

We might want to add some comments on the necessity of this check.

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.

Using _NAN instead for better readability.

Comment on lines +134 to +141
ifs=[
sge.If(
this=sge.func("ABS", expr.expr) > sge.convert(709.78),
true=sge.func("SIGN", expr.expr)
* sge.func("IEEE_DIVIDE", sge.convert(1), sge.convert(0)),
)
],
default=sge.func("SINH", expr.expr),

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.

We might want to add some comments on the necessity of this check, specially for that seemingly magic number 709.78

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.

Updated to _FLOAT64_EXP_BOUND with some comments.

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.

Could you reformat the test functions in this file such that arrange/action/assert code blocks are demarcated with empty lines?

go/unit-testing-practices?polyglot=python#structure

Maybe ask Gemini for help?

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.

Done. Good points.

@chelsea-lin
chelsea-lin force-pushed the main_chelsealin_migratescalar branch from d1a4c07 to 5c0dcbf Compare July 18, 2025 21:19
@chelsea-lin
chelsea-lin requested a review from sycai July 18, 2025 21:19
@chelsea-lin chelsea-lin changed the title chore: Migrate up to 10 scalar operators to SQLGlot chore: migrate 10 scalar operators to SQLGlot Jul 18, 2025

@sycai sycai 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.

Nice!

@chelsea-lin
chelsea-lin enabled auto-merge (squash) July 21, 2025 21:15
@chelsea-lin
chelsea-lin merged commit 25ab0d5 into main Jul 21, 2025
18 of 25 checks passed
@chelsea-lin
chelsea-lin deleted the main_chelsealin_migratescalar branch July 21, 2025 21:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

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