Skip to content

Conversation

@ktmud
Copy link
Member

@ktmud ktmud commented Mar 5, 2021

SUMMARY

Closes #13423

#13057 Added sortby metrics for some multi-series charts but it causes duplicate metrics and wrong queries.

This is a bandaid PR to quickly fix the issue from the surface. I'm making the handling of sort by metrics more robust in #13434 .

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

Snip20210304_25
Snip20210304_26

After

Snip20210304_28

Snip20210304_27

TEST PLAN

  1. Go to a Area chart
  2. Add a metric and a sort by metric of the same name

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

@codecov
Copy link

codecov bot commented Mar 5, 2021

Codecov Report

Merging #13473 (7f80b8a) into master (9fc03f0) will decrease coverage by 5.84%.
The diff coverage is 10.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13473      +/-   ##
==========================================
- Coverage   77.44%   71.60%   -5.85%     
==========================================
  Files         906      804     -102     
  Lines       45721    40629    -5092     
  Branches     5449     4159    -1290     
==========================================
- Hits        35410    29093    -6317     
- Misses      10184    11536    +1352     
+ Partials      127        0     -127     
Flag Coverage Δ
cypress 57.95% <ø> (+0.04%) ⬆️
hive ?
javascript ?
mysql 80.36% <10.00%> (+<0.01%) ⬆️
postgres 80.40% <10.00%> (+<0.01%) ⬆️
presto 80.06% <10.00%> (+<0.01%) ⬆️
python 80.63% <10.00%> (-0.27%) ⬇️
sqlite 80.02% <10.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset/viz.py 56.10% <10.00%> (+0.09%) ⬆️
...uperset-frontend/src/dashboard/util/dnd-reorder.js 0.00% <0.00%> (-100.00%) ⬇️
...rset-frontend/src/dashboard/util/getEmptyLayout.js 0.00% <0.00%> (-100.00%) ⬇️
...et-frontend/src/components/Pagination/Ellipsis.tsx 0.00% <0.00%> (-100.00%) ⬇️
...-frontend/src/components/OmniContainer/Omnibar.tsx 0.00% <0.00%> (-100.00%) ⬇️
...frontend/src/components/ErrorMessage/IssueCode.tsx 0.00% <0.00%> (-100.00%) ⬇️
...rc/components/ErrorMessage/TimeoutErrorMessage.tsx 0.00% <0.00%> (-100.00%) ⬇️
...dashboard/components/resizable/ResizableHandle.jsx 0.00% <0.00%> (-100.00%) ⬇️
...tersConfigModal/Footer/CancelConfirmationAlert.tsx 0.00% <0.00%> (-100.00%) ⬇️
...ConfigModal/FiltersConfigForm/FilterScope/state.ts 0.00% <0.00%> (-100.00%) ⬇️
... and 481 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9fc03f0...3e79fb3. Read the comment docs.

@ktmud ktmud requested review from john-bodley and villebro March 5, 2021 20:06
Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for fixing

@ktmud ktmud merged commit 49eeab6 into apache:master Mar 8, 2021
@ktmud ktmud deleted the no-duplicate-sortby-metric branch March 8, 2021 05:13
@junlincc junlincc added the explore:control Related to the controls panel of Explore label Mar 8, 2021
allanco91 pushed a commit to allanco91/superset that referenced this pull request May 21, 2021
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 First shipped in 1.2.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels explore:control Related to the controls panel of Explore size/S 🚢 1.2.0 First shipped in 1.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Explore]SORT BY metric incorrectly re-added to SELECT clause

4 participants