Chart - sum Y column values rather than displaying last Y value #4587

[akc2267]

What type of PR is this? (check all applicable)

• Bug Fix

Description

A couple fixes so that Y values are summed. Without this fix, charts will display the final row for every X value. If multiple rows exist for a given X, then those values will be over-written. Plotly displays this info correctly, so without this fix, redash's hover labels are incorrect while Plotly's visual representation is accurate.

Please ignore the package-lock.json and package.json changes. I couldn't figure out how to remove these.

Related Tickets & Documents

#4587

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Previous:

Now:

[akc2267]

apologies about the datree smart policy failure. I'm using my personal github account on my work laptop, which led to my work email getting caught and misconfigured

[kravets-levko]

Hi @akc2267! Thank you for your contribution, we really appreciate it! Few thing before I review this PR:

1. package upgrades are not related to the fix, so please revert package.json and package-lock.json changes;
2. fix unit tests (npm run test) - but please ensure that you're not just updating fixtures, but check that your changes don't break anything 🙇‍♂️
3. I'm not sure why other check failed (backend and e2e tests) - is this branch up to date with our master?

[akc2267]

I am amending the without-x pie chart tests so that we no longer assume an X when it is not explicitly given. This is similar to how redash currently behaves when there are more than 1 possible X, but my solution is more accurate.

Current when there is more than 1 possible X:

My solution:

[akc2267]

fixing the test further.
"data": [ { "x": "a1", "y": 10 }, { "x": "a2", "y": 60 }, { "x": "a3", "y": 100 }, { "x": "a4", "y": 30 } ]
should not create undefined-X labels of
"text": ["15% (30)", "15% (30)", "15% (30)", "15% (30)"]

rather, they should be aggregated into a single value "100% (200)", until the user specifies what variable should be X

[akc2267]

@kravets-levko

1. I reverted my package changes.

2. I ran npm run test successfully, after repairing the prepareData > pie without-x unit test.

3. I brought my branch up to date with master

4. I fixed my github email. I don't know why Datree is still complaining and I don't know how to resolve this further. I've followed their instructinos here https://docs.datree.io/docs/fix-unrecognized-committers

Please let me know if there's anything else I can do before you review!

[kravets-levko]

Thank you! You can ignore Datree check

[akc2267]

@kravets-levko,

Do you have an estimation on when this will be reviewed and merged?

I can't get more redash server capacity and my dashboard is prohibitively slow without this fix.

[akc2267]

@kravets-levko ?

[akc2267]

@robhudson @kravets-levko @jezdez @jeremi

Why hasn't this been reviewed yet? visualizations of tables with more than 2 columns are broken without this fix

[guidopetri]

@akc2267 , thanks for the PR! We've updated a lot of things now that we're Community-driven so - if you're still interested in getting this merged - would you mind rebasing off master to re-run the CI, as well as updating merge conflicts?

We're trying to clean up our PR todo list, so if you're not interested, that's fine - we'll close the PR in about a week if we don't hear back. If you're interested in reopening the PR afterwards, we would also very much welcome that.

[justinclift]

Ugh, it's a shame this got dropped. The concept and code both seem reasonable.

Oh well. 😦

[akc2267]

hey just saw these comments. Hopefully redash fixed this bug. It's been over 3 years since I've needed to use redash for work

[guidopetri]

@akc2267 no worries. If you want to reopen the PR, feel free - but no pressure either way.