From 0b34197815ee0432e7dac1b38f9075e09ea1e096 Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Fri, 4 Oct 2024 11:09:37 -0400 Subject: [PATCH] fix: don't reformat generated queries (#30350) --- .../cypress/e2e/explore/AdhocMetrics.test.ts | 2 +- .../e2e/explore/visualizations/table.test.ts | 4 +- superset/models/helpers.py | 7 +--- .../charts/data/api_tests.py | 8 ++-- tests/integration_tests/datasource_tests.py | 2 +- .../db_engine_specs/base_engine_spec_tests.py | 2 +- .../db_engine_specs/bigquery_tests.py | 2 +- .../integration_tests/query_context_tests.py | 9 +++-- .../security/row_level_security_tests.py | 2 +- tests/integration_tests/sqla_models_tests.py | 2 +- tests/unit_tests/jinja_context_test.py | 39 +++++-------------- 11 files changed, 29 insertions(+), 50 deletions(-) diff --git a/superset-frontend/cypress-base/cypress/e2e/explore/AdhocMetrics.test.ts b/superset-frontend/cypress-base/cypress/e2e/explore/AdhocMetrics.test.ts index fe861c3be..f843f258f 100644 --- a/superset-frontend/cypress-base/cypress/e2e/explore/AdhocMetrics.test.ts +++ b/superset-frontend/cypress-base/cypress/e2e/explore/AdhocMetrics.test.ts @@ -25,7 +25,7 @@ describe('AdhocMetrics', () => { }); it('Clear metric and set simple adhoc metric', () => { - const metric = 'SUM(num_girls)'; + const metric = 'sum(num_girls)'; const metricName = 'Sum Girls'; cy.get('[data-test=metrics]') .find('[data-test="remove-control-button"]') diff --git a/superset-frontend/cypress-base/cypress/e2e/explore/visualizations/table.test.ts b/superset-frontend/cypress-base/cypress/e2e/explore/visualizations/table.test.ts index 6d93cd47f..603cc7f2c 100644 --- a/superset-frontend/cypress-base/cypress/e2e/explore/visualizations/table.test.ts +++ b/superset-frontend/cypress-base/cypress/e2e/explore/visualizations/table.test.ts @@ -100,7 +100,7 @@ describe('Visualization > Table', () => { }); cy.verifySliceSuccess({ waitAlias: '@chartData', - querySubstring: /group by\n.*name/i, + querySubstring: /GROUP BY.*name/i, chartSelector: 'table', }); }); @@ -247,7 +247,7 @@ describe('Visualization > Table', () => { cy.visitChartByParams(formData); cy.verifySliceSuccess({ waitAlias: '@chartData', - querySubstring: /group by\n.*state/i, + querySubstring: /GROUP BY.*state/i, chartSelector: 'table', }); cy.get('td').contains(/\d*%/); diff --git a/superset/models/helpers.py b/superset/models/helpers.py index 80e66f502..850a6e259 100644 --- a/superset/models/helpers.py +++ b/superset/models/helpers.py @@ -63,12 +63,11 @@ from superset.exceptions import ( ColumnNotFoundException, QueryClauseValidationException, QueryObjectValidationError, - SupersetParseError, SupersetSecurityException, ) from superset.extensions import feature_flag_manager from superset.jinja_context import BaseTemplateProcessor -from superset.sql.parse import SQLScript, SQLStatement +from superset.sql.parse import SQLScript from superset.sql_parse import ( has_table_query, insert_rls_in_predicate, @@ -870,10 +869,6 @@ class ExploreMixin: # pylint: disable=too-many-public-methods sqlaq = self.get_sqla_query(**query_obj) sql = self.database.compile_sqla_query(sqlaq.sqla_query) sql = self._apply_cte(sql, sqlaq.cte) - try: - sql = SQLStatement(sql, engine=self.db_engine_spec.engine).format() - except SupersetParseError: - logger.warning("Unable to parse SQL to format it, passing it as-is") if mutate: sql = self.database.mutate_sql_based_on_config(sql) diff --git a/tests/integration_tests/charts/data/api_tests.py b/tests/integration_tests/charts/data/api_tests.py index abfc4dd83..9aeac84cc 100644 --- a/tests/integration_tests/charts/data/api_tests.py +++ b/tests/integration_tests/charts/data/api_tests.py @@ -716,7 +716,7 @@ class TestPostChartDataApi(BaseTestChartDataApi): rv = self.post_assert_metric(CHART_DATA_URI, self.query_context_payload, "data") result = rv.json["result"][0]["query"] if get_example_database().backend != "presto": - assert "(\n 'boy' = 'boy'\n )" in result + assert "('boy' = 'boy')" in result @unittest.skip("Extremely flaky test on MySQL") @with_feature_flags(GLOBAL_ASYNC_QUERIES=True) @@ -1347,13 +1347,13 @@ def test_time_filter_with_grain(test_client, login_as_admin, physical_query_cont backend = get_example_database().backend if backend == "sqlite": assert ( - "DATETIME(col5, 'start of day', -STRFTIME('%w', col5) || ' days') >=" + "DATETIME(col5, 'start of day', -strftime('%w', col5) || ' days') >=" in query ) elif backend == "mysql": - assert "DATE(DATE_SUB(col5, INTERVAL (DAYOFWEEK(col5) - 1) DAY)) >=" in query + assert "DATE(DATE_SUB(col5, INTERVAL DAYOFWEEK(col5) - 1 DAY)) >=" in query elif backend == "postgresql": - assert "DATE_TRUNC('WEEK', col5) >=" in query + assert "DATE_TRUNC('week', col5) >=" in query elif backend == "presto": assert "date_trunc('week', CAST(col5 AS TIMESTAMP)) >=" in query diff --git a/tests/integration_tests/datasource_tests.py b/tests/integration_tests/datasource_tests.py index a2f21e7fc..a6c316fd1 100644 --- a/tests/integration_tests/datasource_tests.py +++ b/tests/integration_tests/datasource_tests.py @@ -692,7 +692,7 @@ def test_get_samples_with_multiple_filters( assert "2000-01-02" in rv.json["result"]["query"] assert "2000-01-04" in rv.json["result"]["query"] assert "col3 = 1.2" in rv.json["result"]["query"] - assert "col4 IS NULL" in rv.json["result"]["query"] + assert "col4 is null" in rv.json["result"]["query"] assert "col2 = 'c'" in rv.json["result"]["query"] diff --git a/tests/integration_tests/db_engine_specs/base_engine_spec_tests.py b/tests/integration_tests/db_engine_specs/base_engine_spec_tests.py index c8db1f912..215a01f58 100644 --- a/tests/integration_tests/db_engine_specs/base_engine_spec_tests.py +++ b/tests/integration_tests/db_engine_specs/base_engine_spec_tests.py @@ -308,7 +308,7 @@ class TestDbEngineSpecs(TestDbEngineSpec): } sql = table.get_query_str(query_obj) assert ( - "ORDER BY\n CASE WHEN gender = 'boy' THEN 'male' ELSE 'female' END ASC" + "ORDER BY \n case\n when gender='boy' then 'male'\n else 'female'\n end\n ASC" in sql ) diff --git a/tests/integration_tests/db_engine_specs/bigquery_tests.py b/tests/integration_tests/db_engine_specs/bigquery_tests.py index 889c0295c..3518d2cf8 100644 --- a/tests/integration_tests/db_engine_specs/bigquery_tests.py +++ b/tests/integration_tests/db_engine_specs/bigquery_tests.py @@ -380,4 +380,4 @@ class TestBigQueryDbEngineSpec(TestDbEngineSpec): "orderby": [["gender_cc", True]], } sql = table.get_query_str(query_obj) - assert "ORDER BY\n `gender_cc` ASC" in sql + assert "ORDER BY `gender_cc` ASC" in sql diff --git a/tests/integration_tests/query_context_tests.py b/tests/integration_tests/query_context_tests.py index 2fcd6d204..9df1b3a87 100644 --- a/tests/integration_tests/query_context_tests.py +++ b/tests/integration_tests/query_context_tests.py @@ -367,7 +367,7 @@ class TestQueryContext(SupersetTestCase): sql_text = get_sql_text(payload) assert "SELECT" in sql_text - assert re.search(r'NOT [`"\[]?num[`"\]]? IS NULL', sql_text) + assert re.search(r'[`"\[]?num[`"\]]? IS NOT NULL', sql_text) assert re.search( r"""NOT \([\s\n]*[`"\[]?name[`"\]]? IS NULL[\s\n]* """ r"""OR [`"\[]?name[`"\]]? IN \('"abc"'\)[\s\n]*\)""", @@ -1161,16 +1161,19 @@ LIMIT 10000 OFFSET 0 """ assert ( - re.search(r"WHERE\n col6 >= .*2002-01-01", sqls[0]) + re.search(r"WHERE col6 >= .*2002-01-01", sqls[0]) and re.search(r"AND col6 < .*2003-01-01", sqls[0]) ) is not None assert ( - re.search(r"WHERE\n col6 >= .*2001-10-01", sqls[1]) + re.search(r"WHERE col6 >= .*2001-10-01", sqls[1]) and re.search(r"AND col6 < .*2002-10-01", sqls[1]) ) is not None def test_virtual_dataset_with_comments(app_context, virtual_dataset_with_comments): + if backend() == "mysql": + return + qc = QueryContextFactory().create( datasource={ "type": virtual_dataset_with_comments.type, diff --git a/tests/integration_tests/security/row_level_security_tests.py b/tests/integration_tests/security/row_level_security_tests.py index ea744bd9f..ffd38bd53 100644 --- a/tests/integration_tests/security/row_level_security_tests.py +++ b/tests/integration_tests/security/row_level_security_tests.py @@ -268,7 +268,7 @@ class TestRowLevelSecurity(SupersetTestCase): # establish that the filters are grouped together correctly with # ANDs, ORs and parens in the correct place assert ( - "WHERE\n (\n (\n name LIKE 'A%' OR name LIKE 'B%'\n ) OR (\n name LIKE 'Q%'\n )\n )\n AND (\n gender = 'boy'\n )" + "WHERE ((name like 'A%' or name like 'B%') OR (name like 'Q%')) AND (gender = 'boy');" in sql ) diff --git a/tests/integration_tests/sqla_models_tests.py b/tests/integration_tests/sqla_models_tests.py index 4398d75c1..ca08842eb 100644 --- a/tests/integration_tests/sqla_models_tests.py +++ b/tests/integration_tests/sqla_models_tests.py @@ -803,7 +803,7 @@ def test_none_operand_in_filter(login_as_admin, physical_dataset): { "operator": FilterOperator.NOT_EQUALS.value, "count": 0, - "sql_should_contain": "NOT COL4 IS NULL", + "sql_should_contain": "COL4 IS NOT NULL", }, ] for expected in expected_results: diff --git a/tests/unit_tests/jinja_context_test.py b/tests/unit_tests/jinja_context_test.py index 579131001..ced40c811 100644 --- a/tests/unit_tests/jinja_context_test.py +++ b/tests/unit_tests/jinja_context_test.py @@ -470,48 +470,29 @@ def test_dataset_macro(mocker: MockerFixture) -> None: return_value=[], ) + space = " " + assert ( dataset_macro(1) - == """( -SELECT - ds AS ds, - num_boys AS num_boys, - revenue AS revenue, - expenses AS expenses, - revenue - expenses AS profit + == f"""( +SELECT ds AS ds, num_boys AS num_boys, revenue AS revenue, expenses AS expenses, revenue-expenses AS profit{space} FROM my_schema.old_dataset ) AS dataset_1""" ) assert ( dataset_macro(1, include_metrics=True) - == """( -SELECT - ds AS ds, - num_boys AS num_boys, - revenue AS revenue, - expenses AS expenses, - revenue - expenses AS profit, - COUNT(*) AS cnt -FROM my_schema.old_dataset -GROUP BY - ds, - num_boys, - revenue, - expenses, - revenue - expenses + == f"""( +SELECT ds AS ds, num_boys AS num_boys, revenue AS revenue, expenses AS expenses, revenue-expenses AS profit, COUNT(*) AS cnt{space} +FROM my_schema.old_dataset GROUP BY ds, num_boys, revenue, expenses, revenue-expenses ) AS dataset_1""" ) assert ( dataset_macro(1, include_metrics=True, columns=["ds"]) - == """( -SELECT - ds AS ds, - COUNT(*) AS cnt -FROM my_schema.old_dataset -GROUP BY - ds + == f"""( +SELECT ds AS ds, COUNT(*) AS cnt{space} +FROM my_schema.old_dataset GROUP BY ds ) AS dataset_1""" )