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(sql-lab): SQL Lab commit connection even if no CTA query is made #19808

Merged
merged 1 commit into from
Jun 7, 2023

Conversation

snt1017
Copy link
Contributor

@snt1017 snt1017 commented Apr 21, 2022

SUMMARY

SQL Lab query editor on run commits the connection even if all we do is select a database without CTA enabled. This is not really an issue with most database systems however there are problems when using superset with Mongo BI connector which does not support transactions.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue: SQL Lab commit connection even if no CTA query is made #16036
  • 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

# Commit the connection so CTA queries will create the table.
conn.commit()
if apply_ctas:
conn.commit()
Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine, and the commit might not even be needed at all (but does no harm) since we close the connection after (exiting the context manager) and open a new one when running select_star.

(I wonder if the commit was needed before we added the closing context manager...)

@rusackas
Copy link
Member

rusackas commented Jun 1, 2023

Closing/re-opening to kick-start CI 🤞

@rusackas rusackas closed this Jun 1, 2023
@rusackas rusackas reopened this Jun 1, 2023
@codecov
Copy link

codecov bot commented Jun 1, 2023

Codecov Report

Merging #19808 (a76bcb7) into master (3db4a1c) will increase coverage by 1.74%.
The diff coverage is n/a.

❗ Current head a76bcb7 differs from pull request most recent head 04cd48c. Consider uploading reports for the commit 04cd48c to get more accurate results

@@            Coverage Diff             @@
##           master   #19808      +/-   ##
==========================================
+ Coverage   66.54%   68.29%   +1.74%     
==========================================
  Files        1692     1957     +265     
  Lines       64807    75622   +10815     
  Branches     6661     8222    +1561     
==========================================
+ Hits        43129    51643    +8514     
- Misses      19978    21871    +1893     
- Partials     1700     2108     +408     
Flag Coverage Δ
hive 53.39% <ø> (+0.52%) ⬆️
mysql 78.92% <ø> (-2.98%) ⬇️
postgres 79.00% <ø> (-2.95%) ⬇️
presto 53.32% <ø> (+0.60%) ⬆️
python 82.77% <ø> (+0.39%) ⬆️
sqlite ?
unit 53.44% <ø> (+5.49%) ⬆️

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

see 1374 files with indirect coverage changes

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

@rusackas rusackas merged commit e13b80a into apache:master Jun 7, 2023
31 checks passed
@mistercrunch 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 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/XS 🚢 3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants