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

fix: use dynamic time_grains for schema #14009

Merged

Conversation

eschutho
Copy link
Member

@eschutho eschutho commented Apr 8, 2021

SUMMARY

When testing out TIME_GRAIN_ADDONS from the config, although new time grains are working for line charts, they don't work for table charts:
Explore_-_Untitled_Query_2_04_07_2021_18_50_18

This PR uses the built in time grains from the db_engine_spec and adds the time_grain_addons from the config to make the schema fields for the charts

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

after:
Explore_-_Untitled_Query_2_04_07_2021_18_50_18

TEST PLAN

I didn't see any unit tests for schemas. It's typically a very static file. I can add a new test if it feels warranted. Otherwise, to manually test, add a new time grain for the selected db and test with different types of charts.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • 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

@eschutho
Copy link
Member Author

eschutho commented Apr 8, 2021

@betodealmeida @villebro

@codecov
Copy link

codecov bot commented Apr 8, 2021

Codecov Report

Merging #14009 (9f3c0dc) into master (f06d534) will decrease coverage by 0.35%.
The diff coverage is 100.00%.

❗ Current head 9f3c0dc differs from pull request most recent head 4e929c7. Consider uploading reports for the commit 4e929c7 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #14009      +/-   ##
==========================================
- Coverage   79.21%   78.86%   -0.36%     
==========================================
  Files         936      940       +4     
  Lines       47408    47651     +243     
  Branches     5940     5998      +58     
==========================================
+ Hits        37554    37578      +24     
- Misses       9727     9946     +219     
  Partials      127      127              
Flag Coverage Δ
cypress 56.04% <100.00%> (ø)
hive 80.28% <100.00%> (+<0.01%) ⬆️
mysql ?
postgres ?
presto 80.30% <100.00%> (+<0.01%) ⬆️
python 80.79% <100.00%> (-0.36%) ⬇️
sqlite ?

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

Impacted Files Coverage Δ
...end/src/components/IndeterminateCheckbox/index.tsx 94.11% <ø> (ø)
...ntend/src/dashboard/components/CssEditor/index.jsx 70.83% <ø> (ø)
...nd/src/explore/components/DataTablesPane/index.tsx 57.74% <ø> (ø)
...ols/MetricControl/AdhocMetricEditPopover/index.jsx 77.09% <ø> (ø)
...et-frontend/src/components/TableView/TableView.tsx 96.42% <100.00%> (ø)
superset/charts/schemas.py 100.00% <100.00%> (ø)
superset/sql_validators/postgres.py 50.00% <0.00%> (-50.00%) ⬇️
...frontend/src/explore/components/DataTablesPane.tsx 24.69% <0.00%> (-38.28%) ⬇️
.../controls/MetricControl/AdhocMetricEditPopover.jsx 57.57% <0.00%> (-21.97%) ⬇️
superset/databases/commands/create.py 82.35% <0.00%> (-9.81%) ⬇️
... and 21 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 f06d534...4e929c7. Read the comment docs.

@eschutho
Copy link
Member Author

eschutho commented Apr 8, 2021

failed test looks like a connection error.. rerunning.

@eschutho eschutho closed this Apr 8, 2021
@eschutho eschutho reopened this Apr 8, 2021
@eschutho
Copy link
Member Author

eschutho commented Apr 8, 2021

front-end check error is an npm timeout, and I ran the tests/datasets/api_tests.py::TestDatasetApi::test_update_dataset_item_w_override_columns test several times locally and it passes every time.
It doesn't seem to be related, but lmk if someone thinks it is:

assert columns[0].column_name == dataset_data["columns"][0]["column_name"]
E       AssertionError: assert 'id' == 'new_col'

@eschutho
Copy link
Member Author

eschutho commented Apr 8, 2021

🏷 rush!

Copy link
Member

@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!

@eschutho
Copy link
Member Author

eschutho commented Apr 8, 2021

Thanks @suddjian and @villebro!
🏷 ready-to-merge

@superset-github-bot superset-github-bot bot added the need:merge The PR is ready to be merged label Apr 8, 2021
@betodealmeida betodealmeida merged commit 667eb83 into apache:master Apr 8, 2021
@yousoph
Copy link
Member

yousoph commented Apr 8, 2021

🏷️ 2021.13

@eschutho eschutho deleted the elizabeth/dynamic-time-grain-schema branch April 8, 2021 22:43
henryyeh pushed a commit to preset-io/superset that referenced this pull request Apr 9, 2021
allanco91 pushed a commit to allanco91/superset that referenced this pull request May 21, 2021
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
@geido geido added explore:time Related to the time filters in Explore and removed viz:explore:grain labels Feb 9, 2022
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.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 explore:time Related to the time filters in Explore need:merge The PR is ready to be merged preset-io size/M 🚢 1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants