feat(explore): default aggregate for string/numeric columns when creating metric

[kgabryje]

SUMMARY

When user creates a new adhoc metric with drag and dropping a column into metrics control, auto fill default aggregate in metric popover.
If column's generic type is numeric, set SUM aggregate as default.
if column's generic type is string or boolean, set COUNT_DISTINCT aggregate as default.
Otherwise, don't set a default aggregate.

Also, this PR fixes a bug metrics popover - when user created a metric with a custom label and then tried to create next metric, previous label was set instead of a default label. Now it should work as expected.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2021-07-20.at.14.06.18.mov

TESTING INSTRUCTIONS

0. Set ENABLE_EXPLORE_DRAG_AND_DROP and UX_BETA feature flags to True
1. Drag numeric column to metrics control. The default aggregate should be SUM
2. Drag string or boolean column to metrics control. The default aggregate should be COUNT_DISTINCT
3. Drag other type of column to metrics control. Aggregate should be unset.
4. Click Run and make sure the metrics work properly
5. Edit the default label of some metric and then try to add a new one. A default label in the new metric popover should match column's name and selected aggregate. Previously set label shouldn't be shown here

ADDITIONAL INFORMATION

• Has associated issue: fixes [Explore] Metric/Filter Popover should always corresponds to the actual metric/filter item #12364
• 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

[codecov]

Codecov Report

Merging #15798 (6179aed) into master (040b941) will decrease coverage by 0.01%.
The diff coverage is 37.50%.

@@            Coverage Diff             @@
##           master   #15798      +/-   ##
==========================================
- Coverage   76.91%   76.90%   -0.02%     
==========================================
  Files         984      984              
  Lines       51705    51718      +13     
  Branches     6994     7000       +6     
==========================================
+ Hits        39770    39774       +4     
- Misses      11711    11720       +9     
  Partials      224      224              

Flag	Coverage Δ
javascript	71.82% <37.50%> (-0.03%)	⬇️
mysql	81.53% <ø> (ø)
postgres	81.55% <ø> (ø)
python	81.64% <ø> (ø)
sqlite	81.19% <ø> (ø)

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

Impacted Files	Coverage Δ
superset/config.py	91.24% <ø> (ø)
...ontrols/DndColumnSelectControl/DndMetricSelect.tsx	41.48% <16.66%> (-2.97%)	⬇️
...ntrols/MetricControl/AdhocMetricPopoverTrigger.tsx	72.30% <100.00%> (+1.81%)	⬆️

---

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 040b941...6179aed. Read the comment docs.

[zhaoyongjie]

To fill up pre-aggregate function code look good to me.

the bug fix we should take more effect. When two metrics have the same name, I can't change one of the name.

duplicate.metric.mp4

[kgabryje]

the bug fix we should take more effect. When two metrics have the same name, I can't change one of the name.

duplicate.metric.mp4

Fixed, thanks for reporting! Can you take another look and verify?

[michael-s-molina]

Suggested change

	componentDidUpdate(
	prevProps: Readonly<AdhocMetricPopoverTriggerProps>,
	prevState: Readonly<AdhocMetricPopoverTriggerState>,
	snapshot?: any,
	) {
	componentDidUpdate(prevProps: Readonly<AdhocMetricPopoverTriggerProps>) {

Is not the case for getDerivedStateFromProps?

[kgabryje]

Good point, I changed that. That required saving adhocMetric in state though, so that I could compare previous and next in getDerviedStateFromProps. Can you take another look?

[zhaoyongjie]

The code looks great to me.

[kgabryje]

I'll merge once @junlincc gives a green light - we need to decide if we need a feature flag and if we should replace COUNT_DISTINCT with COUNT.

[junlincc]

LGTM. let's put the change behind an umbrella feature flag, e.g. UX_beta and set it to false by default.

[kgabryje]

/testenv up FEATURE_ENABLE_EXPLORE_DRAG_AND_DROP=true

[github-actions]

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

[junlincc]

"I don’t recommend doing count_distinct for string column. This is a precise deduplication operation. It is very resource intensive for the database. imagine, if this column has many values(high cardinality), this query can easily crash the database. some databases have special functions to do this calculation." @zhaoyongjie

I dont think this should be a concern. Even there's a preselected aggregate, users still need to click SAVE and RUN to run query that potentially crush anything. From UX standpoint, having a preselected aggregate has the equal amount of clicks to no preselected if users need to choose COUNT instead. and for most cases, count_distinct is indeed what users choose for STRING columns. I don't see any reasons why we should use COUNT as preselected.

[kgabryje]

@junlincc I added a UX_BETA feature flag, which is set to False by default - the auto filling of aggregates will work when that flag is set to True

[github-actions]

Ephemeral environment shutdown and build artifacts deleted.