-
Notifications
You must be signed in to change notification settings - Fork 754
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(presto): transpile explode/posexplode into (cross join) unnest #1501
Conversation
Ah you're fast, I was hoping to get some open source points here 😄 |
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.
Really nice! Thanks for adding this. Honestly a bit bummed to not get this PR myself, but guess I was too slow. The code looks better than what I did in harmonize though!
@@ -4713,6 +4713,8 @@ def alias_( | |||
|
|||
if table: | |||
table_alias = TableAlias(this=alias) | |||
|
|||
exp = exp.copy() |
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.
only copy if expression is an expression, if it's a string, you don't need this
After tobymao/sqlglot#1501 was merged and released, the transform we're doing is almost redundant. For some reason, it still fails on Postgres, so we can't remove our transform just yet, will make sure that's fixed upstream.
Follow up to #43. After tobymao/sqlglot#1501, we can now fully remove our version of the explode to unnest transform! 🎉 The previous PR didn't really do anything, I just didn't realize we had to import the preprocess functions that the Presto dialect uses, and put these in our DuneSQL dialect's preprocess functions.
Fixes #1495
cc: @vegarsti I included the test cases you've added to the harmonizer repo, adapted appropriately.