chore: Migrate IntegerLabelToDatetimeOp operator to SQLGlot #2310
Conversation
product-auto-label
bot
added
size: l
| to=sge.DataType.build("INT64"), | ||
| ), | ||
| ), | ||
| to=_dtype_to_sql_string(y.dtype), # type: ignore |
There was a problem hiding this comment.
we can call sqlglot_types.from_bigframes_dtype instead and remove "# type: igore"
| this=first, to=sge.DataType.build("BIGNUMERIC") | ||
| ), | ||
| ), | ||
| to=sge.DataType.build("INT64"), |
There was a problem hiding this comment.
to="INT64" works too and can simple the expression a little bit. If so, can we apply to all the "to" parameter of the "sge.Cast" method?
| us = n * 7 * 24 * 60 * 60 * 1000000 | ||
| first = sge.func( | ||
| "UNIX_MICROS", | ||
| sge.Add( | ||
| this=sge.TimestampTrunc( | ||
| this=sge.Cast(this=y.expr, to="TIMESTAMP"), | ||
| unit=sge.Var(this="WEEK(MONDAY)"), | ||
| ), | ||
| expression=sge.Interval( | ||
| this=sge.convert(6), unit=sge.Identifier(this="DAY") | ||
| ), | ||
| ), | ||
| ) | ||
| x_label = sge.Cast( | ||
| this=sge.func( | ||
| "TIMESTAMP_MICROS", | ||
| sge.Cast( | ||
| this=sge.Add( | ||
| this=sge.Mul( | ||
| this=sge.Cast(this=x.expr, to="BIGNUMERIC"), | ||
| expression=sge.convert(us), | ||
| ), | ||
| expression=sge.Cast(this=first, to="BIGNUMERIC"), | ||
| ), | ||
| to="INT64", | ||
| ), | ||
| ), | ||
| to=sqlglot_types.from_bigframes_dtype(y.dtype), | ||
| ) |
There was a problem hiding this comment.
Style nit: could we define separate helper functions to handle different frequencies? This would help reduce the size of the function _integer_label_to_datetime_op_non_fixed_frequency and improve readability.
The improved function should look like this
def _integer_label_to_datetime_op_non_fixed_frequency():
....
if rule_code is weekly:
return _integer_label_to_datetime_op_weekly_freq()
elif rule_code is monthly:
....
| if rule_code in ("YE-DEC", "A-DEC", "Y-DEC"): | ||
| return _integer_label_to_datetime_op_yearly_freq(x, y, op) | ||
|
|
||
| raise ValueError(rule_code) |
There was a problem hiding this comment.
For better readability, can we move this "if" clauses to the integer_label_to_datetime_op function"? In this way, we don't need to raise "ValueError" anymore. The expected function integer_label_to_datetime_op would be like:
if rule_code == "W-SUN":
return _integer_label_to_datetime_op_weekly_freq(x, y, op)
....
# Instead raise ValueError, we can call function directly.
return _integer_label_to_datetime_op_non_fixed_frequency(x, y, op)
sycai
left a comment
sycai
left a comment
There was a problem hiding this comment.
This change PR LGTM besides Chelsea's comments.
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 b/447388852 🦕