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(explore): support saving undefined time grain #22565

Merged
merged 3 commits into from
Jan 3, 2023

Conversation

villebro
Copy link
Member

@villebro villebro commented Jan 2, 2023

SUMMARY

Continuing the work from #21644, when saving a chart without a time grain, the chart reverts to the default time grain after the chart reloads in Explore view. To get around this, the user must choose the "Original value" option from the time grain dropdown. While this works, there should only be one way of clearing the time grain. A summary of changes introduced by this PR:

  • When saving the chart with the time grain control in cleared state, the time_grain_sqla entry is removed from the form data. To get around this, we check if the chart has been saved by looking at the metadata property of the chart state - if it's defined, we know the chart has been saved, in which case we know that the time grain has been explicitly set to undefined.
  • Remove the "Original value" option from the time grain dropdown. After this the time grain should be cleared to be removed.
  • Add a "None" placeholder when the time grain is cleared. Use the same placeholder for "Series limit", as that seems more appropriate for that one, too (currently it shows "Select ...")

The change has been tested to work in the following scenarios:

  • Chart saved with "Original value" prior to the change - it renders the chart without the time grain as expected, but with the time grain control in cleared state (as opposed to with "Original value")
  • Chart saved with time grain
  • Chart saved without time grain
  • All of the above on a dashboard (verified by looking at the rendered query to confirm that the time grain was applied correctly)

AFTER

With these changes, the time grain remains cleared even after saving:

grain-after.mp4

BEFORE

Previously, the chart would revert back to the default time grain P1D when the chart reloaded in Explore view:

grain-before.mp4

TESTING INSTRUCTIONS

  1. Create a line chart and remove the time grain
  2. Save the chart and notice that the default time grain is reapplied.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • 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

@codecov
Copy link

codecov bot commented Jan 2, 2023

Codecov Report

Merging #22565 (38d02a1) into master (38d02a1) will not change coverage.
The diff coverage is n/a.

❗ Current head 38d02a1 differs from pull request most recent head b93d6e3. Consider uploading reports for the commit b93d6e3 to get more accurate results

@@           Coverage Diff           @@
##           master   #22565   +/-   ##
=======================================
  Coverage   66.91%   66.91%           
=======================================
  Files        1851     1851           
  Lines       70709    70709           
  Branches     7766     7766           
=======================================
  Hits        47316    47316           
  Misses      21371    21371           
  Partials     2022     2022           
Flag Coverage Δ
hive 52.46% <0.00%> (ø)
javascript 53.85% <0.00%> (ø)
mysql 77.95% <0.00%> (ø)
postgres 78.03% <0.00%> (ø)
presto 52.36% <0.00%> (ø)
python 81.27% <0.00%> (ø)
sqlite 76.49% <0.00%> (ø)
unit 51.19% <0.00%> (ø)

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

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@apache apache deleted a comment from github-actions bot Jan 2, 2023
@zhaoyongjie
Copy link
Member

/testenv up FEATURE_GENERIC_CHART_AXES=true

@github-actions
Copy link
Contributor

github-actions bot commented Jan 2, 2023

@zhaoyongjie Ephemeral environment spinning up at http://35.91.67.201:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@@ -97,7 +97,6 @@ class TimeGrain(NamedTuple):


builtin_time_grains: Dict[Optional[str], str] = {
None: __("Original value"),
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for getting rid of this option.

@villebro villebro merged commit a7a4561 into apache:master Jan 3, 2023
@villebro villebro deleted the villebro/none-timegrain branch January 3, 2023 08:54
@github-actions
Copy link
Contributor

github-actions bot commented Jan 3, 2023

Ephemeral environment shutdown and build artifacts deleted.

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 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/S 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants