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

Draft: fix Time Shift join key #24328

Closed
wants to merge 1 commit into from

Conversation

zhaoyongjie
Copy link
Member

@zhaoyongjie zhaoyongjie commented Jun 8, 2023

SUMMARY

A draft about Time Shift joined key issue, make it simple and let our life work and life easy.

Original PR at #24176

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

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 Jun 8, 2023

Codecov Report

Merging #24328 (585124f) into master (f5148ef) will decrease coverage by 10.29%.
The diff coverage is 35.89%.

❗ Current head 585124f differs from pull request most recent head 96689b2. Consider uploading reports for the commit 96689b2 to get more accurate results

@@             Coverage Diff             @@
##           master   #24328       +/-   ##
===========================================
- Coverage   67.84%   57.56%   -10.29%     
===========================================
  Files        1944     1943        -1     
  Lines       75328    75235       -93     
  Branches     8218     8216        -2     
===========================================
- Hits        51110    43311     -7799     
- Misses      22107    29813     +7706     
  Partials     2111     2111               
Flag Coverage Δ
hive 53.60% <35.89%> (+0.06%) ⬆️
presto 53.52% <35.89%> (+0.06%) ⬆️
python 60.34% <35.89%> (-21.32%) ⬇️
sqlite ?
unit 53.95% <35.89%> (+0.06%) ⬆️

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

Impacted Files Coverage Δ
...nd/plugins/legacy-preset-chart-nvd3/src/NVD3Vis.js 0.00% <ø> (ø)
...end/plugins/legacy-preset-chart-nvd3/src/preset.js 0.00% <ø> (ø)
superset-frontend/src/dashboard/actions/hydrate.js 2.04% <ø> (ø)
...dashboard/components/AddSliceCard/AddSliceCard.tsx 65.11% <ø> (-0.80%) ⬇️
...end/src/dashboard/components/SliceHeader/index.tsx 90.56% <ø> (-0.18%) ⬇️
...dashboard/components/SliceHeaderControls/index.tsx 69.79% <ø> (ø)
.../src/dashboard/components/gridComponents/Chart.jsx 57.52% <ø> (ø)
...perset-frontend/src/dashboard/containers/Chart.jsx 75.00% <ø> (ø)
...ponents/controls/VizTypeControl/VizTypeGallery.tsx 88.23% <ø> (ø)
.../features/databases/DatabaseModal/ExtraOptions.tsx 66.66% <ø> (+1.96%) ⬆️
... and 9 more

... and 287 files with indirect coverage changes

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

@zhaoyongjie zhaoyongjie closed this Jul 4, 2023
@zhaoyongjie
Copy link
Member Author

Will cherry-pick this one, close it in community.

@villebro
Copy link
Member

villebro commented Jul 5, 2023

@zhaoyongjie is this fix needed upstream? Do you have a test case that shows the problem and is fixed by this?

@zhaoyongjie
Copy link
Member Author

Hey @villebro, this PR was part of review for #24176 and the discussion with @michael-s-molina at #24176 (comment) and #24176 (comment).

From my personal perspective, I don't quite comprehend the modifications made on the master branch and the habit of hardcoding in the codebase and here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants