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/10/21 16:25:14 UTC

[GitHub] [incubator-superset] serenajiang opened a new pull request #11365: fix(sqla): allow 'unknown' type queries in explore view

serenajiang opened a new pull request #11365:
URL: https://github.com/apache/incubator-superset/pull/11365


   ### SUMMARY
   After we deployed #11236, several charts broke because the sql parser could not identify the type of the query and pessimistically classified them as write queries. These problems occur because the sql parser is sensitive to certain syntax issues that may not actually be problems in some dialects of sql (in this case, presto sql).
   
   These errors are confusing to users because their queries work in sql lab but fail in explore view. Debugging and fixing the so-called "incorrect" syntax is very tedious.
   
   In this PR, I am allowing queries with an "UNKNOWN" type (optimistic instead of pessimistic). This couuuuld potentially allow some write queries through, but it's still a step up from how things were before #11236.
   
   ### TEST PLAN
   Some examples of correct queries that failed in explore view due to unknown type:
   ```sql
   WITH x AS(SELECT 1 AS col) SELECT * FROM x
   ```
   
   ```sql
   WITH events AS (SELECT 1 AS col) SELECT * FROM events
   ```
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [X] Has associated issue: #11310
   - [ ] Changes UI
   - [ ] Requires DB Migration.
   - [ ] Confirm DB Migration upgrade and downgrade tested.
   - [ ] 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.

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] serenajiang merged pull request #11365: fix(sqla): allow 'unknown' type queries in explore view

Posted by GitBox <gi...@apache.org>.
serenajiang merged pull request #11365:
URL: https://github.com/apache/incubator-superset/pull/11365


   


----------------------------------------------------------------
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 #11365: fix(sqla): allow 'unknown' type queries in explore view

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #11365:
URL: https://github.com/apache/incubator-superset/pull/11365#issuecomment-713728535


   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/11365?src=pr&el=h1) Report
   > Merging [#11365](https://codecov.io/gh/apache/incubator-superset/pull/11365?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/9f8d0e7a06ed5a30484dc87ec15f5dd20bced796?el=desc) will **decrease** coverage by `10.20%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/11365/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/incubator-superset/pull/11365?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff             @@
   ##           master   #11365       +/-   ##
   ===========================================
   - Coverage   65.75%   55.55%   -10.21%     
   ===========================================
     Files         838      406      -432     
     Lines       39716    13564    -26152     
     Branches     3615     3443      -172     
   ===========================================
   - Hits        26116     7535    -18581     
   + Misses      13499     5851     -7648     
   - Partials      101      178       +77     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #cypress | `55.55% <ø> (-0.86%)` | :arrow_down: |
   | #javascript | `?` | |
   | #python | `?` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/11365?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...uperset-frontend/src/dashboard/util/dnd-reorder.js](https://codecov.io/gh/apache/incubator-superset/pull/11365/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2RuZC1yZW9yZGVyLmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...rset-frontend/src/dashboard/util/getEmptyLayout.js](https://codecov.io/gh/apache/incubator-superset/pull/11365/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldEVtcHR5TGF5b3V0Lmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...et-frontend/src/components/Menu/LanguagePicker.tsx](https://codecov.io/gh/apache/incubator-superset/pull/11365/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTWVudS9MYW5ndWFnZVBpY2tlci50c3g=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...dashboard/components/resizable/ResizableHandle.jsx](https://codecov.io/gh/apache/incubator-superset/pull/11365/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL3Jlc2l6YWJsZS9SZXNpemFibGVIYW5kbGUuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...d/src/views/CRUD/csstemplates/CssTemplatesList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/11365/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvY3NzdGVtcGxhdGVzL0Nzc1RlbXBsYXRlc0xpc3QudHN4) | `2.43% <0.00%> (-94.86%)` | :arrow_down: |
   | [.../src/dashboard/util/getFilterScopeFromNodesTree.js](https://codecov.io/gh/apache/incubator-superset/pull/11365/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldEZpbHRlclNjb3BlRnJvbU5vZGVzVHJlZS5qcw==) | `0.00% <0.00%> (-93.48%)` | :arrow_down: |
   | [...uperset-frontend/src/utils/getClientErrorObject.ts](https://codecov.io/gh/apache/incubator-superset/pull/11365/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3V0aWxzL2dldENsaWVudEVycm9yT2JqZWN0LnRz) | `0.00% <0.00%> (-89.19%)` | :arrow_down: |
   | [.../src/dashboard/components/FilterIndicatorGroup.jsx](https://codecov.io/gh/apache/incubator-superset/pull/11365/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0ZpbHRlckluZGljYXRvckdyb3VwLmpzeA==) | `11.76% <0.00%> (-88.24%)` | :arrow_down: |
   | [...c/explore/components/controls/withVerification.jsx](https://codecov.io/gh/apache/incubator-superset/pull/11365/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy93aXRoVmVyaWZpY2F0aW9uLmpzeA==) | `9.09% <0.00%> (-87.88%)` | :arrow_down: |
   | [...src/dashboard/components/gridComponents/Header.jsx](https://codecov.io/gh/apache/incubator-superset/pull/11365/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL0hlYWRlci5qc3g=) | `10.52% <0.00%> (-86.85%)` | :arrow_down: |
   | ... and [654 more](https://codecov.io/gh/apache/incubator-superset/pull/11365/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/11365?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/11365?src=pr&el=footer). Last update [9f8d0e7...f7de744](https://codecov.io/gh/apache/incubator-superset/pull/11365?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] villebro commented on pull request #11365: fix(sqla): allow 'unknown' type queries in explore view

Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #11365:
URL: https://github.com/apache/incubator-superset/pull/11365#issuecomment-713708642


   Btw, I'm surprised `sqlparse` is not able to identify regular CTEs as read only. Will potentially take a stab at adding that to the library.


----------------------------------------------------------------
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 #11365: fix(sqla): allow 'unknown' type queries in explore view

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #11365:
URL: https://github.com/apache/incubator-superset/pull/11365#issuecomment-713728535


   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/11365?src=pr&el=h1) Report
   > Merging [#11365](https://codecov.io/gh/apache/incubator-superset/pull/11365?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/9f8d0e7a06ed5a30484dc87ec15f5dd20bced796?el=desc) will **decrease** coverage by `9.55%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/11365/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/incubator-superset/pull/11365?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #11365      +/-   ##
   ==========================================
   - Coverage   65.75%   56.20%   -9.56%     
   ==========================================
     Files         838      406     -432     
     Lines       39716    13564   -26152     
     Branches     3615     3443     -172     
   ==========================================
   - Hits        26116     7623   -18493     
   + Misses      13499     5771    -7728     
   - Partials      101      170      +69     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #cypress | `56.20% <ø> (-0.21%)` | :arrow_down: |
   | #javascript | `?` | |
   | #python | `?` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/11365?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...uperset-frontend/src/dashboard/util/dnd-reorder.js](https://codecov.io/gh/apache/incubator-superset/pull/11365/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2RuZC1yZW9yZGVyLmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...rset-frontend/src/dashboard/util/getEmptyLayout.js](https://codecov.io/gh/apache/incubator-superset/pull/11365/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldEVtcHR5TGF5b3V0Lmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...et-frontend/src/components/Menu/LanguagePicker.tsx](https://codecov.io/gh/apache/incubator-superset/pull/11365/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTWVudS9MYW5ndWFnZVBpY2tlci50c3g=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...dashboard/components/resizable/ResizableHandle.jsx](https://codecov.io/gh/apache/incubator-superset/pull/11365/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL3Jlc2l6YWJsZS9SZXNpemFibGVIYW5kbGUuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...d/src/views/CRUD/csstemplates/CssTemplatesList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/11365/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvY3NzdGVtcGxhdGVzL0Nzc1RlbXBsYXRlc0xpc3QudHN4) | `2.43% <0.00%> (-94.86%)` | :arrow_down: |
   | [.../src/dashboard/util/getFilterScopeFromNodesTree.js](https://codecov.io/gh/apache/incubator-superset/pull/11365/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldEZpbHRlclNjb3BlRnJvbU5vZGVzVHJlZS5qcw==) | `0.00% <0.00%> (-93.48%)` | :arrow_down: |
   | [...uperset-frontend/src/utils/getClientErrorObject.ts](https://codecov.io/gh/apache/incubator-superset/pull/11365/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3V0aWxzL2dldENsaWVudEVycm9yT2JqZWN0LnRz) | `0.00% <0.00%> (-89.19%)` | :arrow_down: |
   | [.../src/dashboard/components/FilterIndicatorGroup.jsx](https://codecov.io/gh/apache/incubator-superset/pull/11365/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0ZpbHRlckluZGljYXRvckdyb3VwLmpzeA==) | `11.76% <0.00%> (-88.24%)` | :arrow_down: |
   | [...c/explore/components/controls/withVerification.jsx](https://codecov.io/gh/apache/incubator-superset/pull/11365/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy93aXRoVmVyaWZpY2F0aW9uLmpzeA==) | `9.09% <0.00%> (-87.88%)` | :arrow_down: |
   | [...src/dashboard/components/gridComponents/Header.jsx](https://codecov.io/gh/apache/incubator-superset/pull/11365/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL0hlYWRlci5qc3g=) | `10.52% <0.00%> (-86.85%)` | :arrow_down: |
   | ... and [652 more](https://codecov.io/gh/apache/incubator-superset/pull/11365/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/11365?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/11365?src=pr&el=footer). Last update [9f8d0e7...f7de744](https://codecov.io/gh/apache/incubator-superset/pull/11365?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