fix(viz): missing groupby and broken adhoc metrics for boxplot #12556

ktmud · 2021-01-15T18:41:52Z

SUMMARY

Fix two bugs in Box Plot Viz:

when select "Series" and "Distributed by", SqalTable generates invalid SQL queries
when applying adhoc metrics, post processing fails:

Not sure when did these bugs occur, but they seem to be related to either #9366 or #12091 .

Anyway, the related logics in the new chart data API were not robust enough. Both issues were actually fixed in #10270 as well, but I'm creating a separate PR here to fix BoxPlot first.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

See summary for BEFORE.

TEST PLAN

Manual verification.

Go to Box Plot
Select both "Series" and "Distributed By"
To test Bug 2, make sure to use an adhoc metric

This is a hotfix. I'm going to create a separate task to add tests.

ADDITIONAL INFORMATION

Has associated issue: see SUMMARY closes [charts] Box Plot query generated wrongly breaking charts #12553
Changes UI
Requires DB Migration.
Confirm DB Migration upgrade and downgrade tested.
Introduces new feature or API
Removes existing feature or API

villebro · 2021-01-15T18:53:31Z

A cleanup PR to fix the root of this problem: apache-superset/superset-ui#901

rusackas · 2021-01-15T19:13:53Z

Tested locally, and it looks fixed to me!

junlincc · 2021-01-15T19:25:04Z

tested LGTM!!

villebro

LGTM

ktmud · 2021-01-15T20:04:10Z

Just added one more commit to force is_timeseries to false for QueryObject in samples mode so time column isn't added to groupby, to fix this bug:

codecov-io · 2021-01-15T20:31:52Z

Codecov Report

Merging #12556 (12b27e7) into master (fc5e311) will increase coverage by 2.85%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #12556      +/-   ##
==========================================
+ Coverage   63.82%   66.67%   +2.85%     
==========================================
  Files         486     1015     +529     
  Lines       29958    49629   +19671     
  Branches        0     4839    +4839     
==========================================
+ Hits        19120    33090   +13970     
- Misses      10838    16416    +5578     
- Partials        0      123     +123

Flag	Coverage Δ
cypress	`50.99% <ø> (?)`
javascript	`60.73% <ø> (?)`
python	`63.82% <100.00%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
superset/common/query_context.py	`82.58% <100.00%> (+0.07%)`	⬆️
superset/common/query_object.py	`91.86% <100.00%> (+0.20%)`	⬆️
superset/connectors/sqla/models.py	`90.58% <100.00%> (ø)`
...et-frontend/src/SqlLab/reducers/getInitialState.js	`50.00% <0.00%> (ø)`
...d/src/views/CRUD/csstemplates/CssTemplatesList.tsx	`78.26% <0.00%> (ø)`
...rc/components/ErrorMessage/TimeoutErrorMessage.tsx	`0.00% <0.00%> (ø)`
...et-frontend/src/dashboard/components/SaveModal.tsx	`80.76% <0.00%> (ø)`
...perset-frontend/src/middleware/loggerMiddleware.js	`95.00% <0.00%> (ø)`
.../src/explore/components/AdhocFilterEditPopover.jsx	`58.00% <0.00%> (ø)`
...nd/src/dashboard/util/isInDifferentFilterScopes.js	`66.66% <0.00%> (ø)`
... and 522 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 fc5e311...12b27e7. Read the comment docs.

* release: bump to 1.0.0 and CHANGELOG * fix(explore): long metric name display (#12387) * fix(explore): long metric name display * add tooltip to control * chore: Show datasets when search input is empty (#12391) * chore: Fix typo “Rest” to “Reset” (#12392) * chore: upgrade eslint, babel, and prettier (#12393) * feat(explore): add tooltip to timepicker label (#12401) * chore: change Datasource to Dataset in Explore ui (#12402) * chore(explore):change dataset to datasource in ui * modal * Add space * Changing it back🤦🏾‍♀️ * Chargeback * fix: Refresh Interval Modal dropdown (#12406) * fix(native-filters): incorrect queriesData state (#12409) * refactor: from superset.utils.core break down date_parser (#12408) * Fixes control panel fields styling (#12236) (#12326) * feat: Resizable dataset and controls panels on Explore view (#12411) * Implement resizable panels on explore view * Optimize chart rendering while resizing * Make dataset column narrower Co-authored-by: Evan Rusackas <evan@preset.io> * fix(dashboard): artefacts shown while drag and dropping deck.gl charts (#12418) * [12181] Fix artifacts while drag and dropping deck.gl charts. * Run prettier * bump superset-ui packages for rolling window change (#12426) * chore: bump superset-ui deckgl plugin (#12466) * fix: do not show vertical scrollbar for charts in dashboard (#12478) * fix: do not show vertical scrollbar for charts in dashboard * Proper fix for #11419 Co-authored-by: Jesse Yang <jesse.yang@airbnb.com> * fix(dashboard): use datasource id from slice metadata (#12483) * fix(timepicker): make pyparsing thread safe (#12489) * fix: make pyparsing thread safe * remove parenthesis for decorator * fix (SQL Lab): disappearing results on tab switch (#12472) * fix (SQL Lab): disappearing results on tab switch * Remove state * Fix test * fix: import ZIP files that have been modified (#12425) * fix: import ZIP files that have been modified * Add unit test * update changelog with rc2 entries * fix: impose dataset ownership check on old API (#12491) * fix: impose dataset ownership check on old API * update UPDATING.md * partially protect the old MVC also * prevent metric and column add and update * ci: remove refs/tags from docker tags on a release (#12518) * ci: remove refs/tags from docker tags on a release * wider head * fix: lowercase all columns in examples (#12530) * fix(explore): time table control panel (#12532) * fix(explore): Add Time section back to FilterBox (#12537) * Fixing Pinot queries for time granularities: WEEKS/MONTHS/QUARTERS/YEARS (#12536) * fix: Select options overflowing Save chart modal on Explore view (#12522) * Fix select options overflowing modal * fix unit test Co-authored-by: Ville Brofeldt <ville.v.brofeldt@gmail.com> * Fix list filters vertical alignment (#12497) * feat(db-engine): Add support for Apache Solr (#12403) * [db engine] Add support for Apache Solr * Fixing typo * chore: rename docker image in build_docker_image.sh, docker-compose.yml and helm values.yaml (#12337) * add rc3 changelog entries * fix: Popover closes on change of dropdowns values (#12410) * fix: Add MAX_SQL_ROW value to LIMIT_DROPDOWN (#12555) * fix(viz): missing groupby and broken adhoc metrics for boxplot (#12556) * fix: height on grid results (#12558) * fix: case expression should not have double quotes (#12562) * Fix 500 error when loading dashboards with slice having deleted dataset (#12535) * add rc4 changelog entries * Fixed typo on line 348 * Added files Co-authored-by: Daniel Gaspar <danielvazgaspar@gmail.com> Co-authored-by: Yongjie Zhao <yongjie.zhao@gmail.com> Co-authored-by: Geido <60598000+geido@users.noreply.github.com> Co-authored-by: Junlin Chen <junlin@preset.io> Co-authored-by: Jesse Yang <jesse.yang@airbnb.com> Co-authored-by: Agata Stawarz <47450693+agatapst@users.noreply.github.com> Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com> Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> Co-authored-by: Kamil Gabryjelski <kamil.gabryjelski@gmail.com> Co-authored-by: Evan Rusackas <evan@preset.io> Co-authored-by: Kasia Kucharczyk <2536609+kkucharc@users.noreply.github.com> Co-authored-by: Phillip Kelley-Dotson <pkelleydotson@yahoo.com> Co-authored-by: Grace Guo <grace.guo@airbnb.com> Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> Co-authored-by: Ville Brofeldt <ville.v.brofeldt@gmail.com> Co-authored-by: Xiang Fu <fx19880617@gmail.com> Co-authored-by: Ahmed Adel <github@aadel.io> Co-authored-by: Amit Miran <47772523+amitmiran137@users.noreply.github.com> Co-authored-by: Hugh A. Miles II <hughmil3s@gmail.com> Co-authored-by: Shuyao Bi <shuyaob@andrew.cmu.edu> Co-authored-by: Lyndsi Kay Williams <lyndsikaywilliams@Lyndsis-MacBook-Pro.local>

…e#12556) (cherry picked from commit deebd6f)

ktmud added 2 commits January 15, 2021 10:38

fix(viz): missing groupby for boxplot with distribute by

f0a0ede

fix bug for adhoc metrics

e4565db

ktmud requested a review from villebro January 15, 2021 18:41

pull-request-size bot added the size/S label Jan 15, 2021

ktmud changed the title ~~fix(viz): missing groupby for boxplot with distribute by~~ fix(viz): missing groupby and broken adhoc metrics for boxplot Jan 15, 2021

junlincc added rush! Requires immediate attention hold:testing! On hold for testing viz:charts:boxplot Related to the Boxplot chart v1.0 labels Jan 15, 2021

rusackas self-requested a review January 15, 2021 18:54

junlincc self-requested a review January 15, 2021 18:54

villebro approved these changes Jan 15, 2021

View reviewed changes

junlincc removed the hold:testing! On hold for testing label Jan 15, 2021

Sample mode should not have timeseries groupby, either

12b27e7

ktmud merged commit deebd6f into apache:master Jan 16, 2021

ktmud deleted the fix-boxplot branch January 16, 2021 01:03

villebro pushed a commit that referenced this pull request Jan 16, 2021

fix(viz): missing groupby and broken adhoc metrics for boxplot (#12556)

5786513

junlincc removed the rush! Requires immediate attention label Jan 26, 2021

henryyeh pushed a commit to preset-io/superset that referenced this pull request Jan 26, 2021

fix(viz): missing groupby and broken adhoc metrics for boxplot (apach…

2396256

…e#12556) (cherry picked from commit deebd6f)

ktmud added the need:tests This PR requires tests label Feb 24, 2021

mistercrunch added 🍒 1.0.0 🍒 1.0.1 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(viz): missing groupby and broken adhoc metrics for boxplot #12556

fix(viz): missing groupby and broken adhoc metrics for boxplot #12556

ktmud commented Jan 15, 2021 •

edited

Loading

villebro commented Jan 15, 2021

rusackas commented Jan 15, 2021

junlincc commented Jan 15, 2021

villebro left a comment

ktmud commented Jan 15, 2021 •

edited

Loading

codecov-io commented Jan 15, 2021 •

edited

Loading

fix(viz): missing groupby and broken adhoc metrics for boxplot #12556

fix(viz): missing groupby and broken adhoc metrics for boxplot #12556

Conversation

ktmud commented Jan 15, 2021 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

villebro commented Jan 15, 2021

rusackas commented Jan 15, 2021

junlincc commented Jan 15, 2021

villebro left a comment

Choose a reason for hiding this comment

ktmud commented Jan 15, 2021 • edited Loading

codecov-io commented Jan 15, 2021 • edited Loading

Codecov Report

ktmud commented Jan 15, 2021 •

edited

Loading

ktmud commented Jan 15, 2021 •

edited

Loading

codecov-io commented Jan 15, 2021 •

edited

Loading