You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "curious86 (via GitHub)" <gi...@apache.org> on 2023/06/23 05:22:57 UTC
[GitHub] [superset] curious86 opened a new pull request, #24492: fix(charts): big-number display broken in echarts
curious86 opened a new pull request, #24492:
URL: https://github.com/apache/superset/pull/24492
### SUMMARY
The current implementation of the time-series charts in ECharts seems to have a limitation in properly rendering big numbers. As a result, the display of big numbers in the chart is broken.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
<img width="899" alt="68d58a51-b0f2-42b0-b191-f35372d8b5dd" src="https://github.com/apache/superset/assets/97945303/c403ef96-aa18-49be-84db-8457ef55b071">
After:
<img width="904" alt="19053635-9d5a-42e4-81a0-cd2d7aa51790" src="https://github.com/apache/superset/assets/97945303/a5bf5b93-507f-4792-9d0f-05e5e5f539f1">
### TESTING INSTRUCTIONS
To replicate the issue with big number display in time series charts, you can create a time series chart that includes data points with large numeric values. Here is an example SQL query that can be executed in SQL Lab to generate such data:
```
select 9223372036854775807 as huge_number, 'abc' as str union all
select 9223372036854775807.5 as huge_number, 'foo' as str union all
select 92233720368547758071111 as huge_number, 'bar' as str
```
1. After executing the SQL query in SQL Lab and getting the result set, click on the "Create Chart" button.
2. In the chart creation page, switch to the "LINE CHART" option.
3. On the chart configuration panel, select the 'str' column for the x-axis.
4. Add the "SUM(huge_number)" column to the y-axis to represent the aggregated big numbers.
5. Update the chart to apply the changes and visualize the data.
6. By performing these steps, you will be able to replicate the issue where the ECharts library struggles to properly render the big number values on the time series chart.
--
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 closed pull request #24492: fix(charts): big-number display broken in echarts
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro closed pull request #24492: fix(charts): big-number display broken in echarts
URL: https://github.com/apache/superset/pull/24492
--
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] michael-s-molina commented on pull request #24492: fix(charts): big-number display broken in echarts
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #24492:
URL: https://github.com/apache/superset/pull/24492#issuecomment-1607450385
@justinpark I know that you fixed similar problems recently. Could you review this 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] codecov[bot] commented on pull request #24492: fix(charts): big-number display broken in echarts
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #24492:
URL: https://github.com/apache/superset/pull/24492#issuecomment-1607026282
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/24492?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#24492](https://app.codecov.io/gh/apache/superset/pull/24492?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (8577396) into [master](https://app.codecov.io/gh/apache/superset/commit/51a34d7d58a8c110d403e22db30a799a68a81ed5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (51a34d7) will **increase** coverage by `0.05%`.
> The diff coverage is `51.11%`.
> :exclamation: Current head 8577396 differs from pull request most recent head fefa1b9. Consider uploading reports for the commit fefa1b9 to get more accurate results
```diff
@@ Coverage Diff @@
## master #24492 +/- ##
==========================================
+ Coverage 69.01% 69.06% +0.05%
==========================================
Files 1901 1902 +1
Lines 74034 74027 -7
Branches 8119 8117 -2
==========================================
+ Hits 51091 51128 +37
+ Misses 20835 20788 -47
- Partials 2108 2111 +3
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `55.72% <52.27%> (+0.03%)` | :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=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/superset/pull/24492?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [.../packages/superset-ui-core/src/chart/types/Base.ts](https://app.codecov.io/gh/apache/superset/pull/24492?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvY2hhcnQvdHlwZXMvQmFzZS50cw==) | `100.00% <ø> (ø)` | |
| [.../plugins/plugin-chart-echarts/src/BoxPlot/index.ts](https://app.codecov.io/gh/apache/superset/pull/24492?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvQm94UGxvdC9pbmRleC50cw==) | `50.00% <ø> (ø)` | |
| [...d/plugins/plugin-chart-echarts/src/Funnel/index.ts](https://app.codecov.io/gh/apache/superset/pull/24492?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvRnVubmVsL2luZGV4LnRz) | `50.00% <ø> (ø)` | |
| [...nd/plugins/plugin-chart-echarts/src/Gauge/index.ts](https://app.codecov.io/gh/apache/superset/pull/24492?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvR2F1Z2UvaW5kZXgudHM=) | `50.00% <ø> (ø)` | |
| [...nd/plugins/plugin-chart-echarts/src/Graph/index.ts](https://app.codecov.io/gh/apache/superset/pull/24492?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvR3JhcGgvaW5kZXgudHM=) | `50.00% <ø> (ø)` | |
| [...rts/src/MixedTimeseries/EchartsMixedTimeseries.tsx](https://app.codecov.io/gh/apache/superset/pull/24492?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvTWl4ZWRUaW1lc2VyaWVzL0VjaGFydHNNaXhlZFRpbWVzZXJpZXMudHN4) | `0.00% <ø> (ø)` | |
| [.../plugin-chart-echarts/src/MixedTimeseries/index.ts](https://app.codecov.io/gh/apache/superset/pull/24492?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvTWl4ZWRUaW1lc2VyaWVzL2luZGV4LnRz) | `25.00% <ø> (ø)` | |
| [...hart-echarts/src/MixedTimeseries/transformProps.ts](https://app.codecov.io/gh/apache/superset/pull/24492?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvTWl4ZWRUaW1lc2VyaWVzL3RyYW5zZm9ybVByb3BzLnRz) | `0.00% <0.00%> (ø)` | |
| [...tend/plugins/plugin-chart-echarts/src/Pie/index.ts](https://app.codecov.io/gh/apache/superset/pull/24492?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvUGllL2luZGV4LnRz) | `50.00% <ø> (ø)` | |
| [...nd/plugins/plugin-chart-echarts/src/Radar/index.ts](https://app.codecov.io/gh/apache/superset/pull/24492?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvUmFkYXIvaW5kZXgudHM=) | `50.00% <ø> (ø)` | |
| ... and [24 more](https://app.codecov.io/gh/apache/superset/pull/24492?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | |
... and [9 files with indirect coverage changes](https://app.codecov.io/gh/apache/superset/pull/24492/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
: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=apache)
--
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 #24492: fix(charts): big-number display broken in echarts
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro merged PR #24492:
URL: https://github.com/apache/superset/pull/24492
--
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 #24492: fix(charts): big-number display broken in echarts
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #24492:
URL: https://github.com/apache/superset/pull/24492#discussion_r1239383575
##########
superset-frontend/plugins/plugin-chart-echarts/src/types.ts:
##########
@@ -168,4 +171,21 @@ export interface TreePathInfo {
value: number | number[];
}
+export class EchartsChartPlugin<
+ T extends SqlaFormData = SqlaFormData,
+ P extends ChartProps = ChartProps,
+> extends ChartPlugin<T, P> {
+ constructor(props: any) {
+ const { metadata } = props;
+ const mergedProps = {
+ ...props,
+ metadata: new ChartMetadata({
+ parseMethod: 'json',
+ ...metadata,
+ }),
+ };
+ super(mergedProps);
Review Comment:
nit: Maybe we don't need to create the intermediate `mergedProps`, but could just inline it in `super()`
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/index.test.ts:
##########
@@ -0,0 +1,28 @@
+/**
++ * Licensed to the Apache Software Foundation (ASF) under one
++ * or more contributor license agreements. See the NOTICE file
++ * distributed with this work for additional information
++ * regarding copyright ownership. The ASF licenses this file
++ * to you under the Apache License, Version 2.0 (the
++ * "License"); you may not use this file except in compliance
++ * with the License. You may obtain a copy of the License at
++ *
++ * http://www.apache.org/licenses/LICENSE-2.0
++ *
++ * Unless required by applicable law or agreed to in writing,
++ * software distributed under the License is distributed on an
++ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
++ * KIND, either express or implied. See the License for the
++ * specific language governing permissions and limitations
++ * under the License.
++ */
+import { EchartsTimeseriesChartPlugin } from '@superset-ui/plugin-chart-echarts';
+
+describe('@superset-ui/plugin-chart-echarts', () => {
+ it('exists', () => {
+ const chart = new EchartsTimeseriesChartPlugin().configure({
+ key: 'echarts_timeseries',
+ });
+ expect(chart.metadata.parseMethod).toEqual('json');
+ });
+});
Review Comment:
Looking at the test suite, maybe we could simplify and make this more comprehensive by moving this test into `test/index.test.ts` and have a section there that ensures `parseMethod` is correctly defined for all plugins.
Bycatch: I also noticed that the `exists` test there doesn't cover all plugins. While you're at it, can you add the missing plugins, too?
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/index.test.ts:
##########
@@ -0,0 +1,28 @@
+/**
++ * Licensed to the Apache Software Foundation (ASF) under one
++ * or more contributor license agreements. See the NOTICE file
++ * distributed with this work for additional information
++ * regarding copyright ownership. The ASF licenses this file
++ * to you under the Apache License, Version 2.0 (the
++ * "License"); you may not use this file except in compliance
++ * with the License. You may obtain a copy of the License at
++ *
++ * http://www.apache.org/licenses/LICENSE-2.0
++ *
++ * Unless required by applicable law or agreed to in writing,
++ * software distributed under the License is distributed on an
++ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
++ * KIND, either express or implied. See the License for the
++ * specific language governing permissions and limitations
++ * under the License.
++ */
+import { EchartsTimeseriesChartPlugin } from '@superset-ui/plugin-chart-echarts';
+
+describe('@superset-ui/plugin-chart-echarts', () => {
+ it('exists', () => {
Review Comment:
nit: let's replace `describe` and `it` with just `test` blocks.
--
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