-
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] Adding custom expressions to adhoc metrics #4736
[Explore] Adding custom expressions to adhoc metrics #4736
Conversation
7643d21
to
c0bf117
Compare
Codecov Report
@@ Coverage Diff @@
## master #4736 +/- ##
==========================================
+ Coverage 72.33% 72.36% +0.03%
==========================================
Files 208 208
Lines 15568 15662 +94
Branches 1204 1227 +23
==========================================
+ Hits 11261 11334 +73
- Misses 4304 4325 +21
Partials 3 3
Continue to review full report at Codecov.
|
c0bf117
to
fe895c5
Compare
Updated based on comments from team at Airbnb- a) now the smart defaults in the simple tab will only be inferred when the simple tab can entirely describe the advanced sql expression. if not, the simple tabs defaults will be blank. b) the window can be resized if you want more working space in the advanced sql editor view |
@GabeLoins can we get a design pass on this before merging? I'm mostly concerned about the size of the tabs and the strange offset once you resize the popover container |
fe895c5
to
772e676
Compare
@williaster good points. Adjusted tab widths and made it so the resize doesn't affect the arrow positioning. both features are demoed here: |
@mistercrunch yeah I definitely considered that as well when working on it. The big motivator for going with tabs is that the metric definitions could easily hit 4-5 lines if someone was defining a case/if statment. I wanted to give the Ace editor a good amount of space by default so that would be possible. |
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.
a couple super tiny comments, looks great overall and a 🔥 feature!
|
||
isValid() { | ||
if (this.expressionType === EXPRESSION_TYPES.SIMPLE) { | ||
return !!(this.column && this.aggregate); |
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.
Boolean()
also works fyi
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.
Airbnb js style guide prefer !!
to Boolean(...)
- not sure if we have a different style guide for this project, but I like their advice on the whole.
aggregate: aggregate && aggregate.aggregate, | ||
}), | ||
}); | ||
this.setState({ adhocMetric: this.state.adhocMetric.duplicateWith({ |
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 newline adhocMetric
, wish we had better lint rules enabled! 🙉
} | ||
|
||
componentWillUnmount() { | ||
document.removeEventListener('mouseup', this.onMouseUp); |
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.
unlikely sitch, but could also call document.removeEventListener('mousemove', this.onMouseMove);
to be safe
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.
oooh good call
const stateIsValid = this.state.adhocMetric.column && this.state.adhocMetric.aggregate; | ||
const hasUnsavedChanges = this.state.adhocMetric.equals(this.props.adhocMetric); | ||
const stateIsValid = adhocMetric.isValid(); | ||
const hasUnsavedChanges = adhocMetric.equals(propsAdhocMetric); |
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 this backwards? or I'm reading it wrong? I guess I'd expect equals to return true if they're the same which would mean no unsaved changes?
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.
ah you're totally right- everything works because I'm also using it wrong in the render body 🤕
} | ||
|
||
onPopoverResize() { | ||
this.forceUpdate(); |
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 indent
@@ -44,6 +50,7 @@ export default class AdhocMetricOption extends React.PureComponent { | |||
disabled | |||
overlay={overlay} | |||
rootClose | |||
shouldUpdatePosition |
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.
🎉 thanks again for adding this!
@@ -82,4 +95,95 @@ describe('AdhocMetric', () => { | |||
|
|||
expect(adhocMetric2.label).to.equal('label1'); | |||
}); | |||
|
|||
it('can determines if it is valid', () => { |
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 remove determines
's'
this.expressionType = adhocMetric.expressionType || EXPRESSION_TYPES.SIMPLE; | ||
if (this.expressionType === EXPRESSION_TYPES.SIMPLE) { | ||
const inferredColumn = inferSqlExpressionColumn(adhocMetric); | ||
this.column = adhocMetric.column || (inferredColumn && { column_name: inferredColumn }); |
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 to make sure I understand, inferSqlExpressionColumn
is necessary if adhocMetric was a sql expression typed previously when EXPRESSION_TYPES was SQL correct? There's a lot going on in this section, maybe we could add a few comments to clarify?
772e676
to
db7b41f
Compare
@michellethomas @williaster addressed your comments, should be ready to merge! |
db7b41f
to
6a8dde6
Compare
thanks @GabeLoins! lgtm! |
* adding custom expressions to adhoc metrics * adjusted transitions and made the box expandable
* adding custom expressions to adhoc metrics * adjusted transitions and made the box expandable
* adding custom expressions to adhoc metrics * adjusted transitions and made the box expandable
This is a followup to #4663. Sometimes you want more flexibility than the simple adhoc metric allows for. This PR adds a second tab that allows someone to define a metric as arbitrary sql. It also may be an easier UI for SQL lovers.
I also made sure the transitions between the two tabs are as seamless as possible, so from a user perspective it should feel like two views into the same metric definition.
demo of more advanced expressions:
![advncdexpr](https://user-images.githubusercontent.com/2455694/38208208-256a19e0-3665-11e8-863d-58758a9264df.gif)
demo of seamless transitions:
![transistns2](https://user-images.githubusercontent.com/2455694/38208216-2aa88e96-3665-11e8-9dd7-b7a2767468ae.gif)
Unfortunately had to disable this feature on druid datasources because it seems like superset doesn't support querying druid via sql yet? Or does it? @betodealmeida @mistercrunch any idea how hard it would be to integrate this with druid?
test plan:
SUM(value) + 100
,SUM(value)/COUNT(DISTINCT target)
) and verified that their outputs were correct as shown in video Add pandas-highcharts as requirements #1future work:
reviewers:
@michellethomas @williaster @graceguo-supercat @mistercrunch