You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "snt1017 (via GitHub)" <gi...@apache.org> on 2023/06/01 19:43:48 UTC
[GitHub] [superset] snt1017 opened a new pull request, #19808: fix(sql-lab): SQL Lab commit connection even if no CTA query is made
snt1017 opened a new pull request, #19808:
URL: https://github.com/apache/superset/pull/19808
### 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
- [x] Has associated issue: #16036
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas commented on pull request #19808: fix(sql-lab): SQL Lab commit connection even if no CTA query is made
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #19808:
URL: https://github.com/apache/superset/pull/19808#issuecomment-1572672085
Closing/re-opening to kick-start CI 🤞
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #19808: fix(sql-lab): SQL Lab commit connection even if no CTA query is made
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #19808:
URL: https://github.com/apache/superset/pull/19808#issuecomment-1572729265
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/19808?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#19808](https://app.codecov.io/gh/apache/superset/pull/19808?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (a76bcb7) into [master](https://app.codecov.io/gh/apache/superset/commit/3db4a1cb8016dae71b5bfca09ef723c042dff671?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (3db4a1c) will **increase** coverage by `1.74%`.
> The diff coverage is `n/a`.
> :exclamation: Current head a76bcb7 differs from pull request most recent head 04cd48c. Consider uploading reports for the commit 04cd48c to get more accurate results
```diff
@@ 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%)` | :arrow_up: |
| mysql | `78.92% <ø> (-2.98%)` | :arrow_down: |
| postgres | `79.00% <ø> (-2.95%)` | :arrow_down: |
| presto | `53.32% <ø> (+0.60%)` | :arrow_up: |
| python | `82.77% <ø> (+0.39%)` | :arrow_up: |
| sqlite | `?` | |
| unit | `53.44% <ø> (+5.49%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
[see 1374 files with indirect coverage changes](https://app.codecov.io/gh/apache/superset/pull/19808/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas closed pull request #19808: fix(sql-lab): SQL Lab commit connection even if no CTA query is made
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas closed pull request #19808: fix(sql-lab): SQL Lab commit connection even if no CTA query is made
URL: https://github.com/apache/superset/pull/19808
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas merged pull request #19808: fix(sql-lab): SQL Lab commit connection even if no CTA query is made
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #19808:
URL: https://github.com/apache/superset/pull/19808
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] betodealmeida commented on a diff in pull request #19808: fix(sql-lab): SQL Lab commit connection even if no CTA query is made
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #19808:
URL: https://github.com/apache/superset/pull/19808#discussion_r1213463028
##########
superset/sql_lab.py:
##########
@@ -490,9 +490,9 @@ def execute_sql_statements( # pylint: disable=too-many-arguments, too-many-loca
ex, query, session, payload, prefix_message
)
return payload
-
# Commit the connection so CTA queries will create the table.
- conn.commit()
+ if apply_ctas:
+ conn.commit()
Review Comment:
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...)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org