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

fix(db-migration): new_dataset_models_take_2 error on postgres #21417

Merged
merged 2 commits into from
Nov 21, 2022

Conversation

micsbot
Copy link
Contributor

@micsbot micsbot commented Sep 9, 2022

Previous code is not working. It is showing an error because the subquery is making a reference to the same table sl_columns. This code makes explicit the alias and join the tables NewColumn with the subquery. #20790

fix(superset db upgrade)

SUMMARY

Previous code is not working. It is showing an error because the subquery is making a reference to the same table sl_columns. This code makes explicit the alias and join the tables NewColumn with the subquery.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

run docker, and exec superset update db using a database with data from superset 1.x.x

ADDITIONAL INFORMATION

Previous code is not working. It is showing an error because the subquery is making a reference to the same table sl_columns. This code makes explicit the alias and join the tables NewColumn with the subquery.
@micsbot micsbot requested a review from a team as a code owner September 9, 2022 16:43
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️
We hope to see you in our Slack community too!

Copy link
Member

@ktmud ktmud left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! Would you mind posting before & after generated SQL statements if it's not too much hassle?

@ktmud ktmud changed the title Update 2022-04-01_14-38_a9422eeaae74_new_dataset_models_take_2.py fix(db-migration): new_dataset_models_take_2 error on Postgres Sep 12, 2022
@ktmud ktmud changed the title fix(db-migration): new_dataset_models_take_2 error on Postgres fix(db-migration): new_dataset_models_take_2 error on postgres Sep 12, 2022
@codecov
Copy link

codecov bot commented Sep 12, 2022

Codecov Report

Merging #21417 (437e0f4) into master (d3f9fbb) will decrease coverage by 10.84%.
The diff coverage is 74.25%.

❗ Current head 437e0f4 differs from pull request most recent head 55e55b9. Consider uploading reports for the commit 55e55b9 to get more accurate results

@@             Coverage Diff             @@
##           master   #21417       +/-   ##
===========================================
- Coverage   66.57%   55.72%   -10.85%     
===========================================
  Files        1791     1835       +44     
  Lines       68593    69970     +1377     
  Branches     7319     7590      +271     
===========================================
- Hits        45665    38990     -6675     
- Misses      21031    29014     +7983     
- Partials     1897     1966       +69     
Flag Coverage Δ
hive 52.60% <ø> (-0.34%) ⬇️
mysql ?
postgres ?
presto 52.50% <ø> (-0.34%) ⬇️
python 57.81% <ø> (-23.49%) ⬇️
sqlite ?
unit 50.90% <ø> (+0.09%) ⬆️

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

Impacted Files Coverage Δ
...d/packages/superset-ui-chart-controls/src/index.ts 100.00% <ø> (ø)
...-chart-controls/src/sections/advancedAnalytics.tsx 14.28% <ø> (ø)
...t-controls/src/sections/echartsTimeSeriesQuery.tsx 33.33% <0.00%> (ø)
...i-chart-controls/src/sections/forecastInterval.tsx 100.00% <ø> (ø)
...perset-ui-chart-controls/src/sections/sections.tsx 71.42% <0.00%> (-16.08%) ⬇️
...i-chart-controls/src/utils/expandControlConfig.tsx 100.00% <ø> (ø)
.../packages/superset-ui-core/src/chart/types/Base.ts 100.00% <ø> (ø)
...ntend/packages/superset-ui-core/src/color/index.ts 100.00% <ø> (ø)
...superset-ui-core/src/connection/callApi/callApi.ts 100.00% <ø> (ø)
...uperset-ui-core/src/query/types/AnnotationLayer.ts 100.00% <ø> (ø)
... and 707 more

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

@srinify
Copy link
Contributor

srinify commented Oct 18, 2022

Hey @micsbot thanks for this submission! Can you address the issue with pre-commit and then we can merge!

https://github.com/apache/superset/actions/runs/3023984202/jobs/4897820803

If you're new to pre-commit, I wrote about that here: https://preset.io/blog/tutorial-contributing-code-to-apache-superset/#crafting-a-git-commit

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.

LGTM - tested to work as expected. Thanks for fixing this!

@villebro villebro merged commit 2e5270c into apache:master Nov 21, 2022
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024
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/M 🚢 2.1.0
Projects
None yet
5 participants