-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
Added average metric AVG() to default metrics #1413
Conversation
@@ -1101,6 +1101,13 @@ def fetch_metadata(self): | |||
metric_type='sum', | |||
expression="SUM({})".format(quoted) | |||
)) | |||
if dbcol.sum: |
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.
s/dbcol.sum/dbcol.avg ?
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'm thinking that the condition for dbcol.sum and dbcol.avg are both dbcol.isnum, so it makes sense to use one variable, as long as there's a sum, there can be an avg metrics as well
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.
dbcol.max and dbcol.min are also in similar relation.
@mistercrunch - should avg be a separate field in the TableColumn ?
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've talked to Max, please implement a separate checkbox for AVG metric.
User may not expect to add 2 metrics when setting the sum
checkbox and will be tricky to explain him if you want to add avg
metrics - please select sum.
@@ -1101,6 +1101,13 @@ def fetch_metadata(self): | |||
metric_type='sum', | |||
expression="SUM({})".format(quoted) | |||
)) | |||
if dbcol.sum: |
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 would be great to have a unit test here
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.
Do we have tests covering metrics/groupby of db queries? I was trying to look for them but didn't seem to exist core_tests.py
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.
nope, you'll be a first one if you'll decide to do it
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.
Since it will be tests covering all metrics, I'll separate it to another PR ;)
b74329d
to
f6e0559
Compare
Added a new commit, making avg a column for SqlaTable and Druid class @bkyryliuk |
please fix the tests |
608e0e2
to
6f4c86c
Compare
e7a5985
to
e3f64ba
Compare
Added migration 😃 @bkyryliuk |
LGTM |
Hi, note: it is available when I load data from csv files directly |
You have to create post-aggregations for averages on Druid. |
…apache#1413) * Update SupersetClientClass.ts * Update SupersetClientClass.ts * add comment * update test
…apache#1413) * Update SupersetClientClass.ts * Update SupersetClientClass.ts * add comment * update test
…apache#1413) * Update SupersetClientClass.ts * Update SupersetClientClass.ts * add comment * update test
…apache#1413) * Update SupersetClientClass.ts * Update SupersetClientClass.ts * add comment * update test
Issue: #804
Done: Added avg to default metrics in models->SqlaTable
needs-review @bkyryliuk @mistercrunch