-
Notifications
You must be signed in to change notification settings - Fork 769
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
fix(mysql): TIMESTAMP -> CAST #2223
Conversation
sqlglot/dialects/mysql.py
Outdated
@@ -243,6 +243,10 @@ class Parser(parser.Parser): | |||
format=exp.Literal.string("%B"), | |||
), | |||
"STR_TO_DATE": _str_to_date, | |||
"TIMESTAMP": lambda args: exp.cast( | |||
args[0], |
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.
note: didn't use seq_get
because it returns an optional type, and cast doesn't accept an optional
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.
you could avoid the cast then
exp.Cast(this=seq_get(), ...)
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 changed it up a bit after reading mysql docs
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.
why is this needed?
trying to transpile |
wouldn't it make more sense to have presto generate the 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.
let's do this on generation
switched to generation. I'm a little scared because TIMESTAMP is also a type |
bigquery also has timestamp func |
Hm, yeah. Seems like current implementation will still work, although we're overloading the |
cc @ginter