-
Notifications
You must be signed in to change notification settings - Fork 572
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(transpiler): handle different hex behavior for dialects #3463
feat(transpiler): handle different hex behavior for dialects #3463
Conversation
Thanks for the PR @viplazylmht, I'll review more carefully tomorrow |
Co-authored-by: Jo <46752250+georgesittas@users.noreply.github.com>
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.
Thanks for the quick iterations here, @viplazylmht.
PR LGTM, left a few final comments. CC @tobymao or @VaggelisD can you please take a look?
One question I had is what do other dialects do? Does this PR cover all of them? We should be careful not to break existing SQL.
Thanks for the PR! Wouldn't it be preferable to encode the upper/lower as an argument and check that when transpiling to other dialects? E.g. if dialect A defaults to upper, then it will create a node like If there are no other differences between |
Depends on what you're looking to optimize. I think either approach is fine, but I'm leaning towards this one. It allows you to easily instance check Another reason I'd like to avoid this pivot is to avoid scope creep. There's some level of inconsistency already on the multiple-expression-types vs additional-args pattern in the AST, so this seems like a topic we could discuss separately if we want to be consistent in what we do. |
Agreed about the inconsistency part, it would probably be best to solidify those patterns so that they can be enforced independently of the reviewer. In such cases where the "payload" is not that complex and doesn't greatly alter the semantics, I'm more in favor of adding it as an arg, there's more checks associated to it but otherwise you'd also maintain a new node across all parsers/generators, to search for it you'd have to @viplazylmht Can you please mention the dialects that have been covered by this PR? |
Thanks for the review, @georgesittas , @VaggelisD. Currently, this PR cover: bigquery, presto, trino, clickhouse, hive, spark, spark2.
This PR is untested with the remaining dialects, which is supported by SQLGlot. |
Sounds good, thanks for checking. These are the remaining ones:
I'll take this to the finish line |
Fixes #3460
SQLGlot
dialect, to uppercase.