fix: use expected label in metrics map #15707

zhaoyongjie · 2021-07-15T11:42:18Z

SUMMARY

ensure the key of metrics expression hashmap and the expected key are consistent

closes: #15693

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

After

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue: [chart] Chart that's using a virtual dataset with an orderby breaks #15693
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

zhaoyongjie · 2021-07-15T11:43:57Z

superset/connectors/sqla/models.py

@@ -1086,7 +1086,7 @@ def get_sqla_query(  # pylint: disable=too-many-arguments,too-many-locals,too-ma

        # To ensure correct handling of the ORDER BY labeling we need to reference the
        # metric instance if defined in the SELECT clause.
-        metrics_exprs_by_label = {m.name: m for m in metrics_exprs}
+        metrics_exprs_by_label = {m.key: m for m in metrics_exprs}


refer to:

superset/superset/connectors/sqla/models.py

Lines 938 to 943 in b489cff

if db_engine_spec.allows_alias_in_select:

label = db_engine_spec.make_label_compatible(label_expected)

sqla_col = sqla_col.label(label)

sqla_col.key = label_expected

return sqla_col

I'd like to have more documentation in this code. It's a critical piece of Superset that is currently quite cryptic for the un-initiated. Would you mind adding a comment at line 942 describing what the purpose is of key, and also here specifying why we use the key attribute?

Thanks for the review! I have added comments here. db_engine_spec of Bigquery changed metric_label.

superset/superset/db_engine_specs/bigquery.py

Line 208 in 0ed97eb

def _mutate_label(label: str) -> str:

codecov · 2021-07-15T11:59:32Z

Codecov Report

Merging #15707 (15547e0) into master (b489cff) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #15707      +/-   ##
==========================================
+ Coverage   76.82%   76.91%   +0.08%     
==========================================
  Files         983      983              
  Lines       51585    51600      +15     
  Branches     6974     6974              
==========================================
+ Hits        39632    39686      +54     
+ Misses      11729    11690      -39     
  Partials      224      224

Flag	Coverage Δ
hive	`81.28% <100.00%> (+<0.01%)`	⬆️
mysql	`81.54% <100.00%> (+<0.01%)`	⬆️
postgres	`81.56% <100.00%> (+<0.01%)`	⬆️
presto	`81.28% <100.00%> (?)`
python	`82.10% <100.00%> (+0.15%)`	⬆️
sqlite	`81.18% <100.00%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
superset/connectors/sqla/models.py	`89.90% <100.00%> (+1.40%)`	⬆️
superset/utils/mock_data.py	`24.81% <0.00%> (-0.37%)`	⬇️
superset/reports/api.py	`87.78% <0.00%> (ø)`
superset/models/reports.py	`100.00% <0.00%> (ø)`
superset/config.py	`91.24% <0.00%> (+0.02%)`	⬆️
superset/reports/schemas.py	`98.71% <0.00%> (+0.06%)`	⬆️
superset/models/core.py	`90.05% <0.00%> (+0.26%)`	⬆️
superset/utils/webdriver.py	`79.48% <0.00%> (+0.82%)`	⬆️
superset/db_engine_specs/presto.py	`90.31% <0.00%> (+5.89%)`	⬆️

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 b489cff...15547e0. Read the comment docs.

suddjian · 2021-07-15T17:30:13Z

/testenv up

github-actions · 2021-07-15T17:32:12Z

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

suddjian

Tested, and investigated the code. All looks good as far as I can tell, but my experience in this part of the codebase is limited.

github-actions · 2021-07-16T06:02:25Z

Ephemeral environment shutdown and build artifacts deleted.

rosemarie-chiu · 2021-07-16T19:01:29Z

🏷 2021.27

* fix: use expected label in metrics map * added comments * fix type (cherry picked from commit 0721f54)

* fix: use expected label in metrics map * added comments * fix type

fix: use expected label in metrics map

04e0dcc

zhaoyongjie requested review from betodealmeida and suddjian July 15, 2021 11:42

zhaoyongjie commented Jul 15, 2021

View reviewed changes

suddjian approved these changes Jul 15, 2021

View reviewed changes

zhaoyongjie added 2 commits July 16, 2021 12:00

added comments

ed93512

fix type

15547e0

zhaoyongjie merged commit 0721f54 into apache:master Jul 16, 2021

superset-github-bot bot added the preset:2021.27 label Jul 16, 2021

junlincc added rush! Requires immediate attention #bug:blocking! Blocking issues with high priority labels Jul 16, 2021

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

fix: use expected label in the metrics map (apache#15707)

65a4504

* fix: use expected label in metrics map * added comments * fix type (cherry picked from commit 0721f54)

cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021

fix: use expected label in the metrics map (apache#15707)

663270d

* fix: use expected label in metrics map * added comments * fix type

QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021

fix: use expected label in the metrics map (apache#15707)

bffa533

* fix: use expected label in metrics map * added comments * fix type

cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024

fix: use expected label in the metrics map (apache#15707)

ea6e426

* fix: use expected label in metrics map * added comments * fix type

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: use expected label in metrics map #15707

fix: use expected label in metrics map #15707

zhaoyongjie commented Jul 15, 2021 •

edited

Loading

zhaoyongjie Jul 15, 2021

suddjian Jul 15, 2021 •

edited

Loading

zhaoyongjie Jul 16, 2021

codecov bot commented Jul 15, 2021 •

edited

Loading

suddjian commented Jul 15, 2021

github-actions bot commented Jul 15, 2021

suddjian left a comment

github-actions bot commented Jul 16, 2021

rosemarie-chiu commented Jul 16, 2021

	if db_engine_spec.allows_alias_in_select:
	label = db_engine_spec.make_label_compatible(label_expected)
	sqla_col = sqla_col.label(label)

	sqla_col.key = label_expected
	return sqla_col

fix: use expected label in metrics map #15707

fix: use expected label in metrics map #15707

Conversation

zhaoyongjie commented Jul 15, 2021 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

After

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

zhaoyongjie Jul 15, 2021

Choose a reason for hiding this comment

suddjian Jul 15, 2021 • edited Loading

Choose a reason for hiding this comment

zhaoyongjie Jul 16, 2021

Choose a reason for hiding this comment

codecov bot commented Jul 15, 2021 • edited Loading

Codecov Report

suddjian commented Jul 15, 2021

github-actions bot commented Jul 15, 2021

suddjian left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 16, 2021

rosemarie-chiu commented Jul 16, 2021

zhaoyongjie commented Jul 15, 2021 •

edited

Loading

suddjian Jul 15, 2021 •

edited

Loading

codecov bot commented Jul 15, 2021 •

edited

Loading