fix(sqllab): throw errors of commented out query #23378

justinpark · 2023-03-14T23:33:43Z

SUMMARY

sqllab threw an error for undefined parameters even when those parameters are commented out of/not used by the query.
This commit cleans out the commented out part from the requested query to fix the issue (and also reduce the payload size since commented-out parts are unnecessary)

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

After:

Before:

TESTING INSTRUCTIONS

Go to sqllab and then run a query including a template string within comment-out block like select 123 --,{{ds}}

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 · 2023-03-15T00:26:09Z

Codecov Report

Merging #23378 (b82e57e) into master (db95a93) will increase coverage by 1.72%.
The diff coverage is 86.90%.

❗ Current head b82e57e differs from pull request most recent head 2e3b6c5. Consider uploading reports for the commit 2e3b6c5 to get more accurate results

@@            Coverage Diff             @@
##           master   #23378      +/-   ##
==========================================
+ Coverage   65.91%   67.64%   +1.72%     
==========================================
  Files        1907     1908       +1     
  Lines       73453    73741     +288     
  Branches     7976     7987      +11     
==========================================
+ Hits        48420    49881    +1461     
+ Misses      22983    21814    -1169     
+ Partials     2050     2046       -4

Flag	Coverage Δ
javascript	`53.82% <77.24%> (+0.08%)`	⬆️

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

Impacted Files	Coverage Δ
...-core/src/hooks/useChangeEffect/useChangeEffect.ts	`100.00% <ø> (ø)`
...hooks/useComponentDidMount/useComponentDidMount.ts	`100.00% <ø> (ø)`
...oks/useComponentDidUpdate/useComponentDidUpdate.ts	`100.00% <ø> (ø)`
...src/hooks/useElementOnScreen/useElementOnScreen.ts	`100.00% <ø> (ø)`
...erset-ui-core/src/hooks/usePrevious/usePrevious.ts	`100.00% <ø> (ø)`
...re/src/hooks/useTruncation/useCSSTextTruncation.ts	`100.00% <ø> (ø)`
...c/hooks/useTruncation/useChildElementTruncation.ts	`100.00% <ø> (ø)`
...ackages/superset-ui-core/src/utils/featureFlags.ts	`100.00% <ø> (ø)`
...s/legacy-plugin-chart-country-map/src/countries.ts	`100.00% <ø> (ø)`
...chart-echarts/src/Timeseries/Area/controlPanel.tsx	`40.00% <ø> (ø)`
... and 102 more

... and 74 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

justinpark · 2023-03-21T17:28:35Z

@michael-s-molina could you help this code review?

michael-s-molina

Thank you for the PR @justinpark. I left a couple of comments.

michael-s-molina · 2023-03-17T13:45:36Z

superset-frontend/src/SqlLab/actions/sqlLab.js

+export function cleanSqlComments(sql) {
+ if (!sql) return '';
+
+ return sql.replace(/(\/\*[^*]*\*\/)|(\/\/[^*]*)|(--[^.].*)/gm, '').trim();


Suggested change

return sql.replace(/(\/\*[^*]*\*\/)|(\/\/[^*]*)|(--[^.].*)/gm, '').trim();

// group 1 -> /* */

// group 2 -> //

// group 3 -> --

return sql.replace(/(\/\*[^*]*\*\/)|(\/\/[^*]*)|(--[^.].*)/gm, '').trim();

select * queries are only supported by double dashes. I'm not sure if you want to support them for other comment types.

good catch. I'll update the pr to support other styles too.

According to the current editor inspection, it won't accept the group 2 style //.
I'll update to clean up both cases of /* */ and --

It seems there's a case with empty spaces where it's matching but it shouldn’t.

@michael-s-molina just update to preserve the newlines.

@justinpark The latest update broke // comments. You can test the regex here.

@michael-s-molina that's intended.

As I mentioned above, // comment visually not recognized as comment-out block so it doesn't sanitize the // comments.

Oh I see. Thank you for the clarification.

justinpark · 2023-03-23T15:59:20Z

@michael-s-molina could you help the review for updates?

michael-s-molina · 2023-03-23T16:21:04Z

@michael-s-molina could you help the review for updates?

@justinpark I reviewed and left this comment.

michael-s-molina

LGTM

This reverts commit a0bab8d.

…che#23378)"" This reverts commit 0ee676e.

This reverts commit d1947f7.

fix(sqllab): throw errors of commented out query

cc3a790

pull-request-size bot added the size/M label Mar 14, 2023

michael-s-molina reviewed Mar 21, 2023

View reviewed changes

update regex to clean up select * with /* */

724cfa9

preserve newlines in the middle

2e3b6c5

michael-s-molina reviewed Mar 23, 2023

View reviewed changes

michael-s-molina approved these changes Mar 23, 2023

View reviewed changes

justinpark merged commit d1947f7 into apache:master Mar 23, 2023

justinpark added a commit to airbnb/superset-fork that referenced this pull request Mar 23, 2023

fix(sqllab): throw errors of commented out query (apache#23378)

a0bab8d

justinpark added a commit to airbnb/superset-fork that referenced this pull request Mar 24, 2023

Revert "fix(sqllab): throw errors of commented out query (apache#23378)"

0ee676e

This reverts commit a0bab8d.

justinpark added a commit to airbnb/superset-fork that referenced this pull request Apr 18, 2023

Revert "Revert "fix(sqllab): throw errors of commented out query (apa…

e74ec39

…che#23378)"" This reverts commit 0ee676e.

justinpark mentioned this pull request May 2, 2023

fix(sqllab): clean comments within quotes #23908

Merged

9 tasks

justinpark added a commit to justinpark/superset that referenced this pull request May 10, 2023

Revert "fix(sqllab): throw errors of commented out query (apache#23378)"

67ac8e8

This reverts commit d1947f7.

justinpark mentioned this pull request May 10, 2023

fix(sqllab): rollback clean comments out #24009

Merged

9 tasks

justinpark added a commit to airbnb/superset-fork that referenced this pull request May 10, 2023

Revert "fix(sqllab): throw errors of commented out query (apache#23378)"

ea8b785

This reverts commit d1947f7.

john-bodley added a commit to airbnb/superset-fork that referenced this pull request May 10, 2023

Revert "fix(sqllab): throw errors of commented out query (apache#23378)"

98ae166

This reverts commit d1947f7.

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.0.0 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(sqllab): throw errors of commented out query #23378

fix(sqllab): throw errors of commented out query #23378

justinpark commented Mar 14, 2023

codecov bot commented Mar 15, 2023 •

edited

Loading

justinpark commented Mar 21, 2023

michael-s-molina left a comment

michael-s-molina Mar 17, 2023

michael-s-molina Mar 17, 2023

justinpark Mar 22, 2023

justinpark Mar 22, 2023

michael-s-molina Mar 23, 2023

justinpark Mar 23, 2023

michael-s-molina Mar 23, 2023

justinpark Mar 23, 2023

michael-s-molina Mar 23, 2023

justinpark commented Mar 23, 2023

michael-s-molina commented Mar 23, 2023

michael-s-molina left a comment

- return sql.replace(/(\/\*[^*]*\*\/)|(\/\/[^*]*)|(--[^.].*)/gm, '').trim();
+ // group 1 -> /* */
+ // group 2 -> //
+ // group 3 -> --
+ return sql.replace(/(\/\*[^*]*\*\/)|(\/\/[^*]*)|(--[^.].*)/gm, '').trim();

fix(sqllab): throw errors of commented out query #23378

fix(sqllab): throw errors of commented out query #23378

Conversation

justinpark commented Mar 14, 2023

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Mar 15, 2023 • edited Loading

Codecov Report

justinpark commented Mar 21, 2023

michael-s-molina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinpark commented Mar 23, 2023

michael-s-molina commented Mar 23, 2023

michael-s-molina left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 15, 2023 •

edited

Loading