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

Merged

Conversation

serenajiang
Copy link
Contributor

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:

WITH x AS(SELECT 1 AS col) SELECT * FROM x
WITH events AS (SELECT 1 AS col) SELECT * FROM events

ADDITIONAL INFORMATION

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.

Thanks @serenajiang for the fix, apologies for the inconvenience!

@villebro
Copy link
Member

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.

@codecov-io
Copy link

codecov-io commented Oct 21, 2020

Codecov Report

Merging #11365 into master will decrease coverage by 9.55%.
The diff coverage is n/a.

Impacted file tree graph

@@            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%) ⬇️
#javascript ?
#python ?

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

Impacted Files Coverage Δ
...uperset-frontend/src/dashboard/util/dnd-reorder.js 0.00% <0.00%> (-100.00%) ⬇️
...rset-frontend/src/dashboard/util/getEmptyLayout.js 0.00% <0.00%> (-100.00%) ⬇️
...et-frontend/src/components/Menu/LanguagePicker.tsx 0.00% <0.00%> (-100.00%) ⬇️
...dashboard/components/resizable/ResizableHandle.jsx 0.00% <0.00%> (-100.00%) ⬇️
...d/src/views/CRUD/csstemplates/CssTemplatesList.tsx 2.43% <0.00%> (-94.86%) ⬇️
.../src/dashboard/util/getFilterScopeFromNodesTree.js 0.00% <0.00%> (-93.48%) ⬇️
...uperset-frontend/src/utils/getClientErrorObject.ts 0.00% <0.00%> (-89.19%) ⬇️
.../src/dashboard/components/FilterIndicatorGroup.jsx 11.76% <0.00%> (-88.24%) ⬇️
...c/explore/components/controls/withVerification.jsx 9.09% <0.00%> (-87.88%) ⬇️
...src/dashboard/components/gridComponents/Header.jsx 10.52% <0.00%> (-86.85%) ⬇️
... and 652 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f8d0e7...f7de744. Read the comment docs.

@serenajiang serenajiang merged commit 54c2ad4 into apache:master Oct 21, 2020
serenajiang added a commit to airbnb/superset-fork that referenced this pull request Oct 21, 2020
@villebro villebro added the v0.38 label Nov 4, 2020
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
@mistercrunch mistercrunch added 🍒 0.38.0 🍒 0.38.1 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.0.0 labels Mar 12, 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 v0.38 🍒 0.38.0 🍒 0.38.1 🚢 1.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants