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 2021/01/22 02:46:24 UTC
[GitHub] [superset] bryanck opened a new pull request #12674: Add implicit orderby to certain queries
bryanck opened a new pull request #12674:
URL: https://github.com/apache/superset/pull/12674
### SUMMARY
This PR adds implicit ordering for the line chart and the word cloud chart. Currently, if a line chart has a group by, the series is limited in number, and there is no sort specified, the series selected will be arbitrary rather than the top series based on the metric. Similarly, the word cloud chart, if limited, will select an arbitrary set of results, rather than the top results based on the metric.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
### TEST PLAN
Create a line chart with group by, limit the series number, and the top N series should be shown. Similarly with the word cloud chart, apply a limit, and the top results should be displayed rather than an arbitrary set.
### ADDITIONAL INFORMATION
- [ ] 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
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov-io edited a comment on pull request #12674: fix: Add implicit orderby to certain queries
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #12674:
URL: https://github.com/apache/superset/pull/12674#issuecomment-765084183
# [Codecov](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=h1) Report
> Merging [#12674](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=desc) (ae10fdd) into [master](https://codecov.io/gh/apache/superset/commit/734fb75219c96abd2a8323a0650f2e93caa054e8?el=desc) (734fb75) will **decrease** coverage by `3.17%`.
> The diff coverage is `75.00%`.
[](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #12674 +/- ##
==========================================
- Coverage 66.85% 63.68% -3.18%
==========================================
Files 1018 486 -532
Lines 49776 29992 -19784
Branches 4869 0 -4869
==========================================
- Hits 33278 19099 -14179
+ Misses 16375 10893 -5482
+ Partials 123 0 -123
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `?` | |
| javascript | `?` | |
| python | `63.68% <75.00%> (-0.33%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/charts/commands/data.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY2hhcnRzL2NvbW1hbmRzL2RhdGEucHk=) | `94.82% <66.66%> (-1.54%)` | :arrow_down: |
| [superset/viz.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdml6LnB5) | `60.06% <80.00%> (+0.06%)` | :arrow_up: |
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `54.61% <0.00%> (-29.24%)` | :arrow_down: |
| [superset/views/database/mixins.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `80.70% <0.00%> (-1.76%)` | :arrow_down: |
| [superset/models/core.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbW9kZWxzL2NvcmUucHk=) | `88.04% <0.00%> (-0.82%)` | :arrow_down: |
| [superset/tasks/schedules.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdGFza3Mvc2NoZWR1bGVzLnB5) | `76.36% <0.00%> (ø)` | |
| [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `81.38% <0.00%> (ø)` | |
| [superset-frontend/src/utils/reducerUtils.js](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3V0aWxzL3JlZHVjZXJVdGlscy5qcw==) | | |
| [superset-frontend/src/views/App.tsx](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0FwcC50c3g=) | | |
| ... and [531 more](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/12674?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/superset/pull/12674?src=pr&el=footer). Last update [734fb75...ae10fdd](https://codecov.io/gh/apache/superset/pull/12674?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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov-io edited a comment on pull request #12674: fix: Add implicit orderby to certain queries
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #12674:
URL: https://github.com/apache/superset/pull/12674#issuecomment-765084183
# [Codecov](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=h1) Report
> Merging [#12674](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=desc) (ae10fdd) into [master](https://codecov.io/gh/apache/superset/commit/734fb75219c96abd2a8323a0650f2e93caa054e8?el=desc) (734fb75) will **decrease** coverage by `0.20%`.
> The diff coverage is `75.00%`.
[](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #12674 +/- ##
==========================================
- Coverage 66.85% 66.64% -0.21%
==========================================
Files 1018 1018
Lines 49776 49800 +24
Branches 4869 4877 +8
==========================================
- Hits 33278 33191 -87
- Misses 16375 16482 +107
- Partials 123 127 +4
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `50.58% <ø> (-0.38%)` | :arrow_down: |
| javascript | `60.94% <ø> (+0.01%)` | :arrow_up: |
| python | `63.80% <75.00%> (-0.21%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/charts/commands/data.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY2hhcnRzL2NvbW1hbmRzL2RhdGEucHk=) | `94.82% <66.66%> (-1.54%)` | :arrow_down: |
| [superset/viz.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdml6LnB5) | `60.06% <80.00%> (+0.06%)` | :arrow_up: |
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `54.61% <0.00%> (-29.24%)` | :arrow_down: |
| [...et-frontend/src/SqlLab/reducers/getInitialState.js](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9yZWR1Y2Vycy9nZXRJbml0aWFsU3RhdGUuanM=) | `33.33% <0.00%> (-16.67%)` | :arrow_down: |
| [superset-frontend/src/reduxUtils.ts](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3JlZHV4VXRpbHMudHM=) | `70.88% <0.00%> (-8.87%)` | :arrow_down: |
| [...rontend/src/SqlLab/components/TabbedSqlEditors.jsx](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RhYmJlZFNxbEVkaXRvcnMuanN4) | `76.58% <0.00%> (-5.07%)` | :arrow_down: |
| [superset-frontend/src/SqlLab/actions/sqlLab.js](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9hY3Rpb25zL3NxbExhYi5qcw==) | `58.59% <0.00%> (-3.83%)` | :arrow_down: |
| [superset-frontend/src/SqlLab/reducers/sqlLab.js](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9yZWR1Y2Vycy9zcWxMYWIuanM=) | `38.42% <0.00%> (-1.66%)` | :arrow_down: |
| [superset-frontend/src/CRUD/CollectionTable.tsx](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL0NSVUQvQ29sbGVjdGlvblRhYmxlLnRzeA==) | `61.60% <0.00%> (-1.01%)` | :arrow_down: |
| ... and [11 more](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/12674?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/superset/pull/12674?src=pr&el=footer). Last update [734fb75...ae10fdd](https://codecov.io/gh/apache/superset/pull/12674?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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a change in pull request #12674: fix: Add implicit orderby to certain queries
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #12674:
URL: https://github.com/apache/superset/pull/12674#discussion_r563076967
##########
File path: superset/charts/commands/data.py
##########
@@ -82,6 +82,10 @@ def set_query_context(self, form_data: Dict[str, Any]) -> QueryContext:
except ValidationError as error:
raise error
+ for query in self._query_context.queries:
+ if query.row_limit and query.metrics and not query.orderby:
+ query.orderby = [(query.metrics[0], False)]
Review comment:
While I understand the use case and agree with it from an end user perspective, always adding orderbys to queries can have a significant performance impact on queries referencing large tables. Going forward we want to make less implicit assumptions about ordering and let the viz' explicitly request ordering if it's needed. Therefore I feel this logic should be moved to the individual plugins rather than apply it on all queries.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov-io edited a comment on pull request #12674: fix: Add implicit orderby to certain queries
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #12674:
URL: https://github.com/apache/superset/pull/12674#issuecomment-765084183
# [Codecov](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=h1) Report
> Merging [#12674](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=desc) (f897088) into [master](https://codecov.io/gh/apache/superset/commit/734fb75219c96abd2a8323a0650f2e93caa054e8?el=desc) (734fb75) will **decrease** coverage by `0.28%`.
> The diff coverage is `75.00%`.
[](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #12674 +/- ##
==========================================
- Coverage 66.85% 66.57% -0.29%
==========================================
Files 1018 1018
Lines 49776 49800 +24
Branches 4869 4877 +8
==========================================
- Hits 33278 33154 -124
- Misses 16375 16519 +144
- Partials 123 127 +4
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `50.58% <ø> (-0.38%)` | :arrow_down: |
| javascript | `60.94% <ø> (+0.01%)` | :arrow_up: |
| python | `63.68% <75.00%> (-0.33%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/charts/commands/data.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY2hhcnRzL2NvbW1hbmRzL2RhdGEucHk=) | `94.82% <66.66%> (-1.54%)` | :arrow_down: |
| [superset/viz.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdml6LnB5) | `60.06% <80.00%> (+0.06%)` | :arrow_up: |
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `54.61% <0.00%> (-29.24%)` | :arrow_down: |
| [...et-frontend/src/SqlLab/reducers/getInitialState.js](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9yZWR1Y2Vycy9nZXRJbml0aWFsU3RhdGUuanM=) | `33.33% <0.00%> (-16.67%)` | :arrow_down: |
| [superset-frontend/src/reduxUtils.ts](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3JlZHV4VXRpbHMudHM=) | `70.88% <0.00%> (-8.87%)` | :arrow_down: |
| [...rontend/src/SqlLab/components/TabbedSqlEditors.jsx](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RhYmJlZFNxbEVkaXRvcnMuanN4) | `76.58% <0.00%> (-5.07%)` | :arrow_down: |
| [superset-frontend/src/SqlLab/actions/sqlLab.js](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9hY3Rpb25zL3NxbExhYi5qcw==) | `58.59% <0.00%> (-3.83%)` | :arrow_down: |
| [superset/views/database/mixins.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `80.70% <0.00%> (-1.76%)` | :arrow_down: |
| [superset-frontend/src/SqlLab/reducers/sqlLab.js](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9yZWR1Y2Vycy9zcWxMYWIuanM=) | `38.42% <0.00%> (-1.66%)` | :arrow_down: |
| ... and [13 more](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/12674?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/superset/pull/12674?src=pr&el=footer). Last update [734fb75...ae10fdd](https://codecov.io/gh/apache/superset/pull/12674?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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov-io edited a comment on pull request #12674: fix: Add implicit orderby to certain queries
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #12674:
URL: https://github.com/apache/superset/pull/12674#issuecomment-765084183
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a change in pull request #12674: fix: Add implicit orderby to certain queries
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #12674:
URL: https://github.com/apache/superset/pull/12674#discussion_r563193676
##########
File path: superset/charts/commands/data.py
##########
@@ -82,6 +82,10 @@ def set_query_context(self, form_data: Dict[str, Any]) -> QueryContext:
except ValidationError as error:
raise error
+ for query in self._query_context.queries:
+ if query.row_limit and query.metrics and not query.orderby:
+ query.orderby = [(query.metrics[0], False)]
Review comment:
IIRC, word cloud was using the legacy API pre 0.37. Maybe that's why?
I added a [“sort by metric” option](https://github.com/apache-superset/superset-ui/pull/831) for Sankey not long ago (disabled by default), maybe you can take a similar 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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] bryanck commented on a change in pull request #12674: fix: Add implicit orderby to certain queries
Posted by GitBox <gi...@apache.org>.
bryanck commented on a change in pull request #12674:
URL: https://github.com/apache/superset/pull/12674#discussion_r563179826
##########
File path: superset/charts/commands/data.py
##########
@@ -82,6 +82,10 @@ def set_query_context(self, form_data: Dict[str, Any]) -> QueryContext:
except ValidationError as error:
raise error
+ for query in self._query_context.queries:
+ if query.row_limit and query.metrics and not query.orderby:
+ query.orderby = [(query.metrics[0], False)]
Review comment:
If no one else has a problem with this, you can go ahead and close it and we'll patch it on our side.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov-io edited a comment on pull request #12674: fix: Add implicit orderby to certain queries
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #12674:
URL: https://github.com/apache/superset/pull/12674#issuecomment-765084183
# [Codecov](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=h1) Report
> Merging [#12674](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=desc) (ae10fdd) into [master](https://codecov.io/gh/apache/superset/commit/734fb75219c96abd2a8323a0650f2e93caa054e8?el=desc) (734fb75) will **decrease** coverage by `4.26%`.
> The diff coverage is `75.00%`.
[](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #12674 +/- ##
==========================================
- Coverage 66.85% 62.59% -4.27%
==========================================
Files 1018 1018
Lines 49776 49781 +5
Branches 4869 4877 +8
==========================================
- Hits 33278 31160 -2118
- Misses 16375 18423 +2048
- Partials 123 198 +75
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `?` | |
| javascript | `60.94% <ø> (+0.01%)` | :arrow_up: |
| python | `63.68% <75.00%> (-0.33%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/charts/commands/data.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY2hhcnRzL2NvbW1hbmRzL2RhdGEucHk=) | `94.82% <66.66%> (-1.54%)` | :arrow_down: |
| [superset/viz.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdml6LnB5) | `60.06% <80.00%> (+0.06%)` | :arrow_up: |
| [superset-frontend/src/SqlLab/App.jsx](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9BcHAuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset-frontend/src/explore/App.jsx](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvQXBwLmpzeA==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset-frontend/src/dashboard/App.jsx](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9BcHAuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset-frontend/src/chart/ChartContainer.jsx](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0Q29udGFpbmVyLmpzeA==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset-frontend/src/explore/reducers/index.js](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvcmVkdWNlcnMvaW5kZXguanM=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...et-frontend/src/dashboard/containers/Dashboard.jsx](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb250YWluZXJzL0Rhc2hib2FyZC5qc3g=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...et-frontend/src/filters/components/Select/types.ts](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9TZWxlY3QvdHlwZXMudHM=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...t-frontend/src/dashboard/containers/SliceAdder.jsx](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb250YWluZXJzL1NsaWNlQWRkZXIuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [202 more](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/12674?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/superset/pull/12674?src=pr&el=footer). Last update [734fb75...ae10fdd](https://codecov.io/gh/apache/superset/pull/12674?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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov-io commented on pull request #12674: fix: Add implicit orderby to certain queries
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #12674:
URL: https://github.com/apache/superset/pull/12674#issuecomment-765084183
# [Codecov](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=h1) Report
> Merging [#12674](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=desc) (f897088) into [master](https://codecov.io/gh/apache/superset/commit/734fb75219c96abd2a8323a0650f2e93caa054e8?el=desc) (734fb75) will **decrease** coverage by `3.53%`.
> The diff coverage is `75.00%`.
[](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #12674 +/- ##
==========================================
- Coverage 66.85% 63.32% -3.54%
==========================================
Files 1018 486 -532
Lines 49776 29977 -19799
Branches 4869 0 -4869
==========================================
- Hits 33278 18983 -14295
+ Misses 16375 10994 -5381
+ Partials 123 0 -123
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `?` | |
| javascript | `?` | |
| python | `63.32% <75.00%> (-0.69%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/charts/commands/data.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY2hhcnRzL2NvbW1hbmRzL2RhdGEucHk=) | `94.82% <66.66%> (-1.54%)` | :arrow_down: |
| [superset/viz.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdml6LnB5) | `60.06% <80.00%> (+0.06%)` | :arrow_up: |
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| [superset/databases/commands/create.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2NvbW1hbmRzL2NyZWF0ZS5weQ==) | `32.65% <0.00%> (-59.19%)` | :arrow_down: |
| [superset/sql\_validators/postgres.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvc3FsX3ZhbGlkYXRvcnMvcG9zdGdyZXMucHk=) | `50.00% <0.00%> (-50.00%)` | :arrow_down: |
| [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `54.61% <0.00%> (-29.24%)` | :arrow_down: |
| [superset/views/database/mixins.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `59.64% <0.00%> (-22.81%)` | :arrow_down: |
| [superset/databases/commands/update.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2NvbW1hbmRzL3VwZGF0ZS5weQ==) | `85.71% <0.00%> (-8.17%)` | :arrow_down: |
| [superset/databases/schemas.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWJhc2VzL3NjaGVtYXMucHk=) | `93.40% <0.00%> (-6.05%)` | :arrow_down: |
| [superset/databases/dao.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2Rhby5weQ==) | `94.11% <0.00%> (-5.89%)` | :arrow_down: |
| ... and [556 more](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/12674?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/superset/pull/12674?src=pr&el=footer). Last update [734fb75...ae10fdd](https://codecov.io/gh/apache/superset/pull/12674?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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov-io edited a comment on pull request #12674: fix: Add implicit orderby to certain queries
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #12674:
URL: https://github.com/apache/superset/pull/12674#issuecomment-765084183
# [Codecov](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=h1) Report
> Merging [#12674](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=desc) (f897088) into [master](https://codecov.io/gh/apache/superset/commit/734fb75219c96abd2a8323a0650f2e93caa054e8?el=desc) (734fb75) will **decrease** coverage by `0.19%`.
> The diff coverage is `75.00%`.
[](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #12674 +/- ##
==========================================
- Coverage 66.85% 66.66% -0.20%
==========================================
Files 1018 1018
Lines 49776 49800 +24
Branches 4869 4877 +8
==========================================
- Hits 33278 33197 -81
- Misses 16375 16480 +105
Partials 123 123
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `50.95% <ø> (-0.01%)` | :arrow_down: |
| javascript | `60.94% <ø> (+0.01%)` | :arrow_up: |
| python | `63.68% <75.00%> (-0.33%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/charts/commands/data.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY2hhcnRzL2NvbW1hbmRzL2RhdGEucHk=) | `94.82% <66.66%> (-1.54%)` | :arrow_down: |
| [superset/viz.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdml6LnB5) | `60.06% <80.00%> (+0.06%)` | :arrow_up: |
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `54.61% <0.00%> (-29.24%)` | :arrow_down: |
| [superset/views/database/mixins.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `80.70% <0.00%> (-1.76%)` | :arrow_down: |
| [...set-frontend/src/dashboard/util/getDropPosition.js](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldERyb3BQb3NpdGlvbi5qcw==) | `92.06% <0.00%> (-1.59%)` | :arrow_down: |
| [superset-frontend/src/CRUD/CollectionTable.tsx](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL0NSVUQvQ29sbGVjdGlvblRhYmxlLnRzeA==) | `61.60% <0.00%> (-1.01%)` | :arrow_down: |
| [superset/models/core.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbW9kZWxzL2NvcmUucHk=) | `88.04% <0.00%> (-0.82%)` | :arrow_down: |
| [...erset-frontend/src/datasource/DatasourceEditor.jsx](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2RhdGFzb3VyY2UvRGF0YXNvdXJjZUVkaXRvci5qc3g=) | `66.95% <0.00%> (-0.15%)` | :arrow_down: |
| [superset/tasks/schedules.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdGFza3Mvc2NoZWR1bGVzLnB5) | `76.36% <0.00%> (ø)` | |
| ... and [8 more](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/12674?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/superset/pull/12674?src=pr&el=footer). Last update [734fb75...ae10fdd](https://codecov.io/gh/apache/superset/pull/12674?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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] bryanck commented on a change in pull request #12674: fix: Add implicit orderby to certain queries
Posted by GitBox <gi...@apache.org>.
bryanck commented on a change in pull request #12674:
URL: https://github.com/apache/superset/pull/12674#discussion_r564055186
##########
File path: superset/charts/commands/data.py
##########
@@ -82,6 +82,10 @@ def set_query_context(self, form_data: Dict[str, Any]) -> QueryContext:
except ValidationError as error:
raise error
+ for query in self._query_context.queries:
+ if query.row_limit and query.metrics and not query.orderby:
+ query.orderby = [(query.metrics[0], False)]
Review comment:
I see. IMHO I feel like the best course of actions is to revert #11153, also revert #12661 by me a few days ago, close this PR, and then open a ticket to move order by to the charts.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov-io edited a comment on pull request #12674: fix: Add implicit orderby to certain queries
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #12674:
URL: https://github.com/apache/superset/pull/12674#issuecomment-765084183
# [Codecov](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=h1) Report
> Merging [#12674](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=desc) (ae10fdd) into [master](https://codecov.io/gh/apache/superset/commit/734fb75219c96abd2a8323a0650f2e93caa054e8?el=desc) (734fb75) will **decrease** coverage by `1.65%`.
> The diff coverage is `75.00%`.
[](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #12674 +/- ##
==========================================
- Coverage 66.85% 65.20% -1.66%
==========================================
Files 1018 1018
Lines 49776 49799 +23
Branches 4869 4877 +8
==========================================
- Hits 33278 32471 -807
- Misses 16375 17177 +802
- Partials 123 151 +28
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `44.52% <ø> (-6.43%)` | :arrow_down: |
| javascript | `60.94% <ø> (+0.01%)` | :arrow_up: |
| python | `63.68% <75.00%> (-0.33%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/charts/commands/data.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY2hhcnRzL2NvbW1hbmRzL2RhdGEucHk=) | `94.82% <66.66%> (-1.54%)` | :arrow_down: |
| [superset/viz.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdml6LnB5) | `60.06% <80.00%> (+0.06%)` | :arrow_up: |
| [superset-frontend/src/SqlLab/App.jsx](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9BcHAuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...t-frontend/src/dashboard/containers/SliceAdder.jsx](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb250YWluZXJzL1NsaWNlQWRkZXIuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| [...c/dashboard/components/dnd/AddSliceDragPreview.jsx](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2RuZC9BZGRTbGljZURyYWdQcmV2aWV3LmpzeA==) | `35.71% <0.00%> (-64.29%)` | :arrow_down: |
| [...et-frontend/src/dashboard/actions/sliceEntities.js](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9hY3Rpb25zL3NsaWNlRW50aXRpZXMuanM=) | `12.90% <0.00%> (-58.07%)` | :arrow_down: |
| [...frontend/src/dashboard/components/AddSliceCard.jsx](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0FkZFNsaWNlQ2FyZC5qc3g=) | `50.00% <0.00%> (-50.00%)` | :arrow_down: |
| [...rc/explore/components/controls/AnnotationLayer.jsx](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9Bbm5vdGF0aW9uTGF5ZXIuanN4) | `2.34% <0.00%> (-48.83%)` | :arrow_down: |
| [...-frontend/src/explore/reducers/saveModalReducer.js](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvcmVkdWNlcnMvc2F2ZU1vZGFsUmVkdWNlci5qcw==) | `26.66% <0.00%> (-46.67%)` | :arrow_down: |
| ... and [91 more](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/12674?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/superset/pull/12674?src=pr&el=footer). Last update [734fb75...ae10fdd](https://codecov.io/gh/apache/superset/pull/12674?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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on pull request #12674: fix: Add implicit orderby to certain queries
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #12674:
URL: https://github.com/apache/superset/pull/12674#issuecomment-779142403
Supersedes by https://github.com/apache/superset/pull/13058 and https://github.com/apache/superset/pull/13057
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] bryanck commented on a change in pull request #12674: fix: Add implicit orderby to certain queries
Posted by GitBox <gi...@apache.org>.
bryanck commented on a change in pull request #12674:
URL: https://github.com/apache/superset/pull/12674#discussion_r563179826
##########
File path: superset/charts/commands/data.py
##########
@@ -82,6 +82,10 @@ def set_query_context(self, form_data: Dict[str, Any]) -> QueryContext:
except ValidationError as error:
raise error
+ for query in self._query_context.queries:
+ if query.row_limit and query.metrics and not query.orderby:
+ query.orderby = [(query.metrics[0], False)]
Review comment:
If no one else has a problem with this, you can go ahead and close it and we'll patch it on our side for now, until the components are updated (I'll look into that).
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov-io commented on pull request #12674: fix: Add implicit orderby to certain queries
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #12674:
URL: https://github.com/apache/superset/pull/12674#issuecomment-765084183
# [Codecov](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=h1) Report
> Merging [#12674](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=desc) (f897088) into [master](https://codecov.io/gh/apache/superset/commit/734fb75219c96abd2a8323a0650f2e93caa054e8?el=desc) (734fb75) will **decrease** coverage by `3.53%`.
> The diff coverage is `75.00%`.
[](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #12674 +/- ##
==========================================
- Coverage 66.85% 63.32% -3.54%
==========================================
Files 1018 486 -532
Lines 49776 29977 -19799
Branches 4869 0 -4869
==========================================
- Hits 33278 18983 -14295
+ Misses 16375 10994 -5381
+ Partials 123 0 -123
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `?` | |
| javascript | `?` | |
| python | `63.32% <75.00%> (-0.69%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/charts/commands/data.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY2hhcnRzL2NvbW1hbmRzL2RhdGEucHk=) | `94.82% <66.66%> (-1.54%)` | :arrow_down: |
| [superset/viz.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdml6LnB5) | `60.06% <80.00%> (+0.06%)` | :arrow_up: |
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| [superset/databases/commands/create.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2NvbW1hbmRzL2NyZWF0ZS5weQ==) | `32.65% <0.00%> (-59.19%)` | :arrow_down: |
| [superset/sql\_validators/postgres.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvc3FsX3ZhbGlkYXRvcnMvcG9zdGdyZXMucHk=) | `50.00% <0.00%> (-50.00%)` | :arrow_down: |
| [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `54.61% <0.00%> (-29.24%)` | :arrow_down: |
| [superset/views/database/mixins.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `59.64% <0.00%> (-22.81%)` | :arrow_down: |
| [superset/databases/commands/update.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2NvbW1hbmRzL3VwZGF0ZS5weQ==) | `85.71% <0.00%> (-8.17%)` | :arrow_down: |
| [superset/databases/schemas.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWJhc2VzL3NjaGVtYXMucHk=) | `93.40% <0.00%> (-6.05%)` | :arrow_down: |
| [superset/databases/dao.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2Rhby5weQ==) | `94.11% <0.00%> (-5.89%)` | :arrow_down: |
| ... and [556 more](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/12674?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/superset/pull/12674?src=pr&el=footer). Last update [734fb75...ae10fdd](https://codecov.io/gh/apache/superset/pull/12674?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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov-io edited a comment on pull request #12674: fix: Add implicit orderby to certain queries
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #12674:
URL: https://github.com/apache/superset/pull/12674#issuecomment-765084183
# [Codecov](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=h1) Report
> Merging [#12674](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=desc) (ae10fdd) into [master](https://codecov.io/gh/apache/superset/commit/734fb75219c96abd2a8323a0650f2e93caa054e8?el=desc) (734fb75) will **decrease** coverage by `3.20%`.
> The diff coverage is `75.00%`.
[](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #12674 +/- ##
==========================================
- Coverage 66.85% 63.64% -3.21%
==========================================
Files 1018 486 -532
Lines 49776 29977 -19799
Branches 4869 0 -4869
==========================================
- Hits 33278 19080 -14198
+ Misses 16375 10897 -5478
+ Partials 123 0 -123
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `?` | |
| javascript | `?` | |
| python | `63.64% <75.00%> (-0.36%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/charts/commands/data.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY2hhcnRzL2NvbW1hbmRzL2RhdGEucHk=) | `94.82% <66.66%> (-1.54%)` | :arrow_down: |
| [superset/viz.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdml6LnB5) | `60.06% <80.00%> (+0.06%)` | :arrow_up: |
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| [superset/sql\_validators/postgres.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvc3FsX3ZhbGlkYXRvcnMvcG9zdGdyZXMucHk=) | `50.00% <0.00%> (-50.00%)` | :arrow_down: |
| [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `54.61% <0.00%> (-29.24%)` | :arrow_down: |
| [superset/db\_engine\_specs/postgres.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3Bvc3RncmVzLnB5) | `93.42% <0.00%> (-2.64%)` | :arrow_down: |
| [superset/views/database/mixins.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `80.70% <0.00%> (-1.76%)` | :arrow_down: |
| [superset/models/core.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbW9kZWxzL2NvcmUucHk=) | `88.04% <0.00%> (-0.82%)` | :arrow_down: |
| [superset/views/base\_api.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvYmFzZV9hcGkucHk=) | `97.68% <0.00%> (-0.47%)` | :arrow_down: |
| [superset/reports/notifications/base.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvcmVwb3J0cy9ub3RpZmljYXRpb25zL2Jhc2UucHk=) | `95.00% <0.00%> (-0.46%)` | :arrow_down: |
| ... and [542 more](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/12674?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/superset/pull/12674?src=pr&el=footer). Last update [734fb75...ae10fdd](https://codecov.io/gh/apache/superset/pull/12674?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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] bryanck commented on a change in pull request #12674: fix: Add implicit orderby to certain queries
Posted by GitBox <gi...@apache.org>.
bryanck commented on a change in pull request #12674:
URL: https://github.com/apache/superset/pull/12674#discussion_r564055186
##########
File path: superset/charts/commands/data.py
##########
@@ -82,6 +82,10 @@ def set_query_context(self, form_data: Dict[str, Any]) -> QueryContext:
except ValidationError as error:
raise error
+ for query in self._query_context.queries:
+ if query.row_limit and query.metrics and not query.orderby:
+ query.orderby = [(query.metrics[0], False)]
Review comment:
I see. IMHO I feel like the best course of actions is to revert #11153, also revert #12661 by me a few days ago, close this PR, and then open a ticket to move order by to the charts.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov-io edited a comment on pull request #12674: fix: Add implicit orderby to certain queries
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #12674:
URL: https://github.com/apache/superset/pull/12674#issuecomment-765084183
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] bryanck commented on a change in pull request #12674: fix: Add implicit orderby to certain queries
Posted by GitBox <gi...@apache.org>.
bryanck commented on a change in pull request #12674:
URL: https://github.com/apache/superset/pull/12674#discussion_r563175027
##########
File path: superset/charts/commands/data.py
##########
@@ -82,6 +82,10 @@ def set_query_context(self, form_data: Dict[str, Any]) -> QueryContext:
except ValidationError as error:
raise error
+ for query in self._query_context.queries:
+ if query.row_limit and query.metrics and not query.orderby:
+ query.orderby = [(query.metrics[0], False)]
Review comment:
That sounds reasonable, however the behavior of returning unordered results in these cases seems to be a regression, as this wasn't happening in v0.37. Perhaps the viz components changed?
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud closed pull request #12674: fix: Add implicit orderby to certain queries
Posted by GitBox <gi...@apache.org>.
ktmud closed pull request #12674:
URL: https://github.com/apache/superset/pull/12674
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov-io edited a comment on pull request #12674: fix: Add implicit orderby to certain queries
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #12674:
URL: https://github.com/apache/superset/pull/12674#issuecomment-765084183
# [Codecov](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=h1) Report
> Merging [#12674](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=desc) (f897088) into [master](https://codecov.io/gh/apache/superset/commit/734fb75219c96abd2a8323a0650f2e93caa054e8?el=desc) (734fb75) will **decrease** coverage by `0.80%`.
> The diff coverage is `75.00%`.
[](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #12674 +/- ##
==========================================
- Coverage 66.85% 66.05% -0.81%
==========================================
Files 1018 1018
Lines 49776 49800 +24
Branches 4869 4877 +8
==========================================
- Hits 33278 32894 -384
- Misses 16375 16777 +402
- Partials 123 129 +6
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `46.82% <ø> (-4.14%)` | :arrow_down: |
| javascript | `60.94% <ø> (+0.01%)` | :arrow_up: |
| python | `63.68% <75.00%> (-0.33%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/charts/commands/data.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY2hhcnRzL2NvbW1hbmRzL2RhdGEucHk=) | `94.82% <66.66%> (-1.54%)` | :arrow_down: |
| [superset/viz.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdml6LnB5) | `60.06% <80.00%> (+0.06%)` | :arrow_up: |
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| [...ontend/src/dashboard/util/serializeFilterScopes.js](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL3NlcmlhbGl6ZUZpbHRlclNjb3Blcy5qcw==) | `40.00% <0.00%> (-60.00%)` | :arrow_down: |
| [...-frontend/src/explore/reducers/saveModalReducer.js](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvcmVkdWNlcnMvc2F2ZU1vZGFsUmVkdWNlci5qcw==) | `26.66% <0.00%> (-46.67%)` | :arrow_down: |
| [...s/controls/DateFilterControl/frame/CommonFrame.tsx](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9EYXRlRmlsdGVyQ29udHJvbC9mcmFtZS9Db21tb25GcmFtZS50c3g=) | `41.66% <0.00%> (-41.67%)` | :arrow_down: |
| [...controls/DateFilterControl/frame/CalendarFrame.tsx](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9EYXRlRmlsdGVyQ29udHJvbC9mcmFtZS9DYWxlbmRhckZyYW1lLnRzeA==) | `46.15% <0.00%> (-38.47%)` | :arrow_down: |
| [...et-frontend/src/dashboard/components/SaveModal.tsx](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL1NhdmVNb2RhbC50c3g=) | `46.15% <0.00%> (-34.62%)` | :arrow_down: |
| [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `54.61% <0.00%> (-29.24%)` | :arrow_down: |
| [...t-frontend/src/explore/actions/saveModalActions.js](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvYWN0aW9ucy9zYXZlTW9kYWxBY3Rpb25zLmpz) | `61.29% <0.00%> (-29.04%)` | :arrow_down: |
| ... and [56 more](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/12674?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/superset/pull/12674?src=pr&el=footer). Last update [734fb75...ae10fdd](https://codecov.io/gh/apache/superset/pull/12674?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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a change in pull request #12674: fix: Add implicit orderby to certain queries
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #12674:
URL: https://github.com/apache/superset/pull/12674#discussion_r563465840
##########
File path: superset/charts/commands/data.py
##########
@@ -82,6 +82,10 @@ def set_query_context(self, form_data: Dict[str, Any]) -> QueryContext:
except ValidationError as error:
raise error
+ for query in self._query_context.queries:
+ if query.row_limit and query.metrics and not query.orderby:
+ query.orderby = [(query.metrics[0], False)]
Review comment:
@bryanck originally this regression was caused by #11153 which removed the implicit orderby from chart data requests. I agree with the fix on a chart-by-chart basis, and definitely think it makes sense on word cloud. The problem with implicitly assuming an orderby clause is that it will cause unnecessary query overhead in cases where it is potentially not required, and sometimes cause unwanted side-effects. One side-effect that was noticed by this implicit ordering was on timeseries charts, where ordering by metric caused "scanning" from top to bottom if the limit was reached (=the observations with the lowest metric values didn't get through). In the case of a timeseries chart it feels more natural to order the results by series, not metrics, so as to make sure the request contains as many full series as possible.
Again I'd urge adding this fix to each chart separately, and also adding a sort control to charts where one might be needed.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a change in pull request #12674: fix: Add implicit orderby to certain queries
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #12674:
URL: https://github.com/apache/superset/pull/12674#discussion_r563465840
##########
File path: superset/charts/commands/data.py
##########
@@ -82,6 +82,10 @@ def set_query_context(self, form_data: Dict[str, Any]) -> QueryContext:
except ValidationError as error:
raise error
+ for query in self._query_context.queries:
+ if query.row_limit and query.metrics and not query.orderby:
+ query.orderby = [(query.metrics[0], False)]
Review comment:
@bryanck originally this regression was caused by #11153 which removed the implicit orderby from chart data requests. I agree with the fix on a chart-by-chart basis, and definitely think it makes sense on word cloud. The problem with implicitly assuming an orderby clause is that it will cause unnecessary query overhead in cases where it is potentially not required, and sometimes cause unwanted side-effects. One side-effect that was noticed by this implicit ordering was on timeseries charts, where ordering by metric caused "scanning" from top to bottom if the limit was reached (=the observations with the lowest metric values didn't get through). In the case of a timeseries chart it feels more natural to order the results by series, not metrics, so as to make sure the request contains as many full series as possible.
Again I'd urge adding this fix to each chart separately, and also adding a sort control to charts where one might be needed.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov-io edited a comment on pull request #12674: fix: Add implicit orderby to certain queries
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #12674:
URL: https://github.com/apache/superset/pull/12674#issuecomment-765084183
# [Codecov](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=h1) Report
> Merging [#12674](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=desc) (ae10fdd) into [master](https://codecov.io/gh/apache/superset/commit/734fb75219c96abd2a8323a0650f2e93caa054e8?el=desc) (734fb75) will **decrease** coverage by `3.17%`.
> The diff coverage is `75.00%`.
[](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #12674 +/- ##
==========================================
- Coverage 66.85% 63.68% -3.18%
==========================================
Files 1018 486 -532
Lines 49776 29992 -19784
Branches 4869 0 -4869
==========================================
- Hits 33278 19099 -14179
+ Misses 16375 10893 -5482
+ Partials 123 0 -123
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `?` | |
| javascript | `?` | |
| python | `63.68% <75.00%> (-0.33%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/12674?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/charts/commands/data.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY2hhcnRzL2NvbW1hbmRzL2RhdGEucHk=) | `94.82% <66.66%> (-1.54%)` | :arrow_down: |
| [superset/viz.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdml6LnB5) | `60.06% <80.00%> (+0.06%)` | :arrow_up: |
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `54.61% <0.00%> (-29.24%)` | :arrow_down: |
| [superset/views/database/mixins.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `80.70% <0.00%> (-1.76%)` | :arrow_down: |
| [superset/models/core.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbW9kZWxzL2NvcmUucHk=) | `88.04% <0.00%> (-0.82%)` | :arrow_down: |
| [superset/tasks/schedules.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdGFza3Mvc2NoZWR1bGVzLnB5) | `76.36% <0.00%> (ø)` | |
| [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `81.38% <0.00%> (ø)` | |
| [...erset-frontend/src/SqlLab/components/ResultSet.tsx](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1Jlc3VsdFNldC50c3g=) | | |
| [...et-frontend/src/dashboard/actions/dashboardInfo.js](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9hY3Rpb25zL2Rhc2hib2FyZEluZm8uanM=) | | |
| ... and [531 more](https://codecov.io/gh/apache/superset/pull/12674/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/12674?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/superset/pull/12674?src=pr&el=footer). Last update [734fb75...ae10fdd](https://codecov.io/gh/apache/superset/pull/12674?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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org