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.

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

test: rename column names for birth_names dataset #12082

Merged
merged 2 commits into from
Jan 8, 2021

Conversation

ktmud
Copy link
Member

@ktmud ktmud commented Dec 16, 2020

SUMMARY

Rename sum_girls to num_girls and sum_boys to num_boys in the example birth_names dataset as they are kind of confusing when used in combination with adhoc metrics.

Snip20201216_4

Must wait for: apache-superset/examples-data#7

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TEST PLAN

CI and local visual verification

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@junlincc
Copy link
Member

thanks for making the change.
for transparency - mihir and i had some confusion while auditing the table viz control panel by mistaken sum_girls as a aggregated field.

@codecov-io
Copy link

codecov-io commented Dec 16, 2020

Codecov Report

Merging #12082 (34e29a4) into master (fd15dff) will decrease coverage by 3.55%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12082      +/-   ##
==========================================
- Coverage   66.36%   62.81%   -3.56%     
==========================================
  Files        1014     1014              
  Lines       49513    49499      -14     
  Branches     5066     5066              
==========================================
- Hits        32861    31094    -1767     
- Misses      16508    18196    +1688     
- Partials      144      209      +65     
Flag Coverage Δ
cypress ?
javascript 60.69% <ø> (ø)
python 64.20% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset/examples/birth_names.py 98.83% <100.00%> (ø)
superset-frontend/src/SqlLab/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/dashboard/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/chart/ChartContainer.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/reducers/index.js 0.00% <0.00%> (-100.00%) ⬇️
...et-frontend/src/dashboard/containers/Dashboard.jsx 0.00% <0.00%> (-100.00%) ⬇️
...et-frontend/src/filters/components/Select/types.ts 0.00% <0.00%> (-100.00%) ⬇️
...t-frontend/src/explore/reducers/getInitialState.js 0.00% <0.00%> (-100.00%) ⬇️
...-frontend/src/visualizations/presets/MainPreset.js 0.00% <0.00%> (-100.00%) ⬇️
... and 168 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 fd15dff...34e29a4. Read the comment docs.

@ktmud ktmud closed this Dec 16, 2020
@ktmud ktmud reopened this Dec 16, 2020
@ktmud ktmud force-pushed the birth-names-columns branch from 952da23 to 80bd4d5 Compare January 6, 2021 00:42
@@ -63,7 +63,7 @@ def gen_filter(


def load_data(tbl_name: str, database: Database, sample: bool = False) -> None:
pdf = pd.read_json(get_example_data("birth_names.json.gz"))
pdf = pd.read_json(get_example_data("birth_names2.json.gz"))
Copy link
Member Author

Choose a reason for hiding this comment

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

The dataset was updated in apache-superset/examples-data#7

@ktmud ktmud force-pushed the birth-names-columns branch from ac3cbf6 to 34e29a4 Compare January 8, 2021 20:19
@ktmud ktmud requested a review from junlincc January 8, 2021 20:50
Copy link
Member

@junlincc junlincc left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you ktmud!
Screen Shot 2021-01-08 at 2 09 56 PM

fyi: @adam-stasiak you may wanna update test cases accordingly

@junlincc junlincc merged commit 2210015 into apache:master Jan 8, 2021
@ktmud ktmud deleted the birth-names-columns branch January 9, 2021 02:08
@robdiciuccio
Copy link
Member

Looks like this introduced a regression with example dashboards. @junlincc let's ensure PRs are code reviewed before being approved.

@ktmud
Copy link
Member Author

ktmud commented Jan 22, 2021

I wouldn't categorize this as a regression, at most an inconvenience for developers. I personally would nuke examples database all the time whenever a new example dashboard is added.

We were aware of the risk of breaking the example dashboard, but decided to take the risk. In addition to much more work, it would also be a little weird to write a db migration or anything special just for this when most production deployments wouldn't load the examples to begin with.

Maybe better documentation next time.

@junlincc
Copy link
Member

junlincc commented Jan 22, 2021

@robdiciuccio i don't consider this as a regression neither, it's an improvement. As I added in the comment, those column names are not clear which mislead users to read the chart. And i actually did the code review as well, I understood all the changes in 11 files.

In terms of the risk of breaking dashboard, I specifically asked Adam to update the test case accordingly.
Screen Shot 2021-01-22 at 1 14 22 PM

@robdiciuccio
Copy link
Member

Definitely seems worthy of a mention in UPDATING.md at the very least, as use cases for example dashboards are clearly different for different organizations.

@junlincc
Copy link
Member

junlincc commented Jan 23, 2021

sure.
i also suggest organizations to read through PR's description and details to understand the changes and risks, before merging a PR into their fork.

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.0.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 1.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants