-
Notifications
You must be signed in to change notification settings - Fork 13.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Explore] Streamlined metric definitions for SQLA and Druid #4663
[Explore] Streamlined metric definitions for SQLA and Druid #4663
Conversation
0b0dc07
to
b6f2246
Compare
This is super neat and innovative. Can we make the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a few minor comments on the code but overall this looks great. Surprisingly easier to use than I thought and much better than my MetricsControl
attempt that I never merged. Glad I held off on it as it just didn't feel right. I think this does.
Few usability ideas:
- it's not a common or expected flow, maybe provide hints as people type or when the Select element has focus? Maybe like a small yellow info bubble
Pick an aggregate function or a column to aggregate
? - a
click to edit the metric
popover when hovering a button-option ?
I wonder if it's eventually in-scope to allow free form SUM(CASE WHEN...)
or APPROX_DISTINCT_COUNT...
in that context. Seemed possible to extend into this if that becomes a priority somehow, but it's probably ok to send people to the old flow for those 10-20% use-cases, at least for now. Maybe by then I'll have rewritten the "Table Editor" into something much more usable.
My MetricsControl
had a big of logic around handling aggregation types based on column type, but that doesn't seem super necessary for MVP here. Could be nice to have eventually.
</FormGroup> | ||
<Button | ||
disabled={!stateIsValid} | ||
bsStyle={(hasUnsavedChanges || !stateIsValid) ? 'default' : 'primary'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: we've veen using bsSize="small"
throughout the app
> | ||
Save | ||
</Button> | ||
<Button onClick={this.props.onClose}>Close</Button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: bsSize="small"
overlay={overlay} | ||
rootClose | ||
> | ||
<Label style={{ cursor: 'pointer' }}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was told to avoid double curlies whenever possible since they generate new objectids at every render and will trigger PureComponent
s to rerender when they don't need to, just declare labelStyle
in module scope and reference it here.
@@ -0,0 +1,20 @@ | |||
import React from 'react'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's ok to declare more than one component in a module if the component is only used once in that one module. Point being to avoid juggling with lots of small files when possible, the components can be refactored out to its own module when it grows to be used in more than one component.
I've been guilty of the other extreme (very long modules) so I'm ok with this if you prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do prefer more files in general.. I think in this instance it makes sense so all the *Option
components are declared in the same way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++ small files. it simplifies everything vastly if everything has it's own file.
superset/connectors/sqla/models.py
Outdated
label = metric.get('label') | ||
if aggregate == 'COUNT_DISTINCT': | ||
sa_metric = sa.func.COUNT(sa.distinct(column(column_name))) | ||
if aggregate == 'COUNT': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use a bunch of elif here to save some comparisons
@mistercrunch thanks for the comments. I'll add it to the metric control as well- forgot about that one. Also will fix python tests that are failing and add a couple more. |
Also like the idea of |
b6f2246
to
934312c
Compare
Codecov Report
@@ Coverage Diff @@
## master #4663 +/- ##
==========================================
+ Coverage 71.56% 71.87% +0.31%
==========================================
Files 191 204 +13
Lines 14958 15323 +365
Branches 1100 1177 +77
==========================================
+ Hits 10705 11014 +309
- Misses 4250 4306 +56
Partials 3 3
Continue to review full report at Codecov.
|
@@ -0,0 +1,6 @@ | |||
import PropTypes from 'prop-types'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see this being used anywhere, what's this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, good catch! I actually don't use this file anywhere at all! I will delete it.
export default PropTypes.shape({ | ||
metric_name: PropTypes.string.isRequired, | ||
expression: PropTypes.string.isRequired, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, why have this in a separate file instead of defined as a shape in the file it's used? This isn't being used in multiple places and it doesn't have a complicated structure (with imports). Is it a best practice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm yeah I could move it into MetricsControl- I agree this doesn't help much.
} | ||
|
||
getDefaultLabel() { | ||
return `${this.aggregate || ''}(${(this.column && this.column.column_name) || ''})`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When would this.aggregate be null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wouldn't be often, but in the case that someone clears the aggregate input in the edit popover, and then goes to edit the title, the default title they would see without this logic would be like null(column_name)
, which seems like a bad user experience. Adding this logic would make the default title appear as (column_name)
superset/connectors/sqla/models.py
Outdated
column_name = metric.get('column').get('column_name') | ||
aggregate = metric.get('aggregate') | ||
label = metric.get('label') | ||
if aggregate == 'COUNT_DISTINCT': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I would make this a dict()
mapping for instance:
aggregate_dict = {
'SUM' : sa.func.COUNT(column(column_name))
...
}
sa_metric = aggregate_dict.get(aggregate)
I think this is much cleaner and would allow others to add more metrics easily later.
PS: This is so dope! i can't wait till this lands so we can had some geo functions!
1aad37e
to
0dc1954
Compare
@mistercrunch @michellethomas thanks for the reviews! I addressed all comments. For Max's more in depth comments:
I also added another feature that HIDES any auto generated metric that is just an aggregate + column. Please take another look when you have the chance! |
I'm going to pull the branch personally but would be nice to have gifs in the PR. |
Removed the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few mostly minor comments, great addition!
@@ -38,5 +38,8 @@ | |||
"react/no-unescaped-entities": 0, | |||
"react/no-unused-prop-types": 0, | |||
"react/no-string-refs": 0, | |||
"indent": 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume these are due to the new eslint
version which flagged them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah exactly
if (type === '' || type === 'expression') { | ||
if (!type) { | ||
stringIcon = '?'; | ||
} else if (type === '' || type === 'expression') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type === ''
is not possible given the first if statement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch I'll clarify the first statement
this.hasCustomLabel = !!(adhocMetric.hasCustomLabel && adhocMetric.label); | ||
this.fromFormData = !!adhocMetric.optionName; | ||
|
||
if (this.hasCustomLabel) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could consolidate with
this.label = this.hasCustomLabel ? adhocMetric.label : this.getDefaultLabel();
} | ||
|
||
this.optionName = adhocMetric.optionName || | ||
`metric_${Math.random().toString(36).substring(2, 15)}_${Math.random().toString(36).substring(2, 15)}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a util func to generate ids that we could use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is what my internet research suggested i do 🤕
<Popover | ||
id="metrics-edit-popover" | ||
title={popoverTitle} | ||
{...rest} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd call this ...popoverProps
instead of rest
return option.column_name && (option.column_name.indexOf(valueAfterAggregate) >= 0); | ||
} | ||
|
||
const autoGeneratedMetricRegex = /^(LONG|DOUBLE|FLOAT)?(SUM|AVG|MAX|MIN|COUNT)\([A-Z_][A-Z0-9_]*\)$/i; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice to have tests for this sort of thing esp if people introduce changes in other files in the future.
optionRenderer={VirtualizedRendererWrap(option => ( | ||
<MetricDefinitionOption option={option} /> | ||
))} | ||
valueRenderer={option => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again lots of render functions
@@ -134,3 +138,7 @@ | |||
.datasource-container { | |||
overflow: auto; | |||
} | |||
|
|||
.edit-adhoc-metric-save-btn { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you use the existing m-r-5
class? (I think this is the name)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh cool! I didn't realize that existed
@@ -49,8 +49,8 @@ export default class OnPasteSelect extends React.Component { | |||
render() { | |||
const SelectComponent = this.props.selectWrap; | |||
const refFunc = (ref) => { | |||
if (this.props.ref) { | |||
this.props.ref(ref); | |||
if (this.props.refFunc) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yay for readability!
@@ -0,0 +1,37 @@ | |||
export default class AdhocMetric { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dig the class approach!
0dc1954
to
e316bda
Compare
e316bda
to
3b8c519
Compare
@williaster thanks for all the comments- they should be addressed now. Good callout on the PropTypes.object stragglers- I feel better now that they're cleaned up. |
3b8c519
to
7d59385
Compare
@mistercrunch @michellethomas @williaster I believe this PR is ready to merge--- any objectors? |
lgtm, will let @mistercrunch pull the trigger tho 📈 |
This PR looks brilliant, great work! Thanks! Which release/version would this functionilty go into? Jiaxu |
It should be in |
mistercrunch, Thanks for your explanation! Regards, |
Hey, Thank |
This PR streamlines the definition of
AGGREGATE( column )
-style metrics in the explore view. It circumvents the fab view entirely for a much improved user experience.The metric is stored as a simple object of form {column, aggregate, label} in form data. When a query is run, the adhoc metric objects are sent along with saved metrics to the flask backend, where they are manually parsed in SQLA/Druid expressions.
Here are some quick videos demoing what interactions look like:
Adding and editing metrics:
![out5](https://user-images.githubusercontent.com/2455694/37745197-c6cd2a86-2d30-11e8-911b-9769f4b1dc55.gif)
Applying labels to metrics:
![out4](https://user-images.githubusercontent.com/2455694/37745211-d17c0ae2-2d30-11e8-9d29-5f6e87442444.gif)
Guide to reviewers:
adhocMetrics
. Traditional metrics I refer to assavedMetrics
Open questions:
[...columns, ...aggregates, ...saved metrics]
. I prefer this ordering because there are many saved metrics that this feature will replace. However I can see it being confusing initially to users to have this dropdown default to unexpected content.reviewers:
@michellethomas @john-bodley @williaster @graceguo-supercat @mistercrunch
(after putting up this PR I'll continue testing locally so there may be some bug fixes to come, but this is the done deal!)