-
Couldn't load subscription status.
- Fork 64
refactor: enable engine tests for datetime ops #2196
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
Conversation
636578e to
a7fe5c6
Compare
|
|
||
| # Helpers | ||
| def dayofweek_op_impl(expr: TypedExpr) -> sge.Expression: | ||
| # Adjust the 1-based day-of-week index (from SQL) to a 0-based index. |
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.
This comment does not seem to be relevant any more. Remove it?
| extract_expr = sge.Extract( | ||
| this=sge.Identifier(this="DAYOFWEEK"), expression=expr.expr | ||
| ) | ||
| return sge.Cast( |
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 should add a comment above line 68, explaining that:
- The SQL function returns values in the range [1,7] with Sunday as the first day of of the week.
- pandas assumed the week starts on Monday, which is denoted by 0 and ends on Sunday which is denoted by 6
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.
Good points. Fixed.
a7fe5c6 to
ee7be9a
Compare
Fixes internal issue 446726636 🦕