You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/04/29 22:51:23 UTC
[GitHub] [incubator-superset] nytai opened a new pull request #9692: [sqllab] fix, strip comments before parsing statements
nytai opened a new pull request #9692:
URL: https://github.com/apache/incubator-superset/pull/9692
### CATEGORY
Choose one
- [x] Bug Fix
- [ ] Enhancement (new features, refinement)
- [ ] Refactor
- [ ] Add tests
- [ ] Build / Development Environment
- [ ] Documentation
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
running the following statement in sqllab causes an error if DML is not allowed for a DB.
```sql
-- Note: Unless you save your query, these tabs will NOT persist if you clear your cookies or change browsers.;
SELECT * from flights;
-- test.
```
The issue is that parsesql is parsing the second comment as a separate statement. We check to see if the statement is a SELECT (it's a comment) and throw an error since the db is readonly.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TEST PLAN
<!--- What steps should be taken to verify the changes -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
### REVIEWERS
----------------------------------------------------------------
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.
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] [incubator-superset] nytai edited a comment on pull request #9692: [sqllab] fix, strip comments before parsing statements
Posted by GitBox <gi...@apache.org>.
nytai edited a comment on pull request #9692:
URL: https://github.com/apache/incubator-superset/pull/9692#issuecomment-622572706
@dpgaspar it looks like SQL_QUERY_MUTATOR is applied after. However it is called with the original (unparsed) sql query, which I think is also what is actually passed to the engine. This change only applies to the parsed_sql which is used for validation purposes only, I believe.
----------------------------------------------------------------
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.
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] [incubator-superset] willbarrett commented on pull request #9692: [sqllab] fix, strip comments before parsing statements
Posted by GitBox <gi...@apache.org>.
willbarrett commented on pull request #9692:
URL: https://github.com/apache/incubator-superset/pull/9692#issuecomment-621960705
I think it's a holiday in Portugal tomorrow, so maybe we could also ask @villebro for a review?
----------------------------------------------------------------
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.
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] [incubator-superset] codecov-io commented on pull request #9692: [sqllab] fix, strip comments before parsing statements
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #9692:
URL: https://github.com/apache/incubator-superset/pull/9692#issuecomment-621511509
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9692?src=pr&el=h1) Report
> Merging [#9692](https://codecov.io/gh/apache/incubator-superset/pull/9692?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/62a15f027dc1e8701ef82599842169b4d23bf8b5&el=desc) will **increase** coverage by `4.70%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9692?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9692 +/- ##
==========================================
+ Coverage 65.80% 70.50% +4.70%
==========================================
Files 581 182 -399
Lines 30323 17815 -12508
Branches 3095 0 -3095
==========================================
- Hits 19953 12561 -7392
+ Misses 10189 5254 -4935
+ Partials 181 0 -181
```
| Flag | Coverage Δ | |
|---|---|---|
| #javascript | `?` | |
| #python | `70.50% <100.00%> (-0.23%)` | :arrow_down: |
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9692?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/sql\_parse.py](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQvc3FsX3BhcnNlLnB5) | `99.33% <100.00%> (ø)` | |
| [superset/views/database/mixins.py](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `58.92% <0.00%> (-21.43%)` | :arrow_down: |
| [superset/utils/cache.py](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY2FjaGUucHk=) | `45.83% <0.00%> (-20.84%)` | :arrow_down: |
| [superset/db\_engine\_specs/mysql.py](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL215c3FsLnB5) | `78.26% <0.00%> (-13.05%)` | :arrow_down: |
| [superset/views/database/api.py](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvYXBpLnB5) | `83.90% <0.00%> (-3.45%)` | :arrow_down: |
| [superset/db\_engine\_specs/postgres.py](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3Bvc3RncmVzLnB5) | `97.29% <0.00%> (-2.71%)` | :arrow_down: |
| [superset/models/core.py](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbW9kZWxzL2NvcmUucHk=) | `84.47% <0.00%> (-2.39%)` | :arrow_down: |
| [superset/security/manager.py](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQvc2VjdXJpdHkvbWFuYWdlci5weQ==) | `88.81% <0.00%> (-0.34%)` | :arrow_down: |
| [superset/views/core.py](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `74.88% <0.00%> (-0.23%)` | :arrow_down: |
| [superset/connectors/sqla/models.py](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `88.41% <0.00%> (-0.16%)` | :arrow_down: |
| ... and [399 more](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9692?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9692?src=pr&el=footer). Last update [62a15f0...b46565b](https://codecov.io/gh/apache/incubator-superset/pull/9692?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
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] [incubator-superset] graceguo-supercat commented on pull request #9692: [sqllab] fix, strip comments before parsing statements
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on pull request #9692:
URL: https://github.com/apache/incubator-superset/pull/9692#issuecomment-623752780
@nytai Master build is broken since this PR merged in. Could you take a look?
----------------------------------------------------------------
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.
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] [incubator-superset] nytai commented on pull request #9692: [sqllab] fix, strip comments before parsing statements
Posted by GitBox <gi...@apache.org>.
nytai commented on pull request #9692:
URL: https://github.com/apache/incubator-superset/pull/9692#issuecomment-622572706
@dpgaspar it looks like SQL_QUERY_MUTATOR is applied after.
----------------------------------------------------------------
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.
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] [incubator-superset] graceguo-supercat edited a comment on pull request #9692: [sqllab] fix, strip comments before parsing statements
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #9692:
URL: https://github.com/apache/incubator-superset/pull/9692#issuecomment-623752780
@nytai Master branch build is broken since this PR merged in. Could you take a look?
----------------------------------------------------------------
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.
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] [incubator-superset] codecov-io edited a comment on pull request #9692: [sqllab] fix, strip comments before parsing statements
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #9692:
URL: https://github.com/apache/incubator-superset/pull/9692#issuecomment-621511509
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9692?src=pr&el=h1) Report
> Merging [#9692](https://codecov.io/gh/apache/incubator-superset/pull/9692?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/62a15f027dc1e8701ef82599842169b4d23bf8b5&el=desc) will **increase** coverage by `4.79%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9692?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9692 +/- ##
==========================================
+ Coverage 65.80% 70.59% +4.79%
==========================================
Files 581 581
Lines 30323 30323
Branches 3095 3095
==========================================
+ Hits 19953 21407 +1454
+ Misses 10189 8805 -1384
+ Partials 181 111 -70
```
| Flag | Coverage Δ | |
|---|---|---|
| #cypress | `53.57% <ø> (?)` | |
| #javascript | `58.77% <ø> (ø)` | |
| #python | `70.73% <100.00%> (ø)` | |
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9692?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/sql\_parse.py](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQvc3FsX3BhcnNlLnB5) | `99.33% <100.00%> (ø)` | |
| [...rontend/src/SqlLab/components/AceEditorWrapper.tsx](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL0FjZUVkaXRvcldyYXBwZXIudHN4) | `56.98% <0.00%> (+1.07%)` | :arrow_up: |
| [superset-frontend/src/components/EditableTitle.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvRWRpdGFibGVUaXRsZS5qc3g=) | `81.69% <0.00%> (+1.40%)` | :arrow_up: |
| [...perset-frontend/src/components/CopyToClipboard.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ29weVRvQ2xpcGJvYXJkLmpzeA==) | `36.36% <0.00%> (+1.51%)` | :arrow_up: |
| [...hboard/components/resizable/ResizableContainer.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL3Jlc2l6YWJsZS9SZXNpemFibGVDb250YWluZXIuanN4) | `71.87% <0.00%> (+1.56%)` | :arrow_up: |
| [...ashboard/components/gridComponents/ChartHolder.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL0NoYXJ0SG9sZGVyLmpzeA==) | `81.35% <0.00%> (+1.69%)` | :arrow_up: |
| [superset-frontend/src/utils/common.js](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3V0aWxzL2NvbW1vbi5qcw==) | `69.64% <0.00%> (+1.78%)` | :arrow_up: |
| [...src/explore/components/controls/VizTypeControl.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9WaXpUeXBlQ29udHJvbC5qc3g=) | `78.84% <0.00%> (+1.92%)` | :arrow_up: |
| [...rset-frontend/src/explore/components/SaveModal.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9TYXZlTW9kYWwuanN4) | `94.73% <0.00%> (+2.10%)` | :arrow_up: |
| [superset-frontend/src/explore/controlUtils.js](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29udHJvbFV0aWxzLmpz) | `96.80% <0.00%> (+2.12%)` | :arrow_up: |
| ... and [135 more](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9692?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9692?src=pr&el=footer). Last update [62a15f0...b46565b](https://codecov.io/gh/apache/incubator-superset/pull/9692?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
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] [incubator-superset] nytai edited a comment on pull request #9692: [sqllab] fix, strip comments before parsing statements
Posted by GitBox <gi...@apache.org>.
nytai edited a comment on pull request #9692:
URL: https://github.com/apache/incubator-superset/pull/9692#issuecomment-622471036
The issue is that `sqlparse` is returning 2 statements when the statements look like
```sql
select * from table;
-- comment
```
I suspect is splits along the `;`.
[The logic that determines if the statement is DML](https://github.com/preset-io/incubator-superset/blob/master/superset/sql_parse.py#L117) is just checking that all statements are "SELECT" or "EXPLAIN". The second statement is a comment, hence it's not SELECT|EXPLAIN, therefore it fails the is DML test. What this PR does is it strips comments before feeding the statements to `sqlparse` so we're only parsing actual SQL statements and not comments.
----------------------------------------------------------------
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.
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] [incubator-superset] nytai commented on pull request #9692: [sqllab] fix, strip comments before parsing statements
Posted by GitBox <gi...@apache.org>.
nytai commented on pull request #9692:
URL: https://github.com/apache/incubator-superset/pull/9692#issuecomment-622536056
Additionally `sqlparse` doesn't seem to tag comments. The type it's resolving `-- some comment` to is `'UNKNOWN'`. We could add `UNKNOWN` to the `is_readonly` check, but that seems less safe than strictly treating only `SELECT|EXPLAIN` as readonly
----------------------------------------------------------------
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.
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] [incubator-superset] codecov-io edited a comment on pull request #9692: [sqllab] fix, strip comments before parsing statements
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #9692:
URL: https://github.com/apache/incubator-superset/pull/9692#issuecomment-621511509
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9692?src=pr&el=h1) Report
> Merging [#9692](https://codecov.io/gh/apache/incubator-superset/pull/9692?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/62a15f027dc1e8701ef82599842169b4d23bf8b5&el=desc) will **increase** coverage by `4.54%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9692?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9692 +/- ##
==========================================
+ Coverage 65.80% 70.34% +4.54%
==========================================
Files 581 581
Lines 30323 30323
Branches 3095 3095
==========================================
+ Hits 19953 21330 +1377
+ Misses 10189 8875 -1314
+ Partials 181 118 -63
```
| Flag | Coverage Δ | |
|---|---|---|
| #cypress | `52.75% <ø> (?)` | |
| #javascript | `58.77% <ø> (ø)` | |
| #python | `70.66% <100.00%> (-0.07%)` | :arrow_down: |
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9692?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/sql\_parse.py](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQvc3FsX3BhcnNlLnB5) | `99.33% <100.00%> (ø)` | |
| [superset/db\_engine\_specs/mysql.py](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL215c3FsLnB5) | `78.26% <0.00%> (-13.05%)` | :arrow_down: |
| [superset/models/core.py](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbW9kZWxzL2NvcmUucHk=) | `85.97% <0.00%> (-0.90%)` | :arrow_down: |
| [superset/views/core.py](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `74.88% <0.00%> (-0.23%)` | :arrow_down: |
| [superset-frontend/src/SqlLab/actions/sqlLab.js](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9hY3Rpb25zL3NxbExhYi5qcw==) | `61.13% <0.00%> (+0.65%)` | :arrow_up: |
| [...erset-frontend/src/SqlLab/components/SqlEditor.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NxbEVkaXRvci5qc3g=) | `53.84% <0.00%> (+1.28%)` | :arrow_up: |
| [superset-frontend/src/components/EditableTitle.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvRWRpdGFibGVUaXRsZS5qc3g=) | `81.69% <0.00%> (+1.40%)` | :arrow_up: |
| [...perset-frontend/src/components/CopyToClipboard.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ29weVRvQ2xpcGJvYXJkLmpzeA==) | `36.36% <0.00%> (+1.51%)` | :arrow_up: |
| [...hboard/components/resizable/ResizableContainer.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL3Jlc2l6YWJsZS9SZXNpemFibGVDb250YWluZXIuanN4) | `71.87% <0.00%> (+1.56%)` | :arrow_up: |
| [...ashboard/components/gridComponents/ChartHolder.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL0NoYXJ0SG9sZGVyLmpzeA==) | `81.35% <0.00%> (+1.69%)` | :arrow_up: |
| ... and [136 more](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9692?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9692?src=pr&el=footer). Last update [62a15f0...b46565b](https://codecov.io/gh/apache/incubator-superset/pull/9692?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
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] [incubator-superset] nytai edited a comment on pull request #9692: [sqllab] fix, strip comments before parsing statements
Posted by GitBox <gi...@apache.org>.
nytai edited a comment on pull request #9692:
URL: https://github.com/apache/incubator-superset/pull/9692#issuecomment-622471036
The issue is that `sqlparse` is returning 2 statements when the statements look like
```sql
select * from table;
-- comment
```
I suspect is splits along the `;`.
[The logic that determines if the statement is DML](https://github.com/preset-io/incubator-superset/blob/master/superset/sql_parse.py#L117) is just checking that all statements are "SELECT" or "EXPLAIN". The second statement is a comment, hence it's not SELECT|EXPLAIN, therefore it fails the 'is DML' test. What this PR does is it strips comments before feeding the statements to `sqlparse` so we're only parsing actual SQL statements and not comments.
----------------------------------------------------------------
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.
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] [incubator-superset] villebro commented on pull request #9692: [sqllab] fix, strip comments before parsing statements
Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #9692:
URL: https://github.com/apache/incubator-superset/pull/9692#issuecomment-622119047
If I understand this correctly, `sqlparse` is flagging a false positive on DML. I'm fine with a workaround for this, but in that case I'd prefer to make sure we've made sure this isn't fixed in an upcoming version of `sqlparse`, and also adding a unit test with a `# TODO:` to make sure we can remove the workaround later.
----------------------------------------------------------------
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.
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] [incubator-superset] codecov-io edited a comment on pull request #9692: [sqllab] fix, strip comments before parsing statements
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #9692:
URL: https://github.com/apache/incubator-superset/pull/9692#issuecomment-621511509
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9692?src=pr&el=h1) Report
> Merging [#9692](https://codecov.io/gh/apache/incubator-superset/pull/9692?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/62a15f027dc1e8701ef82599842169b4d23bf8b5&el=desc) will **decrease** coverage by `0.13%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9692?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9692 +/- ##
==========================================
- Coverage 65.80% 65.66% -0.14%
==========================================
Files 581 581
Lines 30323 30323
Branches 3095 3095
==========================================
- Hits 19953 19913 -40
- Misses 10189 10229 +40
Partials 181 181
```
| Flag | Coverage Δ | |
|---|---|---|
| #javascript | `58.77% <ø> (ø)` | |
| #python | `70.50% <100.00%> (-0.23%)` | :arrow_down: |
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9692?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/sql\_parse.py](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQvc3FsX3BhcnNlLnB5) | `99.33% <100.00%> (ø)` | |
| [superset/views/database/mixins.py](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `58.92% <0.00%> (-21.43%)` | :arrow_down: |
| [superset/utils/cache.py](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY2FjaGUucHk=) | `45.83% <0.00%> (-20.84%)` | :arrow_down: |
| [superset/db\_engine\_specs/mysql.py](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL215c3FsLnB5) | `78.26% <0.00%> (-13.05%)` | :arrow_down: |
| [superset/views/database/api.py](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvYXBpLnB5) | `83.90% <0.00%> (-3.45%)` | :arrow_down: |
| [superset/db\_engine\_specs/postgres.py](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3Bvc3RncmVzLnB5) | `97.29% <0.00%> (-2.71%)` | :arrow_down: |
| [superset/models/core.py](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbW9kZWxzL2NvcmUucHk=) | `84.47% <0.00%> (-2.39%)` | :arrow_down: |
| [superset/security/manager.py](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQvc2VjdXJpdHkvbWFuYWdlci5weQ==) | `88.81% <0.00%> (-0.34%)` | :arrow_down: |
| [superset/views/core.py](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `74.88% <0.00%> (-0.23%)` | :arrow_down: |
| [superset/connectors/sqla/models.py](https://codecov.io/gh/apache/incubator-superset/pull/9692/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `88.41% <0.00%> (-0.16%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9692?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9692?src=pr&el=footer). Last update [62a15f0...b46565b](https://codecov.io/gh/apache/incubator-superset/pull/9692?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
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] [incubator-superset] nytai commented on pull request #9692: [sqllab] fix, strip comments before parsing statements
Posted by GitBox <gi...@apache.org>.
nytai commented on pull request #9692:
URL: https://github.com/apache/incubator-superset/pull/9692#issuecomment-622471036
The issue is that `sqlparse` is returning 2 statements when the statements look like
```sql
select * from table;
-- comment
```
I suspect is splits along the `;`.
[The logic that determines if the statement is DML](https://github.com/preset-io/incubator-superset/blob/master/superset/sql_parse.py#L117) is just checking that all statements are "SELECT". The second statement is a comment, hence it's not SELECT, therefore it fails the is DML test. What this PR does is it strips comments before feeding the statements to `sqlparse` so we're only parsing actual SQL statements and not comments.
----------------------------------------------------------------
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.
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] [incubator-superset] dpgaspar commented on pull request #9692: [sqllab] fix, strip comments before parsing statements
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on pull request #9692:
URL: https://github.com/apache/incubator-superset/pull/9692#issuecomment-622330685
Have not dig too deep into it, but `SQL_QUERY_MUTATOR` is applied before or after this?
----------------------------------------------------------------
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.
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] [incubator-superset] nytai edited a comment on pull request #9692: [sqllab] fix, strip comments before parsing statements
Posted by GitBox <gi...@apache.org>.
nytai edited a comment on pull request #9692:
URL: https://github.com/apache/incubator-superset/pull/9692#issuecomment-622572706
@dpgaspar it looks like SQL_QUERY_MUTATOR is applied after. However it is called with the original (unparsed) sql query, which I think is also what is actually passed to the engine. This change only applies to the parsed_query which is used for validation purposes only, I believe.
----------------------------------------------------------------
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.
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