-
Notifications
You must be signed in to change notification settings - Fork 55
refactor: add apply_window_if_present and get_window_order_by methods #1947
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
This change is one of break-down changes for #1889 |
0345f55
to
f5bee10
Compare
) | ||
self.assertEqual( | ||
result.sql(dialect="bigquery"), | ||
"value OVER (ORDER BY `col1` ASC NULLS LAST RANGE BETWEEN 86400000000 PRECEDING AND 43200000000 FOLLOWING)", |
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.
For time window rolling like this, we need to wrap the rolling column with unix_micros()
The result should look like this:
... OVER (ORDER BY UNIX_MICROS(`col1`) ASC ...)
Could you update the compiler code to make it happen?
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.
I have an example code for you to quickly verify whether the generated SQL is valid E2E:
import bigframes.pandas as bpd
import pandas as pd
df = bpd.read_gbq("bigframes-dev.sycai.test_rolling")
df.set_index('ts_col').sort_index().rolling(window=pd.Timedelta('3s')).min().sql
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 pull request (PR) focuses on implementing just two utility functions for windows, while the full windows compilation is drafted in PR #1889. With the full implementation, the e2e codes can generate the right golden SQL (see here). IIUC, the ops.UnixMicros
operator will response for the SQL UNIX_MICROS
generation. Let me know if you have further questions.
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.
Got it! Thanks for the explanation
05e9e0c
to
55bb9b9
Compare
Fixes internal issue 430350912 🦕