Fix(presto, spark): remove WITHIN GROUP when transpiling percentile_[cont|disc] #1565
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tobymao
reviewed
May 8, 2023
georgesittas
commented
May 8, 2023
| @@ -177,31 +177,34 @@ class Generator(Hive.Generator): | |||
| TRANSFORMS = { | |||
There was a problem hiding this comment.
Added exp.WithinGroup transform, the rest were just sorted in alphabetical order.
| and isinstance(expression.expression, exp.Order) | ||
| ): | ||
| quantile = expression.this.this | ||
| input_value = t.cast(exp.Ordered, expression.find(exp.Ordered)).this |
There was a problem hiding this comment.
The cast was added to suppress a MyPy error. Since expression.expression is an exp.Order, it'll always contain an exp.Ordered in its expressions arg, so this should be safe to use.
Comment on lines
+267
to
+283
| _sql_handler = getattr(self, expression.key + "_sql", None) | ||
| if _sql_handler: | ||
| return _sql_handler(expression) | ||
|
|
||
| transforms_handler = self.TRANSFORMS.get(type(expression)) | ||
| if transforms_handler: | ||
| # Ensures we don't enter an infinite loop. This can happen when the original expression | ||
| # has the same type as the final expression and there's no _sql method available for it, | ||
| # because then it'd re-enter _to_sql. | ||
| if expression_type is type(expression): | ||
| raise ValueError( | ||
| f"Expression type {expression.__class__.__name__} requires a _sql method in order to be transformed." | ||
| ) | ||
|
|
||
| return transforms_handler(self, expression) | ||
|
|
||
| raise ValueError(f"Unsupported expression type {expression.__class__.__name__}.") |
There was a problem hiding this comment.
The motivation behind these changes was that we don't have a _sql method for exp.ApproxQuantile, so the transformation I added would fail at getattr. I think it makes _to_sql a bit more robust.
georgesittas
changed the title
tobymao
approved these changes
May 8, 2023
adrianisk
pushed a commit
to adrianisk/sqlglot
that referenced
this pull request
Jun 21, 2023
…cont|disc] (tobymao#1565) * Fix(presto): remove WITHIN GROUP when transpiling percentile_[cont|disc] * Cleanup * Remove redundant test * Include databricks in the added test
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Example usage:
In Postgres:
In Trino:
From what I can see, there's no exact percentile function in Presto / Trino, hence the different column value. Not sure if we can do something better than what's proposed in this PR.
cc: @vegarsti
References: