From 07d6d211f23f1d9d1d69fd54e8054bccd515bc8c Mon Sep 17 00:00:00 2001 From: Federico Caselli Date: Thu, 16 Apr 2020 23:16:32 +0200 Subject: [PATCH] Deprecate ``DISTINCT ON`` when not targeting PostgreSQL Deprecate usage of ``DISTINCT ON`` in dialect other than PostgreSQL. Previously this was silently ignored. Deprecate old usage of string distinct in MySQL dialect Fixes: #4002 Change-Id: I38fc64aef75e77748083c11d388ec831f161c9c9 --- doc/build/changelog/unreleased_14/4002.rst | 6 ++++++ lib/sqlalchemy/dialects/firebird/base.py | 3 +-- lib/sqlalchemy/dialects/mssql/base.py | 12 +++--------- lib/sqlalchemy/dialects/mysql/base.py | 21 ++++++++++++--------- lib/sqlalchemy/dialects/postgresql/base.py | 2 ++ lib/sqlalchemy/orm/query.py | 3 +++ lib/sqlalchemy/sql/compiler.py | 11 ++++++++++- lib/sqlalchemy/sql/selectable.py | 3 +++ lib/sqlalchemy/testing/assertions.py | 1 - lib/sqlalchemy/testing/requirements.py | 5 +++++ lib/sqlalchemy/testing/suite/test_select.py | 15 +++++++++++++++ test/dialect/mysql/test_deprecations.py | 20 ++++++++++++++++++++ test/requirements.py | 5 +++++ test/sql/test_compiler.py | 7 +++++++ 14 files changed, 92 insertions(+), 22 deletions(-) create mode 100644 doc/build/changelog/unreleased_14/4002.rst diff --git a/doc/build/changelog/unreleased_14/4002.rst b/doc/build/changelog/unreleased_14/4002.rst new file mode 100644 index 0000000000..53dc0cfa0c --- /dev/null +++ b/doc/build/changelog/unreleased_14/4002.rst @@ -0,0 +1,6 @@ +.. change:: + :tags: bug, sql + :tickets: 4002 + + Deprecate usage of ``DISTINCT ON`` in dialect other than PostgreSQL. + Deprecate old usage of string distinct in MySQL dialect diff --git a/lib/sqlalchemy/dialects/firebird/base.py b/lib/sqlalchemy/dialects/firebird/base.py index a43413780a..5779ac8858 100644 --- a/lib/sqlalchemy/dialects/firebird/base.py +++ b/lib/sqlalchemy/dialects/firebird/base.py @@ -525,8 +525,7 @@ class FBCompiler(sql.compiler.SQLCompiler): result += "FIRST %s " % self.process(select._limit_clause, **kw) if select._offset_clause is not None: result += "SKIP %s " % self.process(select._offset_clause, **kw) - if select._distinct: - result += "DISTINCT " + result += super(FBCompiler, self).get_select_precolumns(select, **kw) return result def limit_clause(self, select, **kw): diff --git a/lib/sqlalchemy/dialects/mssql/base.py b/lib/sqlalchemy/dialects/mssql/base.py index df6196baef..0dd2ac11b5 100644 --- a/lib/sqlalchemy/dialects/mssql/base.py +++ b/lib/sqlalchemy/dialects/mssql/base.py @@ -1636,9 +1636,7 @@ class MSSQLCompiler(compiler.SQLCompiler): def get_select_precolumns(self, select, **kw): """ MS-SQL puts TOP, it's version of LIMIT here """ - s = "" - if select._distinct: - s += "DISTINCT " + s = super(MSSQLCompiler, self).get_select_precolumns(select, **kw) if select._simple_int_limit and ( select._offset_clause is None @@ -1649,12 +1647,8 @@ class MSSQLCompiler(compiler.SQLCompiler): # so have to use literal here. kw["literal_execute"] = True s += "TOP %s " % self.process(select._limit_clause, **kw) - if s: - return s - else: - return compiler.SQLCompiler.get_select_precolumns( - self, select, **kw - ) + + return s def get_from_hint_text(self, table, text): return text diff --git a/lib/sqlalchemy/dialects/mysql/base.py b/lib/sqlalchemy/dialects/mysql/base.py index 53c9163049..38f3fa6111 100644 --- a/lib/sqlalchemy/dialects/mysql/base.py +++ b/lib/sqlalchemy/dialects/mysql/base.py @@ -1450,19 +1450,22 @@ class MySQLCompiler(compiler.SQLCompiler): def get_select_precolumns(self, select, **kw): """Add special MySQL keywords in place of DISTINCT. - .. note:: - - this usage is deprecated. :meth:`_expression.Select.prefix_with` - should be used for special keywords at the start - of a SELECT. + .. deprecated 1.4:: this usage is deprecated. + :meth:`_expression.Select.prefix_with` should be used for special + keywords at the start of a SELECT. """ if isinstance(select._distinct, util.string_types): + util.warn_deprecated( + "Sending string values for 'distinct' is deprecated in the " + "MySQL dialect and will be removed in a future release. " + "Please use :meth:`.Select.prefix_with` for special keywords " + "at the start of a SELECT statement", + version="1.4", + ) return select._distinct.upper() + " " - elif select._distinct: - return "DISTINCT " - else: - return "" + + return super(MySQLCompiler, self).get_select_precolumns(select, **kw) def visit_join(self, join, asfrom=False, from_linter=None, **kwargs): if from_linter: diff --git a/lib/sqlalchemy/dialects/postgresql/base.py b/lib/sqlalchemy/dialects/postgresql/base.py index 20540ac020..ca2f6a8a43 100644 --- a/lib/sqlalchemy/dialects/postgresql/base.py +++ b/lib/sqlalchemy/dialects/postgresql/base.py @@ -1693,6 +1693,8 @@ class PGCompiler(compiler.SQLCompiler): return "ONLY " + sqltext def get_select_precolumns(self, select, **kw): + # Do not call super().get_select_precolumns because + # it will warn/raise when distinct on is present if select._distinct or select._distinct_on: if select._distinct_on: return ( diff --git a/lib/sqlalchemy/orm/query.py b/lib/sqlalchemy/orm/query.py index 4d7eee9ba2..ab49a4dccc 100644 --- a/lib/sqlalchemy/orm/query.py +++ b/lib/sqlalchemy/orm/query.py @@ -3156,6 +3156,9 @@ class Query(Generative): the PostgreSQL dialect will render a ``DISTINCT ON ()`` construct. + .. deprecated:: 1.4 Using \*expr in other dialects is deprecated + and will raise :class:`_exc.CompileError` in a future version. + """ if not expr: self._distinct = True diff --git a/lib/sqlalchemy/sql/compiler.py b/lib/sqlalchemy/sql/compiler.py index bc16b14296..23eff773c4 100644 --- a/lib/sqlalchemy/sql/compiler.py +++ b/lib/sqlalchemy/sql/compiler.py @@ -2868,7 +2868,16 @@ class SQLCompiler(Compiled): before column list. """ - return select._distinct and "DISTINCT " or "" + if select._distinct_on: + util.warn_deprecated( + "DISTINCT ON is currently supported only by the PostgreSQL " + "dialect. Use of DISTINCT ON for other backends is currently " + "silently ignored, however this usage is deprecated, and will " + "raise CompileError in a future release for all backends " + "that do not support this syntax.", + version="1.4", + ) + return "DISTINCT " if select._distinct else "" def group_by_clause(self, select, **kw): """allow dialects to customize how GROUP BY is rendered.""" diff --git a/lib/sqlalchemy/sql/selectable.py b/lib/sqlalchemy/sql/selectable.py index 89ee7e28ff..c8df637baf 100644 --- a/lib/sqlalchemy/sql/selectable.py +++ b/lib/sqlalchemy/sql/selectable.py @@ -4138,6 +4138,9 @@ class Select( the PostgreSQL dialect will render a ``DISTINCT ON (>)`` construct. + .. deprecated:: 1.4 Using \*expr in other dialects is deprecated + and will raise :class:`_exc.CompileError` in a future version. + """ if expr: self._distinct = True diff --git a/lib/sqlalchemy/testing/assertions.py b/lib/sqlalchemy/testing/assertions.py index 61d272160a..05dcf230b2 100644 --- a/lib/sqlalchemy/testing/assertions.py +++ b/lib/sqlalchemy/testing/assertions.py @@ -412,7 +412,6 @@ class AssertsCompiledSQL(object): c = clause.compile(dialect=dialect, **kw) param_str = repr(getattr(c, "params", {})) - if util.py3k: param_str = param_str.encode("utf-8").decode("ascii", "ignore") print( diff --git a/lib/sqlalchemy/testing/requirements.py b/lib/sqlalchemy/testing/requirements.py index 644483b79d..31011a9708 100644 --- a/lib/sqlalchemy/testing/requirements.py +++ b/lib/sqlalchemy/testing/requirements.py @@ -1169,6 +1169,11 @@ class SuiteRequirements(Requirements): computed columns""" return exclusions.closed() + @property + def supports_distinct_on(self): + """If a backend supports the DISTINCT ON in a select""" + return exclusions.closed() + @property def supports_is_distinct_from(self): """Supports some form of "x IS [NOT] DISTINCT FROM y" construct. diff --git a/lib/sqlalchemy/testing/suite/test_select.py b/lib/sqlalchemy/testing/suite/test_select.py index c1f77a7c18..ad17ebb4ac 100644 --- a/lib/sqlalchemy/testing/suite/test_select.py +++ b/lib/sqlalchemy/testing/suite/test_select.py @@ -12,6 +12,7 @@ from ..schema import Column from ..schema import Table from ... import bindparam from ... import case +from ... import column from ... import Computed from ... import false from ... import func @@ -20,6 +21,7 @@ from ... import literal_column from ... import null from ... import select from ... import String +from ... import table from ... import testing from ... import text from ... import true @@ -1016,6 +1018,19 @@ class ComputedColumnTest(fixtures.TablesTest): eq_(res, [(100, 40), (1764, 168)]) +class DistinctOnTest(AssertsCompiledSQL, fixtures.TablesTest): + __backend__ = True + __requires__ = ("standard_cursor_sql",) + + @testing.fails_if(testing.requires.supports_distinct_on) + def test_distinct_on(self): + stm = select(["*"]).distinct(column("q")).select_from(table("foo")) + with testing.expect_deprecated( + "DISTINCT ON is currently supported only by the PostgreSQL " + ): + self.assert_compile(stm, "SELECT DISTINCT * FROM foo") + + class IsOrIsNotDistinctFromTest(fixtures.TablesTest): __backend__ = True __requires__ = ("supports_is_distinct_from",) diff --git a/test/dialect/mysql/test_deprecations.py b/test/dialect/mysql/test_deprecations.py index 544284a219..b2bd99d828 100644 --- a/test/dialect/mysql/test_deprecations.py +++ b/test/dialect/mysql/test_deprecations.py @@ -1,9 +1,29 @@ +from sqlalchemy import select +from sqlalchemy import table +from sqlalchemy.dialects.mysql import base as mysql from sqlalchemy.dialects.mysql import ENUM from sqlalchemy.dialects.mysql import SET +from sqlalchemy.testing import AssertsCompiledSQL +from sqlalchemy.testing import expect_deprecated from sqlalchemy.testing import expect_deprecated_20 from sqlalchemy.testing import fixtures +class CompileTest(AssertsCompiledSQL, fixtures.TestBase): + + __dialect__ = mysql.dialect() + + def test_distinct_string(self): + s = select(["*"]).select_from(table("foo")) + s._distinct = "foo" + + with expect_deprecated( + "Sending string values for 'distinct' is deprecated in the MySQL " + "dialect and will be removed in a future release" + ): + self.assert_compile(s, "SELECT FOO * FROM foo") + + class DeprecateQuoting(fixtures.TestBase): def test_enum_warning(self): ENUM("a", "b") diff --git a/test/requirements.py b/test/requirements.py index aac376dba1..cf9168f5a2 100644 --- a/test/requirements.py +++ b/test/requirements.py @@ -1611,3 +1611,8 @@ class DefaultRequirements(SuiteRequirements): @property def computed_columns_reflect_persisted(self): return self.computed_columns + skip_if("oracle") + + @property + def supports_distinct_on(self): + """If a backend supports the DISTINCT ON in a select""" + return only_if(["postgresql"]) diff --git a/test/sql/test_compiler.py b/test/sql/test_compiler.py index e44deed90d..440eaa05a7 100644 --- a/test/sql/test_compiler.py +++ b/test/sql/test_compiler.py @@ -1462,6 +1462,13 @@ class SelectTest(fixtures.TestBase, AssertsCompiledSQL): "SELECT count(DISTINCT mytable.myid) AS count_1 FROM mytable", ) + def test_distinct_on(self): + with testing.expect_deprecated( + "DISTINCT ON is currently supported only by the PostgreSQL " + "dialect" + ): + select(["*"]).distinct(table1.c.myid).compile() + def test_where_empty(self): self.assert_compile( select([table1.c.myid]).where( -- 2.39.5