-
Notifications
You must be signed in to change notification settings - Fork 51
chore: migrate 10 scalar operators to SQLGlot #1922
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Migrated cos, hash, isnull, notnull, and sin operators.
Migrated tan_op, arcsin_op, arccos_op, arctan_op, and sinh_op scalar operators to SQLGlot.
sge.If( | ||
this=sge.func("ABS", expr.expr) > sge.convert(1), | ||
true=sge.func("IEEE_DIVIDE", sge.convert(0), sge.convert(0)), | ||
) | ||
], |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
ifs=[ | ||
sge.If( | ||
this=sge.func("ABS", expr.expr) > sge.convert(1), | ||
true=sge.func("IEEE_DIVIDE", sge.convert(0), sge.convert(0)), | ||
) | ||
], |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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), |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Good points.
d1a4c07
to
5c0dcbf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
This change contains three commits generated by Gemini CLI tool:
Fixes internal issue 430133370 🦕