You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "qoega (via GitHub)" <gi...@apache.org> on 2023/09/08 18:24:20 UTC
[GitHub] [superset] qoega opened a new pull request, #25236: [WIP] fix(superset-frontend): Partition chart multiple fixes
qoega opened a new pull request, #25236:
URL: https://github.com/apache/superset/pull/25236
### SUMMARY
We(https://github.com/clickHouse/clickHouse) are using Superset for internal analytics and found Partition charts to be very useful. But bugs such as https://github.com/apache/superset/issues/10586 make this powerful feature almost unusable.
- Debugging showed that order of nodes with depth more than 2 is not ordered within parent node.
- Issue is related to partition limit feature that removes nodes from a tree if number of nodes under the single parent is higher than the limit. It dropped data without leaving sum of values dropped and thus icicle node scale was inconsistent.
- Feature partition threshold never worked as 0-1 float parameter was parsed as integer 0 all the time.
- Some recent changes removed time column picker that did not allow to use types other than `Not a time series`
- Incorrect logic to compute place of deeper nodes did not start to draw child nodes within parent height
Proposed solution:
- Feature `Partition threshold` was fixed to get correct setting value
- Added time column picker back
- A separate node with `<other>` name is created to replace all removed nodes within the same parent. This node has cumulative value of all removed nodes. It allows to see significance of pruned partitions and have correct scale for all depth levels. Partition threshold still allows to improve drawing speed significantly for results with many nodes. Ordering of drawn nodes adds `<other>` node after all remaining nodes within same parent
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
1. Partition Limit feature
BEFORE: Partition Limit=2. Removed partitions are just skipped. 4th level nodes are shuffled(not shown within parent/prefix).
<img width="1436" alt="Screenshot 2023-09-07 at 21 21 33" src="https://github.com/apache/superset/assets/2159081/3f5b653e-abc8-4d00-8642-92dcb943759d">
AFTER: Partition Limit=2. Smaller nodes replaced to `<other>`. Child nodes are drawn within height.
<img width="1428" alt="Screenshot 2023-09-07 at 21 20 06" src="https://github.com/apache/superset/assets/2159081/293cbd96-a5ce-4fc7-acf1-ac2b531e20a4">
2. Partition threshold
BEFORE:
- Feature does not work at all. All nodes are drawn as 0.5 is parsed as 0.
- Order of 4th column does not respect parents at all which violates the main icicle visualisation logic
<img width="1445" alt="Screenshot 2023-09-07 at 21 21 21" src="https://github.com/apache/superset/assets/2159081/c70b30b8-a577-4f38-a7a0-59db968868de">
AFTER:
- Nodes with less than 0.5 share within parent are removed and substituted with `<other>`
<img width="1436" alt="Screenshot 2023-09-07 at 21 20 32" src="https://github.com/apache/superset/assets/2159081/12ddb4bc-2e0d-49e5-8f81-ed7f3c15e74b">
3. No partition limit or threshold
BEFORE:
- Order of 4th level is incorrect and is mixed between different parents.
<img width="1445" alt="Screenshot 2023-09-07 at 21 21 10" src="https://github.com/apache/superset/assets/2159081/88da8c5f-0f61-435a-bf9f-0ac9b5ca838d">
TBD
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
Create a Partition chart.
Choose COUNT(*) as metric and add several (3+) dimensions.
Set Partition Limit to 0
Set Partition Limit to 2
Set Partition Limit to 0 and Partition Threshold to 0.8
Set Partition Limit to 2 and Partition Threshold to 0.8
Set Partition Limit to 0 and Partition Threshold to 0
### 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:
- [x] 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
Re: [PR] [WIP] fix(superset-frontend): Partition chart multiple fixes [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #25236:
URL: https://github.com/apache/superset/pull/25236#issuecomment-1945413089
@qoega let us know if you can rebase this PR and address the questions on the thread. Meanwhile, I'll set it to draft mode, but please set it as "ready for review" when the time is right. Thanks in advance!
--
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
Re: [PR] fix(superset-frontend): [WIP] Partition chart multiple fixes [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas closed pull request #25236: fix(superset-frontend): [WIP] Partition chart multiple fixes
URL: https://github.com/apache/superset/pull/25236
--
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
Re: [PR] fix(superset-frontend): [WIP] Partition chart multiple fixes [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #25236:
URL: https://github.com/apache/superset/pull/25236#issuecomment-2529162728
Closing this since it hasn't been touched in over a year, but happy to reopen it if you ever want to revisit/update 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] michael-s-molina commented on pull request #25236: [WIP] fix(superset-frontend): Partition chart multiple fixes
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #25236:
URL: https://github.com/apache/superset/pull/25236#issuecomment-1726334913
Thank you for the PR @qoega. As part of [[SIP-50] Proposal for using ECharts as our main charting library](https://github.com/apache/superset/issues/10418), we're migrating legacy charts to ECharts. In that context, I have some questions:
- Do you think we could increment the [Treemap](https://github.com/apache/superset/tree/master/superset-frontend/plugins/plugin-chart-echarts/src/Treemap) chart and use it as the replacement for the Partition chart? It seems that partition size and threshold could be valid controls for that chart.
- If not, would you be interested in creating the Partition chart using ECharts instead of fixing the legacy 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