fix: Workaround for sqlparse issue #652 #17995

john-bodley · 2022-01-11T02:50:50Z

SUMMARY

This PR provides a proposed workaround—by way of monkey patching—to andialbrecht/sqlparse#652 which seems to have an incorrect regex pattern when matching single string tokens which contain an escape.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Added unit tests.

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 · 2022-01-11T03:19:58Z

Codecov Report

Merging #17995 (711106f) into master (f7add54) will decrease coverage by 0.73%.
The diff coverage is 63.29%.

❗ Current head 711106f differs from pull request most recent head b1d84d7. Consider uploading reports for the commit b1d84d7 to get more accurate results

@@            Coverage Diff             @@
##           master   #17995      +/-   ##
==========================================
- Coverage   67.05%   66.32%   -0.74%     
==========================================
  Files        1612     1569      -43     
  Lines       64999    61660    -3339     
  Branches     6872     6232     -640     
==========================================
- Hits        43586    40894    -2692     
+ Misses      19546    19167     -379     
+ Partials     1867     1599     -268

Flag	Coverage Δ
hive	`53.23% <29.93%> (-0.09%)`	⬇️
mysql	`82.09% <63.05%> (-0.13%)`	⬇️
postgres	`82.14% <63.05%> (-0.13%)`	⬇️
presto	`53.07% <29.93%> (?)`
python	`82.58% <63.05%> (-0.01%)`	⬇️
sqlite	`81.83% <63.05%> (-0.13%)`	⬇️

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

Impacted Files	Coverage Δ
...rontend/src/components/Select/DeprecatedSelect.tsx	`37.33% <ø> (-8.23%)`	⬇️
superset-frontend/src/components/Select/styles.tsx	`67.92% <ø> (+1.82%)`	⬆️
superset/connectors/sqla/models.py	`88.31% <ø> (+1.23%)`	⬆️
superset/dashboards/filter_state/api.py	`100.00% <ø> (ø)`
superset/views/core.py	`77.74% <0.00%> (-0.08%)`	⬇️
superset/utils/encrypt.py	`41.46% <22.41%> (-46.54%)`	⬇️
superset/cli.py	`54.19% <37.50%> (-0.63%)`	⬇️
superset/reports/commands/create.py	`89.18% <92.30%> (+0.66%)`	⬆️
...rontend/src/dashboard/components/DashboardGrid.jsx	`65.11% <100.00%> (ø)`
superset/config.py	`91.66% <100.00%> (+0.02%)`	⬆️
... and 492 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 f7add54...b1d84d7. Read the comment docs.

etr2460

one question, but lgtm otherwise

etr2460 · 2022-01-11T03:50:45Z

superset/sql_parse.py

@@ -41,6 +42,16 @@
 logger = logging.getLogger(__name__)


+# Workaround for https://github.com/andialbrecht/sqlparse/issues/652.
+sqlparse.keywords.SQL_REGEX.insert(


do we not need to replace the erroneous regex?

@etr2460 I guess one could replace it though the position could change. This is a pattern I've used elsewhere where I simply increase the priority of the match by inserting it at the top of the list.

ah, that makes sense. didn't realize the list was prioritized

etr2460 · 2022-01-11T03:51:56Z

Oh also, should we consider forking sqlparse to:

Fix the issue preventing us from upgrading beyond 0.3.0
Let us fix other issues without cherry picking

cc @betodealmeida @villebro @amitmiran137 who might have thoughts here too

villebro

The change LGTM with some minor nits. It might also be worthwhile considering adding a unit test for this.

villebro · 2022-01-11T08:03:27Z

superset/sql_parse.py

@@ -41,6 +42,16 @@
 logger = logging.getLogger(__name__)


+# Workaround for https://github.com/andialbrecht/sqlparse/issues/652.


Could we add a TODO: here to remove when this is fixed upstream and also add a comment in setup.py where the version is pinned to remove this if it's been fixed?

@villebro I added a TODO. Note that sqlparse is pinned in setup.py here for other reasons—as already documented.

superset/sql_parse.py

etr2460 · 2022-01-11T20:06:28Z

It might also be worthwhile considering adding a unit test for this

@villebro there's a unit test already added

villebro · 2022-01-11T20:50:41Z

It might also be worthwhile considering adding a unit test for this

villebro there's a unit test already added

@etr2460 🤦 Right you are, not sure how I missed that. Thanks for setting me straight!

villebro

Approving, as my comments were mostly non-blocking suggestions

Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>

* fix: Workaround for sqlparse issue apache#652 * Update superset/sql_parse.py Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com> * Update sql_parse.py Co-authored-by: John Bodley <john.bodley@airbnb.com> Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>

fix: Workaround for sqlparse issue apache#652

bbb4aad

pull-request-size bot added the size/S label Jan 11, 2022

john-bodley requested review from etr2460 and villebro January 11, 2022 02:51

etr2460 approved these changes Jan 11, 2022

View reviewed changes

villebro reviewed Jan 11, 2022

View reviewed changes

villebro approved these changes Jan 11, 2022

View reviewed changes

john-bodley and others added 2 commits January 12, 2022 13:39

Update superset/sql_parse.py

07b9b7f

Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>

Update sql_parse.py

b1d84d7

john-bodley merged commit 63ca09e into apache:master Jan 12, 2022

john-bodley deleted the john-bodley--monkey-patch-sqlparse-issue-652 branch January 12, 2022 01:05

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Workaround for sqlparse issue #652 #17995

fix: Workaround for sqlparse issue #652 #17995

john-bodley commented Jan 11, 2022

codecov bot commented Jan 11, 2022 •

edited

Loading

etr2460 left a comment

etr2460 Jan 11, 2022

john-bodley Jan 11, 2022

etr2460 Jan 11, 2022

etr2460 commented Jan 11, 2022

villebro left a comment

villebro Jan 11, 2022

john-bodley Jan 12, 2022 •

edited

Loading

etr2460 commented Jan 11, 2022

villebro commented Jan 11, 2022

villebro left a comment

		@@ -41,6 +42,16 @@
		logger = logging.getLogger(__name__)


		# Workaround for https://github.com/andialbrecht/sqlparse/issues/652.

fix: Workaround for sqlparse issue #652 #17995

fix: Workaround for sqlparse issue #652 #17995

Conversation

john-bodley commented Jan 11, 2022

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Jan 11, 2022 • edited Loading

Codecov Report

etr2460 left a comment

Choose a reason for hiding this comment

etr2460 Jan 11, 2022

Choose a reason for hiding this comment

john-bodley Jan 11, 2022

Choose a reason for hiding this comment

etr2460 Jan 11, 2022

Choose a reason for hiding this comment

etr2460 commented Jan 11, 2022

villebro left a comment

Choose a reason for hiding this comment

villebro Jan 11, 2022

Choose a reason for hiding this comment

john-bodley Jan 12, 2022 • edited Loading

Choose a reason for hiding this comment

etr2460 commented Jan 11, 2022

villebro commented Jan 11, 2022

villebro left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 11, 2022 •

edited

Loading

john-bodley Jan 12, 2022 •

edited

Loading