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 2022/11/17 03:20:16 UTC
[GitHub] [superset] sinhashubham95 opened a new pull request, #22147: fix: Unhandled exception for Any metric type
sinhashubham95 opened a new pull request, #22147:
URL: https://github.com/apache/superset/pull/22147
### SUMMARY
The metric is a list of `Any`, so that may or may not have the attribute `get`. This pull request adds a check for that and avoids the APIs using the same throwing internal server error.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
NA
### TESTING INSTRUCTIONS
NA
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] 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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] sinhashubham95 commented on pull request #22147: fix: Unhandled exception Str Column Type
Posted by GitBox <gi...@apache.org>.
sinhashubham95 commented on PR #22147:
URL: https://github.com/apache/superset/pull/22147#issuecomment-1321161289
@villebro please check.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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[bot] commented on pull request #22147: fix: Unhandled exception for Any metric type
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #22147:
URL: https://github.com/apache/superset/pull/22147#issuecomment-1318012085
# [Codecov](https://codecov.io/gh/apache/superset/pull/22147?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#22147](https://codecov.io/gh/apache/superset/pull/22147?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (efe250d) into [master](https://codecov.io/gh/apache/superset/commit/394fb2f2d0e05f27ced88e8ff4fc6994696cab68?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (394fb2f) will **increase** coverage by `2.49%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #22147 +/- ##
==========================================
+ Coverage 53.22% 55.71% +2.49%
==========================================
Files 1833 1833
Lines 69935 69935
Branches 7571 7571
==========================================
+ Hits 37221 38964 +1743
+ Misses 30755 29012 -1743
Partials 1959 1959
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `52.60% <ø> (ø)` | |
| presto | `52.49% <ø> (?)` | |
| python | `57.80% <ø> (+5.20%)` | :arrow_up: |
| unit | `50.89% <ø> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/22147?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset/connectors/base/models.py](https://codecov.io/gh/apache/superset/pull/22147/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9iYXNlL21vZGVscy5weQ==) | `64.75% <ø> (+0.30%)` | :arrow_up: |
| [superset/sql\_lab.py](https://codecov.io/gh/apache/superset/pull/22147/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsX2xhYi5weQ==) | `79.08% <0.00%> (+0.38%)` | :arrow_up: |
| [superset/models/dashboard.py](https://codecov.io/gh/apache/superset/pull/22147/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvbW9kZWxzL2Rhc2hib2FyZC5weQ==) | `47.01% <0.00%> (+0.39%)` | :arrow_up: |
| [superset/models/slice.py](https://codecov.io/gh/apache/superset/pull/22147/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvbW9kZWxzL3NsaWNlLnB5) | `58.91% <0.00%> (+0.49%)` | :arrow_up: |
| [superset/models/core.py](https://codecov.io/gh/apache/superset/pull/22147/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvbW9kZWxzL2NvcmUucHk=) | `78.11% <0.00%> (+1.61%)` | :arrow_up: |
| [superset/exceptions.py](https://codecov.io/gh/apache/superset/pull/22147/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhjZXB0aW9ucy5weQ==) | `89.56% <0.00%> (+1.73%)` | :arrow_up: |
| [superset/models/sql\_lab.py](https://codecov.io/gh/apache/superset/pull/22147/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvbW9kZWxzL3NxbF9sYWIucHk=) | `70.47% <0.00%> (+1.96%)` | :arrow_up: |
| [superset/initialization/\_\_init\_\_.py](https://codecov.io/gh/apache/superset/pull/22147/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvaW5pdGlhbGl6YXRpb24vX19pbml0X18ucHk=) | `91.00% <0.00%> (+2.00%)` | :arrow_up: |
| [superset/utils/log.py](https://codecov.io/gh/apache/superset/pull/22147/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvbG9nLnB5) | `85.13% <0.00%> (+2.02%)` | :arrow_up: |
| ... and [83 more](https://codecov.io/gh/apache/superset/pull/22147/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] sinhashubham95 commented on pull request #22147: fix: Unhandled exception Str Column Type
Posted by GitBox <gi...@apache.org>.
sinhashubham95 commented on PR #22147:
URL: https://github.com/apache/superset/pull/22147#issuecomment-1321737194
@villebro applied the requested changes. Please check.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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 diff in pull request #22147: fix: Unhandled exception for Any metric type
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #22147:
URL: https://github.com/apache/superset/pull/22147#discussion_r1024800714
##########
superset/connectors/base/models.py:
##########
@@ -313,7 +313,9 @@ def data_for_slices( # pylint: disable=too-many-locals
metric_names.add(utils.get_metric_name(metric))
if utils.is_adhoc_metric(metric):
column_names.add(
- (metric.get("column") or {}).get("column_name")
+ (
+ (hasattr(metric, "get") and metric.get("column")) or {}
+ ).get("column_name")
Review Comment:
On line 314 we're already calling a type guard making sure that the type is in fact `AdhocMetric`. So it appears there may be something wrong either in the declaration of the `AdhocMetric` type or the type guard. Can you post the full stack trace of the error you're seeing and see if we need to update either one of those instead?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] sinhashubham95 commented on pull request #22147: fix: Unhandled exception for Any metric type
Posted by GitBox <gi...@apache.org>.
sinhashubham95 commented on PR #22147:
URL: https://github.com/apache/superset/pull/22147#issuecomment-1318010185
@villebro can you please check this?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] sinhashubham95 commented on a diff in pull request #22147: fix: Unhandled exception for Any metric type
Posted by GitBox <gi...@apache.org>.
sinhashubham95 commented on code in PR #22147:
URL: https://github.com/apache/superset/pull/22147#discussion_r1027245816
##########
superset/connectors/base/models.py:
##########
@@ -313,7 +313,9 @@ def data_for_slices( # pylint: disable=too-many-locals
metric_names.add(utils.get_metric_name(metric))
if utils.is_adhoc_metric(metric):
column_names.add(
- (metric.get("column") or {}).get("column_name")
+ (
+ (hasattr(metric, "get") and metric.get("column")) or {}
+ ).get("column_name")
Review Comment:
@villebro sorry for the late reply. As you mentioned correctly, I missed the check for `AdhocMetric`. The issue was not with `metric` param, rather with `column`. Added a check around that.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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 diff in pull request #22147: fix: Unhandled exception Str Column Type
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #22147:
URL: https://github.com/apache/superset/pull/22147#discussion_r1027699625
##########
superset/connectors/base/models.py:
##########
@@ -312,9 +312,9 @@ def data_for_slices( # pylint: disable=too-many-locals
for metric in utils.get_iterable(form_data.get(metric_param) or []):
metric_names.add(utils.get_metric_name(metric))
if utils.is_adhoc_metric(metric):
- column_names.add(
- (metric.get("column") or {}).get("column_name")
- )
+ column = metric.get("column") or {}
+ if hasattr(column, "get") and "column_name" in column:
+ column_names.add(column.get("column_name"))
Review Comment:
Looking at the `AdhocMetric` class, it appears `column` is `Optional[AdhocMetricColumn]`, which in turn is a `TypedDict`. So `hasattr(column, "get")` will always be true based on line 315. So I believe this can be simplified to
```suggestion
if column_name := column.get("column_name"):
column_names.add(column_name)
```
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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 merged pull request #22147: fix: Unhandled exception Str Column Type
Posted by GitBox <gi...@apache.org>.
villebro merged PR #22147:
URL: https://github.com/apache/superset/pull/22147
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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