From 736f93473d351cf3ab7680e4ed373e19f997b3bb Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Wed, 15 Dec 2021 15:00:28 -0500 Subject: [PATCH] use load_scalar_attributes() for undefer At some point, undeferral of attributes started emitting a full ORM query for the entity, including that subclass columns would use a JOIN. Seems to be at least in 1.3 / 1.4 and possibly earlier. This JOINs to the superclass table unnecessarily. Use load_scalar_attributes() here which should handle the whole thing and emits a more efficient query for joined inheritance. As this behavior seems to have been throughout 1.3 and 1.4 at least, targeting at 2.0 is likely best. Fixes: #7463 Change-Id: Ie4bae767747bba0d03fb13eaff579d4bab0b1bc2 --- doc/build/changelog/unreleased_20/7463.rst | 13 +++ lib/sqlalchemy/orm/loading.py | 18 +---- lib/sqlalchemy/orm/strategies.py | 17 +--- test/orm/inheritance/test_basic.py | 94 +++++++++++++++++++++- test/orm/inheritance/test_poly_loading.py | 8 +- 5 files changed, 115 insertions(+), 35 deletions(-) create mode 100644 doc/build/changelog/unreleased_20/7463.rst diff --git a/doc/build/changelog/unreleased_20/7463.rst b/doc/build/changelog/unreleased_20/7463.rst new file mode 100644 index 0000000000..766dd530a1 --- /dev/null +++ b/doc/build/changelog/unreleased_20/7463.rst @@ -0,0 +1,13 @@ +.. change:: + :tags: bug, orm + :tickets: 7463 + + Fixed performance regression which appeared at least in version 1.3 if not + earlier (sometime after 1.0) where the loading of deferred columns, those + explicitly mapped with :func:`_orm.defer` as opposed to non-deferred + columns that were expired, from a joined inheritance subclass would not use + the "optimized" query which only queried the immediate table that contains + the unloaded columns, instead running a full ORM query which would emit a + JOIN for all base tables, which is not necessary when only loading columns + from the subclass. + diff --git a/lib/sqlalchemy/orm/loading.py b/lib/sqlalchemy/orm/loading.py index d6ee9b7a72..a37d83cfea 100644 --- a/lib/sqlalchemy/orm/loading.py +++ b/lib/sqlalchemy/orm/loading.py @@ -16,7 +16,6 @@ as well as some of the attribute loading strategies. from . import attributes from . import exc as orm_exc from . import path_registry -from . import strategy_options from .base import _DEFER_FOR_STATE from .base import _RAISE_FOR_STATE from .base import _SET_DEFERRED_EXPIRED @@ -1386,25 +1385,14 @@ def load_scalar_attributes(mapper, state, attribute_names, passive): attribute_names = attribute_names.intersection(mapper.attrs.keys()) if mapper.inherits and not mapper.concrete: - # because we are using Core to produce a select() that we - # pass to the Query, we aren't calling setup() for mapped - # attributes; in 1.0 this means deferred attrs won't get loaded - # by default statement = mapper._optimized_get_statement(state, attribute_names) if statement is not None: - # this was previously aliased(mapper, statement), however, - # statement is a select() and Query's coercion now raises for this - # since you can't "select" from a "SELECT" statement. only - # from_statement() allows this. - # note: using from_statement() here means there is an adaption - # with adapt_on_names set up. the other option is to make the - # aliased() against a subquery which affects the SQL. from .query import FromStatement - stmt = FromStatement(mapper, statement).options( - strategy_options.Load(mapper).undefer("*") - ) + # undefer() isn't needed here because statement has the + # columns needed already, this implicitly undefers that column + stmt = FromStatement(mapper, statement) result = load_on_ident( session, diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py index ff4ac9d335..51e81e104d 100644 --- a/lib/sqlalchemy/orm/strategies.py +++ b/lib/sqlalchemy/orm/strategies.py @@ -24,6 +24,7 @@ from . import util as orm_util from .base import _DEFER_FOR_STATE from .base import _RAISE_FOR_STATE from .base import _SET_DEFERRED_EXPIRED +from .base import PASSIVE_OFF from .context import _column_descriptions from .context import ORMCompileState from .context import QueryContext @@ -512,19 +513,9 @@ class DeferredColumnLoader(LoaderStrategy): if self.raiseload: self._invoke_raise_load(state, passive, "raise") - if ( - loading.load_on_ident( - session, - sql.select(localparent).set_label_style( - LABEL_STYLE_TABLENAME_PLUS_COL - ), - state.key, - only_load_props=group, - refresh_state=state, - ) - is None - ): - raise orm_exc.ObjectDeletedError(state) + loading.load_scalar_attributes( + state.mapper, state, set(group), PASSIVE_OFF + ) return attributes.ATTR_WAS_SET diff --git a/test/orm/inheritance/test_basic.py b/test/orm/inheritance/test_basic.py index 339a6b9566..1dc01053b5 100644 --- a/test/orm/inheritance/test_basic.py +++ b/test/orm/inheritance/test_basic.py @@ -1777,7 +1777,19 @@ class PassiveDeletesTest(fixtures.MappedTest): class OptimizedGetOnDeferredTest(fixtures.MappedTest): - """test that the 'optimized get' path accommodates deferred columns.""" + """test that the 'optimized get' path accommodates deferred columns. + + Original issue tested is #3468, where loading of a deferred column + in an inherited subclass would fail. + + At some point, the logic tested was no longer used and a less efficient + query was used to load these columns, but the test here did not inspect + the SQL such that this would be detected. + + Test was then revised to more carefully test and now targets + #7463 as well. + + """ @classmethod def define_tables(cls, metadata): @@ -1787,6 +1799,7 @@ class OptimizedGetOnDeferredTest(fixtures.MappedTest): Column( "id", Integer, primary_key=True, test_needs_autoincrement=True ), + Column("type", String(10)), ) Table( "b", @@ -1808,11 +1821,12 @@ class OptimizedGetOnDeferredTest(fixtures.MappedTest): A, B = cls.classes("A", "B") a, b = cls.tables("a", "b") - cls.mapper_registry.map_imperatively(A, a) + cls.mapper_registry.map_imperatively(A, a, polymorphic_on=a.c.type) cls.mapper_registry.map_imperatively( B, b, inherits=A, + polymorphic_identity="b", properties={ "data": deferred(b.c.data), "expr": column_property(b.c.data + "q", deferred=True), @@ -1825,8 +1839,17 @@ class OptimizedGetOnDeferredTest(fixtures.MappedTest): b1 = B(data="x") sess.add(b1) sess.flush() + b_id = b1.id - eq_(b1.expr, "xq") + with self.sql_execution_asserter(testing.db) as asserter: + eq_(b1.expr, "xq") + asserter.assert_( + CompiledSQL( + "SELECT b.data || :data_1 AS anon_1 " + "FROM b WHERE :param_1 = b.id", + [{"param_1": b_id, "data_1": "q"}], + ) + ) def test_expired_column(self): A, B = self.classes("A", "B") @@ -1834,9 +1857,74 @@ class OptimizedGetOnDeferredTest(fixtures.MappedTest): b1 = B(data="x") sess.add(b1) sess.flush() + b_id = b1.id sess.expire(b1, ["data"]) + with self.sql_execution_asserter(testing.db) as asserter: + eq_(b1.data, "x") + # uses efficient statement w/o JOIN to a + asserter.assert_( + CompiledSQL( + "SELECT b.data AS b_data FROM b WHERE :param_1 = b.id", + [{"param_1": b_id}], + ) + ) + + def test_load_from_unloaded_subclass(self): + A, B = self.classes("A", "B") + sess = fixture_session() + b1 = B(data="x") + sess.add(b1) + sess.commit() + b_id = b1.id + sess.close() + + # load polymorphically in terms of A, so that B needs another + # SELECT + b1 = sess.execute(select(A)).scalar() + + # it's not loaded + assert "data" not in b1.__dict__ + + # but it loads successfully when requested + with self.sql_execution_asserter(testing.db) as asserter: + eq_(b1.data, "x") + + # uses efficient statement w/o JOIN to a + asserter.assert_( + CompiledSQL( + "SELECT b.data AS b_data FROM b WHERE :param_1 = b.id", + [{"param_1": b_id}], + ) + ) + + def test_load_from_expired_subclass(self): + A, B = self.classes("A", "B") + sess = fixture_session() + b1 = B(data="x") + sess.add(b1) + sess.commit() + b_id = b1.id + sess.close() + + b1 = sess.execute(select(A)).scalar() + + # it's not loaded + assert "data" not in b1.__dict__ + eq_(b1.data, "x") + sess.expire(b1, ["data"]) + + with self.sql_execution_asserter(testing.db) as asserter: + eq_(b1.data, "x") + + # uses efficient statement w/o JOIN to a + asserter.assert_( + CompiledSQL( + "SELECT b.data AS b_data FROM b WHERE :param_1 = b.id", + [{"param_1": b_id}], + ) + ) class JoinedNoFKSortingTest(fixtures.MappedTest): diff --git a/test/orm/inheritance/test_poly_loading.py b/test/orm/inheritance/test_poly_loading.py index fcaf470b45..0a03388314 100644 --- a/test/orm/inheritance/test_poly_loading.py +++ b/test/orm/inheritance/test_poly_loading.py @@ -846,10 +846,10 @@ class IgnoreOptionsOnSubclassAttrLoad(fixtures.DeclarativeMappedTest): # call to the deferred load put a deferred loader on the attribute expected.append( CompiledSQL( - "SELECT sub_entity.name AS sub_entity_name FROM entity " - "JOIN sub_entity ON entity.id = sub_entity.id " - "WHERE entity.id = :pk_1", - [{"pk_1": entity_id}], + "SELECT sub_entity.name AS sub_entity_name " + "FROM sub_entity " + "WHERE :param_1 = sub_entity.id", + [{"param_1": entity_id}], ) ) -- 2.47.2