From b7226379ac06c9a1a78e783deaa60c701b1b7e88 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Wed, 6 Oct 2021 18:51:08 -0400 Subject: fixes for usage of the null() and similar constants Adjusted the "column disambiguation" logic that's new in 1.4, where the same expression repeated gets an "extra anonymous" label, so that the logic more aggressively deduplicates those labels when the repeated element is the same Python expression object each time, as occurs in cases like when using "singleton" values like :func:`_sql.null`. This is based on the observation that at least some databases (e.g. MySQL, but not SQLite) will raise an error if the same label is repeated inside of a subquery. Related to :ticket:`7153`, fixed an issue where result column lookups would fail for "adapted" SELECT statements that selected for "constant" value expressions most typically the NULL expression, as would occur in such places as joined eager loading in conjunction with limit/offset. This was overall a regression due to issue :ticket:`6259` which removed all "adaption" for constants like NULL, "true", and "false", but this broke the case where the same adaption logic were used to match the constant to a labeled expression referring to the constant in a subquery. Fixes: #7153 Fixes: #7154 Change-Id: I43823343721b9e70524ea3f5e8f39dd543a3e92b --- lib/sqlalchemy/sql/base.py | 20 +++++++++++------ lib/sqlalchemy/sql/elements.py | 46 ++++++++++++++++++++++++++++------------ lib/sqlalchemy/sql/selectable.py | 30 ++++++++++++++++++++------ lib/sqlalchemy/sql/util.py | 31 ++++++++++++++++++++++++--- 4 files changed, 98 insertions(+), 29 deletions(-) (limited to 'lib/sqlalchemy/sql') diff --git a/lib/sqlalchemy/sql/base.py b/lib/sqlalchemy/sql/base.py index a6870f8d4..b235f5132 100644 --- a/lib/sqlalchemy/sql/base.py +++ b/lib/sqlalchemy/sql/base.py @@ -39,6 +39,8 @@ NO_ARG = util.symbol("NO_ARG") class Immutable(object): """mark a ClauseElement as 'immutable' when expressions are cloned.""" + _is_immutable = True + def unique_params(self, *optionaldict, **kwargs): raise NotImplementedError("Immutable objects do not support copying") @@ -55,6 +57,8 @@ class Immutable(object): class SingletonConstant(Immutable): """Represent SQL constants like NULL, TRUE, FALSE""" + _is_singleton_constant = True + def __new__(cls, *arg, **kw): return cls._singleton @@ -62,14 +66,16 @@ class SingletonConstant(Immutable): def _create_singleton(cls): obj = object.__new__(cls) obj.__init__() - cls._singleton = obj - # don't proxy singletons. this means that a SingletonConstant - # will never be a "corresponding column" in a statement; the constant - # can be named directly and as it is often/usually compared against using - # "IS", it can't be adapted to a subquery column in any case. - # see :ticket:`6259`. - proxy_set = frozenset() + # for a long time this was an empty frozenset, meaning + # a SingletonConstant would never be a "corresponding column" in + # a statement. This referred to #6259. However, in #7154 we see + # that we do in fact need "correspondence" to work when matching cols + # in result sets, so the non-correspondence was moved to a more + # specific level when we are actually adapting expressions for SQL + # render only. + obj.proxy_set = frozenset([obj]) + cls._singleton = obj def _from_objects(*elements): diff --git a/lib/sqlalchemy/sql/elements.py b/lib/sqlalchemy/sql/elements.py index 8f02527b9..6f1756af3 100644 --- a/lib/sqlalchemy/sql/elements.py +++ b/lib/sqlalchemy/sql/elements.py @@ -214,6 +214,8 @@ class ClauseElement( _is_bind_parameter = False _is_clause_list = False _is_lambda_element = False + _is_singleton_constant = False + _is_immutable = False _order_by_label_element = None @@ -1023,19 +1025,39 @@ class ColumnElement( """ return Label(name, self, self.type) - def _anon_label(self, seed): + def _anon_label(self, seed, add_hash=None): while self._is_clone_of is not None: self = self._is_clone_of # as of 1.4 anonymous label for ColumnElement uses hash(), not id(), # as the identifier, because a column and its annotated version are # the same thing in a SQL statement + hash_value = hash(self) + + if add_hash: + # this path is used for disambiguating anon labels that would + # otherwise be the same name for the same element repeated. + # an additional numeric value is factored in for each label. + + # shift hash(self) (which is id(self), typically 8 byte integer) + # 16 bits leftward. fill extra add_hash on right + assert add_hash < (2 << 15) + assert seed + hash_value = (hash_value << 16) | add_hash + + # extra underscore is added for labels with extra hash + # values, to isolate the "deduped anon" namespace from the + # regular namespace. eliminates chance of these + # manufactured hash values overlapping with regular ones for some + # undefined python interpreter + seed = seed + "_" + if isinstance(seed, _anonymous_label): return _anonymous_label.safe_construct( - hash(self), "", enclosing_label=seed + hash_value, "", enclosing_label=seed ) - return _anonymous_label.safe_construct(hash(self), seed or "anon") + return _anonymous_label.safe_construct(hash_value, seed or "anon") @util.memoized_property def _anon_name_label(self): @@ -1093,8 +1115,7 @@ class ColumnElement( def anon_key_label(self): return self._anon_key_label - @util.memoized_property - def _dedupe_anon_label(self): + def _dedupe_anon_label_idx(self, idx): """label to apply to a column that is anon labeled, but repeated in the SELECT, so that we have to make an "extra anon" label that disambiguates it from the previous appearance. @@ -1113,9 +1134,9 @@ class ColumnElement( # "CAST(casttest.v1 AS DECIMAL) AS anon__1" if label is None: - return self._dedupe_anon_tq_label + return self._dedupe_anon_tq_label_idx(idx) else: - return self._anon_label(label + "_") + return self._anon_label(label, add_hash=idx) @util.memoized_property def _anon_tq_label(self): @@ -1125,10 +1146,10 @@ class ColumnElement( def _anon_tq_key_label(self): return self._anon_label(getattr(self, "_tq_key_label", None)) - @util.memoized_property - def _dedupe_anon_tq_label(self): + def _dedupe_anon_tq_label_idx(self, idx): label = getattr(self, "_tq_label", None) or "anon" - return self._anon_label(label + "_") + + return self._anon_label(label, add_hash=idx) class WrapsColumnExpression(object): @@ -1178,14 +1199,13 @@ class WrapsColumnExpression(object): return wce._anon_name_label return super(WrapsColumnExpression, self)._anon_name_label - @property - def _dedupe_anon_label(self): + def _dedupe_anon_label_idx(self, idx): wce = self.wrapped_column_expression nal = wce._non_anon_label if nal: return self._anon_label(nal + "_") else: - return self._dedupe_anon_tq_label + return self._dedupe_anon_tq_label_idx(idx) class BindParameter(roles.InElementRole, ColumnElement): diff --git a/lib/sqlalchemy/sql/selectable.py b/lib/sqlalchemy/sql/selectable.py index 8f8e6b2a7..8e71dfb97 100644 --- a/lib/sqlalchemy/sql/selectable.py +++ b/lib/sqlalchemy/sql/selectable.py @@ -6066,6 +6066,14 @@ class Select( table_qualified = self._label_style is LABEL_STYLE_TABLENAME_PLUS_COL label_style_none = self._label_style is LABEL_STYLE_NONE + # a counter used for "dedupe" labels, which have double underscores + # in them and are never referred by name; they only act + # as positional placeholders. they need only be unique within + # the single columns clause they're rendered within (required by + # some dbs such as mysql). So their anon identity is tracked against + # a fixed counter rather than hash() identity. + dedupe_hash = 1 + for c in cols: repeated = False @@ -6099,9 +6107,15 @@ class Select( # here, "required_label_name" is sent as # "None" and "fallback_label_name" is sent. if table_qualified: - fallback_label_name = c._dedupe_anon_tq_label + fallback_label_name = ( + c._dedupe_anon_tq_label_idx(dedupe_hash) + ) + dedupe_hash += 1 else: - fallback_label_name = c._dedupe_anon_label + fallback_label_name = c._dedupe_anon_label_idx( + dedupe_hash + ) + dedupe_hash += 1 else: fallback_label_name = c._anon_name_label else: @@ -6142,11 +6156,13 @@ class Select( if table_qualified: required_label_name = ( fallback_label_name - ) = c._dedupe_anon_tq_label + ) = c._dedupe_anon_tq_label_idx(dedupe_hash) + dedupe_hash += 1 else: required_label_name = ( fallback_label_name - ) = c._dedupe_anon_label + ) = c._dedupe_anon_label_idx(dedupe_hash) + dedupe_hash += 1 repeated = True else: names[required_label_name] = c @@ -6156,11 +6172,13 @@ class Select( if table_qualified: required_label_name = ( fallback_label_name - ) = c._dedupe_anon_tq_label + ) = c._dedupe_anon_tq_label_idx(dedupe_hash) + dedupe_hash += 1 else: required_label_name = ( fallback_label_name - ) = c._dedupe_anon_label + ) = c._dedupe_anon_label_idx(dedupe_hash) + dedupe_hash += 1 repeated = True else: names[effective_name] = c diff --git a/lib/sqlalchemy/sql/util.py b/lib/sqlalchemy/sql/util.py index da5e31655..7fcb45709 100644 --- a/lib/sqlalchemy/sql/util.py +++ b/lib/sqlalchemy/sql/util.py @@ -847,7 +847,7 @@ class ClauseAdapter(visitors.ReplacingExternalTraversal): return newcol @util.preload_module("sqlalchemy.sql.functions") - def replace(self, col): + def replace(self, col, _include_singleton_constants=False): functions = util.preloaded.sql_functions if isinstance(col, FromClause) and not isinstance( @@ -881,6 +881,14 @@ class ClauseAdapter(visitors.ReplacingExternalTraversal): elif not isinstance(col, ColumnElement): return None + elif not _include_singleton_constants and col._is_singleton_constant: + # dont swap out NULL, TRUE, FALSE for a label name + # in a SQL statement that's being rewritten, + # leave them as the constant. This is first noted in #6259, + # however the logic to check this moved here as of #7154 so that + # it is made specific to SQL rewriting and not all column + # correspondence + return None if "adapt_column" in col._annotations: col = col._annotations["adapt_column"] @@ -1001,8 +1009,25 @@ class ColumnAdapter(ClauseAdapter): return newcol def _locate_col(self, col): - - c = ClauseAdapter.traverse(self, col) + # both replace and traverse() are overly complicated for what + # we are doing here and we would do better to have an inlined + # version that doesn't build up as much overhead. the issue is that + # sometimes the lookup does in fact have to adapt the insides of + # say a labeled scalar subquery. However, if the object is an + # Immutable, i.e. Column objects, we can skip the "clone" / + # "copy internals" part since those will be no-ops in any case. + # additionally we want to catch singleton objects null/true/false + # and make sure they are adapted as well here. + + if col._is_immutable: + for vis in self.visitor_iterator: + c = vis.replace(col, _include_singleton_constants=True) + if c is not None: + break + else: + c = col + else: + c = ClauseAdapter.traverse(self, col) if self._wrap: c2 = self._wrap._locate_col(c) -- cgit v1.2.1