Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

fix(legacy-plugin-chart-pivot-table): pivot table chart string aggregation empty values #880

Merged
merged 12 commits into from
Jan 11, 2021

Conversation

mayurnewase
Copy link
Contributor

@mayurnewase mayurnewase commented Dec 23, 2020

🐛 Bug Fix
Fixes; #868

@mayurnewase mayurnewase requested a review from a team as a code owner December 23, 2020 19:24
@vercel
Copy link

vercel bot commented Dec 23, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/superset/superset-ui/7ai9w2ys2
✅ Preview: https://superset-ui-git-fork-mayurnewase-fix-868.superset.vercel.app

@codecov
Copy link

codecov bot commented Dec 24, 2020

Codecov Report

Merging #880 (786e2df) into master (8b28452) will increase coverage by 0.14%.
The diff coverage is 53.12%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #880      +/-   ##
==========================================
+ Coverage   26.58%   26.73%   +0.14%     
==========================================
  Files         380      405      +25     
  Lines        8192     8248      +56     
  Branches     1118     1126       +8     
==========================================
+ Hits         2178     2205      +27     
- Misses       5886     5914      +28     
- Partials      128      129       +1     
Impacted Files Coverage Δ
.../legacy-plugin-chart-pivot-table/src/PivotTable.js 0.00% <0.00%> (ø)
...-plugin-chart-pivot-table/src/utils/formatCells.ts 70.83% <70.83%> (ø)
...plugin-chart-echarts/src/BoxPlot/transformProps.ts 50.00% <0.00%> (-2.64%) ⬇️
plugins/plugin-chart-table/test/testData.ts 83.33% <0.00%> (ø)
plugins/plugin-chart-table/src/controlPanel.tsx 37.03% <0.00%> (ø)
packages/superset-ui-core/src/models/Registry.ts 100.00% <0.00%> (ø)
plugins/plugin-chart-table/src/transformProps.ts 71.60% <0.00%> (ø)
...ackages/superset-ui-core/src/query/api/v1/index.ts 100.00% <0.00%> (ø)
...ckages/superset-ui-chart-controls/src/sections.tsx 100.00% <0.00%> (ø)
...gins/legacy-preset-chart-nvd3/src/NVD3Controls.tsx 0.00% <0.00%> (ø)
... and 60 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b28452...786e2df. Read the comment docs.

@mayurnewase mayurnewase changed the title [WIP] fix: pivot table chart string aggregation empty values fix: pivot table chart string aggregation empty values Dec 24, 2020
@villebro villebro self-assigned this Dec 24, 2020
@villebro
Copy link
Contributor

@mayurnewase this seems to reintroduce displaying of null values (see #839). It would be great to add a test that ensures that "null" cell values are replaced with "" and add a test to ensure that those are handled properly. Other than that looks great!

Copy link
Contributor

@villebro villebro left a comment

Choose a reason for hiding this comment

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

A few minor comments, other than that LGTM!

@mayurnewase
Copy link
Contributor Author

mayurnewase commented Jan 10, 2021

@villebro Getting float values from db_engine's cursor.fetchall() for avg(any_string_column),it doesn't seem to be introduced by this pr.
in sql lab,
image

@mayurnewase mayurnewase requested a review from villebro January 10, 2021 06:23
Copy link
Contributor

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM, thanks so much for fixing and improving this code @mayurnewase !

@villebro villebro changed the title fix: pivot table chart string aggregation empty values fix(legacy-plugin-chart-pivot-table): pivot table chart string aggregation empty values Jan 11, 2021
@villebro villebro merged commit cc11fdb into apache-superset:master Jan 11, 2021
}
}
// @ts-ignore
const attr = ('data-sort', sortAttributeValue);
Copy link
Contributor

Choose a reason for hiding this comment

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

Eh... I don't think this is a valid JavaScript syntax....

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants