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

feat: apply Time Grain to X-Axis column #21163

Merged
merged 27 commits into from
Sep 7, 2022

Conversation

zhaoyongjie
Copy link
Member

@zhaoyongjie zhaoyongjie commented Aug 23, 2022

SUMMARY

Currently, the Time Grain control defines a couple of date truncate functions on the Time Column. Time Grain only be applied if the Time column and X-Axis are set to the same value. This PR is the first step to removing the entire time section.

After this PR, when enabling the GENERIC_CHART_AXES

  1. Time Grain Control removes from Time Section.
  2. Time Grain Control will show if i) applies a Date Column in the X-Axis. ii) applies a Adhoc Column
  3. Time Grain Control will be displayed under X-Axis.

This change allows Time Column and Time Filter to use different columns, Time Series Chart will support different column to set Time Filter and X-Axis, and Time Grain will be applied to the X-Axis.

Supported Charts

  • Mixed Timeseries Chart
  • Area Chart
  • Bar V2 Chart
  • Line V2 Chart
  • Scatter Chart
  • SmoothLine Chart
  • Step Chart

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

SQL Pattern

image

Apply Categorical Column or Date Column to the X-Axis

Apply.Time.Grain.to.X-Axis.mov

Custom SQL

Custom.SQL.mov

TESTING INSTRUCTIONS

In order to test this PR, the multiple Date column should provide, so I prepare a CSV (superstore_orders.csv) file for that. Just load this CSV to test this PR.

  1. Native filters should work as before
  2. Time Grain should work as before
  3. Time Grain should apply to X-Axis

ADDITIONAL INFORMATION

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

@zhaoyongjie zhaoyongjie marked this pull request as ready for review August 23, 2022 14:25
@codecov
Copy link

codecov bot commented Aug 23, 2022

Codecov Report

Merging #21163 (1de9492) into master (f71ee2e) will decrease coverage by 0.02%.
The diff coverage is 84.09%.

❗ Current head 1de9492 differs from pull request most recent head f6d31b3. Consider uploading reports for the commit f6d31b3 to get more accurate results

@@            Coverage Diff             @@
##           master   #21163      +/-   ##
==========================================
- Coverage   66.45%   66.42%   -0.03%     
==========================================
  Files        1789     1789              
  Lines       68291    68322      +31     
  Branches     7273     7273              
==========================================
+ Hits        45382    45384       +2     
- Misses      21034    21063      +29     
  Partials     1875     1875              
Flag Coverage Δ
hive 53.04% <70.27%> (+0.02%) ⬆️
mysql 80.83% <81.08%> (+<0.01%) ⬆️
postgres ?
presto 52.94% <70.27%> (+0.02%) ⬆️
python 81.28% <81.08%> (-0.08%) ⬇️
sqlite 79.43% <81.08%> (+<0.01%) ⬆️
unit 50.74% <16.21%> (-0.04%) ⬇️

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

Impacted Files Coverage Δ
...t-controls/src/sections/echartsTimeSeriesQuery.tsx 50.00% <ø> (ø)
...ackages/superset-ui-core/src/query/types/Column.ts 100.00% <ø> (ø)
...chart-echarts/src/MixedTimeseries/controlPanel.tsx 54.16% <ø> (ø)
...chart-echarts/src/Timeseries/Area/controlPanel.tsx 40.00% <ø> (ø)
...charts/src/Timeseries/Regular/Bar/controlPanel.tsx 31.25% <ø> (ø)
...harts/src/Timeseries/Regular/Line/controlPanel.tsx 33.33% <ø> (ø)
...ts/src/Timeseries/Regular/Scatter/controlPanel.tsx 40.00% <ø> (ø)
...src/Timeseries/Regular/SmoothLine/controlPanel.tsx 40.00% <ø> (ø)
...chart-echarts/src/Timeseries/Step/controlPanel.tsx 33.33% <ø> (ø)
...perset/utils/pandas_postprocessing/contribution.py 100.00% <ø> (ø)
... and 21 more

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

@zhaoyongjie zhaoyongjie changed the title feat: time grain into column feat: move time grain into column Aug 24, 2022
@pull-request-size pull-request-size bot added size/L and removed size/M labels Aug 24, 2022
@zhaoyongjie zhaoyongjie force-pushed the feat/time_grain_into_column branch 2 times, most recently from 9188b3b to f1c8e04 Compare August 27, 2022 02:13
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.

Pretty cool stuff on the way here..! 👏 Left a few comments. Also, it's slightly difficult to give a good review in the absence of having seen designs or frontend POC code, so if there's anything like that out there I'd love to see it to get a more complete understanding of how this is meant to work eventually.

superset/superset_typing.py Outdated Show resolved Hide resolved
tests/integration_tests/query_context_tests.py Outdated Show resolved Hide resolved
superset/connectors/sqla/utils.py Show resolved Hide resolved
superset/connectors/sqla/utils.py Show resolved Hide resolved
superset/connectors/sqla/models.py Outdated Show resolved Hide resolved
@zhaoyongjie
Copy link
Member Author

Pretty cool stuff on the way here..! 👏 Left a few comments. Also, it's slightly difficult to give a good review in the absence of having seen designs or frontend POC code, so if there's anything like that out there I'd love to see it to get a more complete understanding of how this is meant to work eventually.

@villebro Thanks for the review! I will add a frontend part this week for convenience to review.

@zhaoyongjie zhaoyongjie changed the title feat: move time grain into column [WIP] feat: move time grain into column Aug 31, 2022
@zhaoyongjie zhaoyongjie changed the title [WIP] feat: move time grain into column [WIP] feat: apply time grain to X-Axis column Sep 1, 2022
@zhaoyongjie zhaoyongjie changed the title [WIP] feat: apply time grain to X-Axis column feat: apply time grain to X-Axis column Sep 1, 2022
@zhaoyongjie zhaoyongjie changed the title feat: apply time grain to X-Axis column feat: apply Time Grain to X-Axis column Sep 1, 2022
@apache apache deleted a comment from github-actions bot Sep 1, 2022
@apache apache deleted a comment from github-actions bot Sep 1, 2022
Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

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

Thank you for PR @zhaoyongjie. I left some suggestions.

@zhaoyongjie
Copy link
Member Author

@michael-s-molina Thanks for the review, I have addressed your comments.

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.

Tested to work with the FF enabled and disabled with both legacy and new Timeseries charts. A few minor nits, but apart from that LGTM and hoping to see this merged soon! Also, we may want to consider adding a line in UPDATING.md to state that with the feature flag enabled, the time grain control will move below the x-axis control.

Copy link
Member

@michael-s-molina michael-s-molina 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 for addressing the suggestions @zhaoyongjie. I agree with @villebro about adding something to UPDATING.md before merging this PR. Another suggestion would be to get approval from @jinghua-qa as well. Awesome work @zhaoyongjie!

@apache apache deleted a comment from github-actions bot Sep 2, 2022
@zhaoyongjie
Copy link
Member Author

/testenv up FEATURE_GENERIC_CHART_AXES=true FEATURE_DRILL_TO_DETAIL=true FEATURE_DASHBOARD_CROSS_FILTERS=true

@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2022

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

@jinghua-qa
Copy link
Member

Tested in ephemeral env, and the new feature LGTM.

However i have found 2 issues that also in master branch, i will file tickets to follow up.
1, default datetime is not autoselected as the x-axis when multiple column set is temporal

Screen.Recording.2022-09-07.at.1.07.16.AM.mov

2, when db=sqlite, and use custom sql in x-axis, time grain did not apply

Screen.Recording.2022-09-07.at.1.11.07.AM.mov

@zhaoyongjie
Copy link
Member Author

@jinghua-qa Thanks for testing, I'm gonna follow up in a separate PR.

@zhaoyongjie zhaoyongjie merged commit ce3d38d into apache:master Sep 7, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2022

Ephemeral environment shutdown and build artifacts deleted.

jinghua-qa added a commit to preset-io/superset that referenced this pull request Sep 20, 2022
@@ -27,6 +27,8 @@ assists people when migrating to a new version.
- [20606](https://github.com/apache/superset/pull/20606): When user clicks on chart title or "Edit chart" button in Dashboard page, Explore opens in the same tab. Clicking while holding cmd/ctrl opens Explore in a new tab. To bring back the old behaviour (always opening Explore in a new tab), flip feature flag `DASHBOARD_EDIT_CHART_IN_NEW_TAB` to `True`.
- [20799](https://github.com/apache/superset/pull/20799): Presto and Trino engine will now display tracking URL for running queries in SQL Lab. If for some reason you don't want to show the tracking URL (for example, when your data warehouse hasn't enable access for to Presto or Trino UI), update `TRACKING_URL_TRANSFORMER` in `config.py` to return `None`.
- [21002](https://github.com/apache/superset/pull/21002): Support Python 3.10 and bump pandas 1.4 and pyarrow 6.
- [21163](https://github.com/apache/superset/pull/21163): When `GENERIC_CHART_AXES` feature flags set to `True`, the Time Grain control will move below the X-Axis control.
Copy link
Member

Choose a reason for hiding this comment

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

@zhaoyongjie would you mind expanding on this bullet point? From reading through the PR description et al. it seems that the biggest change is that the time grain is now decoupled from the time filter column. As it currently reads now "... will move below the X-Axis control." it sounds like it's purely a cosmetic change as opposed to a scoping change.

Copy link
Member Author

Choose a reason for hiding this comment

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

@john-bodley update the document at PR

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/XL 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants