fix: Revert "fix: don't strip SQL comments in Explore (#28363)"

[michael-s-molina]

SUMMARY

This reverts commit c618767.

After deploying Superset 4.0 to production a number of users reported issues where the rendered SQL statements were invalid if the underlying virtual dataset contained comments—likely trailing comments. Specifically the subquery lacked the trailing ).

We were able to determine that #28363 was the problematic PR and reverting it remedied the issue.

TESTING INSTRUCTIONS

CI and confirmed that the issue was mitigated after reverting.

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]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.54%. Comparing base (76d897e) to head (faeacc0).
Report is 142 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #28567      +/-   ##
==========================================
+ Coverage   60.48%   64.54%   +4.05%     
==========================================
  Files        1931      521    -1410     
  Lines       76236    37484   -38752     
  Branches     8568        0    -8568     
==========================================
- Hits        46114    24194   -21920     
+ Misses      28017    13290   -14727     
+ Partials     2105        0    -2105     

Flag	Coverage Δ
hive	49.09% <0.00%> (-0.07%)	⬇️
javascript	?
presto	53.64% <33.33%> (-0.16%)	⬇️
python	64.54% <100.00%> (+1.05%)	⬆️
unit	58.94% <66.66%> (+1.31%)	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mistercrunch]

Mmmmh. Seems we'll need to start over and write some unit tests with trailing comments (?) Did we save some of that SQL that was demonstrated to fail? Were they dash-dash comments or dash-star comments? I'm thinking we can write a failing unit test against #28363, and proceed to fix the issue. Hopefully that would provide enough confidence to move forward.

[villebro]

@mistercrunch this seems easy enough to fix this by adding a newline at the end if the query contains --. I've done something similar for adhoc metrics/columns back in the day, and there should be unit tests for those.

[michael-s-molina]

Seems we'll need to start over and write some unit tests with trailing comments (?) Did we save some of that SQL that was demonstrated to fail? Were they dash-dash comments or dash-star comments?

@mistercrunch We had problems with dash-dash comments. We can definitely test a future PR using our production data as multiple dashboards failed.

[mistercrunch]

A problem with dash-star is they don't embed into one-another well. My use case was around looking for a user hint (in the form of a dash-dash comment) and based on that altering the SQL. Now I wanted for the "view query" panel to show them the before and after transformation, but the dash-dash turns slash-star, and the end of their hint closes the opening /*.

Solution is either to keep dash-dash as is, or have the whole SQL_QUERY_MUTATOR feature have a way to keep track of the before and after and surface both (as opposed to doing this as a comment)