-
Notifications
You must be signed in to change notification settings - Fork 609
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
feat(api): add DateValue.epoch
api for computing days since epoch
#9856
Conversation
4bec42e
to
26b224f
Compare
Propose to codify the word |
25e9f5f
to
2c2215c
Compare
Is this operation semantically the same as Alternatively, could we spell this as |
It's not insufficient, just more verbose than I'd like.
My original plan was to have |
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.
Overall happy with the user-facing API, just a few comments on the implementation.
ibis/backends/sql/dialects.py
Outdated
@@ -27,6 +28,32 @@ | |||
sge.ArraySort: rename_func("arraySort"), | |||
sge.LogicalAnd: rename_func("min"), | |||
sge.LogicalOr: rename_func("max"), | |||
sge.DateFromParts: lambda self, e: sg.func( |
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 actually find reading these in the sqlglot dialect harder to understand than the previous version in the compiler. These are pretty complicated implementations (length-wise), doing more in the dialect means we have additional places to check for complex logic when trying to understand how something is compiled (compiler, rewrite rules, our dialects.py
file, sqlglot's dialects, sqlglot's rewrite rules). If possible, I'd find it easier to understand compilation if we try to keep most of our implemented logic in the compiler itself rather than the dialects here.
Just a -0.25 though, if you feel strongly about doing it this way then that's fine.
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.
Unfortunately without these (or something like it), it's a non-starter to implement this without using a new operation. Happy to try the no-new-op route, but to do that we need sqlglot to correctly compile DateFromParts
correctly for all the backends it can, which requires porting the previous code to this.
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.
In general I agree with your point, but this happened to be an annoying case where it seemed better to centralize this logic in sqlglot rather than repeating in the compiler.
32cb541
to
52ddabf
Compare
Co-authored-by: Jim Crist-Harif <jcristharif@gmail.com>
LGTM! Leaving the merge to you - I assume you want to squash some/all of these commits. |
Description of changes
Adds
DateValue.epoch
method and underlyingops.UnixDate
.This was originally meant to help with window function queries with range
bounds where the backend doesn't support intervals in preceding/following, but
I think it has general utility and is much more clear IMO than casting a
date value to an integer.