-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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(sqllab): Do not strip comments when executing SQL statements #29248
fix(sqllab): Do not strip comments when executing SQL statements #29248
Conversation
5f49448
to
d27b3e8
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #29248 +/- ##
===========================================
+ Coverage 60.48% 83.73% +23.24%
===========================================
Files 1931 518 -1413
Lines 76236 37541 -38695
Branches 8568 0 -8568
===========================================
- Hits 46114 31435 -14679
+ Misses 28017 6106 -21911
+ Partials 2105 0 -2105
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
... I think this is the first time I've ever heard someone use the correct nomenclature with this expression. |
) (cherry picked from commit b50e3af)
SUMMARY
Man, the SQL Lab execution/parsing logic is a real rabbit warren—so many methods and diversions to execute SQL.
#27725 attempted to remove stripping comments when executing queries in SQL Lab, however it likely wasn't suffice. #28363 (which also preventing stripping of comments in Explore) remedied this, however it was later reverted in #28567.
The original issue with #27725 is it seems like the SQL is pre-processed/rendered in the command prior to the typical SQL Lab execution logic via Celery et al. This PR simply ensures that comments are never stripped within the context of SQL Lab. This change shouldn't cause the issue surfaced in #28567.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
CI. The relevant tests were updated using logic from #28363.
ADDITIONAL INFORMATION