fix: Time shifts with different granularity for ECharts #24176

michael-s-molina · 2023-05-22T16:50:34Z

SUMMARY

There's an option in the Advanced Analytics section of Explore called Time Shift that allows an user to compare the query results with the same query dislocated in time by the amount specified in the control. The way this is implemented, is to fire a query for each specified Time Shift and later join the results with the original query. To make the join work the algorithm modifies the temporal column of the time shift results to match the one in the original query. The problem is that the algorithm is not taking into account that the time granularity and time shifts can influence on the temporal column and affect the resulting timestamps. For example, if the Time Grain is Quarter and Time Shift is 28 days, the results could have different timestamps but still belong to the same quarter. Another example is when the Time Grain is Week and after applying the time shifts, the results fall in a different year which may produce different week numbers for the timestamps. The objective of this PR is to fix these problems by generating an artificial join key that takes into consideration the Time Grain when joining the results.

The following table illustrates how the join keys are calculated depending on the Time Grain:

Time Grain	Join key
Week	year-week
Month	year-month
Quarter	year-quarter
Year	year

This join key is only used when joining the results and is discarded in the end.

This PR also improves the Python codebase by creating a TimeGrain class and replacing all string literals with references to the new class.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

1 - Choose a dataset with plenty of temporal data available
2 - Create a Line Chart (ECharts)
3 - Play with different Time Grains and Time Shifts
4 - Make sure the offsets are correctly calculated

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

villebro

I know it's still in draft, but a few initial thoughts

superset/common/query_context_processor.py

superset/db_engine_specs/ascend.py

superset/db_engine_specs/base.py

codecov · 2023-05-31T18:29:00Z

Codecov Report

Merging #24176 (559b370) into master (1d9a761) will increase coverage by 0.15%.
The diff coverage is 97.12%.

❗ Current head 559b370 differs from pull request most recent head 13ed830. Consider uploading reports for the commit 13ed830 to get more accurate results

@@            Coverage Diff             @@
##           master   #24176      +/-   ##
==========================================
+ Coverage   68.29%   68.45%   +0.15%     
==========================================
  Files        1957     1951       -6     
  Lines       75624    75528      -96     
  Branches     8223     8218       -5     
==========================================
+ Hits        51649    51699      +50     
+ Misses      21867    21718     -149     
- Partials     2108     2111       +3

Flag	Coverage Δ
hive	`53.53% <66.41%> (+0.13%)`	⬆️
postgres	`79.14% <92.36%> (+0.14%)`	⬆️
presto	`53.45% <66.41%> (+0.13%)`	⬆️
python	`82.97% <98.47%> (+0.19%)`	⬆️
sqlite	`77.69% <92.36%> (+0.15%)`	⬆️
unit	`53.92% <77.09%> (+0.48%)`	⬆️

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

Impacted Files	Coverage Δ
...s/plugin-chart-pivot-table/src/PivotTableChart.tsx	`0.00% <0.00%> (ø)`
...rset-frontend/src/components/ReportModal/index.tsx	`79.03% <ø> (ø)`
...src/dashboard/components/PropertiesModal/index.tsx	`64.14% <ø> (+0.10%)`	⬆️
...ponents/controls/VizTypeControl/VizTypeGallery.tsx	`88.23% <ø> (ø)`
...-frontend/src/features/alerts/AlertReportModal.tsx	`54.44% <ø> (ø)`
...-frontend/src/visualizations/presets/MainPreset.js	`100.00% <ø> (ø)`
superset/charts/post_processing.py	`92.59% <ø> (+3.23%)`	⬆️
superset/examples/birth_names.py	`70.00% <ø> (ø)`
superset/examples/supported_charts_dashboard.py	`0.00% <ø> (ø)`
superset/examples/world_bank.py	`30.66% <ø> (ø)`
... and 44 more

... and 2 files with indirect coverage changes

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

zhaoyongjie · 2023-05-31T22:13:11Z

@michael-s-molina

IMO, the renaming/refining of TimeGrain should be put in a separate chore pull request rather than this one.
I can't get result of your screenshot. (on this PR branch)

BTW, If you could provide a dataset for testing, it would be helpful in testing this PR. We should use a same dataset to test the accuracy of the result.

zhaoyongjie · 2023-05-31T22:20:58Z

Additionally, I believe that the time grain and time offset granularity should be consistent. We might use same dataset to discuss this topic.

michael-s-molina · 2023-06-01T14:19:39Z

IMO, the renaming/refining of TimeGrain should be put in a separate chore pull request rather than this one.

I could do that but I guess I was excited to fix the problem and ended up doing all at once. Sorry if this makes it more difficult to review but I think the important part is that I took the time to improve the code base.

I can't get result of your screenshot. (on this PR branch)

It was not working indeed with GENERIC_CHART_AXES enabled because the query object didn't contain a time_grain_sqla field. I submitted a fix to get this from the form_data and now it should be working.

superset/common/query_context_processor.py

zhaoyongjie · 2023-06-02T18:09:35Z

superset/common/query_context_processor.py

                offset_metrics_df[index] = offset_metrics_df[index] - DateOffset(
                    **normalize_time_delta(offset)
                )


the index of offset_metrics_df seems constructing an appropriate index for the result of dataframe, if the align of the results incorrect, I think we should find root cause from here.

I'm not sure if I understand what you're suggesting. Can you explain more?

Thanks for provide more information. If I have more time, will continue review.

villebro

A few second pass comments

superset/common/query_context_processor.py

superset/db_engine_specs/base.py

tests/unit_tests/common/test_get_aggregated_join_column.py

michael-s-molina · 2023-06-07T14:18:57Z

@villebro I addressed your comments.

villebro

A few small optional stylistic comments

superset/constants.py

superset/db_engine_specs/base.py

tests/unit_tests/common/test_get_aggregated_join_column.py

villebro

LGTM!

zhaoyongjie · 2023-06-08T16:44:55Z

@villebro @michael-s-molina I'll take another look this PR, give me some time. Thanks!

zhaoyongjie · 2023-06-08T18:55:38Z

@michael-s-molina @villebro

Draft a PR to refine the join key generator. please review. Thanks both!

michael-s-molina · 2023-06-08T19:32:43Z

Draft a PR to refine the join key generator. please review. Thanks both!

@zhaoyongjie That's not the best way to help reviewing a PR. I spent a lot of time debugging the issue, addressing comments and executing tests using Airbnb charts to get to a solution. If you think the solution can be improved, you can make suggestions, provide more context, contribute code to the PR, etc.

Completely ignoring the PR and opening your own is not cool 😕

zhaoyongjie · 2023-06-08T19:49:36Z

@michael-s-molina
I've made the following suggestions during the code review:

It would be better to keep this pull request focused on the necessary changes rather than including refactorings and unrelated fixes. Let's aim to minimize the scope of the PR.
We should avoid hard coding values without considering the maintainability for other engineers.

michael-s-molina · 2023-06-08T19:54:42Z

@zhaoyongjie I thought I have addressed both your suggestions with my latest commits. You even removed the Request Changes status. Then you asked time to review the PR and suddenly opened your own which was really confusing to me.

pull-request-size bot added the size/L label May 22, 2023

villebro reviewed May 24, 2023

View reviewed changes

superset/common/query_context_processor.py Outdated Show resolved Hide resolved

superset/common/query_context_processor.py Outdated Show resolved Hide resolved

zhaoyongjie previously requested changes May 24, 2023

View reviewed changes

superset/common/query_context_processor.py Show resolved Hide resolved

michael-s-molina force-pushed the fix-time-shifts branch from fb8cb90 to bbb8788 Compare May 30, 2023 14:08

pull-request-size bot added size/XL and removed size/L labels May 31, 2023

michael-s-molina force-pushed the fix-time-shifts branch 2 times, most recently from 4e4d95c to 7a5a2ac Compare May 31, 2023 14:38

michael-s-molina marked this pull request as ready for review May 31, 2023 14:39

michael-s-molina requested review from john-bodley, zhaoyongjie and villebro May 31, 2023 14:39

john-bodley reviewed May 31, 2023

View reviewed changes

superset/common/query_context_processor.py Show resolved Hide resolved

superset/common/query_context_processor.py Show resolved Hide resolved

superset/db_engine_specs/ascend.py Show resolved Hide resolved

superset/db_engine_specs/base.py Show resolved Hide resolved

pull-request-size bot added size/XXL and removed size/XL labels May 31, 2023

michael-s-molina force-pushed the fix-time-shifts branch 3 times, most recently from 06116f7 to c87c24d Compare June 1, 2023 15:01

michael-s-molina commented Jun 1, 2023

View reviewed changes

superset/common/query_context_processor.py Outdated Show resolved Hide resolved

michael-s-molina added 7 commits June 2, 2023 09:47

fix: Time shifts with different granularity

44e647a

Extracts constants

c6b4552

Replaces literals with constants

45bab6a

Adds tests

6367ce5

Fixes lint errors

847b41d

Account for no time grain

bce26b1

Fixes druid time grains

dd16814

michael-s-molina added 4 commits June 2, 2023 09:51

Removes columns used for join

134381d

Gets time grain from form_data

088113f

Adds a method to get the time grain

78cd291

Adds config to set join column producerr

fd57a5b

michael-s-molina force-pushed the fix-time-shifts branch from c87c24d to fd57a5b Compare June 2, 2023 14:17

Extracts method

8070513

zhaoyongjie reviewed Jun 2, 2023

View reviewed changes

villebro reviewed Jun 5, 2023

View reviewed changes

Addresses comments

ac7596e

michael-s-molina requested review from villebro and john-bodley June 7, 2023 14:18

villebro approved these changes Jun 8, 2023

View reviewed changes

superset/constants.py Outdated Show resolved Hide resolved

superset/db_engine_specs/base.py Show resolved Hide resolved

tests/unit_tests/common/test_get_aggregated_join_column.py Outdated Show resolved Hide resolved

Uses a fixture in the test

d8a7524

villebro approved these changes Jun 8, 2023

View reviewed changes

Fixes types

13ed830

zhaoyongjie mentioned this pull request Jun 8, 2023

Draft: fix Time Shift join key #24328

Closed

9 tasks

michael-s-molina merged commit e5b7f7c into apache:master Jun 8, 2023

zhaoyongjie mentioned this pull request Jul 7, 2023

feat: support mulitple temporal filters in AdhocFilter and move the Time Section away #21767

Merged

9 tasks

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.0.0 labels Mar 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Time shifts with different granularity for ECharts #24176

fix: Time shifts with different granularity for ECharts #24176

michael-s-molina commented May 22, 2023 •

edited

Loading

villebro left a comment

codecov bot commented May 31, 2023 •

edited

Loading

zhaoyongjie commented May 31, 2023 •

edited

Loading

zhaoyongjie commented May 31, 2023

michael-s-molina commented Jun 1, 2023

zhaoyongjie Jun 2, 2023

michael-s-molina Jun 5, 2023

villebro left a comment

michael-s-molina commented Jun 7, 2023

villebro left a comment

villebro left a comment

zhaoyongjie commented Jun 8, 2023

zhaoyongjie commented Jun 8, 2023 •

edited

Loading

michael-s-molina commented Jun 8, 2023

zhaoyongjie commented Jun 8, 2023

michael-s-molina commented Jun 8, 2023 •

edited

Loading

fix: Time shifts with different granularity for ECharts #24176

fix: Time shifts with different granularity for ECharts #24176

Conversation

michael-s-molina commented May 22, 2023 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

villebro left a comment

Choose a reason for hiding this comment

codecov bot commented May 31, 2023 • edited Loading

Codecov Report

zhaoyongjie commented May 31, 2023 • edited Loading

zhaoyongjie commented May 31, 2023

michael-s-molina commented Jun 1, 2023

zhaoyongjie Jun 2, 2023

Choose a reason for hiding this comment

michael-s-molina Jun 5, 2023

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

michael-s-molina commented Jun 7, 2023

villebro left a comment

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

zhaoyongjie commented Jun 8, 2023

zhaoyongjie commented Jun 8, 2023 • edited Loading

michael-s-molina commented Jun 8, 2023

zhaoyongjie commented Jun 8, 2023

michael-s-molina commented Jun 8, 2023 • edited Loading

michael-s-molina commented May 22, 2023 •

edited

Loading

codecov bot commented May 31, 2023 •

edited

Loading

zhaoyongjie commented May 31, 2023 •

edited

Loading

zhaoyongjie commented Jun 8, 2023 •

edited

Loading

michael-s-molina commented Jun 8, 2023 •

edited

Loading