feat: add control grouping functionality #485
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/superset/superset-ui/ognt2go4k |
Codecov Report
@@ Coverage Diff @@
## master #485 +/- ##
==========================================
- Coverage 22.75% 22.63% -0.12%
==========================================
Files 276 281 +5
Lines 6672 6692 +20
Branches 645 648 +3
==========================================
- Hits 1518 1515 -3
- Misses 5115 5138 +23
Partials 39 39
Continue to review full report at Codecov.
|
|
Thanks for the review @kristw ; my apologies for the brief description, I meant to expand on the initial description but got sidetracked. Will add more context to describe why this change is necessary and pointers for some design decisions. Also, as this is my first real stab at TS, I fully expect there to be inconsistencies, so if something looks weird, it's probably because I've done it wrong. |
|
I think the term "control group" is kind of confusing. It took me quite a while to understand what problem this PR tries to solve. The term control group is too commonly used in Data Science in the context of hypothesis testing, i.e., A/B tests and I just couldn't get over it. At some point, I also thought this was about arranging the UI as "control" seems to indicate the tangible form control element. If I'm getting it right, this is actually about consolidating control fields required for constructing DB queries ( It's not grouped form controls as in UI components, but more like grouped query API parameters or consolidated query fields. Can we maybe rename the variables/functions as such: |
| >; | ||
| export type ResidualFormData = { | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| [key: string]: any; |
There was a problem hiding this comment.
Actually, are the items in ResidualFormData a QueryFormDataMetric? buildGroupedControlData seems to imply that.
There was a problem hiding this comment.
Yes, I was also puzzled by this. ResidualFormData (in retrospect should be QueryFormResidualData) is currently expected to contain either column names (string) or metrics (QueryFormDataMetric which is the same as string | AdhocMetric). When these are combined, the union is reduced to string | AdhocMetric which is the same as QueryFormDataMetric. I'm tempted to remove QueryFormDataMetric and just pass around QueryFormResidualData, which can later be split into QueryFormDataMetric and QueryFormDataColumn when that need arises (Down the line we'll need to add AdhocColumn which is a column with a SQL expression). I will actually do that, we'll see how it looks.
| const [key, residualValue] = entry; | ||
| if (Object.prototype.hasOwnProperty.call(defaultedControlGroups, key)) { | ||
| const controlGroup: string = defaultedControlGroups[key]; | ||
| const controlValue = residualFormData[key]; |
There was a problem hiding this comment.
I'm a little confused by what this function is doing. Aren't controlValue and residualValue the same thing?
There was a problem hiding this comment.
Here's what I think this function is essentially doing, correct me if I'm wrong?
const normalizedKey = Object.prototype.hasOwnProperty.call(defaultedControlGroups, key)
? defaultedControlGroups[key]
: key;
groupedControls[normalizedKey] = (groupedControls[normalizedKey] || []).concat(residualValue);
There was a problem hiding this comment.
I, too, was a bit puzzled here, so I've tried to write a simplified version of this block as well. Curious if this is missing something important (I'm a little tired, so... probably):
Object.keys(residualFormData).forEach(key => {
const conformedKey = defaultedControlGroups[key] || key;
groupedControls[conformedKey] = [...groupedControls[conformedKey] || [], residualFormData[key]]
});
There was a problem hiding this comment.
You're both right, this was poorly written 🤦 Will address.
|
Thanks all for the comments! ❤️ Will implement changes and update description + comment where necessary. |
villebro
force-pushed
the
villebro/autogroup-controls
branch
from
6e40a24
to
e42de00
Compare
🏆 Enhancements
This adds support for control value grouping introduced in apache/superset#9740 . If a control has specified the property
queryFields, those are used to group control values together when the query is build. ExampleformData:{ "series": ["gender", "nationality"], "primary_metric": "age", "secondary_metric": "height", "queryFields": { "primary_metric": "metrics", "secondary_metric": "metrics", "series': 'groupby" } }This will result in the following query object:
{ "metrics": ["primary_metric", "secondary_metric"], "groupby": ["gender", "nationality'] }This will help decouple
superset-uifrom individual plugins, as plugins can specify an arbitrary number ofmetric,secondary_metric,series,groupbywithout there being a need forsuperset-uito know which is which. This will also bring the query logic closer to the control panel where controls are defined, as thequeryFieldcan be defined in the control metadata, hence won't need to be mapped over inbuildQuery.{js,ts}.