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.

Sign up for GitHub

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

Jump to bottom

refactor(plugins): BigNumber Time Comparison with existing time_offset API #27718

Merged
merged 27 commits into from
May 16, 2024
Merged

refactor(plugins): BigNumber Time Comparison with existing time_offset API #27718

merged 27 commits into from
May 16, 2024
+1,533 −195

Conversation

Antonio-RiveroMartnez
Copy link
Member

@Antonio-RiveroMartnez Antonio-RiveroMartnez commented Mar 27, 2024 •
edited
Loading

SUMMARY

Previously we implemented the time comparison in BigNumber viz using some new custom properties in the queryObject to compute the offsets. We are moving away from that approach in order to be more consistent with the way Superset handles time comparison in other visualizations.

Some of the changes of this PR are:

  • Use existing time_offset APIs instead of the instant comparison ones
  • Create new shared section to include the controls for time comparison, some taken from Advanced Analytics
  • Create a new control label to display the comparison ranges using existing time_range API and not a new one

There will be a follow up PR cleaning up the customizations from previous approach, that clean up is left out of this PR to ease the review process.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

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

@Antonio-RiveroMartnez Antonio-RiveroMartnez force-pushed the bn_time_comparison_current_api branch 2 times, most recently from e3584b0 to 5542188 Compare March 27, 2024 22:44
@codecov Codecov
Copy link

codecov bot commented Mar 27, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 11.29032% with 55 lines in your changes are missing coverage. Please review.

Project coverage is 69.73%. Comparing base (349e496) to head (61d9923).
Report is 44 commits behind head on master.

Files Patch % Lines
...plore/components/controls/ComparisonRangeLabel.tsx 9.09% 20 Missing ⚠️
...ontrols/DateFilterControl/utils/dateFilterUtils.ts 15.78% 15 Missing and 1 partial ⚠️
...Number/BigNumberPeriodOverPeriod/transformProps.ts 0.00% 10 Missing ⚠️
.../BigNumber/BigNumberPeriodOverPeriod/buildQuery.ts 0.00% 9 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #27718      +/-   ##
==========================================
- Coverage   69.77%   69.73%   -0.05%     
==========================================
  Files        1911     1922      +11     
  Lines       75056    75273     +217     
  Branches     8362     8434      +72     
==========================================
+ Hits        52374    52493     +119     
- Misses      20630    20717      +87     
- Partials     2052     2063      +11
Flag Coverage Δ
javascript 57.35% <9.83%> (-0.14%) ⬇️
mysql 77.89% <100.00%> (+<0.01%) ⬆️
postgres 78.00% <100.00%> (-0.02%) ⬇️
presto 53.63% <0.00%> (?)
python 83.03% <100.00%> (+0.11%) ⬆️
sqlite 77.44% <100.00%> (-0.02%) ⬇️
unit 56.77% <0.00%> (+0.01%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Antonio-RiveroMartnez
Copy link
Member Author

/testenv up

@github-actions GitHub Actions
Copy link
Contributor

@Antonio-RiveroMartnez Ephemeral environment spinning up at http://54.184.250.188:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@Antonio-RiveroMartnez
Copy link
Member Author

/testenv up FEATURE_CHART_PLUGINS_EXPERIMENTAL=true

@github-actions GitHub Actions
Copy link
Contributor

@Antonio-RiveroMartnez Ephemeral environment spinning up at http://35.166.80.185:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@github-actions github-actions bot added api Related to the REST API packages labels Mar 29, 2024
@Antonio-RiveroMartnez
Copy link
Member Author

/testenv up FEATURE_CHART_PLUGINS_EXPERIMENTAL=true

@github-actions GitHub Actions
Copy link
Contributor

@Antonio-RiveroMartnez Ephemeral environment spinning up at http://35.167.243.15:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@Antonio-RiveroMartnez
Copy link
Member Author

/testenv up FEATURE_CHART_PLUGINS_EXPERIMENTAL=true

@github-actions GitHub Actions
Copy link
Contributor

@Antonio-RiveroMartnez Ephemeral environment spinning up at http://34.221.145.242:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@Antonio-RiveroMartnez
Copy link
Member Author

/testenv up FEATURE_CHART_PLUGINS_EXPERIMENTAL=true

@github-actions GitHub Actions
Copy link
Contributor

@Antonio-RiveroMartnez Ephemeral environment spinning up at http://34.215.133.163:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@lilykuang lilykuang force-pushed the bn_time_comparison_current_api branch from 4c4cfe0 to 82b639f Compare April 26, 2024 05:40
@Antonio-RiveroMartnez
Copy link
Member Author

/testenv up FEATURE_CHART_PLUGINS_EXPERIMENTAL=true

@github-actions GitHub Actions
Copy link
Contributor

@Antonio-RiveroMartnez Ephemeral environment spinning up at http://54.245.212.136:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@Antonio-RiveroMartnez Antonio-RiveroMartnez changed the title [DO NOT MERGE] Demo - BigNumber with existing time_offset API refactor(plugins): BigNumber Time Comparison with existing time_offset API Apr 26, 2024
lilykuang and others added 23 commits May 16, 2024 15:50
@lilykuang @Antonio-RiveroMartnez
custom and inherit start date
9d3d149 
cherry picked from d3d3b31
@Antonio-RiveroMartnez
Table with Time Comparison:
57e7aed 
- When calling the time range method ensure we pass an array so we can remove the array check and follow the method signature
@kgabryje @Antonio-RiveroMartnez
Fixes
908cea3 
cherry picked from 669d060
@kgabryje @Antonio-RiveroMartnez
Improve coverage
333e93e 
cherry picked from c1f73ab
@Antonio-RiveroMartnez
Big Number with Time Comparison:
701a38b 
- Remove time grain from time_comparison controls
@Antonio-RiveroMartnez
Table with Time Comparison:
b90fa30 
- Support textual columns in time_offsets comparisons
- Because textual columns have no time grain nor they use xaxis labels all the time, the time_grain and xaxis are not required for the comparison queries that use textual columns only

cherry picked from 5819e3a
@Antonio-RiveroMartnez
Table with Time Comparison:
41f52d9 
- Support time comparison with no columns, meaning we want to compare just metrics. If so, we do an outer join
@Antonio-RiveroMartnez
BigNumber with Time Comparison:
29b9880 
- Do not pass a column because  it's not required anymore
@lilykuang @Antonio-RiveroMartnez
added getTimeOffset
74e3127
@Antonio-RiveroMartnez
BigNumber with Time Comparison:
2407e00 
- Use the new getTimeOffset to render the correct time range in the tooltip
- Update the lock file with moment
@Antonio-RiveroMartnez
BigNumber with Time Comparison:
fcc3797 
- Process data when custom or inherit offset is used
- Handle responses from time_range api that don't include the comparison ranges
@Antonio-RiveroMartnez
BigNumber with Time Comparison:
7f66348 
- Add tests for the new getTimeOffset method
- Hide label if no comparison is being made
@lilykuang @Antonio-RiveroMartnez
user parseDttmToMoment
7c16e60
@lilykuang @Antonio-RiveroMartnez
change parse dttm from moment to date
131a6e6
@lilykuang @Antonio-RiveroMartnez
update tests
669a5bf
@Antonio-RiveroMartnez
BigNumber with Time Comparison:
f5f534a 
- Update lock file so we remove moment
- Stop displaying Calculation Type since this viz only uses Actual Values (postporocessing: [])
- Handle nullish dttm
@Antonio-RiveroMartnez
BigNumber with Time Comparison:
9e59a97 
- Add new prop so we can show full list of choices or just a reduced amount based on the boolean value
- Update the copy so it better reflects and help the user
@Antonio-RiveroMartnez
Time Comparison:
9d66fc3 
- Add migration to port from old time comparison params to new approach
- Add tests for migration file
@Antonio-RiveroMartnez
Time Comparison:
10c716f 
- Fix delta calculation for shifts
- Add tests for new changes
@Antonio-RiveroMartnez
Time Comparison:
336fc9a 
- Fix lint error
- Fix tests
@Antonio-RiveroMartnez
Time Comparison:
661adcd 
- Pass a config object instead of raw bollean flags to the time comparison control so the intention is clear when using it
- Split getTimeOffset tests so parseDttmToDate has its own file
- use inlined test instead of blocks with describe and it
@Antonio-RiveroMartnez
Time Comparison:
5a6b770 
- Fix how we separate the time_grain scenario with join keys
- Simplify when we raise time grain required exception
@Antonio-RiveroMartnez
- Fix migration order
85a7494
@michael-s-molina michael-s-molina removed the hold:testing! On hold for testing label May 16, 2024
@Antonio-RiveroMartnez Antonio-RiveroMartnez merged commit b1f85dc into master May 16, 2024
32 of 33 checks passed
@github-actions GitHub Actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

EnxDev pushed a commit to EnxDev/superset that referenced this pull request May 31, 2024
@Antonio-RiveroMartnez @lilykuang
@kgabryje @EnxDev
refactor(plugins): BigNumber Time Comparison with existing time_offse…
ca88151 
…t API (apache#27718)

Co-authored-by: lilykuang <jialikuang@gmail.com>
Co-authored-by: Kamil Gabryjelski <kamil.gabryjelski@gmail.com>
@rusackas rusackas deleted the bn_time_comparison_current_api branch September 27, 2024 21:05
vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024
@Antonio-RiveroMartnez @lilykuang @kgabryje
refactor(plugins): BigNumber Time Comparison with existing time_offse…
fbb17a9 
…t API (apache#27718)

Co-authored-by: lilykuang <jialikuang@gmail.com>
Co-authored-by: Kamil Gabryjelski <kamil.gabryjelski@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eschutho eschutho eschutho approved these changes

@betodealmeida betodealmeida Awaiting requested review from betodealmeida betodealmeida is a code owner

@zhaoyongjie zhaoyongjie Awaiting requested review from zhaoyongjie

@kgabryje kgabryje Awaiting requested review from kgabryje

@lilykuang lilykuang Awaiting requested review from lilykuang

@justinpark justinpark Awaiting requested review from justinpark

@john-bodley john-bodley Awaiting requested review from john-bodley

@villebro villebro Awaiting requested review from villebro

@michael-s-molina michael-s-molina Awaiting requested review from michael-s-molina michael-s-molina is a code owner

Assignees
No one assigned
Labels
api Related to the REST API packages plugins risk:db-migration PRs that require a DB migration size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Antonio-RiveroMartnez @michael-s-molina @eschutho @lilykuang @kgabryje