feat(echarts-funnel): Implement % calculation type #26290

kgabryje · 2023-12-16T15:05:53Z

SUMMARY

Add "% Calculation" field to Funnel chart's control panel. Currently, the % of each level of funnel chart is calculated as % of total. This PR allows user to also display % of each level as % of the first step or % of previous step.

Example of calculations:

Value	Percent of total	Calculate from first step	Calculate from previous step
100	57%	100%	100%
50	28%	50%	50%
25	14%	25%	50%

The default value for new charts is "Calculate from first step". For existing charts, a migration was added to keep "Percent of total" as selected option.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Create a funnel chart
Verify that the calculated percents of each step match expected values for each option

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
  Current: 0.21 s
  10+: 0.21 s
  100+: 0.12 s
  1000+: 0.13 s
Introduces new feature or API
Removes existing feature or API

codecov · 2023-12-16T15:11:00Z

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (5e85f5c) 69.18% compared to head (adec022) 69.18%.

Files	Patch %	Lines
.../plugin-chart-echarts/src/Funnel/transformProps.ts	57.14%	3 Missing and 3 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #26290   +/-   ##
=======================================
  Coverage   69.18%   69.18%           
=======================================
  Files        1945     1945           
  Lines       75971    75981   +10     
  Branches     8467     8474    +7     
=======================================
+ Hits        52559    52566    +7     
  Misses      21225    21225           
- Partials     2187     2190    +3

Flag	Coverage Δ
hive	`53.67% <ø> (ø)`
javascript	`56.52% <57.14%> (+<0.01%)`	⬆️
mysql	`78.06% <ø> (-0.03%)`	⬇️
postgres	`78.18% <ø> (ø)`
presto	`53.63% <ø> (ø)`
python	`82.86% <ø> (+<0.01%)`	⬆️
sqlite	`76.83% <ø> (ø)`
unit	`55.79% <ø> (ø)`

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

eschutho

Looks great! Thanks for including the migration timings, too.

villebro

Great improvement! Small theoretical scalability improvement for the migration, other than that LGTM

villebro · 2023-12-20T20:42:15Z

superset-frontend/plugins/plugin-chart-echarts/src/Funnel/transformProps.ts

    const isFiltered =
      filterState.selectedValues && !filterState.selectedValues.includes(name);
+    const firstStepPercent = value / (data[0][metricLabel] as number);
+    const prevStepPercent =
+      index === 0 ? 1 : value / (data[index - 1][metricLabel] as number);


nit: maybe this slightly shortens it, although it's maybe less explicit..

Suggested change

index === 0 ? 1 : value / (data[index - 1][metricLabel] as number);

index ? value / (data[index - 1][metricLabel] as number) : 1;

TBH in case such as this, I think it's better to be explicit that we perform an operation only for the first element of an array, rather than when "index is not falsy"

Yes, very good point. This was mostly a knee-jerk reaction that I often get when I get when I see something that can be shortened with truthy/falsy checking. Carry on 👍

villebro · 2023-12-20T20:44:32Z

...et/migrations/versions/2023-12-15_17-58_06dd9ff00fe8_add_percent_calculation_type_funnel_.py

+    bind = op.get_bind()
+    session = db.Session(bind=bind)
+
+    for slc in session.query(Slice).filter(Slice.viz_type == "funnel"):


Could we use paginated_update here? I'm not expecting people to have 100k funnel charts, but as a principle, I think it's a good idea to always paginate updates when possible.

Good point!

villebro · 2023-12-20T20:44:38Z

...et/migrations/versions/2023-12-15_17-58_06dd9ff00fe8_add_percent_calculation_type_funnel_.py

+    bind = op.get_bind()
+    session = db.Session(bind=bind)
+
+    for slc in session.query(Slice).filter(Slice.viz_type == "funnel"):


villebro

We should remove the explicit commit, as that's already happening in the paginator

villebro · 2023-12-21T22:06:47Z

...et/migrations/versions/2023-12-15_17-58_06dd9ff00fe8_add_percent_calculation_type_funnel_.py

+        if not percent_calculation:
+            params["percent_calculation_type"] = "total"
+            slc.params = json.dumps(params)
+            session.commit()


I don't think we should be committing on the changes (paginated_update should do that for us)

Suggested change

session.commit()

villebro · 2023-12-21T22:06:57Z

...et/migrations/versions/2023-12-15_17-58_06dd9ff00fe8_add_percent_calculation_type_funnel_.py

+        if percent_calculation:
+            del params["percent_calculation_type"]
+            slc.params = json.dumps(params)
+            session.commit()


same here

Suggested change

session.commit()

kgabryje · 2023-12-22T09:46:53Z

We should remove the explicit commit, as that's already happening in the paginator

TIL 🙂 done

kgabryje requested review from betodealmeida, yousoph and geido December 16, 2023 15:05

kgabryje requested a review from a team as a code owner December 16, 2023 15:05

pull-request-size bot added the size/L label Dec 16, 2023

kgabryje requested a review from villebro December 16, 2023 15:06

eschutho approved these changes Dec 20, 2023

View reviewed changes

villebro approved these changes Dec 20, 2023

View reviewed changes

kgabryje added 2 commits December 21, 2023 17:52

feat(echarts-funnel): Implement % calculation type

e54f29d

Use paginated_update

8586275

kgabryje force-pushed the feat/funnel-percent-calc branch from cb5d447 to 8586275 Compare December 21, 2023 16:55

villebro requested changes Dec 21, 2023

View reviewed changes

Remove session.commit()

adec022

villebro approved these changes Dec 22, 2023

View reviewed changes

kgabryje merged commit 5400d30 into apache:master Dec 22, 2023
33 checks passed

sfirke pushed a commit to sfirke/superset that referenced this pull request Mar 22, 2024

feat(echarts-funnel): Implement % calculation type (apache#26290)

15e878a

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

vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024

feat(echarts-funnel): Implement % calculation type (apache#26290)

7179861

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(echarts-funnel): Implement % calculation type #26290

feat(echarts-funnel): Implement % calculation type #26290

kgabryje commented Dec 16, 2023

codecov bot commented Dec 16, 2023 •

edited

Loading

eschutho left a comment

villebro left a comment

villebro Dec 20, 2023

kgabryje Dec 21, 2023

villebro Dec 21, 2023

villebro Dec 20, 2023

michael-s-molina Dec 21, 2023

kgabryje Dec 21, 2023

villebro Dec 20, 2023

villebro left a comment

villebro Dec 21, 2023

villebro Dec 21, 2023

kgabryje commented Dec 22, 2023

	index === 0 ? 1 : value / (data[index - 1][metricLabel] as number);
	index ? value / (data[index - 1][metricLabel] as number) : 1;

feat(echarts-funnel): Implement % calculation type #26290

feat(echarts-funnel): Implement % calculation type #26290

Conversation

kgabryje commented Dec 16, 2023

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Dec 16, 2023 • edited Loading

Codecov Report

eschutho left a comment

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgabryje commented Dec 22, 2023

codecov bot commented Dec 16, 2023 •

edited

Loading