Remove comments from queries in SQL Lab that break Explore view

[villebro]

This fixes an issue where comments on the last line of the source query comment out the closing parenthesis of the subquery. Fixes issue #4412 .

[xrmx]

LGTM, tests would have been nice though :)

[villebro]

Sorry, I'm kinda new to this, will add a test shortly.

[rumbin]

Will this strip the comments when transferring to a table definition, or will they still exist right there but stripped when the table is queried on?

In the first case, I feel this would be an unexpected behavior. The user may have good reasons for commenting their code, e.g., why was something included or excluded in the query.

[villebro]

The comments are only removed when the table is queried on, i.e. they remain in the table definition.

[rumbin]

Perfect. Thanks for the explanation...

[villebro]

@xrmx I would appreciate some guidance on what type of test case is requested, e.g. a similar case or functionality, as I was unable to find anything that I could extend easily. The code base is still very new to me, so I'm still having a hard time understanding all the details of it's inner workings.

[xrmx]

@villebro I'd add a test that creates an instance of SqlaTable with a sql field with comments and check that calling get_from_clause() will return a FROM without the comments

[villebro]

Something along these lines?

diff --git a/tests/core_tests.py b/tests/core_tests.py
index 8b4dd27e..bd222226 100644
--- a/tests/core_tests.py
+++ b/tests/core_tests.py
@@ -871,6 +871,12 @@ class CoreTests(SupersetTestCase):
             {'data': pd.Timestamp('2017-11-18 22:06:30.061810+0100', tz=tz)},
         )
 
+    def test_comment_in_sqlatable_query(self):
+        query = "SELECT '--' as c1, c2 FROM tbl -- COMMENT"
+        table = SqlaTable(sql=query)
+        rendered_query = str(table.get_from_clause())
+        self.assertIn("'--' as c1", rendered_query)
+        self.assertNotIn("-- COMMENT", rendered_query)
 
 if __name__ == '__main__':
     unittest.main()

[xrmx]

@villebro I'd assert precisely what i expect table.get_from_clause() to return:

rendered_query = six.text_type(table.get_from_clause())
self.assertEqual(rendered_query, "SELECT '--' as c1, c2 FROM tbl")

[villebro]

Thanks again for the support; did the proposed changes and also added slash+asterisk comments. The test fails if #4413 is not merged. Will push this tomorrow if no other comments pop up.

diff --git a/tests/core_tests.py b/tests/core_tests.py
index 8b4dd27e..5c26e9ec 100644
--- a/tests/core_tests.py
+++ b/tests/core_tests.py
@@ -18,6 +18,7 @@ import unittest
 from flask import escape
 import pandas as pd
 import psycopg2
+from six import text_type
 import sqlalchemy as sqla
 
 from superset import appbuilder, dataframe, db, jinja_context, sm, sql_lab, utils
@@ -871,6 +872,11 @@ class CoreTests(SupersetTestCase):
             {'data': pd.Timestamp('2017-11-18 22:06:30.061810+0100', tz=tz)},
         )
 
+    def test_comment_in_sqlatable_query(self):
+        query = "/* comment 1 */SELECT '/* val 1 */' as c1, '-- val 2' as c2 FROM tbl-- comment 2"
+        table = SqlaTable(sql=query)
+        rendered_query = text_type(table.get_from_clause())
+        self.assertEqual("SELECT '/* val 1 */' as c1, '-- val 2' as c2 FROM tbl", rendered_query)
 
 if __name__ == '__main__':
     unittest.main()

[xrmx]

@villebro please fix https://travis-ci.org/apache/incubator-superset/jobs/342242520#L510

[wanyaxing]

Can this feature be optional?

Sometimes, the comments of sql are useable and necessary, I don't want them are removed.

Thanks.