feat: explicit distribute columns on BoxPlot and apply time grain #21593

zhaoyongjie · 2022-09-27T05:59:42Z

SUMMARY

This PR intends to improve BoxPlot UX and apply Time Grain to all of the time columns in the Distribute Across control.

Originally, the default time column implicitly apply in the Distribute Across control until there are some columns set on this control. After this PR the default time column will explicitly display in Distribute Across.

Additionally, the Time Grian also move below Distribute Across control and the time grain would apply to all of the time columns.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

After

BoxPlot.mov

TESTING INSTRUCTIONS

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

codecov · 2022-09-27T07:12:39Z

Codecov Report

Merging #21593 (b5b4184) into master (15c3c34) will decrease coverage by 0.00%.
The diff coverage is 58.33%.

❗ Current head b5b4184 differs from pull request most recent head eb4acc6. Consider uploading reports for the commit eb4acc6 to get more accurate results

@@            Coverage Diff             @@
##           master   #21593      +/-   ##
==========================================
- Coverage   66.72%   66.72%   -0.01%     
==========================================
  Files        1796     1797       +1     
  Lines       68731    68751      +20     
  Branches     7316     7323       +7     
==========================================
+ Hits        45864    45877      +13     
- Misses      21007    21013       +6     
- Partials     1860     1861       +1

Flag	Coverage Δ
javascript	`52.98% <58.33%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
...d/packages/superset-ui-chart-controls/src/index.ts	`100.00% <ø> (ø)`
...chart-controls/src/shared-controls/dndControls.tsx	`58.33% <ø> (ø)`
...t-ui-chart-controls/src/shared-controls/mixins.tsx	`33.33% <0.00%> (ø)`
...s/plugin-chart-echarts/src/BoxPlot/controlPanel.ts	`5.55% <0.00%> (-11.12%)`	⬇️
...ins/plugin-chart-echarts/src/BoxPlot/buildQuery.ts	`60.00% <60.00%> (-23.34%)`	⬇️
...d/packages/superset-ui-chart-controls/src/types.ts	`100.00% <100.00%> (ø)`
...-ui-chart-controls/src/utils/getTemporalColumns.ts	`100.00% <100.00%> (ø)`
...packages/superset-ui-core/src/query/types/Query.ts	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

villebro

A few comments, curious to hear your thoughts

superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/constants.tsx

villebro · 2022-09-27T07:48:03Z

superset-frontend/plugins/plugin-chart-echarts/src/BoxPlot/buildQuery.ts

+          if (
+            isPhysicalColumn(col) &&
+            formData.time_grain_sqla &&
+            formData?.datetime_columns_lookup?.[col]
+          ) {
+            return {
+              timeGrain: formData.time_grain_sqla,
+              columnType: 'BASE_AXIS',
+              sqlExpression: col,
+              label: col,
+              expressionType: 'SQL',
+            } as AdhocColumn;
+          }


Do we want to check for the GENERIC_CHART_AXES FF here similar to what we're doing in the other plugins?

Boxplot is a bit special. the Distribute Across is similar to XAxis control in time series charts, but the Distribute Across control is shown even though FF is disabled(the original logic). So the build query logic skips checking FF, which means always transforming the Physical Column into BaseAxisAdhocColumn.

Additionally, the Distribute Across control use the initialValue property to set the initial value as the default time column, so the BoxPlot is "native" to support generic xaxis.

Got it, thanks for the explanation. There may be the special case where a user wants to apply a time grain on "Dimensions" but not "Distribute across" or vice versa. But maybe that can be addressed once time grain is moved into the column control.

villebro

LGTM

villebro · 2022-09-27T11:10:42Z

superset-frontend/plugins/plugin-chart-echarts/src/BoxPlot/buildQuery.ts

+          if (
+            isPhysicalColumn(col) &&
+            formData.time_grain_sqla &&
+            formData?.datetime_columns_lookup?.[col]
+          ) {
+            return {
+              timeGrain: formData.time_grain_sqla,
+              columnType: 'BASE_AXIS',
+              sqlExpression: col,
+              label: col,
+              expressionType: 'SQL',
+            } as AdhocColumn;
+          }


Got it, thanks for the explanation. There may be the special case where a user wants to apply a time grain on "Dimensions" but not "Distribute across" or vice versa. But maybe that can be addressed once time grain is moved into the column control.

pull-request-size bot added the size/L label Sep 27, 2022

zhaoyongjie requested a review from villebro September 27, 2022 06:17

villebro reviewed Sep 27, 2022

View reviewed changes

villebro approved these changes Sep 27, 2022

View reviewed changes

pull-request-size bot added size/XL and removed size/L labels Sep 28, 2022

zhaoyongjie added 9 commits September 28, 2022 09:47

feat: boxplot

42832f7

time grain to boxplot

4996fa6

fix ut

e6ec562

addressing comments

00c6ad9

license

be9282d

constants->mixins

cef3c7f

test coverage

1d6618a

fix UT

db873ab

move UT

432fe35

zhaoyongjie force-pushed the feat_boxplot branch from a190a2b to 432fe35 Compare September 28, 2022 01:48

fix cypress

eb4acc6

zhaoyongjie merged commit 93f08e7 into apache:master Sep 28, 2022

mistercrunch added the 🚢 2.1.3 label Feb 18, 2024

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: explicit distribute columns on BoxPlot and apply time grain #21593

feat: explicit distribute columns on BoxPlot and apply time grain #21593

zhaoyongjie commented Sep 27, 2022 •

edited

Loading

codecov bot commented Sep 27, 2022 •

edited

Loading

villebro left a comment

villebro Sep 27, 2022

zhaoyongjie Sep 27, 2022

villebro Sep 27, 2022

villebro left a comment

villebro Sep 27, 2022

feat: explicit distribute columns on BoxPlot and apply time grain #21593

feat: explicit distribute columns on BoxPlot and apply time grain #21593

Conversation

zhaoyongjie commented Sep 27, 2022 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

After

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Sep 27, 2022 • edited Loading

Codecov Report

villebro left a comment

Choose a reason for hiding this comment

villebro Sep 27, 2022

Choose a reason for hiding this comment

zhaoyongjie Sep 27, 2022

Choose a reason for hiding this comment

villebro Sep 27, 2022

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

villebro Sep 27, 2022

Choose a reason for hiding this comment

zhaoyongjie commented Sep 27, 2022 •

edited

Loading

codecov bot commented Sep 27, 2022 •

edited

Loading