From 1e65d93a8367f6cbea1f66b3544cdea4548dcd3e Mon Sep 17 00:00:00 2001 From: Bexultan Date: Fri, 14 Aug 2026 23:07:10 +0500 Subject: [PATCH] fix(sql): handle ORDER BY in embedded MSSQL queries (#43127) Co-authored-by: Bexultan Mustafin Co-authored-by: Amin Ghadersohi --- superset/models/helpers.py | 22 +++++++ superset/sql/parse.py | 12 ++++ .../models/test_virtual_dataset_format.py | 58 +++++++++++++++++++ tests/unit_tests/sql/parse_tests.py | 16 +++++ 4 files changed, 108 insertions(+) diff --git a/superset/models/helpers.py b/superset/models/helpers.py index 0f086c831e1..b87427eaf41 100644 --- a/superset/models/helpers.py +++ b/superset/models/helpers.py @@ -181,6 +181,24 @@ OFFSET_JOIN_COLUMN_SUFFIX = "__offset_join_column_" R_SUFFIX = "__right_suffix" +def _normalize_mssql_virtual_dataset_sql( + sql: str, parsed_script: SQLScript, engine: str +) -> str: + """Remove SQL Server ordering that is invalid inside a derived table.""" + if engine != "mssql" or not parsed_script.statements: + return sql + + statement = parsed_script.statements[0] + if not isinstance(statement, SQLStatement): + return sql + + return ( + parsed_script.format() + if statement.remove_unbounded_top_level_order_by() + else sql + ) + + def _as_wall_clock(series: pd.Series) -> pd.Series: """ Return a datetime series as local wall-clock readings, dropping any @@ -3174,6 +3192,10 @@ class ExploreMixin: # pylint: disable=too-many-public-methods ex, ) + from_sql = _normalize_mssql_virtual_dataset_sql( + from_sql, parsed_script, self.db_engine_spec.engine + ) + cte = self.db_engine_spec.get_cte_query(from_sql) from_clause = ( sa.table(self.db_engine_spec.cte_alias) diff --git a/superset/sql/parse.py b/superset/sql/parse.py index dc6d88acbd7..b1b93892d47 100644 --- a/superset/sql/parse.py +++ b/superset/sql/parse.py @@ -1451,6 +1451,18 @@ class SQLStatement(BaseSQLStatement[exp.Expression]): """ return bool(self._parsed.args.get("with_")) + def remove_unbounded_top_level_order_by(self) -> bool: + """Drop ordering that becomes invalid when this query is embedded.""" + if ( + self._parsed.args.get("order") + and not self._parsed.args.get("limit") + and not self._parsed.args.get("offset") + and not self._parsed.args.get("for_") + ): + self._parsed.set("order", None) + return True + return False + def as_cte(self, alias: str = "__cte") -> SQLStatement: """ Rewrite the statement as a CTE. diff --git a/tests/unit_tests/models/test_virtual_dataset_format.py b/tests/unit_tests/models/test_virtual_dataset_format.py index 5df5a4ac2f1..c063d32efb7 100644 --- a/tests/unit_tests/models/test_virtual_dataset_format.py +++ b/tests/unit_tests/models/test_virtual_dataset_format.py @@ -164,6 +164,64 @@ class TestVirtualDatasetNoRLS: inner_sql = _get_subquery_sql(virtual_datasource) assert "::varchar(256)" in inner_sql + @patch("superset.models.helpers.apply_rls", return_value=False) + def test_mssql_unbounded_order_by_removed_when_embedded( + self, + mock_apply_rls: MagicMock, + virtual_datasource: MagicMock, + app: Flask, + ) -> None: + """MSSQL derived tables omit an unbounded top-level ordering.""" + virtual_datasource.db_engine_spec.engine = "mssql" + _set_virtual_sql( + virtual_datasource, + "SELECT category, amount FROM sample_events ORDER BY category, amount", + ) + + assert "ORDER BY" not in _get_subquery_sql(virtual_datasource) + + @patch("superset.models.helpers.apply_rls", return_value=False) + def test_mssql_hint_survives_order_by_rewrite( + self, + mock_apply_rls: MagicMock, + virtual_datasource: MagicMock, + app: Flask, + ) -> None: + """Required T-SQL syntax survives the unavoidable AST round trip.""" + virtual_datasource.db_engine_spec.engine = "mssql" + _set_virtual_sql( + virtual_datasource, + "SELECT [category] FROM [dbo].[sample_events] WITH (NOLOCK) " + "ORDER BY [category]", + ) + + inner_sql = _get_subquery_sql(virtual_datasource) + assert "ORDER BY" not in inner_sql + assert "WITH (NOLOCK)" in inner_sql + assert "[category]" in inner_sql + + @pytest.mark.parametrize( + "sql", + [ + "SELECT TOP 10 PERCENT category FROM sample_events ORDER BY category", + "SELECT TOP 1 WITH TIES category FROM sample_events ORDER BY category", + "SELECT category FROM sample_events ORDER BY category FOR XML PATH('')", + ], + ) + @patch("superset.models.helpers.apply_rls", return_value=False) + def test_mssql_required_order_by_preserved_when_embedded( + self, + mock_apply_rls: MagicMock, + virtual_datasource: MagicMock, + app: Flask, + sql: str, + ) -> None: + """TOP and serialization clauses retain their semantic ordering.""" + virtual_datasource.db_engine_spec.engine = "mssql" + _set_virtual_sql(virtual_datasource, sql) + + assert "ORDER BY" in _get_subquery_sql(virtual_datasource) + class TestVirtualDatasetWithRLS: """ diff --git a/tests/unit_tests/sql/parse_tests.py b/tests/unit_tests/sql/parse_tests.py index 8d2924f1c98..3a8370ace7b 100644 --- a/tests/unit_tests/sql/parse_tests.py +++ b/tests/unit_tests/sql/parse_tests.py @@ -2824,6 +2824,22 @@ def test_as_cte_called_twice() -> None: stmt.as_cte() +@pytest.mark.parametrize( + ("sql", "removed"), + [ + ("SELECT value FROM source ORDER BY value", True), + ("SELECT TOP 1 value FROM source ORDER BY value", False), + ("SELECT value FROM source ORDER BY value OFFSET 0 ROWS", False), + ("SELECT value FROM source ORDER BY value FOR JSON AUTO", False), + ], +) +def test_remove_unbounded_top_level_order_by(sql: str, removed: bool) -> None: + statement = SQLStatement(sql, "mssql") + + assert statement.remove_unbounded_top_level_order_by() is removed + assert ("ORDER BY" not in statement.format()) is removed + + @pytest.mark.parametrize( "sql, rules, expected", [