Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(partition): zero value sectors cause max stack call #2260

Merged
merged 3 commits into from
Nov 29, 2023
Merged

fix(partition): zero value sectors cause max stack call #2260

merged 3 commits into from
Nov 29, 2023

Conversation

markov00
Copy link
Member

@markov00 markov00 commented Nov 28, 2023 •
edited

Summary

After this change #1983 sectors with zero can appear in the data transformation. This could cause a max call stack error due to a uncached division by zero.

The PR fixes the division without touching the current recursive loop.

Issues

will fix elastic/kibana#164730 when merged into Kibana

Checklist

  • The proper chart type label has been added (e.g. :xy, :partition)
  • The proper feature labels have been added (e.g. :interactions, :axis)
  • All related issues have been linked (i.e. closes #123, fixes #123)
  • Unit tests have been added or updated to match the most common scenarios
  • The proper documentation and/or storybook story has been added or updated

@markov00 markov00 added bug Something isn't working :data Data/series/scales related issue :partition Partition/PieChart/Donut/Sunburst/Treemap chart related labels Nov 28, 2023
@markov00 markov00 force-pushed the 2023_11_28-fix_division_by_zero branch from 9131972 to e314c70 Compare November 28, 2023 11:34
@markov00 markov00 marked this pull request as ready for review November 28, 2023 14:30
dej611
dej611 approved these changes Nov 28, 2023
View reviewed changes
Copy link
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review 👍

@markov00 markov00 merged commit 4b30db7 into elastic:main Nov 29, 2023
13 checks passed
@markov00 markov00 deleted the 2023_11_28-fix_division_by_zero branch November 29, 2023 10:33
markov00 added a commit to markov00/elastic-charts that referenced this pull request Dec 7, 2023
@markov00
fix(partition): zero value sectors cause max stack call (elastic#2260)
fd7c27a
@markov00 markov00 mentioned this pull request Dec 7, 2023
Merged
markov00 added a commit that referenced this pull request Dec 7, 2023
@markov00
fix(partition): zero value sectors cause max stack call (#2260) [60.x] (
5034852 
#2274)

fix(partition): zero value sectors cause max stack call (#2260)
nickofthyme pushed a commit that referenced this pull request Dec 7, 2023
@semantic-release-bot
chore(release): 60.0.1 [skip ci]
aab63c7 
## [60.0.1](v60.0.0...v60.0.1) (2023-12-07)

### Bug Fixes

* clamp brushing min of last bucket ([#2227](#2227)) [60.x] ([#2276](#2276)) ([7582811](7582811))
* **partition:** zero value sectors cause max stack call ([#2260](#2260)) [60.x] ([#2274](#2274)) ([5034852](5034852))
nickofthyme pushed a commit that referenced this pull request Dec 19, 2023
@semantic-release-bot
chore(release): 61.2.0 [skip ci]
dc705ad 
# [61.2.0](v61.1.0...v61.2.0) (2023-12-19)

### Bug Fixes

* **deps:** update dependency @elastic/eui to ^91.1.0 ([#2267](#2267)) ([308e974](308e974))
* **deps:** update dependency @elastic/eui to ^91.2.0 ([#2268](#2268)) ([29cdcb3](29cdcb3))
* **metric:** background colors and sparkline rendering ([#2255](#2255)) ([5abddfc](5abddfc))
* **partition:** rendering with small radius ([#2273](#2273)) ([95a8537](95a8537))
* **partition:** zero value sectors cause max stack call ([#2260](#2260)) ([4b30db7](4b30db7))
* **theme:** legacy margins ([#2262](#2262)) ([299c869](299c869))

### Features

* increase tooltip width to 500px and truncate items to 2 lines ([#2261](#2261)) ([afdef1c](afdef1c))
@markov00 markov00 mentioned this pull request Dec 20, 2023
Merged
1 task
markov00 added a commit to elastic/kibana that referenced this pull request Dec 21, 2023
@renovate @markov00
Update dependency @elastic/charts to v61.2.0 (main) (#173329)
68883b3 
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [@elastic/charts](https://togithub.com/elastic/elastic-charts) |
[`61.0.3` ->
`61.2.0`](https://renovatebot.com/diffs/npm/@elastic%2fcharts/61.0.3/61.2.0)
|
[![age](https://developer.mend.io/api/mc/badges/age/npm/@elastic%2fcharts/61.2.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/@elastic%2fcharts/61.2.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/@elastic%2fcharts/61.0.3/61.2.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/@elastic%2fcharts/61.0.3/61.2.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>elastic/elastic-charts (@&#8203;elastic/charts)</summary>

###
[61.2.0](elastic/elastic-charts@v61.1.0...v61.2.0)
(2023-12-19)


##### Bug Fixes

* **deps:** update dependency @elastic/eui to ^91.1.0
([#2267](elastic/elastic-charts#2267))
([308e974](elastic/elastic-charts@308e974))
* **deps:** update dependency @elastic/eui to ^91.2.0
([#2268](elastic/elastic-charts#2268))
([29cdcb3](elastic/elastic-charts@29cdcb3))
* **metric:** background colors and sparkline rendering
([#2255](elastic/elastic-charts#2255))
([5abddfc](elastic/elastic-charts@5abddfc))
* **partition:** rendering with small radius
([#2273](elastic/elastic-charts#2273))
([95a8537](elastic/elastic-charts@95a8537))
* **partition:** zero value sectors cause max stack call
([#2260](elastic/elastic-charts#2260))
([4b30db7](elastic/elastic-charts@4b30db7))
* **theme:** legacy margins
([#2262](elastic/elastic-charts#2262))
([299c869](elastic/elastic-charts@299c869))


##### Features

* increase tooltip width to 500px and truncate items to 2 lines
([#2261](elastic/elastic-charts#2261))
([afdef1c](elastic/elastic-charts@afdef1c))

###
[`v61.1.0`](https://togithub.com/elastic/elastic-charts/blob/HEAD/CHANGELOG.md#6110-2023-11-20)

[Compare
Source](https://togithub.com/elastic/elastic-charts/compare/v61.0.3...v61.1.0)

##### Bug Fixes

- **deps:** update dependency
[@&#8203;elastic/eui](https://togithub.com/elastic/eui) to v91
([#&#8203;2233](https://togithub.com/elastic/elastic-charts/issues/2233))
([e89f623](https://togithub.com/elastic/elastic-charts/commit/e89f623792312c4f6b609ebb975de0800f3c297e))
- **metric:** add option to set empty cell background color
([#&#8203;2247](https://togithub.com/elastic/elastic-charts/issues/2247))
([6a9fb86](https://togithub.com/elastic/elastic-charts/commit/6a9fb86bee5212a47060c5070f260961097014b4))
- **metric:** background color for bar with interactions
([#&#8203;2248](https://togithub.com/elastic/elastic-charts/issues/2248))
([dcb56fa](https://togithub.com/elastic/elastic-charts/commit/dcb56fa08540631a9b3b0e588352ee6daf3d54a0))

##### Features

- **bullet:** improve header layout and positioning
([#&#8203;2243](https://togithub.com/elastic/elastic-charts/issues/2243))
([b3a95d2](https://togithub.com/elastic/elastic-charts/commit/b3a95d24fb02690ca6599622352c743c04624690))
- **bullet:** new design style and implementation
([#&#8203;2156](https://togithub.com/elastic/elastic-charts/issues/2156))
([fca6cdd](https://togithub.com/elastic/elastic-charts/commit/fca6cdd5bc34a65c0792dbab7d756404bf43501b))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/elastic/kibana).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy44Ny4yIiwidXBkYXRlZEluVmVyIjoiMzcuMTAzLjEiLCJ0YXJnZXRCcmFuY2giOiJtYWluIn0=-->

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Marco Vettorello <marco.vettorello@elastic.co>
@nickofthyme nickofthyme mentioned this pull request Jan 30, 2024
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dej611 dej611 dej611 approved these changes

@nickofthyme nickofthyme Awaiting requested review from nickofthyme

Assignees
No one assigned
Labels
bug Something isn't working :data Data/series/scales related issue :partition Partition/PieChart/Donut/Sunburst/Treemap chart related
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Lens] Treemap error on sparse table
2 participants
@markov00 @dej611