-
Notifications
You must be signed in to change notification settings - Fork 590
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
BUG: Fix range_window for base_sql backend (continued) #2710
BUG: Fix range_window for base_sql backend (continued) #2710
Conversation
…ix_base_sql_range_window
…ix_base_sql_range_window
|
The CI is passing now The pre-merge CI does not test BigQuery, but it does hit the relevant code when it tests the other SQL backends. I also confirmed locally that the current state of the PR resolves the error with the BigQuery backend mentioned in #2608. |
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.
looks good. can you add a whatsnew note (ref the original issue). ping on green.
ibis/backends/base_sql/__init__.py
Outdated
| } | ||
|
|
||
|
|
||
| def _replace_interval_with_scalar(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.
can you type in and out
|
thanks @timothydijamco and @LeeTZ |
closes #2608
This PR is a duplicate of #2614 (includes @LeeTZ 's bug fix and new test) + additional changes from me to debug the new test failing on some backends. (Creating a new PR just to make it easier for me to iterate and check if CI passes).
Please see #2614 for a full description of the original problem / solution