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

Revert "perf(sqllab): Rendering perf improvement using immutable stat… #21298

Closed

Conversation

hughhhh
Copy link
Member

@hughhhh hughhhh commented Sep 1, 2022

…e (#20877)"

This reverts commit f77b910.

SUMMARY

Fixes the issue with the tablemetadata schema not loading properly

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

SQL.lab.mp4

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 Sep 1, 2022

Codecov Report

Merging #21298 (15c713e) into master (1aeb8fd) will decrease coverage by 0.07%.
The diff coverage is 40.65%.

@@            Coverage Diff             @@
##           master   #21298      +/-   ##
==========================================
- Coverage   66.43%   66.36%   -0.08%     
==========================================
  Files        1786     1784       -2     
  Lines       68267    68240      -27     
  Branches     7264     7262       -2     
==========================================
- Hits        45352    45285      -67     
- Misses      21045    21092      +47     
+ Partials     1870     1863       -7     
Flag Coverage Δ
hive 53.02% <ø> (ø)
javascript 52.32% <40.65%> (-0.16%) ⬇️
postgres 80.88% <ø> (ø)
presto 52.92% <ø> (ø)
python 81.32% <ø> (ø)
sqlite 79.43% <ø> (ø)
unit 50.78% <ø> (ø)

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

Impacted Files Coverage Δ
superset-frontend/src/SqlLab/App.jsx 0.00% <ø> (ø)
...d/src/SqlLab/components/AceEditorWrapper/index.tsx 42.62% <ø> (-5.07%) ⬇️
...qlLab/components/EstimateQueryCostButton/index.tsx 5.26% <0.00%> (-58.38%) ⬇️
...d/src/SqlLab/components/SqlEditorLeftBar/index.tsx 49.20% <ø> (-0.80%) ⬇️
...c/SqlLab/components/TemplateParamsEditor/index.tsx 75.00% <0.00%> (-7.36%) ⬇️
superset-frontend/src/SqlLab/fixtures.ts 100.00% <ø> (ø)
superset-frontend/src/SqlLab/types.ts 57.14% <ø> (ø)
...uperset-frontend/src/components/Dropdown/index.tsx 100.00% <ø> (ø)
...et-frontend/src/components/TableSelector/index.tsx 76.08% <ø> (-2.18%) ⬇️
superset-frontend/src/SqlLab/actions/sqlLab.js 59.77% <14.28%> (-1.28%) ⬇️
... and 15 more

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

@EugeneTorap
Copy link
Contributor

EugeneTorap commented Sep 1, 2022

@hughhhh Why should we revert the big commit with useful perf and refactoring changes?
That's a lot of work put in here. For the sake of the small bug that can be fixed with a few lines of code, do we revert it?
@justinpark Can we prepare small hot fix for this issue in another PR?

@EugeneTorap
Copy link
Contributor

I understand that an easy way to revert it than spend more time fixing it.
In this case we can revert whole project, and we will never have bugs & issues.

@justinpark
Copy link
Member

justinpark commented Sep 1, 2022

@EugeneTorap
I posted a hotfix here: #21304
if it doesn't work, we can revert the changes.

@EugeneTorap
Copy link
Contributor

@justinpark Thanks. I appreciate your work.

@EugeneTorap
Copy link
Contributor

EugeneTorap commented Sep 2, 2022

@zhaoyongjie @villebro Can we close this PR? We've fixed the bug.

@villebro
Copy link
Member

villebro commented Sep 2, 2022

zhaoyongjie villebro Can we close this PR? We've fixed the bug.

@EugeneTorap I defer to @hughhhh (also a committer) who has opened the PR, as I have not participated in this discussion

@hughhhh hughhhh closed this Sep 2, 2022
@justinpark
Copy link
Member

@EugeneTorap There's one more hotfix related to this change. Please add this patch together

#21311

@mistercrunch mistercrunch deleted the revert-f77b910e2cc9f1bd90ac0f3a9097ec5d394b582d branch March 26, 2024 18:00
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.

4 participants