john-bodley opened a new pull request #9065: [sqla] Fixing ORDER BY logic URL: https://github.com/apache/incubator-superset/pull/9065 ### CATEGORY Choose one - [x] Bug Fix - [ ] Enhancement (new features, refinement) - [ ] Refactor - [ ] Add tests - [ ] Build / Development Environment - [ ] Documentation ### SUMMARY When upgrading Presto from v0.188 to v0.224 we noticed an issue with the `ORDER BY` logic due to ambiguity which had changed, i.e., per [here](https://support.treasuredata.com/hc/en-us/articles/360008414094-Presto-0-188-to-0-205-Migration): > The general rules for ORDER BY behavior are now: > > - ORDER BY is executed after projection > - ORDER BY prefers an ordinal alias wherever possible > - Input and output columns can be used in the ORDER BY clause, but if there is ambiguity, the output columns have higher precedence Though it's up to the individual SQLAlchemy dialect to handle how it translates a SQLAlchemy statement to a string we need to ensure that the SQLAlchemy statement is structured correctly. i.e., the `ORDER BY` clause references the same objects as the SELECT clause where appropriate. Here's a basic toy example, ```python from sqlalchemy.sql import literal_column, select a = literal_column("SUM(x)").label("total") b = literal_column("SUM(x)").label("total") print(str(select([a]).order_by(b))) print(str(select([a]).order_by(a))) ``` which prints, ``` SELECT SUM(x) AS total ORDER BY SUM(x) SELECT SUM(x) AS total ORDER BY total ``` the former doesn't use the label (output column) whereas the later does given it's referencing the same object (even though both objects are equivalent). For the table viz type we add the `SORT BY` to the list of metrics (this is probably a requirement for some dialects) as the `ORDER BY` clause is executed after the projection (`SELECT`) and thus the `ORDER BY` expressions need to be present in the `SELECT` clause. This PR simply ensures that we reference the metric object in the `ORDER BY` clause if defined. ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF <!--- Skip this if not applicable --> ### TEST PLAN <!--- What steps should be taken to verify the changes --> ### ADDITIONAL INFORMATION <!--- Check any relevant boxes with "x" --> <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue --> - [ ] Has associated issue: - [ ] Changes UI - [ ] Requires DB Migration. - [ ] Confirm DB Migration upgrade and downgrade tested. - [ ] Introduces new feature or API - [ ] Removes existing feature or API ### REVIEWERS to: @betodealmeida @michellethomas @mistercrunch @villebro
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
