fix(dashboard): Return columns and verbose_map for groupby values of Pivot Table v2 [ID-7] #17287

kgabryje · 2021-10-29T13:46:21Z

SUMMARY

Columns used by Pivot Table v2 were not processed correctly by /dashboard/{id}/datasets endpoint, resulting in chart not displaying custom column labels when added to dashboard. This PR fixes the issue.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

After:

TESTING INSTRUCTIONS

Create a pivot table v2 chart using columns with custom labels
Add the chart to dashboard
Verify that custom labels are used instead of column_names

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

CC @junlincc

…Pivot Table v2

codecov · 2021-10-29T14:09:16Z

Codecov Report

Merging #17287 (6695113) into master (f0c0ef7) will decrease coverage by 0.24%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master   #17287      +/-   ##
==========================================
- Coverage   77.04%   76.80%   -0.25%     
==========================================
  Files        1037     1037              
  Lines       55635    55718      +83     
  Branches     7594     7594              
==========================================
- Hits        42863    42792      -71     
- Misses      12522    12676     +154     
  Partials      250      250

Flag	Coverage Δ
hive	`?`
mysql	`81.90% <75.00%> (-0.07%)`	⬇️
postgres	`81.91% <75.00%> (-0.07%)`	⬇️
presto	`?`
python	`82.00% <75.00%> (-0.49%)`	⬇️
sqlite	`81.59% <75.00%> (-0.07%)`	⬇️

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

Impacted Files	Coverage Δ
superset/models/slice.py	`85.05% <60.00%> (-1.37%)`	⬇️
superset/connectors/base/models.py	`88.36% <100.00%> (+0.11%)`	⬆️
superset/examples/birth_names.py	`71.02% <100.00%> (-2.76%)`	⬇️
superset/db_engines/hive.py	`0.00% <0.00%> (-85.19%)`	⬇️
superset/db_engine_specs/hive.py	`69.76% <0.00%> (-17.06%)`	⬇️
superset/db_engine_specs/presto.py	`83.47% <0.00%> (-6.91%)`	⬇️
superset/views/dashboard/views.py	`68.91% <0.00%> (-4.06%)`	⬇️
superset/datasets/dao.py	`93.70% <0.00%> (-2.80%)`	⬇️
superset/examples/paris.py	`25.00% <0.00%> (-1.93%)`	⬇️
... and 32 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 f0c0ef7...6695113. Read the comment docs.

superset/connectors/base/models.py

zhaoyongjie · 2021-11-05T04:21:53Z

superset/models/slice.py

+ def get_query_context(self) -> Dict[str, Any]:
+ query_context: Dict[str, Any] = {}
+ if not self.query_context:
+ return query_context
+ try:
+ query_context = json.loads(self.query_context)
+ except json.decoder.JSONDecodeError as ex:
+ logger.error("Malformed json in slice's query context", exc_info=True)
+ logger.exception(ex)
+ return query_context
+


We can make this function more generic. like this:

def get_query_context(self) -> QueryContext: if self.query_context: try: return self.query_context(**json.loads(self.query_context)) except json.decoder.JSONDecodeError as ex: logger.error("Malformed json in slice's query context", exc_info=True) logger.exception(ex) return QueryContext( datasource={"type": self.datasource_type, "id": self.datasource_id}, queries=[self.viz.query_obj()],)

@zhaoyongjie on the first return statement I believe we could instantiate the QueryContext. Also, for non v1 charts or in the case there isn't a query context payload, we probably should just return None, as the legacy charts aren't really compatible with QueryContext. Could this work?

def get_query_context(self) -> Optional[QueryContext]: if self.query_context: try: return QueryContext(**json.loads(self.query_context)) except json.decoder.JSONDecodeError as ex: logger.error("Malformed json in slice's query context", exc_info=True) logger.exception(ex) return None

Nice! It makes sense return None in legacy chart.

Thanks for suggestions! I made the change, would appreciate another look 🙂

villebro

Looking good! get_query_context may still need some iterations, would be interested in hearing @zhaoyongjie's thoughts

villebro · 2021-11-05T07:21:05Z

superset/models/slice.py

+ def get_query_context(self) -> Dict[str, Any]:
+ query_context: Dict[str, Any] = {}
+ if not self.query_context:
+ return query_context
+ try:
+ query_context = json.loads(self.query_context)
+ except json.decoder.JSONDecodeError as ex:
+ logger.error("Malformed json in slice's query context", exc_info=True)
+ logger.exception(ex)
+ return query_context
+


@zhaoyongjie on the first return statement I believe we could instantiate the QueryContext. Also, for non v1 charts or in the case there isn't a query context payload, we probably should just return None, as the legacy charts aren't really compatible with QueryContext. Could this work?

def get_query_context(self) -> Optional[QueryContext]: if self.query_context: try: return QueryContext(**json.loads(self.query_context)) except json.decoder.JSONDecodeError as ex: logger.error("Malformed json in slice's query context", exc_info=True) logger.exception(ex) return None

kgabryje · 2021-11-05T13:51:01Z

/testenv up

github-actions · 2021-11-05T13:53:10Z

@kgabryje Ephemeral environment spinning up at http://54.213.130.117:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

villebro

LGTM, solid work @kgabryje ! 🎉

zhaoyongjie

LGTM.

github-actions · 2021-11-05T15:06:12Z

Ephemeral environment shutdown and build artifacts deleted.

sadpandajoe · 2021-11-17T00:36:58Z

🏷️ 2021.46

…Pivot Table v2 [ID-7] (#17287) * fix(dashboard): Return columns and verbose_map for groupby values of Pivot Table v2 * Refactor * Fix test and lint * Fix test * Refactor * Fix lint

kgabryje requested review from zhaoyongjie and villebro October 29, 2021 13:46

pull-request-size bot added the size/XS label Oct 29, 2021

fix(dashboard): Return columns and verbose_map for groupby values of …

eb55f31

…Pivot Table v2

kgabryje force-pushed the fix/pivot-table-colnames branch from d654531 to eb55f31 Compare October 29, 2021 13:47

Refactor

8ff4836

pull-request-size bot added size/M and removed size/XS labels Nov 4, 2021

kgabryje changed the title ~~fix(dashboard): Return columns and verbose_map for groupby values of Pivot Table v2~~ fix(dashboard): Return columns and verbose_map for groupby values of Pivot Table v2 [ID-7] Nov 4, 2021

zhaoyongjie requested changes Nov 4, 2021

View reviewed changes

superset/connectors/base/models.py Outdated Show resolved Hide resolved

zhaoyongjie reviewed Nov 5, 2021

View reviewed changes

villebro reviewed Nov 5, 2021

View reviewed changes

kgabryje added 3 commits November 5, 2021 09:11

Fix test and lint

9b42b69

Fix test

9045548

Refactor

1eb8558

pull-request-size bot added size/L and removed size/M labels Nov 5, 2021

Fix lint

6695113

villebro approved these changes Nov 5, 2021

View reviewed changes

zhaoyongjie self-requested a review November 5, 2021 15:03

zhaoyongjie approved these changes Nov 5, 2021

View reviewed changes

kgabryje merged commit fa51b32 into apache:master Nov 5, 2021

superset-github-bot bot added the preset:2021.46 label Nov 17, 2021

kgabryje added the lts-v1 label Mar 22, 2022

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(dashboard): Return columns and verbose_map for groupby values of Pivot Table v2 [ID-7] #17287

fix(dashboard): Return columns and verbose_map for groupby values of Pivot Table v2 [ID-7] #17287

kgabryje commented Oct 29, 2021

codecov bot commented Oct 29, 2021 •

edited

Loading

zhaoyongjie Nov 5, 2021

villebro Nov 5, 2021

zhaoyongjie Nov 5, 2021

kgabryje Nov 5, 2021

villebro left a comment

villebro Nov 5, 2021

kgabryje commented Nov 5, 2021

github-actions bot commented Nov 5, 2021

villebro left a comment

zhaoyongjie left a comment

github-actions bot commented Nov 5, 2021

sadpandajoe commented Nov 17, 2021

fix(dashboard): Return columns and verbose_map for groupby values of Pivot Table v2 [ID-7] #17287

fix(dashboard): Return columns and verbose_map for groupby values of Pivot Table v2 [ID-7] #17287

Conversation

kgabryje commented Oct 29, 2021

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Oct 29, 2021 • edited Loading

Codecov Report

zhaoyongjie Nov 5, 2021

Choose a reason for hiding this comment

villebro Nov 5, 2021

Choose a reason for hiding this comment

zhaoyongjie Nov 5, 2021

Choose a reason for hiding this comment

kgabryje Nov 5, 2021

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

villebro Nov 5, 2021

Choose a reason for hiding this comment

kgabryje commented Nov 5, 2021

github-actions bot commented Nov 5, 2021

villebro left a comment

Choose a reason for hiding this comment

zhaoyongjie left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 5, 2021

sadpandajoe commented Nov 17, 2021

codecov bot commented Oct 29, 2021 •

edited

Loading