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: extract tables doesn't work with reserved keywords #17654

Merged
merged 1 commit into from
Dec 8, 2021

Conversation

betodealmeida
Copy link
Member

@betodealmeida betodealmeida commented Dec 6, 2021

SUMMARY

I decided to convert some integration tests to unit tests. The low hanging fruit for this are the sqlparse tests, since they're self contained, so I decided to start there.

As soon as I created my first test, I found a bug. I changed the SQL:

SELECT * FROM tbname

To:

SELECT * FROM table_name

And the test failed to identify table_name as a table in the query, since it's a reserved word in sqlparse.

I fixed the problem, and converted all the integration tests.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TESTING INSTRUCTIONS

Added unit tests covering the problem.

ADDITIONAL INFORMATION

  • Has associated issue:
  • 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

@codecov
Copy link

codecov bot commented Dec 6, 2021

Codecov Report

Merging #17654 (bccf8b9) into master (8e02d11) will increase coverage by 0.18%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17654      +/-   ##
==========================================
+ Coverage   68.66%   68.85%   +0.18%     
==========================================
  Files        1596     1597       +1     
  Lines       65224    65291      +67     
  Branches     6950     6950              
==========================================
+ Hits        44789    44957     +168     
+ Misses      18550    18449     -101     
  Partials     1885     1885              
Flag Coverage Δ
hive 81.40% <100.00%> (?)
javascript 57.45% <ø> (ø)
mysql 82.09% <100.00%> (+0.28%) ⬆️
postgres 82.10% <100.00%> (+0.28%) ⬆️
presto ?
python 82.45% <100.00%> (+0.37%) ⬆️
sqlite 81.79% <100.00%> (+0.28%) ⬆️

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

Impacted Files Coverage Δ
superset/sql_parse.py 99.44% <100.00%> (+0.02%) ⬆️
superset/db_engine_specs/presto.py 84.34% <0.00%> (-5.64%) ⬇️
superset/connectors/sqla/models.py 87.07% <0.00%> (-1.13%) ⬇️
superset/models/core.py 89.26% <0.00%> (-0.74%) ⬇️
superset/key_value/commands/exceptions.py 100.00% <0.00%> (ø)
superset/dashboards/filter_state/commands/entry.py 100.00% <0.00%> (ø)
superset/utils/core.py 89.76% <0.00%> (+0.11%) ⬆️
superset/utils/cache_manager.py 100.00% <0.00%> (+3.84%) ⬆️
superset/db_engine_specs/hive.py 87.25% <0.00%> (+16.98%) ⬆️
superset/dashboards/filter_state/api.py 100.00% <0.00%> (+17.02%) ⬆️
... and 11 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 8e02d11...bccf8b9. Read the comment docs.

@betodealmeida betodealmeida marked this pull request as ready for review December 6, 2021 01:39
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.

Stellar work @betodealmeida ! LGTM

@etr2460
Copy link
Member

etr2460 commented Jan 11, 2022

@betodealmeida it looks like this PR broke parsing table names from queries with sub selects. Here's a repro case:

SELECT *
   FROM
     (SELECT 1 as foo, 2 as bar)
   ORDER BY foo ASC, bar

ParsedQuery now thinks that ORDER BY and bar are tables:
Screen Shot 2022-01-10 at 4 36 36 PM

Do you think this is something you could easily fix, or should we consider reverting this PR? I could possibly look into fixing myself, but the sql parse codebase is something i'm a bit nervous about jumping into

etr2460 pushed a commit to etr2460/incubator-superset that referenced this pull request Jan 12, 2022
shcoderAlex pushed a commit to casual-precision/superset that referenced this pull request Feb 7, 2022
bwang221 pushed a commit to casual-precision/superset that referenced this pull request Feb 10, 2022
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 2024
@mistercrunch mistercrunch deleted the sqlparse_unit_tests branch March 26, 2024 16:20
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/XXL 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants