You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/02/01 00:42:50 UTC
[GitHub] [incubator-superset] john-bodley opened a new pull request #9065:
[sqla] Fixing ORDER BY logic
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull
request #9065: [sqla] Fixing ORDER BY logic
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9065: [sqla] Fixing ORDER BY logic
URL: https://github.com/apache/incubator-superset/pull/9065#discussion_r373742102
##########
File path: superset/connectors/sqla/models.py
##########
@@ -146,12 +146,12 @@ class TableColumn(Model, BaseColumn):
def get_sqla_col(self, label: Optional[str] = None) -> Column:
label = label or self.column_name
- if not self.expression:
+ if self.expression:
Review comment:
This logic is unchanged. I just removed the `not` and switched the order.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on issue #9065: [sqla]
Fixing ORDER BY logic
Posted by GitBox <gi...@apache.org>.
villebro commented on issue #9065: [sqla] Fixing ORDER BY logic
URL: https://github.com/apache/incubator-superset/pull/9065#issuecomment-581010978
@john-bodley yes, that would be even DRYer. In addition to considering key column+label properties, I think memoization will probably need to include `db_engine_spec.engine` in the key to ensure engine specific behaviour doesn't leak.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on issue #9065: [sqla]
Fixing ORDER BY logic
Posted by GitBox <gi...@apache.org>.
john-bodley commented on issue #9065: [sqla] Fixing ORDER BY logic
URL: https://github.com/apache/incubator-superset/pull/9065#issuecomment-581050916
@villebro the `db_engine_spec` is bound to the table per `self.table.database.db_engine_spec` and thus I’m unsure whether leaking is an issue, i.e., the column cache will be at the table level.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull
request #9065: [sqla] Fixing ORDER BY logic
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9065: [sqla] Fixing ORDER BY logic
URL: https://github.com/apache/incubator-superset/pull/9065#discussion_r373742361
##########
File path: superset/connectors/sqla/models.py
##########
@@ -841,13 +841,27 @@ def get_sqla_query( # sqla
if not orderby and not columns:
orderby = [(main_metric_expr, not order_desc)]
+ # To ensure correct handling of the ORDER BY labeling we need to reference the
+ # metric instance if defined in the SELECT clause.
+ metrics_exprs_by_name = {str(m): m for m in metrics_exprs}
+
for col, ascending in orderby:
direction = asc if ascending else desc
- if utils.is_adhoc_metric(col):
- col = self.adhoc_metric_to_sqla(col, cols)
- elif col in cols:
- col = cols[col].get_sqla_col()
- qry = qry.order_by(direction(col))
+ sqla_col = None
+
+ if isinstance(col, Column):
+ sqla_col = col
+ else:
+ if utils.is_adhoc_metric(col):
+ sqla_col = self.adhoc_metric_to_sqla(col, cols)
+ elif col in cols:
+ sqla_col = cols[col].get_sqla_col()
+
+ if sqla_col is not None and str(sqla_col) in metrics_exprs_by_name:
Review comment:
Matching of the string representation of a SQLAlchemy column seems kludgy though I'm not certain of a better approach. I initially was going to create an `OrderedDict` of `metrics` to `metric_expr` however the `metric` can be a `dict` which isn't hashable.
I guess using a tuple of `str(...)` and `_label` could be preferred.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on issue #9065: [sqla]
Fixing ORDER BY logic
Posted by GitBox <gi...@apache.org>.
john-bodley commented on issue #9065: [sqla] Fixing ORDER BY logic
URL: https://github.com/apache/incubator-superset/pull/9065#issuecomment-581002955
@villebro great idea. I’ll make the change.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io commented on issue #9065: [sqla]
Fixing ORDER BY logic
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #9065: [sqla] Fixing ORDER BY logic
URL: https://github.com/apache/incubator-superset/pull/9065#issuecomment-580977872
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9065?src=pr&el=h1) Report
> Merging [#9065](https://codecov.io/gh/apache/incubator-superset/pull/9065?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/7364024bba6df87ce2127afc0c7aba60af9ff18b?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9065?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9065 +/- ##
=======================================
Coverage 59.45% 59.45%
=======================================
Files 369 369
Lines 11747 11747
Branches 2888 2888
=======================================
Hits 6984 6984
Misses 4584 4584
Partials 179 179
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9065?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9065?src=pr&el=footer). Last update [7364024...49c8e35](https://codecov.io/gh/apache/incubator-superset/pull/9065?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io edited a comment on issue #9065:
[sqla] Fixing ORDER BY logic
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #9065: [sqla] Fixing ORDER BY logic
URL: https://github.com/apache/incubator-superset/pull/9065#issuecomment-580977872
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9065?src=pr&el=h1) Report
> Merging [#9065](https://codecov.io/gh/apache/incubator-superset/pull/9065?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/7364024bba6df87ce2127afc0c7aba60af9ff18b?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9065?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9065 +/- ##
=======================================
Coverage 59.45% 59.45%
=======================================
Files 369 369
Lines 11747 11747
Branches 2888 2888
=======================================
Hits 6984 6984
Misses 4584 4584
Partials 179 179
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9065?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9065?src=pr&el=footer). Last update [7364024...6577355](https://codecov.io/gh/apache/incubator-superset/pull/9065?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro edited a comment on issue #9065:
[sqla] Fixing ORDER BY logic
Posted by GitBox <gi...@apache.org>.
villebro edited a comment on issue #9065: [sqla] Fixing ORDER BY logic
URL: https://github.com/apache/incubator-superset/pull/9065#issuecomment-581062140
@john-bodley agreed, major rabbit hole risk here, gives me flashbacks of `sqla/models.py` work in late 2018. I think we should do some more thorough refactoring + unit testing this time if we do decide to fix this more permanently. So perhaps just go ahead and merge this and defer the elegant solution to a later date.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on issue #9065: [sqla]
Fixing ORDER BY logic
Posted by GitBox <gi...@apache.org>.
john-bodley commented on issue #9065: [sqla] Fixing ORDER BY logic
URL: https://github.com/apache/incubator-superset/pull/9065#issuecomment-581065275
@villebro agreed. The architecture of the sqla model should probably be refactored at some stage. Also there’s few few unit tests for the entire SQL generation, which we should address if we refactor, given how crucial this logic is.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro edited a comment on issue #9065:
[sqla] Fixing ORDER BY logic
Posted by GitBox <gi...@apache.org>.
villebro edited a comment on issue #9065: [sqla] Fixing ORDER BY logic
URL: https://github.com/apache/incubator-superset/pull/9065#issuecomment-580999623
I wonder if we could instead ensure that the `.get_sqla_col()` methods in `TableColumn` and `SqlMetric` only instantiate each SqlAlchemy object once, and return the first instantiation upon subsequent calls. I believe this could be achieved by adding a `_sqla_col: Optional[Column]` attribute to said classes and return that if defined. This would save us from having to make the downstream code aware of this problem.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on issue #9065: [sqla]
Fixing ORDER BY logic
Posted by GitBox <gi...@apache.org>.
villebro commented on issue #9065: [sqla] Fixing ORDER BY logic
URL: https://github.com/apache/incubator-superset/pull/9065#issuecomment-580999623
I wonder if we could instead ensure that the `.get_sqla_col()` methods in `TableColumn` and `SqlMetric` only instantiate each SqlAlchemy object once, and return the first instantiation upon subsequent calls. I believe this could be achieved by adding a `_sqla_col: Optional[Column]` attribute to said classes and return that if defined. This would save us from having having to make the downstream code aware of this problem.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro edited a comment on issue #9065:
[sqla] Fixing ORDER BY logic
Posted by GitBox <gi...@apache.org>.
villebro edited a comment on issue #9065: [sqla] Fixing ORDER BY logic
URL: https://github.com/apache/incubator-superset/pull/9065#issuecomment-581054986
@john-bodley That's true; if the table id is included in the key this won't be a problem. I think if we have table id, column type (value of `column.is_literal`), value (column name or expression) and label, I think we should be ok.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on issue #9065: [sqla]
Fixing ORDER BY logic
Posted by GitBox <gi...@apache.org>.
john-bodley commented on issue #9065: [sqla] Fixing ORDER BY logic
URL: https://github.com/apache/incubator-superset/pull/9065#issuecomment-581061608
@villebro reading through the code I’m somewhat hesitant to add a class level corpus of columns to the `Table` class especially as this is scoped to the SQL query for a specific query context.
An alternative would be to create a local corpus for each query however this would need to be passed around everywhere. We could make the SQL query it’s own class which stores the corpus, but I’m also concerned that we may be going down a rabbit hole.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull
request #9065: [sqla] Fixing ORDER BY logic
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9065: [sqla] Fixing ORDER BY logic
URL: https://github.com/apache/incubator-superset/pull/9065#discussion_r373742361
##########
File path: superset/connectors/sqla/models.py
##########
@@ -841,13 +841,27 @@ def get_sqla_query( # sqla
if not orderby and not columns:
orderby = [(main_metric_expr, not order_desc)]
+ # To ensure correct handling of the ORDER BY labeling we need to reference the
+ # metric instance if defined in the SELECT clause.
+ metrics_exprs_by_name = {str(m): m for m in metrics_exprs}
+
for col, ascending in orderby:
direction = asc if ascending else desc
- if utils.is_adhoc_metric(col):
- col = self.adhoc_metric_to_sqla(col, cols)
- elif col in cols:
- col = cols[col].get_sqla_col()
- qry = qry.order_by(direction(col))
+ sqla_col = None
+
+ if isinstance(col, Column):
+ sqla_col = col
+ else:
+ if utils.is_adhoc_metric(col):
+ sqla_col = self.adhoc_metric_to_sqla(col, cols)
+ elif col in cols:
+ sqla_col = cols[col].get_sqla_col()
+
+ if sqla_col is not None and str(sqla_col) in metrics_exprs_by_name:
Review comment:
Matching of the string representation of a SQLAlchemy column seems kludgy though I'm not certain of a better approach. I initially was going to create an `OrderedDict` of `metrics` to `metric_expr` however the `metric` can be a `dict` which isn't hashable.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro edited a comment on issue #9065:
[sqla] Fixing ORDER BY logic
Posted by GitBox <gi...@apache.org>.
villebro edited a comment on issue #9065: [sqla] Fixing ORDER BY logic
URL: https://github.com/apache/incubator-superset/pull/9065#issuecomment-581054986
@john-bodley That's true; if the table id is included in the key this won't be a problem. I think if we have table id, column type (value of `sqla_col.is_literal` which is a proxy for groupable column or expression/metric), value (the value of `sqla_col.name`) and alias (the value of `label_expected`), I think we should be ok.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io edited a comment on issue #9065:
[sqla] Fixing ORDER BY logic
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #9065: [sqla] Fixing ORDER BY logic
URL: https://github.com/apache/incubator-superset/pull/9065#issuecomment-580977872
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9065?src=pr&el=h1) Report
> Merging [#9065](https://codecov.io/gh/apache/incubator-superset/pull/9065?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/7364024bba6df87ce2127afc0c7aba60af9ff18b?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9065?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9065 +/- ##
=======================================
Coverage 59.45% 59.45%
=======================================
Files 369 369
Lines 11747 11747
Branches 2888 2888
=======================================
Hits 6984 6984
Misses 4584 4584
Partials 179 179
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9065?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9065?src=pr&el=footer). Last update [7364024...c82d104](https://codecov.io/gh/apache/incubator-superset/pull/9065?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley edited a comment on issue #9065:
[sqla] Fixing ORDER BY logic
Posted by GitBox <gi...@apache.org>.
john-bodley edited a comment on issue #9065: [sqla] Fixing ORDER BY logic
URL: https://github.com/apache/incubator-superset/pull/9065#issuecomment-581002955
@villebro I think that’s the right direction. A couple of caveats; i) the label is optional and thus we probably need different objects depending on the presence of a label, and ii) we also create column from ad-hoc metrics.
I wonder if memoizing `make_sqla_column_compatible` (in essence making a corpus of unique columns) would be the right approach.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull
request #9065: [sqla] Fixing ORDER BY logic
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9065: [sqla] Fixing ORDER BY logic
URL: https://github.com/apache/incubator-superset/pull/9065#discussion_r373742361
##########
File path: superset/connectors/sqla/models.py
##########
@@ -841,13 +841,27 @@ def get_sqla_query( # sqla
if not orderby and not columns:
orderby = [(main_metric_expr, not order_desc)]
+ # To ensure correct handling of the ORDER BY labeling we need to reference the
+ # metric instance if defined in the SELECT clause.
+ metrics_exprs_by_name = {str(m): m for m in metrics_exprs}
+
for col, ascending in orderby:
direction = asc if ascending else desc
- if utils.is_adhoc_metric(col):
- col = self.adhoc_metric_to_sqla(col, cols)
- elif col in cols:
- col = cols[col].get_sqla_col()
- qry = qry.order_by(direction(col))
+ sqla_col = None
+
+ if isinstance(col, Column):
+ sqla_col = col
+ else:
+ if utils.is_adhoc_metric(col):
+ sqla_col = self.adhoc_metric_to_sqla(col, cols)
+ elif col in cols:
+ sqla_col = cols[col].get_sqla_col()
+
+ if sqla_col is not None and str(sqla_col) in metrics_exprs_by_name:
Review comment:
Matching of the string representation of a SQLAlchemy column seems kludgy though I'm not certain of a better approach. I initially was going to create an `OrderedDict` of `metrics` to `metric_expr` however the `metric` can be a `dict` which isn't hashable.
I guess using a tuple of `str(...)` and `_label` could be preferred.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley merged pull request #9065: [sqla]
Fixing ORDER BY logic
Posted by GitBox <gi...@apache.org>.
john-bodley merged pull request #9065: [sqla] Fixing ORDER BY logic
URL: https://github.com/apache/incubator-superset/pull/9065
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io edited a comment on issue #9065:
[sqla] Fixing ORDER BY logic
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #9065: [sqla] Fixing ORDER BY logic
URL: https://github.com/apache/incubator-superset/pull/9065#issuecomment-580977872
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9065?src=pr&el=h1) Report
> Merging [#9065](https://codecov.io/gh/apache/incubator-superset/pull/9065?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/7364024bba6df87ce2127afc0c7aba60af9ff18b?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9065?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9065 +/- ##
=======================================
Coverage 59.45% 59.45%
=======================================
Files 369 369
Lines 11747 11747
Branches 2888 2888
=======================================
Hits 6984 6984
Misses 4584 4584
Partials 179 179
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9065?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9065?src=pr&el=footer). Last update [7364024...c82d104](https://codecov.io/gh/apache/incubator-superset/pull/9065?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro edited a comment on issue #9065:
[sqla] Fixing ORDER BY logic
Posted by GitBox <gi...@apache.org>.
villebro edited a comment on issue #9065: [sqla] Fixing ORDER BY logic
URL: https://github.com/apache/incubator-superset/pull/9065#issuecomment-581054986
@john-bodley That's true; if the table id is included in the key this won't be a problem. I think if we have table id, column type (column or expression), value (column name or expression) and label, I think we should be ok.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on issue #9065: [sqla]
Fixing ORDER BY logic
Posted by GitBox <gi...@apache.org>.
villebro commented on issue #9065: [sqla] Fixing ORDER BY logic
URL: https://github.com/apache/incubator-superset/pull/9065#issuecomment-581062140
@john_bodley agreed, major rabbit hole risk here, gives me flashbacks of `sqla/models.py` work in late 2018. I think we should do some more thorough refactoring + unit testing this time if we do decide to fix this more permanently. So perhaps just go ahead and merge this and defer the elegant solution to a later date.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on issue #9065: [sqla]
Fixing ORDER BY logic
Posted by GitBox <gi...@apache.org>.
villebro commented on issue #9065: [sqla] Fixing ORDER BY logic
URL: https://github.com/apache/incubator-superset/pull/9065#issuecomment-581054986
@john-bodley That's true; if the database id is included in the key this won't be a problem. I think if we have database id, column type (column or expression), value (column name or expression) and label, I think we should be ok.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org