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/07/08 11:44:19 UTC
[GitHub] [superset] zhaoyongjie opened a new pull request, #20651: fix: annotation broken
zhaoyongjie opened a new pull request, #20651:
URL: https://github.com/apache/superset/pull/20651
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Currently, the annotation on the time series chart has been broken.
1. This error is from the TypeScript [type guard function](https://github.com/apache/superset/blob/e33164024a17bb7e6de9d40b9fc60996dc9406fc/superset-frontend/packages/superset-ui-core/src/query/types/AnnotationLayer.ts#L183).
2. The annotation on the master branch doesn't apply annotation_data from the query response.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] 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] zhaoyongjie commented on pull request #20651: fix: annotation broken
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on PR #20651:
URL: https://github.com/apache/superset/pull/20651#issuecomment-1181244695
@rusackas the coverage issue didn't bring from this PR, the master branch has broken some days. I will look into / fix coverage issue in the separated pr.
--
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] john-bodley commented on pull request #20651: fix: annotation broken
Posted by GitBox <gi...@apache.org>.
john-bodley commented on PR #20651:
URL: https://github.com/apache/superset/pull/20651#issuecomment-1179436252
Thanks @zhaoyongjie for the fix and adding unit tests. I'm running into the exact same issue whilst testing a prior Preset release internally at Airbnb and was wondering whether you knew what PR introduced said regression?
--
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] zhaoyongjie commented on a diff in pull request #20651: fix: annotation broken
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20651:
URL: https://github.com/apache/superset/pull/20651#discussion_r917609193
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -93,12 +105,12 @@ export default function transformProps(
queriesData,
datasource,
theme,
- annotationData = {},
} = chartProps;
const { verboseMap = {} } = datasource;
const [queryData] = queriesData;
const { data = [] } = queryData as TimeseriesChartDataResponseResult;
const dataTypes = getColtypesMapping(queryData);
+ const annotationData = getAnnotationData(chartProps);
Review Comment:
done! thanks for the heads up.
--
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] stephenLYZ commented on a diff in pull request #20651: fix: annotation broken
Posted by GitBox <gi...@apache.org>.
stephenLYZ commented on code in PR #20651:
URL: https://github.com/apache/superset/pull/20651#discussion_r917549918
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -93,12 +105,12 @@ export default function transformProps(
queriesData,
datasource,
theme,
- annotationData = {},
} = chartProps;
const { verboseMap = {} } = datasource;
const [queryData] = queriesData;
const { data = [] } = queryData as TimeseriesChartDataResponseResult;
const dataTypes = getColtypesMapping(queryData);
+ const annotationData = getAnnotationData(chartProps);
Review Comment:
Mixd-timeseries chart may also need to revert here.
--
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 #20651: fix: annotation broken
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #20651:
URL: https://github.com/apache/superset/pull/20651#issuecomment-1179161989
# [Codecov](https://codecov.io/gh/apache/superset/pull/20651?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 [#20651](https://codecov.io/gh/apache/superset/pull/20651?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (edb6487) into [master](https://codecov.io/gh/apache/superset/commit/e33164024a17bb7e6de9d40b9fc60996dc9406fc?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e331640) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## master #20651 +/- ##
=======================================
Coverage 66.78% 66.78%
=======================================
Files 1754 1754
Lines 65857 65862 +5
Branches 7030 7030
=======================================
+ Hits 43982 43987 +5
Misses 20086 20086
Partials 1789 1789
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `51.94% <100.00%> (+<0.01%)` | :arrow_up: |
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/20651?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...art-controls/src/sections/annotationsAndLayers.tsx](https://codecov.io/gh/apache/superset/pull/20651/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-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3NlY3Rpb25zL2Fubm90YXRpb25zQW5kTGF5ZXJzLnRzeA==) | `100.00% <ø> (ø)` | |
| [...uperset-ui-core/src/query/types/AnnotationLayer.ts](https://codecov.io/gh/apache/superset/pull/20651/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-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvcXVlcnkvdHlwZXMvQW5ub3RhdGlvbkxheWVyLnRz) | `100.00% <100.00%> (ø)` | |
| [...gin-chart-echarts/src/Timeseries/transformProps.ts](https://codecov.io/gh/apache/superset/pull/20651/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvVGltZXNlcmllcy90cmFuc2Zvcm1Qcm9wcy50cw==) | `57.95% <100.00%> (+3.19%)` | :arrow_up: |
| [...ars/src/components/Handlebars/HandlebarsViewer.tsx](https://codecov.io/gh/apache/superset/pull/20651/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtaGFuZGxlYmFycy9zcmMvY29tcG9uZW50cy9IYW5kbGViYXJzL0hhbmRsZWJhcnNWaWV3ZXIudHN4) | `0.00% <0.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/20651?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/20651?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [e331640...edb6487](https://codecov.io/gh/apache/superset/pull/20651?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] zhaoyongjie commented on pull request #20651: fix: annotation broken
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on PR #20651:
URL: https://github.com/apache/superset/pull/20651#issuecomment-1180050431
> Thanks @zhaoyongjie for the fix and adding unit tests. For context I'm running into the exact same issue whilst testing a prior Preset release internally at Airbnb.
>
> I was wondering whether you knew what PR introduced said regression and/or whether it was possible to simply revert it?
thanks for the review, this issue from the [PR](https://github.com/apache/superset/pull/19761)
--
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] rusackas commented on pull request #20651: fix: annotation broken
Posted by GitBox <gi...@apache.org>.
rusackas commented on PR #20651:
URL: https://github.com/apache/superset/pull/20651#issuecomment-1180907401
@zhaoyongjie any idea why codecov is failing on this PR? There seem to be similarly confusing codecov failures on lots of PRs right now, but I'm not sure yet what to make of it.
--
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] ktmud commented on a diff in pull request #20651: fix: annotation broken
Posted by GitBox <gi...@apache.org>.
ktmud commented on code in PR #20651:
URL: https://github.com/apache/superset/pull/20651#discussion_r918201346
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/annotation.ts:
##########
@@ -130,3 +134,13 @@ export function extractAnnotationLabels(
return formulaAnnotationLabels.concat(timeseriesAnnotationLabels);
}
+
+export function getAnnotationData(
+ chartProps: EchartsTimeseriesChartProps | EchartsMixedTimeseriesProps,
+): AnnotationData {
+ const data = chartProps?.queriesData[0]?.annotation_data as AnnotationData;
+ if (!isEmpty(data)) {
+ return data;
+ }
+ return {};
Review Comment:
Nit: ternary seems more readable here
```suggestion
return isEmpty(data) ? {} : data;
```
I'm also not sure `isEmpty` is needed. If for whatever reason `annotation_data` becomes anything other than a plain object, it probably means there's bugs in the code or corrupted data and we'd need to fix it.
--
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] rusackas merged pull request #20651: fix: annotation broken
Posted by GitBox <gi...@apache.org>.
rusackas merged PR #20651:
URL: https://github.com/apache/superset/pull/20651
--
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] zhaoyongjie commented on a diff in pull request #20651: fix: annotation broken
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20651:
URL: https://github.com/apache/superset/pull/20651#discussion_r918494549
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/annotation.ts:
##########
@@ -130,3 +134,13 @@ export function extractAnnotationLabels(
return formulaAnnotationLabels.concat(timeseriesAnnotationLabels);
}
+
+export function getAnnotationData(
+ chartProps: EchartsTimeseriesChartProps | EchartsMixedTimeseriesProps,
+): AnnotationData {
+ const data = chartProps?.queriesData[0]?.annotation_data as AnnotationData;
+ if (!isEmpty(data)) {
+ return data;
+ }
+ return {};
Review Comment:
@ktmud Thanks, i will look into this case.
--
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] slavanorm commented on pull request #20651: fix: annotation broken
Posted by "slavanorm (via GitHub)" <gi...@apache.org>.
slavanorm commented on PR #20651:
URL: https://github.com/apache/superset/pull/20651#issuecomment-1605979332
can we say that this issue is not complete?
in 2.* version annotations are broken:
https://github.com/apache/superset/issues/24374
--
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