You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2021/02/12 03:20:04 UTC
[GitHub] [superset] michellethomas opened a new pull request #13086: fix: accept null groupby in form data for timeseries table viz
michellethomas opened a new pull request #13086:
URL: https://github.com/apache/superset/pull/13086
### SUMMARY
In existing (previously created) timeseries table viz charts the groupby can be null if there are multiple metrics selected and no groupby. In newly created charts in this situation groupby is `[]`. I'm making this change to avoid the error on old charts if groupby is null in the form data.
### TEST PLAN
Tested existing saved charts with multiple metrics and no groupby
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [x] Has associated issue: Fixes #12750
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a change in pull request #13086: fix: accept null groupby in form data for timeseries table viz
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #13086:
URL: https://github.com/apache/superset/pull/13086#discussion_r577344279
##########
File path: superset/viz.py
##########
@@ -824,7 +824,8 @@ def get_data(self, df: pd.DataFrame) -> VizData:
return dict(
records=pt.to_dict(orient="index"),
columns=list(pt.columns),
- is_group_by=len(fd.get("groupby", [])) > 0,
+ # "groupby" may not exist in fd or it may exist and be None
+ is_group_by=len(fd.get("groupby") or []) > 0,
Review comment:
I agree that this is better. However, while we're at it, I think it's more typical to check truthiness of the key in instances like this rather than `len`. Something like this should do the same thing if `fd.get("groupby")` is expected to return `Optional[List[Any]]`: `is_group_by=True if fg.get("groupby") else False`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michellethomas commented on pull request #13086: fix: accept null groupby in form data for timeseries table viz
Posted by GitBox <gi...@apache.org>.
michellethomas commented on pull request #13086:
URL: https://github.com/apache/superset/pull/13086#issuecomment-780126465
ping @john-bodley @villebro any feedback on this PR? Is it ok to get this simple fix in for old chart form data or would we prefer to have a db migration? If we would prefer a migration, would it be possible to get this in first to fix the issues and remove it if we don't want to keep it around?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michellethomas commented on pull request #13086: fix: accept null groupby in form data for timeseries table viz
Posted by GitBox <gi...@apache.org>.
michellethomas commented on pull request #13086:
URL: https://github.com/apache/superset/pull/13086#issuecomment-778443776
That's a good question @john-bodley I made the changes in the code because they were pretty simple, not complicated logic to handle lots of nuances but just simple null handling (which in the case of the code in viz.py needs to happen anyways in some form). I don't think that this case needs to be handled in the code forever, eventually old charts will likely become obsolete. I think in general a db migration would be better to migrate old formats, but since this fix is simple, a db migration to resolve only a fraction of charts of this specific viz type seemed a little overkill. But I'm fine to do that if that's the preferred way. Do you know who else would be good to decide whether a db migration is preferred?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michellethomas commented on pull request #13086: fix: accept null groupby in form data for timeseries table viz
Posted by GitBox <gi...@apache.org>.
michellethomas commented on pull request #13086:
URL: https://github.com/apache/superset/pull/13086#issuecomment-780803718
Thanks for the feedback!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov-io commented on pull request #13086: fix: accept null groupby in form data for timeseries table viz
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #13086:
URL: https://github.com/apache/superset/pull/13086#issuecomment-777954003
# [Codecov](https://codecov.io/gh/apache/superset/pull/13086?src=pr&el=h1) Report
> Merging [#13086](https://codecov.io/gh/apache/superset/pull/13086?src=pr&el=desc) (ee7eaee) into [master](https://codecov.io/gh/apache/superset/commit/2ce79823dfad61bce6196fcacd56a844f44818c0?el=desc) (2ce7982) will **increase** coverage by `13.63%`.
> The diff coverage is `65.45%`.
[](https://codecov.io/gh/apache/superset/pull/13086?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13086 +/- ##
===========================================
+ Coverage 53.06% 66.69% +13.63%
===========================================
Files 489 491 +2
Lines 17314 28934 +11620
Branches 4482 0 -4482
===========================================
+ Hits 9187 19298 +10111
- Misses 8127 9636 +1509
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `?` | |
| python | `66.69% <65.45%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/13086?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/config.py](https://codecov.io/gh/apache/superset/pull/13086/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29uZmlnLnB5) | `90.68% <ø> (ø)` | |
| [...s/260bf0649a77\_migrate\_x\_dateunit\_in\_time\_range.py](https://codecov.io/gh/apache/superset/pull/13086/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbWlncmF0aW9ucy92ZXJzaW9ucy8yNjBiZjA2NDlhNzdfbWlncmF0ZV94X2RhdGV1bml0X2luX3RpbWVfcmFuZ2UucHk=) | `0.00% <0.00%> (ø)` | |
| [superset/viz.py](https://codecov.io/gh/apache/superset/pull/13086/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdml6LnB5) | `59.05% <ø> (ø)` | |
| [superset/utils/core.py](https://codecov.io/gh/apache/superset/pull/13086/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY29yZS5weQ==) | `87.63% <47.36%> (ø)` | |
| [superset/charts/commands/exceptions.py](https://codecov.io/gh/apache/superset/pull/13086/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY2hhcnRzL2NvbW1hbmRzL2V4Y2VwdGlvbnMucHk=) | `92.85% <77.77%> (ø)` | |
| [superset/db\_engine\_specs/elasticsearch.py](https://codecov.io/gh/apache/superset/pull/13086/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2VsYXN0aWNzZWFyY2gucHk=) | `89.74% <93.75%> (ø)` | |
| [superset/utils/date\_parser.py](https://codecov.io/gh/apache/superset/pull/13086/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvZGF0ZV9wYXJzZXIucHk=) | `96.87% <100.00%> (ø)` | |
| [superset-frontend/src/components/ErrorBoundary.jsx](https://codecov.io/gh/apache/superset/pull/13086/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvRXJyb3JCb3VuZGFyeS5qc3g=) | | |
| [superset-frontend/src/setup/setupClient.ts](https://codecov.io/gh/apache/superset/pull/13086/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwQ2xpZW50LnRz) | | |
| [...set-frontend/src/common/components/ProgressBar.tsx](https://codecov.io/gh/apache/superset/pull/13086/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbW1vbi9jb21wb25lbnRzL1Byb2dyZXNzQmFyLnRzeA==) | | |
| ... and [967 more](https://codecov.io/gh/apache/superset/pull/13086/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13086?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/13086?src=pr&el=footer). Last update [7b8a0e1...ee7eaee](https://codecov.io/gh/apache/superset/pull/13086?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michellethomas merged pull request #13086: fix: accept null groupby in form data for timeseries table viz
Posted by GitBox <gi...@apache.org>.
michellethomas merged pull request #13086:
URL: https://github.com/apache/superset/pull/13086
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on pull request #13086: fix: accept null groupby in form data for timeseries table viz
Posted by GitBox <gi...@apache.org>.
john-bodley commented on pull request #13086:
URL: https://github.com/apache/superset/pull/13086#issuecomment-778482150
@michellethomas I agree the change is fairly minor and simple to grok. @villebro any thoughts?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #13086: fix: accept null groupby in form data for timeseries table viz
Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #13086:
URL: https://github.com/apache/superset/pull/13086#issuecomment-780137969
sorry @michellethomas this had gone unnoticed by me. I'll review first thing tomorrow.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org