From f5353ec3c51b292d9f14c6b78aa7226627aea6e6 Mon Sep 17 00:00:00 2001 From: Grant Fitzsimmons <37256050+grantfitzsimmons@users.noreply.github.com> Date: Mon, 21 Sep 2026 10:34:46 +0200 Subject: [PATCH 01/12] fix(performance): reuse tree rank metadata within queries --- .../backend/stored_queries/query_construct.py | 53 ++++++++++++------- .../tests/test_tree_query_performance.py | 47 ++++++++++++++++ 2 files changed, 80 insertions(+), 20 deletions(-) create mode 100644 specifyweb/backend/stored_queries/tests/test_tree_query_performance.py diff --git a/specifyweb/backend/stored_queries/query_construct.py b/specifyweb/backend/stored_queries/query_construct.py index 614ee449d43..f6f389839d4 100644 --- a/specifyweb/backend/stored_queries/query_construct.py +++ b/specifyweb/backend/stored_queries/query_construct.py @@ -11,12 +11,6 @@ logger = logging.getLogger(__name__) -def _safe_filter(query): - count = query.count() - if count <= 1: - return query.first() - raise Exception(f"Got more than one matching: {list(query)}") - class QueryConstruct(namedtuple('QueryConstruct', 'collection objectformatter query join_cache tree_rank_count internal_filters')): def __new__(cls, *args, **kwargs): @@ -27,6 +21,37 @@ def __new__(cls, *args, **kwargs): kwargs['internal_filters'] = [] return super().__new__(cls, *args, **kwargs) + def tree_rank_metadata(self, table, tree_rank): + """Resolve ranks once per query, shared by all paths into the same tree.""" + query = self + defs_key = ('TreeDefinitions', table.name) + if defs_key not in query.join_cache: + query = query._replace(join_cache=query.join_cache.copy()) + query.join_cache[defs_key] = get_treedefs(query.collection, table.name) + treedefs = query.join_cache[defs_key] + + # TreeRankQuery equality does not include the explicit tree definition. + rank_key = ('TreeRankItems', table.name, tree_rank.name, tree_rank.treedef_id) + if rank_key not in query.join_cache: + item_model = getattr(spmodels, table.django_name + 'treedefitem') + def_ids = [ + def_id for def_id, _ in treedefs + if tree_rank.treedef_id is None or tree_rank.treedef_id == def_id + ] + items = item_model.objects.filter( + treedef_id__in=def_ids, name=tree_rank.name + ).values_list('treedef_id', 'id') + by_definition = {} + for def_id, item_id in items: + if def_id in by_definition: + raise Exception('Got more than one matching tree rank') + by_definition[def_id] = item_id + ranks = [(def_id, by_definition[def_id]) for def_id in def_ids if def_id in by_definition] + assert ranks, "Didn't find the tree rank across any tree" + query = query._replace(join_cache=query.join_cache.copy()) + query.join_cache[rank_key] = ranks + return query, treedefs, query.join_cache[rank_key] + def handle_tree_field(self, node, table, tree_rank: TreeRankQuery, next_join_path, current_field_spec: QueryFieldSpec): query = self if query.collection is None: # Not sure it makes sense to query across collections @@ -39,13 +64,13 @@ def handle_tree_field(self, node, table, tree_rank: TreeRankQuery, next_join_pat treedefitem_column = table.name + 'TreeDefItemID' treedef_column = table.name + 'TreeDefID' + query, treedefs, treedefs_with_ranks = query.tree_rank_metadata(table, tree_rank) + cache_key = (node, 'TreeRanks') if cache_key in query.join_cache: logger.debug("using join cache for %r tree ranks.", node) ancestors, treedefs = query.join_cache[cache_key] else: - treedefs = get_treedefs(query.collection, table.name) - # We need to take the max here. Otherwise, it is possible that the same rank # name may not occur at the same level across tree defs. max_depth = max(depth for _, depth in treedefs) @@ -60,18 +85,6 @@ def handle_tree_field(self, node, table, tree_rank: TreeRankQuery, next_join_pat query = query._replace(join_cache=query.join_cache.copy()) query.join_cache[cache_key] = (ancestors, treedefs) - item_model = getattr(spmodels, table.django_name + "treedefitem") - - # TODO: optimize out the ranks that appear? cache them - treedefs_with_ranks: list[tuple[int, int]] = [tup for tup in [ - (treedef_id, _safe_filter(item_model.objects.filter(treedef_id=treedef_id, name=tree_rank.name).values_list('id', flat=True))) - for treedef_id, _ in treedefs - # For constructing tree queries for batch edit - if (tree_rank.treedef_id is None or tree_rank.treedef_id == treedef_id) - ] if tup[1] is not None] - - assert len(treedefs_with_ranks) >= 1, "Didn't find the tree rank across any tree" - treedefitem_params = [treedefitem_id for (_, treedefitem_id) in treedefs_with_ranks] def make_tree_field_spec(tree_node): diff --git a/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py b/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py new file mode 100644 index 00000000000..ebcd6da95b4 --- /dev/null +++ b/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py @@ -0,0 +1,47 @@ +from unittest.mock import patch + +from sqlalchemy import orm + +from specifyweb.backend.stored_queries import models +from specifyweb.backend.stored_queries.query_construct import QueryConstruct +from specifyweb.backend.stored_queries.queryfieldspec import TreeRankQuery +from specifyweb.backend.trees.tests.test_trees import SqlTreeSetup +from specifyweb.specify.models import datamodel + + +class TreeRankMetadataTests(SqlTreeSetup): + def construct(self): + return QueryConstruct( + collection=self.collection, + objectformatter=None, + query=orm.Query(models.Taxon._id), + ) + + def test_rank_metadata_is_reused_within_query(self): + table = datamodel.get_table_strict('Taxon') + rank = TreeRankQuery.create('Kingdom', 'Taxon') + with patch( + 'specifyweb.backend.stored_queries.query_construct.get_treedefs', + return_value=[(self.taxontreedef.id, 11)], + ) as definitions: + with self.assertNumQueries(1): + query, _, ranks = self.construct().tree_rank_metadata(table, rank) + self.assertEqual(ranks, [(self.taxontreedef.id, self.taxon_kingdom.id)]) + with self.assertNumQueries(0): + query, _, repeated = query.tree_rank_metadata(table, rank) + self.assertEqual(repeated, ranks) + with self.assertNumQueries(1): + query.tree_rank_metadata(table, TreeRankQuery.create('Genus', 'Taxon')) + definitions.assert_called_once() + with self.assertNumQueries(1): + self.construct().tree_rank_metadata(table, rank) + self.assertEqual(definitions.call_count, 2) + + def test_explicit_tree_definition_has_separate_cache_entry(self): + table = datamodel.get_table_strict('Taxon') + rank = TreeRankQuery.create('Kingdom', 'Taxon') + query, _, _ = self.construct().tree_rank_metadata(table, rank) + scoped_rank = TreeRankQuery.create('Kingdom', 'Taxon') + scoped_rank.treedef_id = -1 + with self.assertRaisesMessage(AssertionError, "Didn't find the tree rank"): + query.tree_rank_metadata(table, scoped_rank) From 38080a41cea56b41e6060cf9772b33d01ca8c957 Mon Sep 17 00:00:00 2001 From: Grant Fitzsimmons <37256050+grantfitzsimmons@users.noreply.github.com> Date: Mon, 21 Sep 2026 12:02:35 +0200 Subject: [PATCH 02/12] fix(performance): avoid repeated ancestor joins in tree queries Use live node-number ranges for rank ID equality and reusable recursive rank lookups for other positive scalar filters. Preserve parent lookups for negative/empty filters, incomplete numbering, recordsets, and explicit record ID selections. Cover scoping, preferred taxa, OR combinations, tree moves, and result equivalence. --- .../backend/stored_queries/execution.py | 14 +- .../backend/stored_queries/query_construct.py | 87 +++++++- .../backend/stored_queries/queryfield.py | 27 ++- .../backend/stored_queries/queryfieldspec.py | 10 +- .../tests/test_tree_query_performance.py | 210 ++++++++++++++++++ 5 files changed, 342 insertions(+), 6 deletions(-) diff --git a/specifyweb/backend/stored_queries/execution.py b/specifyweb/backend/stored_queries/execution.py index cde46621e8e..12e1c65d41f 100644 --- a/specifyweb/backend/stored_queries/execution.py +++ b/specifyweb/backend/stored_queries/execution.py @@ -1071,6 +1071,17 @@ def build_query( order_by_exprs = [] selected_fields = [] predicates_by_field = defaultdict(list) + # Materializing a subtree can cost more than parent lookups when the caller + # has already restricted the query to explicit record IDs. + optimize_tree = props.recordsetid is None and not any( + len(fs.fieldspec.join_path) == 1 + and base_table is not None + and fs.fieldspec.get_field() == base_table.idField + and fs.op_num in (1, 10) + and not fs.negate + and fs.value not in ('', None) + for fs in field_specs + ) # augment_field_specs(field_specs, formatauditobjs) for fs in field_specs: # sort_type = SORT_TYPES[fs.sort_type] @@ -1082,7 +1093,8 @@ def build_query( continue query, field, predicate = fs.add_to_query( - query, formatauditobjs=props.formatauditobjs, collection=collection, user=user + query, formatauditobjs=props.formatauditobjs, collection=collection, user=user, + optimize_tree=optimize_tree, ) if field is None: diff --git a/specifyweb/backend/stored_queries/query_construct.py b/specifyweb/backend/stored_queries/query_construct.py index f6f389839d4..cd1171a34d8 100644 --- a/specifyweb/backend/stored_queries/query_construct.py +++ b/specifyweb/backend/stored_queries/query_construct.py @@ -2,6 +2,7 @@ from collections import namedtuple, deque from sqlalchemy import orm, sql, or_ +from django.db.models import F, Q import specifyweb.specify.models as spmodels from specifyweb.backend.trees.utils import get_treedefs @@ -52,7 +53,28 @@ def tree_rank_metadata(self, table, tree_rank): query.join_cache[rank_key] = ranks return query, treedefs, query.join_cache[rank_key] - def handle_tree_field(self, node, table, tree_rank: TreeRankQuery, next_join_path, current_field_spec: QueryFieldSpec): + def tree_numbering_available(self, table, treedefs): + """Fall back to parent links for trees with missing or reversed intervals. + + Tree writes maintain the nesting invariant. Do not renumber a tree while + reading it, or cache this check across requests (uploads can change it). + """ + cache_key = ('TreeNumbering', table.name) + query = self + if cache_key not in query.join_cache: + model = getattr(spmodels, table.django_name) + invalid = model.objects.filter( + definition_id__in=[def_id for def_id, _ in treedefs] + ).filter( + Q(nodenumber__isnull=True) + | Q(highestchildnodenumber__isnull=True) + | Q(highestchildnodenumber__lt=F('nodenumber')) + ).exists() + query = query._replace(join_cache=query.join_cache.copy()) + query.join_cache[cache_key] = not invalid + return query, query.join_cache[cache_key] + + def handle_tree_field(self, node, table, tree_rank: TreeRankQuery, next_join_path, current_field_spec: QueryFieldSpec, use_range=False, use_rank_lookup=False): query = self if query.collection is None: # Not sure it makes sense to query across collections raise AssertionError( @@ -66,6 +88,69 @@ def handle_tree_field(self, node, table, tree_rank: TreeRankQuery, next_join_pat query, treedefs, treedefs_with_ranks = query.tree_rank_metadata(table, tree_rank) + if use_range: + query, use_range = query.tree_numbering_available(table, treedefs) + if use_range: + # One matching ancestor at this rank, including the node itself. + # Keep the filter on the ancestor column so the optimizer can start + # with selective ID/name indexes and range-scan descendants. + range_cache_key = (node, 'TreeRankLookup' if use_rank_lookup else 'TreeRankRange', + tree_rank.name, tree_rank.treedef_id) + if range_cache_key in query.join_cache: + ancestor = query.join_cache[range_cache_key] + else: + ancestor = orm.aliased(getattr(models, table.name)) + if use_rank_lookup: + # Map descendants to their ancestor once for this rank. The + # unfiltered mapping preserves displayed values when filters + # are combined with OR, while the PK join avoids a range + # comparison against every node at the requested rank. + model = getattr(models, table.name) + rank_node = orm.aliased(model) + child = orm.aliased(model) + lookup = sql.select( + rank_node._id.label('node_id'), + rank_node._id.label('ancestor_id'), + getattr(rank_node, treedef_column).label('definition_id'), + ).where( + getattr(rank_node, treedefitem_column).in_( + [item_id for _, item_id in treedefs_with_ranks] + ), + ).cte(recursive=True) + lookup = lookup.union(sql.select( + child._id, lookup.c.ancestor_id, getattr(child, treedef_column), + ).join(lookup, sql.and_( + child.ParentID == lookup.c.node_id, + getattr(child, treedef_column) == lookup.c.definition_id, + ))) + query = query._replace(query=query.query.outerjoin( + lookup, node._id == lookup.c.node_id, + ).outerjoin(ancestor, ancestor._id == lookup.c.ancestor_id)) + else: + query = query._replace(query=query.query.outerjoin(ancestor, sql.and_( + getattr(node, treedef_column) == getattr(ancestor, treedef_column), + getattr(ancestor, treedefitem_column).in_( + [item_id for _, item_id in treedefs_with_ranks] + ), + node.nodeNumber.between(ancestor.nodeNumber, ancestor.highestChildNodeNumber), + ))) + query = query._replace(join_cache=query.join_cache.copy()) + query.join_cache[range_cache_key] = ancestor + field_spec = current_field_spec._replace( + root_table=table, + root_sql_table=ancestor, + join_path=next_join_path, + ) + query, column, field, result_table = field_spec.add_spec_to_query(query) + query = query._replace(internal_filters=[ + *query.internal_filters, + or_( + getattr(node, treedef_column).in_([def_id for def_id, _ in treedefs_with_ranks]), + getattr(node, treedef_column).is_(None), + ), + ]) + return query, column, field, result_table + cache_key = (node, 'TreeRanks') if cache_key in query.join_cache: logger.debug("using join cache for %r tree ranks.", node) diff --git a/specifyweb/backend/stored_queries/queryfield.py b/specifyweb/backend/stored_queries/queryfield.py index 09886bb93fe..bcf0ba2be14 100644 --- a/specifyweb/backend/stored_queries/queryfield.py +++ b/specifyweb/backend/stored_queries/queryfield.py @@ -4,7 +4,7 @@ from typing import Any, NamedTuple, Literal from .query_ops import QueryOps, QUERYFIELD_OPERATION_NUMBER -from .queryfieldspec import QueryFieldSpec +from .queryfieldspec import QueryFieldSpec, TreeRankQuery logger = logging.getLogger(__name__) @@ -78,7 +78,7 @@ def from_spqueryfield(cls, field: EphemeralField, value: str | None=None): strict=field.isStrict, ) - def add_to_query(self, query, no_filter=False, formatauditobjs=False, collection=None, user=None): + def add_to_query(self, query, no_filter=False, formatauditobjs=False, collection=None, user=None, optimize_tree=True): logger.info("adding field %s", self) value_required_for_filter = QueryOps.OPERATIONS[self.op_num] not in ( "op_true", # 6 @@ -92,6 +92,24 @@ def add_to_query(self, query, no_filter=False, formatauditobjs=False, collection self.value == "" and value_required_for_filter and not self.negate ) + # Positive scalar filters reject missing ancestors, allowing MariaDB to + # start at the matching rank instead of walking up from every specimen. + # Empty/negated filters need the existing missing-ancestor semantics; + # relationship paths and date transformations retain their usual joins. + path = self.fieldspec.join_path + use_tree_range = ( + optimize_tree + and not no_filter + and not self.negate + and (not value_required_for_filter or isinstance(self.value, str)) + and self.op_num in {0, 1, 2, 3, 4, 5, 6, 7, 9, 10, 11, 15, 18} + and len(path) >= 2 + and isinstance(path[-2], TreeRankQuery) + and not path[-1].is_relationship + and sum(isinstance(part, TreeRankQuery) for part in path) == 1 + and self.fieldspec.date_part is None + ) + return self.fieldspec.add_to_query( query, value=self.value, @@ -102,4 +120,9 @@ def add_to_query(self, query, no_filter=False, formatauditobjs=False, collection strict=self.strict, collection=collection, user=user, + use_tree_range=use_tree_range, + use_rank_lookup=use_tree_range and not ( + self.op_num == 1 + and self.fieldspec.get_field().name.lower() == self.fieldspec.table.idFieldName.lower() + ), ) diff --git a/specifyweb/backend/stored_queries/queryfieldspec.py b/specifyweb/backend/stored_queries/queryfieldspec.py index 3ce298fdf38..3284d3ba5b7 100644 --- a/specifyweb/backend/stored_queries/queryfieldspec.py +++ b/specifyweb/backend/stored_queries/queryfieldspec.py @@ -526,6 +526,8 @@ def add_to_query( strict=False, collection=None, user=None, + use_tree_range=False, + use_rank_lookup=False, ): # print "############################################################################" # print "formatauditobjs " + str(formatauditobjs) @@ -533,7 +535,9 @@ def add_to_query( # print "field name " + self.get_field().name # print "is auditlog obj format field = " + str(self.is_auditlog_obj_format_field(formatauditobjs)) # print "############################################################################" - query, orm_field, field, table = self.add_spec_to_query(query, formatter) + query, orm_field, field, table = self.add_spec_to_query( + query, formatter, use_tree_range=use_tree_range, use_rank_lookup=use_rank_lookup + ) return self.apply_filter( query, orm_field, @@ -549,7 +553,7 @@ def add_to_query( ) def add_spec_to_query( - self, query, formatter=None, aggregator=None, cycle_detector=[] + self, query, formatter=None, aggregator=None, cycle_detector=[], use_tree_range=False, use_rank_lookup=False ): if self.get_field() is None: @@ -587,6 +591,8 @@ def add_spec_to_query( field, self.join_path[tree_rank_idx + 1 :], self, + use_range=use_tree_range, + use_rank_lookup=use_rank_lookup, ) else: try: diff --git a/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py b/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py index ebcd6da95b4..c528d93aad0 100644 --- a/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py +++ b/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py @@ -45,3 +45,213 @@ def test_explicit_tree_definition_has_separate_cache_entry(self): scoped_rank.treedef_id = -1 with self.assertRaisesMessage(AssertionError, "Didn't find the tree rank"): query.tree_rank_metadata(table, scoped_rank) + + +class TreeRangeFilterTests(SqlTreeSetup): + def setUp(self): + super().setUp() + self.root = self.make_taxontree('Life', 'Taxonomy Root') + self.kingdom = self.make_taxontree('Animalia', 'Kingdom', parent=self.root) + self.genus = self.make_taxontree('Alpha', 'Genus', parent=self.kingdom) + self.first = self.make_taxontree('one', 'Species', parent=self.genus) + self.second = self.make_taxontree('two', 'Species', parent=self.genus) + self.other = self.make_taxontree('Beta', 'Genus', parent=self.kingdom) + self.outside = self.make_taxontree('three', 'Species', parent=self.other) + self.skipped = self.make_taxontree('skipped', 'Species', parent=self.root) + + def field(self, stringid, op=8, value='', negate=False, display=True): + from specifyweb.backend.stored_queries.queryfield import QueryField + from specifyweb.backend.stored_queries.queryfieldspec import QueryFieldSpec + return QueryField( + QueryFieldSpec.from_stringid(stringid, False), + op, value, negate, display, None, 0, + ) + + def run_query(self, fields, legacy=False, **props): + from django.db import connection + from sqlalchemy.dialects import mysql + from specifyweb.backend.stored_queries.execution import build_query, BuildQueryProps + handle = QueryConstruct.handle_tree_field + + def parent_walk(query, *args, **kwargs): + kwargs['use_range'] = False + return handle(query, *args, **kwargs) + + with self.__class__.test_session_context() as session, patch.object( + QueryConstruct, 'handle_tree_field', parent_walk if legacy else handle + ): + query, _ = build_query( + session, self.collection, self.specifyuser, + fields[0].fieldspec.root_table.tableId, fields, BuildQueryProps(**props), + ) + compiled = query.statement.compile(dialect=mysql.dialect(), compile_kwargs={'literal_binds': True}) + with connection.cursor() as cursor: + cursor.execute(str(compiled)) + return cursor.fetchall(), str(compiled) + + def assert_equivalent(self, fields, **props): + actual, statement = self.run_query(fields, **props) + expected, _ = self.run_query(fields, legacy=True, **props) + self.assertCountEqual(actual, expected) + return actual, statement + + def test_id_filter_includes_self_and_descendants_only(self): + rows, statement = self.assert_equivalent([ + self.field('4.taxon.name'), + self.field('4.taxon.Genus ID', 1, str(self.genus.id), display=False), + ]) + self.assertEqual({row[0] for row in rows}, {self.genus.id, self.first.id, self.second.id}) + self.assertIn('BETWEEN', statement) + self.assertNotIn('ParentID', statement) + + def test_explicit_record_ids_keep_parent_walk(self): + rows, statement = self.assert_equivalent([ + self.field('4.taxon.name'), + self.field('4.taxon.taxonId', 10, str(self.first.id), display=False), + self.field('4.taxon.Genus', 11, 'Al', display=False), + ]) + self.assertEqual({row[0] for row in rows}, {self.first.id}) + self.assertNotIn('WITH RECURSIVE', statement) + self.assertIn('ParentID', statement) + + def test_recordset_keeps_parent_walk(self): + from specifyweb.specify.models import Recordset, Recordsetitem + recordset = Recordset.objects.create( + collectionmemberid=self.collection.id, dbtableid=4, + name='Small taxonomy selection', specifyuser=self.specifyuser, type=0, + ) + Recordsetitem.objects.create(recordset=recordset, recordid=self.first.id) + rows, statement = self.assert_equivalent([ + self.field('4.taxon.name'), self.field('4.taxon.Genus', 11, 'Al'), + ], recordsetid=recordset.id) + self.assertEqual({row[0] for row in rows}, {self.first.id}) + self.assertNotIn('WITH RECURSIVE', statement) + self.assertIn('ParentID', statement) + + def test_positive_scalar_operators(self): + cases = [(0, 'A%'), (1, 'Alpha'), (2, 'A'), (3, 'Z'), (4, 'Alpha'), + (5, 'Beta'), (9, 'Alpha,Beta'), (10, 'Alpha,Beta'), + (11, 'lph'), (15, 'Al'), (18, 'pha')] + for op, value in cases: + with self.subTest(op=op): + rows, statement = self.assert_equivalent([ + self.field('4.taxon.name'), self.field('4.taxon.Genus', op, value), + ]) + self.assertTrue(rows) + self.assertIn('WITH RECURSIVE', statement) + self.assertEqual(statement.count('LEFT OUTER JOIN taxon '), 1) + + def test_missing_rank_and_negation_keep_existing_null_semantics(self): + for op, value, negate in [(12, '', False), (1, None, False), + (1, 'Alpha', True), (10, 'Alpha,Beta', True)]: + with self.subTest(op=op): + rows, statement = self.assert_equivalent([ + self.field('4.taxon.name'), self.field('4.taxon.Genus', op, value, negate), + ]) + self.assertIn(self.skipped.id, {row[0] for row in rows}) + self.assertIn('ParentID', statement) + + def test_repeated_filters_and_multiple_ranks(self): + for implicit_or in [True, False]: + self.assert_equivalent([ + self.field('4.taxon.name'), + self.field('4.taxon.Genus', 1, 'Alpha', display=False), + self.field('4.taxon.Genus', 1, 'Beta', display=False), + self.field('4.taxon.Kingdom', 1, 'Animalia'), + ], implicit_or=implicit_or) + + def test_geography_filter(self): + rows, statement = self.assert_equivalent([ + self.field('3.geography.name'), + self.field('3.geography.Country', 1, 'USA'), + ]) + self.assertTrue(rows) + self.assertIn('WITH RECURSIVE', statement) + self.assertEqual(statement.count('LEFT OUTER JOIN geography '), 1) + + def test_missing_or_reversed_numbering_falls_back_without_writes(self): + from specifyweb.specify.models import Taxon + for values in [dict(nodenumber=None), dict(highestchildnodenumber=None), + dict(nodenumber=50, highestchildnodenumber=1)]: + with self.subTest(values=values): + Taxon.objects.filter(pk=self.first.id).update(**values) + rows, statement = self.assert_equivalent([ + self.field('4.taxon.name'), + self.field('4.taxon.Genus ID', 1, str(self.genus.id), display=False), + ]) + self.assertIn(self.first.id, {row[0] for row in rows}) + self.assertIn('ParentID', statement) + self.first.refresh_from_db() + for key, value in values.items(): + self.assertEqual(getattr(self.first, key), value) + + def test_rank_id_from_wrong_rank_does_not_match(self): + rows, _ = self.assert_equivalent([ + self.field('4.taxon.name'), + self.field('4.taxon.Genus ID', 1, str(self.kingdom.id), display=False), + ]) + self.assertEqual(rows, ()) + + def test_numbering_is_resolved_again_after_tree_move(self): + fields = [self.field('4.taxon.name'), + self.field('4.taxon.Genus ID', 1, str(self.genus.id), display=False)] + before, _ = self.assert_equivalent(fields) + self.assertIn(self.first.id, {row[0] for row in before}) + self.first.parent = self.other + self.first.save() + after, _ = self.assert_equivalent(fields) + self.assertNotIn(self.first.id, {row[0] for row in after}) + + def test_preferred_taxon_path_and_distinct(self): + from specifyweb.specify.models import Determination + co = self.collectionobjects[0] + determination = Determination.objects.create( + collectionobject=co, taxon=self.outside, iscurrent=True, + ) + Determination.objects.filter(pk=determination.id).update(preferredtaxon=self.first) + fields = [self.field('1.collectionobject.catalogNumber'), + self.field('1,9-determinations,4-preferredTaxon.taxon.Genus ID', + 1, str(self.genus.id), display=False)] + rows, statement = self.assert_equivalent(fields) + self.assertEqual({row[0] for row in rows}, {co.id}) + self.assertIn('PreferredTaxonID', statement) + self.assert_equivalent(fields, distinct=True) + + def test_multiple_tree_definitions_and_explicit_scope(self): + from specifyweb.specify.models import Taxontreedef + second_def = Taxontreedef.objects.create(name='Second taxonomy', discipline=self.discipline) + self.make_taxon_ranks(second_def) + root = self.make_taxontree('Other life', 'Taxonomy Root', treedef=second_def) + genus = self.make_taxontree('Alpha', 'Genus', parent=root, treedef=second_def) + leaf = self.make_taxontree('other species', 'Species', parent=genus, treedef=second_def) + fields = [self.field('4.taxon.name'), self.field('4.taxon.Genus', 1, 'Alpha')] + rows, _ = self.assert_equivalent(fields) + self.assertEqual({row[0] for row in rows}, + {self.genus.id, self.first.id, self.second.id, genus.id, leaf.id}) + + scoped_rank = TreeRankQuery.create('Genus', 'Taxon', treedef_id=self.taxontreedef.id) + scoped_spec = fields[1].fieldspec._replace( + join_path=(scoped_rank, fields[1].fieldspec.join_path[-1]), + ) + rows, _ = self.assert_equivalent([fields[0], fields[1]._replace(fieldspec=scoped_spec)]) + self.assertEqual({row[0] for row in rows}, {self.genus.id, self.first.id, self.second.id}) + + second_def.discipline = None + second_def.save() + rows, _ = self.assert_equivalent(fields) + self.assertEqual({row[0] for row in rows}, {self.genus.id, self.first.id, self.second.id}) + + def test_boolean_and_synonymy_filters(self): + from specifyweb.specify.models import Taxon + Taxon.objects.filter(pk=self.genus.id).update(isaccepted=True) + Taxon.objects.filter(pk=self.other.id).update(isaccepted=False) + for op in (6, 7): + with self.subTest(op=op): + rows, statement = self.assert_equivalent([ + self.field('4.taxon.name'), self.field('4.taxon.Genus isAccepted', op), + ]) + self.assertTrue(rows) + self.assertIn('WITH RECURSIVE', statement) + self.assert_equivalent([ + self.field('4.taxon.name'), self.field('4.taxon.Genus', 1, 'Alpha'), + ], search_synonymy=True) From b0e4bcbafb7d6269f3123612fde9a71aa3599d19 Mon Sep 17 00:00:00 2001 From: Grant Fitzsimmons <37256050+grantfitzsimmons@users.noreply.github.com> Date: Mon, 21 Sep 2026 12:16:11 +0200 Subject: [PATCH 03/12] fix(performance): preserve early limits for shallow tree queries --- .../backend/stored_queries/execution.py | 3 ++ .../backend/stored_queries/queryfield.py | 20 ++++++--- .../tests/test_tree_query_performance.py | 45 ++++++++++++++++++- 3 files changed, 61 insertions(+), 7 deletions(-) diff --git a/specifyweb/backend/stored_queries/execution.py b/specifyweb/backend/stored_queries/execution.py index 12e1c65d41f..8b4b8e41662 100644 --- a/specifyweb/backend/stored_queries/execution.py +++ b/specifyweb/backend/stored_queries/execution.py @@ -73,6 +73,7 @@ class BuildQueryProps(NamedTuple): search_synonymy: bool = False implicit_or: bool = True formatter_props: ObjectFormatterProps = DefaultQueryFormatterProps() + optimize_for_page: bool = False def set_group_concat_max_len(connection): @@ -884,6 +885,7 @@ def execute( series=series, search_synonymy=search_synonymy, formatter_props=formatter_props, + optimize_for_page=bool(limit) and not (count_only or distinct or series), ), ) @@ -1095,6 +1097,7 @@ def build_query( query, field, predicate = fs.add_to_query( query, formatauditobjs=props.formatauditobjs, collection=collection, user=user, optimize_tree=optimize_tree, + prefer_parent_lookup=props.optimize_for_page, ) if field is None: diff --git a/specifyweb/backend/stored_queries/queryfield.py b/specifyweb/backend/stored_queries/queryfield.py index bcf0ba2be14..134a12b65d3 100644 --- a/specifyweb/backend/stored_queries/queryfield.py +++ b/specifyweb/backend/stored_queries/queryfield.py @@ -8,6 +8,9 @@ logger = logging.getLogger(__name__) +# Prefer parent lookups for shallow trees; deeper trees are cheaper to +# materialize as a rank range. +MAX_PAGE_PARENT_LOOKUP_RANKS = 8 QUREYFIELD_SORT_T = Literal[ 0, # NONE @@ -78,7 +81,7 @@ def from_spqueryfield(cls, field: EphemeralField, value: str | None=None): strict=field.isStrict, ) - def add_to_query(self, query, no_filter=False, formatauditobjs=False, collection=None, user=None, optimize_tree=True): + def add_to_query(self, query, no_filter=False, formatauditobjs=False, collection=None, user=None, optimize_tree=True, prefer_parent_lookup=False): logger.info("adding field %s", self) value_required_for_filter = QueryOps.OPERATIONS[self.op_num] not in ( "op_true", # 6 @@ -109,6 +112,16 @@ def add_to_query(self, query, no_filter=False, formatauditobjs=False, collection and sum(isinstance(part, TreeRankQuery) for part in path) == 1 and self.fieldspec.date_part is None ) + use_rank_lookup = use_tree_range and not ( + self.op_num == 1 + and self.fieldspec.get_field().name.lower() == self.fieldspec.table.idFieldName.lower() + ) + if use_rank_lookup and prefer_parent_lookup and query.collection is not None: + query, treedefs, _ = query.tree_rank_metadata(self.fieldspec.table, path[-2]) + # A bounded page can stop after a handful of parent lookups in a + # shallow tree. Materializing the entire rank delays that first page. + if max(depth for _, depth in treedefs) <= MAX_PAGE_PARENT_LOOKUP_RANKS: + use_tree_range = use_rank_lookup = False return self.fieldspec.add_to_query( query, @@ -121,8 +134,5 @@ def add_to_query(self, query, no_filter=False, formatauditobjs=False, collection collection=collection, user=user, use_tree_range=use_tree_range, - use_rank_lookup=use_tree_range and not ( - self.op_num == 1 - and self.fieldspec.get_field().name.lower() == self.fieldspec.table.idFieldName.lower() - ), + use_rank_lookup=use_rank_lookup, ) diff --git a/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py b/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py index c528d93aad0..0150f21c0ab 100644 --- a/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py +++ b/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py @@ -155,8 +155,8 @@ def test_repeated_filters_and_multiple_ranks(self): for implicit_or in [True, False]: self.assert_equivalent([ self.field('4.taxon.name'), - self.field('4.taxon.Genus', 1, 'Alpha', display=False), - self.field('4.taxon.Genus', 1, 'Beta', display=False), + self.field('4.taxon.Genus', 1, 'Alpha'), + self.field('4.taxon.Genus', 1, 'Beta'), self.field('4.taxon.Kingdom', 1, 'Animalia'), ], implicit_or=implicit_or) @@ -169,6 +169,47 @@ def test_geography_filter(self): self.assertIn('WITH RECURSIVE', statement) self.assertEqual(statement.count('LEFT OUTER JOIN geography '), 1) + def test_shallow_pages_keep_parent_lookups_but_deep_pages_use_rank_lookup(self): + _, shallow_sql = self.assert_equivalent([ + self.field('3.geography.name'), self.field('3.geography.Country', 1, 'USA'), + ], optimize_for_page=True) + self.assertNotIn('WITH RECURSIVE', shallow_sql) + self.assertIn('ParentID', shallow_sql) + _, deep_sql = self.assert_equivalent([ + self.field('4.taxon.name'), self.field('4.taxon.Genus', 1, 'Alpha'), + ], optimize_for_page=True) + self.assertIn('WITH RECURSIVE', deep_sql) + for depth, uses_lookup in [(8, False), (9, True)]: + with self.subTest(depth=depth), patch( + 'specifyweb.backend.stored_queries.query_construct.get_treedefs', + return_value=[(self.taxontreedef.id, depth)], + ): + _, statement = self.assert_equivalent([ + self.field('4.taxon.name'), self.field('4.taxon.Genus', 1, 'Alpha'), + ], optimize_for_page=True) + self.assertEqual('WITH RECURSIVE' in statement, uses_lookup) + + def test_execute_only_prefers_parent_lookup_for_ungrouped_pages(self): + from unittest.mock import Mock + from specifyweb.backend.stored_queries.execution import execute + for count_only, limit, distinct, series, expected in [ + (False, 40, False, False, True), + (True, 40, False, False, False), + (False, 0, False, False, False), + (False, None, False, False, False), + (False, 40, True, False, False), + (False, 40, False, True, False), + ]: + with self.subTest(count_only=count_only, limit=limit, distinct=distinct, series=series): + with patch('specifyweb.backend.stored_queries.execution.set_group_concat_max_len'), patch( + 'specifyweb.backend.stored_queries.execution.build_query', + side_effect=RuntimeError('query built'), + ) as build: + with self.assertRaisesMessage(RuntimeError, 'query built'): + execute(Mock(info={'connection': Mock()}), self.collection, self.specifyuser, + 3, distinct, series, False, count_only, [], limit, 0) + self.assertEqual(build.call_args.args[5].optimize_for_page, expected) + def test_missing_or_reversed_numbering_falls_back_without_writes(self): from specifyweb.specify.models import Taxon for values in [dict(nodenumber=None), dict(highestchildnodenumber=None), From b23cc7f12dea302306623dff21806c573ccaa0bb Mon Sep 17 00:00:00 2001 From: Grant Fitzsimmons <37256050+grantfitzsimmons@users.noreply.github.com> Date: Mon, 21 Sep 2026 14:34:46 +0200 Subject: [PATCH 04/12] fix(performance): remove fixed tree lookup cutoff --- specifyweb/backend/stored_queries/execution.py | 3 +++ specifyweb/backend/stored_queries/queryfield.py | 15 ++++++--------- .../tests/test_tree_query_performance.py | 9 +++++---- 3 files changed, 14 insertions(+), 13 deletions(-) diff --git a/specifyweb/backend/stored_queries/execution.py b/specifyweb/backend/stored_queries/execution.py index 8b4b8e41662..6ee618e49d8 100644 --- a/specifyweb/backend/stored_queries/execution.py +++ b/specifyweb/backend/stored_queries/execution.py @@ -74,6 +74,7 @@ class BuildQueryProps(NamedTuple): implicit_or: bool = True formatter_props: ObjectFormatterProps = DefaultQueryFormatterProps() optimize_for_page: bool = False + page_size: int | None = None def set_group_concat_max_len(connection): @@ -886,6 +887,7 @@ def execute( search_synonymy=search_synonymy, formatter_props=formatter_props, optimize_for_page=bool(limit) and not (count_only or distinct or series), + page_size=limit if limit and not (count_only or distinct or series) else None, ), ) @@ -1098,6 +1100,7 @@ def build_query( query, formatauditobjs=props.formatauditobjs, collection=collection, user=user, optimize_tree=optimize_tree, prefer_parent_lookup=props.optimize_for_page, + page_size=props.page_size, ) if field is None: diff --git a/specifyweb/backend/stored_queries/queryfield.py b/specifyweb/backend/stored_queries/queryfield.py index 134a12b65d3..7357ef94f94 100644 --- a/specifyweb/backend/stored_queries/queryfield.py +++ b/specifyweb/backend/stored_queries/queryfield.py @@ -8,10 +8,6 @@ logger = logging.getLogger(__name__) -# Prefer parent lookups for shallow trees; deeper trees are cheaper to -# materialize as a rank range. -MAX_PAGE_PARENT_LOOKUP_RANKS = 8 - QUREYFIELD_SORT_T = Literal[ 0, # NONE 1, # Ascending @@ -81,7 +77,7 @@ def from_spqueryfield(cls, field: EphemeralField, value: str | None=None): strict=field.isStrict, ) - def add_to_query(self, query, no_filter=False, formatauditobjs=False, collection=None, user=None, optimize_tree=True, prefer_parent_lookup=False): + def add_to_query(self, query, no_filter=False, formatauditobjs=False, collection=None, user=None, optimize_tree=True, prefer_parent_lookup=False, page_size=None): logger.info("adding field %s", self) value_required_for_filter = QueryOps.OPERATIONS[self.op_num] not in ( "op_true", # 6 @@ -116,11 +112,12 @@ def add_to_query(self, query, no_filter=False, formatauditobjs=False, collection self.op_num == 1 and self.fieldspec.get_field().name.lower() == self.fieldspec.table.idFieldName.lower() ) - if use_rank_lookup and prefer_parent_lookup and query.collection is not None: + if use_rank_lookup and prefer_parent_lookup and page_size and query.collection is not None: query, treedefs, _ = query.tree_rank_metadata(self.fieldspec.table, path[-2]) - # A bounded page can stop after a handful of parent lookups in a - # shallow tree. Materializing the entire rank delays that first page. - if max(depth for _, depth in treedefs) <= MAX_PAGE_PARENT_LOOKUP_RANKS: + # A parent walk touches one ancestor per rank for every page row. + # Build the shared rank lookup once when that walk exceeds the + # requested page's work budget. + if max(depth for _, depth in treedefs) <= page_size: use_tree_range = use_rank_lookup = False return self.fieldspec.add_to_query( diff --git a/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py b/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py index 0150f21c0ab..cec920598df 100644 --- a/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py +++ b/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py @@ -169,15 +169,15 @@ def test_geography_filter(self): self.assertIn('WITH RECURSIVE', statement) self.assertEqual(statement.count('LEFT OUTER JOIN geography '), 1) - def test_shallow_pages_keep_parent_lookups_but_deep_pages_use_rank_lookup(self): + def test_pages_choose_lookup_from_tree_depth_and_page_size(self): _, shallow_sql = self.assert_equivalent([ self.field('3.geography.name'), self.field('3.geography.Country', 1, 'USA'), - ], optimize_for_page=True) + ], optimize_for_page=True, page_size=20) self.assertNotIn('WITH RECURSIVE', shallow_sql) self.assertIn('ParentID', shallow_sql) _, deep_sql = self.assert_equivalent([ self.field('4.taxon.name'), self.field('4.taxon.Genus', 1, 'Alpha'), - ], optimize_for_page=True) + ], optimize_for_page=True, page_size=10) self.assertIn('WITH RECURSIVE', deep_sql) for depth, uses_lookup in [(8, False), (9, True)]: with self.subTest(depth=depth), patch( @@ -186,7 +186,7 @@ def test_shallow_pages_keep_parent_lookups_but_deep_pages_use_rank_lookup(self): ): _, statement = self.assert_equivalent([ self.field('4.taxon.name'), self.field('4.taxon.Genus', 1, 'Alpha'), - ], optimize_for_page=True) + ], optimize_for_page=True, page_size=8) self.assertEqual('WITH RECURSIVE' in statement, uses_lookup) def test_execute_only_prefers_parent_lookup_for_ungrouped_pages(self): @@ -209,6 +209,7 @@ def test_execute_only_prefers_parent_lookup_for_ungrouped_pages(self): execute(Mock(info={'connection': Mock()}), self.collection, self.specifyuser, 3, distinct, series, False, count_only, [], limit, 0) self.assertEqual(build.call_args.args[5].optimize_for_page, expected) + self.assertEqual(build.call_args.args[5].page_size, limit if expected else None) def test_missing_or_reversed_numbering_falls_back_without_writes(self): from specifyweb.specify.models import Taxon From 37691c537912d956231d8ef273043230dffef147 Mon Sep 17 00:00:00 2001 From: Grant Fitzsimmons <37256050+grantfitzsimmons@users.noreply.github.com> Date: Mon, 21 Sep 2026 14:53:02 +0200 Subject: [PATCH 05/12] fix(performance): choose tree strategy by operator --- .../backend/stored_queries/execution.py | 6 --- .../backend/stored_queries/queryfield.py | 17 ++----- .../tests/test_tree_query_performance.py | 51 +++---------------- 3 files changed, 12 insertions(+), 62 deletions(-) diff --git a/specifyweb/backend/stored_queries/execution.py b/specifyweb/backend/stored_queries/execution.py index 6ee618e49d8..12e1c65d41f 100644 --- a/specifyweb/backend/stored_queries/execution.py +++ b/specifyweb/backend/stored_queries/execution.py @@ -73,8 +73,6 @@ class BuildQueryProps(NamedTuple): search_synonymy: bool = False implicit_or: bool = True formatter_props: ObjectFormatterProps = DefaultQueryFormatterProps() - optimize_for_page: bool = False - page_size: int | None = None def set_group_concat_max_len(connection): @@ -886,8 +884,6 @@ def execute( series=series, search_synonymy=search_synonymy, formatter_props=formatter_props, - optimize_for_page=bool(limit) and not (count_only or distinct or series), - page_size=limit if limit and not (count_only or distinct or series) else None, ), ) @@ -1099,8 +1095,6 @@ def build_query( query, field, predicate = fs.add_to_query( query, formatauditobjs=props.formatauditobjs, collection=collection, user=user, optimize_tree=optimize_tree, - prefer_parent_lookup=props.optimize_for_page, - page_size=props.page_size, ) if field is None: diff --git a/specifyweb/backend/stored_queries/queryfield.py b/specifyweb/backend/stored_queries/queryfield.py index 7357ef94f94..f6f4d2bc09a 100644 --- a/specifyweb/backend/stored_queries/queryfield.py +++ b/specifyweb/backend/stored_queries/queryfield.py @@ -77,7 +77,7 @@ def from_spqueryfield(cls, field: EphemeralField, value: str | None=None): strict=field.isStrict, ) - def add_to_query(self, query, no_filter=False, formatauditobjs=False, collection=None, user=None, optimize_tree=True, prefer_parent_lookup=False, page_size=None): + def add_to_query(self, query, no_filter=False, formatauditobjs=False, collection=None, user=None, optimize_tree=True): logger.info("adding field %s", self) value_required_for_filter = QueryOps.OPERATIONS[self.op_num] not in ( "op_true", # 6 @@ -108,17 +108,10 @@ def add_to_query(self, query, no_filter=False, formatauditobjs=False, collection and sum(isinstance(part, TreeRankQuery) for part in path) == 1 and self.fieldspec.date_part is None ) - use_rank_lookup = use_tree_range and not ( - self.op_num == 1 - and self.fieldspec.get_field().name.lower() == self.fieldspec.table.idFieldName.lower() - ) - if use_rank_lookup and prefer_parent_lookup and page_size and query.collection is not None: - query, treedefs, _ = query.tree_rank_metadata(self.fieldspec.table, path[-2]) - # A parent walk touches one ancestor per rank for every page row. - # Build the shared rank lookup once when that walk exceeds the - # requested page's work budget. - if max(depth for _, depth in treedefs) <= page_size: - use_tree_range = use_rank_lookup = False + # Exact matches can start at the matching ancestor and range over its + # descendants. Other operators share a descendant-to-rank lookup so + # they do not repeatedly walk every node's parent chain. + use_rank_lookup = use_tree_range and self.op_num != 1 return self.fieldspec.add_to_query( query, diff --git a/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py b/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py index cec920598df..b969420df98 100644 --- a/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py +++ b/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py @@ -138,7 +138,11 @@ def test_positive_scalar_operators(self): self.field('4.taxon.name'), self.field('4.taxon.Genus', op, value), ]) self.assertTrue(rows) - self.assertIn('WITH RECURSIVE', statement) + if op == 1: + self.assertNotIn('WITH RECURSIVE', statement) + self.assertIn('BETWEEN', statement) + else: + self.assertIn('WITH RECURSIVE', statement) self.assertEqual(statement.count('LEFT OUTER JOIN taxon '), 1) def test_missing_rank_and_negation_keep_existing_null_semantics(self): @@ -166,51 +170,10 @@ def test_geography_filter(self): self.field('3.geography.Country', 1, 'USA'), ]) self.assertTrue(rows) - self.assertIn('WITH RECURSIVE', statement) + self.assertNotIn('WITH RECURSIVE', statement) + self.assertIn('BETWEEN', statement) self.assertEqual(statement.count('LEFT OUTER JOIN geography '), 1) - def test_pages_choose_lookup_from_tree_depth_and_page_size(self): - _, shallow_sql = self.assert_equivalent([ - self.field('3.geography.name'), self.field('3.geography.Country', 1, 'USA'), - ], optimize_for_page=True, page_size=20) - self.assertNotIn('WITH RECURSIVE', shallow_sql) - self.assertIn('ParentID', shallow_sql) - _, deep_sql = self.assert_equivalent([ - self.field('4.taxon.name'), self.field('4.taxon.Genus', 1, 'Alpha'), - ], optimize_for_page=True, page_size=10) - self.assertIn('WITH RECURSIVE', deep_sql) - for depth, uses_lookup in [(8, False), (9, True)]: - with self.subTest(depth=depth), patch( - 'specifyweb.backend.stored_queries.query_construct.get_treedefs', - return_value=[(self.taxontreedef.id, depth)], - ): - _, statement = self.assert_equivalent([ - self.field('4.taxon.name'), self.field('4.taxon.Genus', 1, 'Alpha'), - ], optimize_for_page=True, page_size=8) - self.assertEqual('WITH RECURSIVE' in statement, uses_lookup) - - def test_execute_only_prefers_parent_lookup_for_ungrouped_pages(self): - from unittest.mock import Mock - from specifyweb.backend.stored_queries.execution import execute - for count_only, limit, distinct, series, expected in [ - (False, 40, False, False, True), - (True, 40, False, False, False), - (False, 0, False, False, False), - (False, None, False, False, False), - (False, 40, True, False, False), - (False, 40, False, True, False), - ]: - with self.subTest(count_only=count_only, limit=limit, distinct=distinct, series=series): - with patch('specifyweb.backend.stored_queries.execution.set_group_concat_max_len'), patch( - 'specifyweb.backend.stored_queries.execution.build_query', - side_effect=RuntimeError('query built'), - ) as build: - with self.assertRaisesMessage(RuntimeError, 'query built'): - execute(Mock(info={'connection': Mock()}), self.collection, self.specifyuser, - 3, distinct, series, False, count_only, [], limit, 0) - self.assertEqual(build.call_args.args[5].optimize_for_page, expected) - self.assertEqual(build.call_args.args[5].page_size, limit if expected else None) - def test_missing_or_reversed_numbering_falls_back_without_writes(self): from specifyweb.specify.models import Taxon for values in [dict(nodenumber=None), dict(highestchildnodenumber=None), From cece53e117dbe9e7106f909adb91a6941ea2e45b Mon Sep 17 00:00:00 2001 From: Grant Fitzsimmons <37256050+grantfitzsimmons@users.noreply.github.com> Date: Mon, 21 Sep 2026 10:34:46 +0200 Subject: [PATCH 06/12] fix(performance): reuse tree rank metadata within queries --- .../backend/stored_queries/query_construct.py | 66 +++++++++++-------- .../tests/test_tree_query_performance.py | 47 +++++++++++++ 2 files changed, 87 insertions(+), 26 deletions(-) create mode 100644 specifyweb/backend/stored_queries/tests/test_tree_query_performance.py diff --git a/specifyweb/backend/stored_queries/query_construct.py b/specifyweb/backend/stored_queries/query_construct.py index f56a1f8d636..9fc4b8dcc7f 100644 --- a/specifyweb/backend/stored_queries/query_construct.py +++ b/specifyweb/backend/stored_queries/query_construct.py @@ -11,12 +11,6 @@ logger = logging.getLogger(__name__) -def _safe_filter(query): - count = query.count() - if count <= 1: - return query.first() - raise Exception(f"Got more than one matching: {list(query)}") - class QueryConstruct(namedtuple('QueryConstruct', 'collection objectformatter query join_cache tree_rank_count internal_filters')): def __new__(cls, *args, **kwargs): @@ -27,6 +21,36 @@ def __new__(cls, *args, **kwargs): kwargs['internal_filters'] = [] return super().__new__(cls, *args, **kwargs) + def tree_rank_metadata(self, table, tree_rank): + """Resolve ranks once per query, shared by all paths into the same tree.""" + query = self + defs_key = ('TreeDefinitions', table.name) + if defs_key not in query.join_cache: + query = query._replace(join_cache=query.join_cache.copy()) + query.join_cache[defs_key] = get_treedefs(query.collection, table.name) + treedefs = query.join_cache[defs_key] + + # TreeRankQuery equality does not include the explicit tree definition. + rank_key = ('TreeRankItems', table.name, tree_rank.name, tree_rank.treedef_id) + if rank_key not in query.join_cache: + item_model = getattr(spmodels, table.django_name + 'treedefitem') + def_ids = [ + def_id for def_id, _ in treedefs + if tree_rank.treedef_id is None or tree_rank.treedef_id == def_id + ] + items = item_model.objects.filter( + treedef_id__in=def_ids, name=tree_rank.name + ).values_list('treedef_id', 'id') + by_definition = {} + for def_id, item_id in items: + if def_id in by_definition: + raise Exception('Got more than one matching tree rank') + by_definition[def_id] = item_id + ranks = [(def_id, by_definition[def_id]) for def_id in def_ids if def_id in by_definition] + query = query._replace(join_cache=query.join_cache.copy()) + query.join_cache[rank_key] = ranks + return query, treedefs, query.join_cache[rank_key] + def handle_tree_field(self, node, table, tree_rank: TreeRankQuery, next_join_path, current_field_spec: QueryFieldSpec): query = self query_before_tree_joins = query @@ -40,13 +64,21 @@ def handle_tree_field(self, node, table, tree_rank: TreeRankQuery, next_join_pat treedefitem_column = table.name + 'TreeDefItemID' treedef_column = table.name + 'TreeDefID' + query, treedefs, treedefs_with_ranks = query.tree_rank_metadata(table, tree_rank) + + if not treedefs_with_ranks: + logger.warning( + "Didn't find tree rank %r across any %s tree; skipping field", + tree_rank.name, + table.name, + ) + return query_before_tree_joins, None, None, table + cache_key = (node, 'TreeRanks') if cache_key in query.join_cache: logger.debug("using join cache for %r tree ranks.", node) ancestors, treedefs = query.join_cache[cache_key] else: - treedefs = get_treedefs(query.collection, table.name) - # We need to take the max here. Otherwise, it is possible that the same rank # name may not occur at the same level across tree defs. max_depth = max(depth for _, depth in treedefs) @@ -61,24 +93,6 @@ def handle_tree_field(self, node, table, tree_rank: TreeRankQuery, next_join_pat query = query._replace(join_cache=query.join_cache.copy()) query.join_cache[cache_key] = (ancestors, treedefs) - item_model = getattr(spmodels, table.django_name + "treedefitem") - - # TODO: optimize out the ranks that appear? cache them - treedefs_with_ranks: list[tuple[int, int]] = [tup for tup in [ - (treedef_id, _safe_filter(item_model.objects.filter(treedef_id=treedef_id, name=tree_rank.name).values_list('id', flat=True))) - for treedef_id, _ in treedefs - # For constructing tree queries for batch edit - if (tree_rank.treedef_id is None or tree_rank.treedef_id == treedef_id) - ] if tup[1] is not None] - - if not treedefs_with_ranks: - logger.warning( - "Didn't find tree rank %r across any %s tree; skipping field", - tree_rank.name, - table.name, - ) - return query_before_tree_joins, None, None, table - treedefitem_params = [treedefitem_id for (_, treedefitem_id) in treedefs_with_ranks] def make_tree_field_spec(tree_node): diff --git a/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py b/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py new file mode 100644 index 00000000000..7f8b9274cb6 --- /dev/null +++ b/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py @@ -0,0 +1,47 @@ +from unittest.mock import patch + +from sqlalchemy import orm + +from specifyweb.backend.stored_queries import models +from specifyweb.backend.stored_queries.query_construct import QueryConstruct +from specifyweb.backend.stored_queries.queryfieldspec import TreeRankQuery +from specifyweb.backend.trees.tests.test_trees import SqlTreeSetup +from specifyweb.specify.models import datamodel + + +class TreeRankMetadataTests(SqlTreeSetup): + def construct(self): + return QueryConstruct( + collection=self.collection, + objectformatter=None, + query=orm.Query(models.Taxon._id), + ) + + def test_rank_metadata_is_reused_within_query(self): + table = datamodel.get_table_strict('Taxon') + rank = TreeRankQuery.create('Kingdom', 'Taxon') + with patch( + 'specifyweb.backend.stored_queries.query_construct.get_treedefs', + return_value=[(self.taxontreedef.id, 11)], + ) as definitions: + with self.assertNumQueries(1): + query, _, ranks = self.construct().tree_rank_metadata(table, rank) + self.assertEqual(ranks, [(self.taxontreedef.id, self.taxon_kingdom.id)]) + with self.assertNumQueries(0): + query, _, repeated = query.tree_rank_metadata(table, rank) + self.assertEqual(repeated, ranks) + with self.assertNumQueries(1): + query.tree_rank_metadata(table, TreeRankQuery.create('Genus', 'Taxon')) + definitions.assert_called_once() + with self.assertNumQueries(1): + self.construct().tree_rank_metadata(table, rank) + self.assertEqual(definitions.call_count, 2) + + def test_explicit_tree_definition_has_separate_cache_entry(self): + table = datamodel.get_table_strict('Taxon') + rank = TreeRankQuery.create('Kingdom', 'Taxon') + query, _, _ = self.construct().tree_rank_metadata(table, rank) + scoped_rank = TreeRankQuery.create('Kingdom', 'Taxon') + scoped_rank.treedef_id = -1 + _, _, ranks = query.tree_rank_metadata(table, scoped_rank) + self.assertEqual(ranks, []) From 89704f4ded3ae71f5fa230a639fc354ce7e68f1c Mon Sep 17 00:00:00 2001 From: Grant Fitzsimmons <37256050+grantfitzsimmons@users.noreply.github.com> Date: Mon, 21 Sep 2026 12:02:35 +0200 Subject: [PATCH 07/12] fix(performance): avoid repeated ancestor joins in tree queries Use live node-number ranges for rank ID equality and reusable recursive rank lookups for other positive scalar filters. Preserve parent lookups for negative/empty filters, incomplete numbering, recordsets, and explicit record ID selections. Cover scoping, preferred taxa, OR combinations, tree moves, and result equivalence. --- .../backend/stored_queries/execution.py | 14 +- .../backend/stored_queries/query_construct.py | 87 +++++++- .../backend/stored_queries/queryfield.py | 27 ++- .../backend/stored_queries/queryfieldspec.py | 10 +- .../tests/test_tree_query_performance.py | 210 ++++++++++++++++++ 5 files changed, 342 insertions(+), 6 deletions(-) diff --git a/specifyweb/backend/stored_queries/execution.py b/specifyweb/backend/stored_queries/execution.py index 679cad49c71..5aa80108ad8 100644 --- a/specifyweb/backend/stored_queries/execution.py +++ b/specifyweb/backend/stored_queries/execution.py @@ -1071,6 +1071,17 @@ def build_query( order_by_exprs = [] selected_fields = [] predicates_by_field = defaultdict(list) + # Materializing a subtree can cost more than parent lookups when the caller + # has already restricted the query to explicit record IDs. + optimize_tree = props.recordsetid is None and not any( + len(fs.fieldspec.join_path) == 1 + and base_table is not None + and fs.fieldspec.get_field() == base_table.idField + and fs.op_num in (1, 10) + and not fs.negate + and fs.value not in ('', None) + for fs in field_specs + ) # augment_field_specs(field_specs, formatauditobjs) for fs in field_specs: # sort_type = SORT_TYPES[fs.sort_type] @@ -1082,7 +1093,8 @@ def build_query( continue query, field, predicate = fs.add_to_query( - query, formatauditobjs=props.formatauditobjs, collection=collection, user=user + query, formatauditobjs=props.formatauditobjs, collection=collection, user=user, + optimize_tree=optimize_tree, ) if field is None: diff --git a/specifyweb/backend/stored_queries/query_construct.py b/specifyweb/backend/stored_queries/query_construct.py index 9fc4b8dcc7f..9170b809e3f 100644 --- a/specifyweb/backend/stored_queries/query_construct.py +++ b/specifyweb/backend/stored_queries/query_construct.py @@ -2,6 +2,7 @@ from collections import namedtuple, deque from sqlalchemy import orm, sql, or_ +from django.db.models import F, Q import specifyweb.specify.models as spmodels from specifyweb.backend.trees.utils import get_treedefs @@ -51,7 +52,28 @@ def tree_rank_metadata(self, table, tree_rank): query.join_cache[rank_key] = ranks return query, treedefs, query.join_cache[rank_key] - def handle_tree_field(self, node, table, tree_rank: TreeRankQuery, next_join_path, current_field_spec: QueryFieldSpec): + def tree_numbering_available(self, table, treedefs): + """Fall back to parent links for trees with missing or reversed intervals. + + Tree writes maintain the nesting invariant. Do not renumber a tree while + reading it, or cache this check across requests (uploads can change it). + """ + cache_key = ('TreeNumbering', table.name) + query = self + if cache_key not in query.join_cache: + model = getattr(spmodels, table.django_name) + invalid = model.objects.filter( + definition_id__in=[def_id for def_id, _ in treedefs] + ).filter( + Q(nodenumber__isnull=True) + | Q(highestchildnodenumber__isnull=True) + | Q(highestchildnodenumber__lt=F('nodenumber')) + ).exists() + query = query._replace(join_cache=query.join_cache.copy()) + query.join_cache[cache_key] = not invalid + return query, query.join_cache[cache_key] + + def handle_tree_field(self, node, table, tree_rank: TreeRankQuery, next_join_path, current_field_spec: QueryFieldSpec, use_range=False, use_rank_lookup=False): query = self query_before_tree_joins = query if query.collection is None: # Not sure it makes sense to query across collections @@ -74,6 +96,69 @@ def handle_tree_field(self, node, table, tree_rank: TreeRankQuery, next_join_pat ) return query_before_tree_joins, None, None, table + if use_range: + query, use_range = query.tree_numbering_available(table, treedefs) + if use_range: + # One matching ancestor at this rank, including the node itself. + # Keep the filter on the ancestor column so the optimizer can start + # with selective ID/name indexes and range-scan descendants. + range_cache_key = (node, 'TreeRankLookup' if use_rank_lookup else 'TreeRankRange', + tree_rank.name, tree_rank.treedef_id) + if range_cache_key in query.join_cache: + ancestor = query.join_cache[range_cache_key] + else: + ancestor = orm.aliased(getattr(models, table.name)) + if use_rank_lookup: + # Map descendants to their ancestor once for this rank. The + # unfiltered mapping preserves displayed values when filters + # are combined with OR, while the PK join avoids a range + # comparison against every node at the requested rank. + model = getattr(models, table.name) + rank_node = orm.aliased(model) + child = orm.aliased(model) + lookup = sql.select( + rank_node._id.label('node_id'), + rank_node._id.label('ancestor_id'), + getattr(rank_node, treedef_column).label('definition_id'), + ).where( + getattr(rank_node, treedefitem_column).in_( + [item_id for _, item_id in treedefs_with_ranks] + ), + ).cte(recursive=True) + lookup = lookup.union(sql.select( + child._id, lookup.c.ancestor_id, getattr(child, treedef_column), + ).join(lookup, sql.and_( + child.ParentID == lookup.c.node_id, + getattr(child, treedef_column) == lookup.c.definition_id, + ))) + query = query._replace(query=query.query.outerjoin( + lookup, node._id == lookup.c.node_id, + ).outerjoin(ancestor, ancestor._id == lookup.c.ancestor_id)) + else: + query = query._replace(query=query.query.outerjoin(ancestor, sql.and_( + getattr(node, treedef_column) == getattr(ancestor, treedef_column), + getattr(ancestor, treedefitem_column).in_( + [item_id for _, item_id in treedefs_with_ranks] + ), + node.nodeNumber.between(ancestor.nodeNumber, ancestor.highestChildNodeNumber), + ))) + query = query._replace(join_cache=query.join_cache.copy()) + query.join_cache[range_cache_key] = ancestor + field_spec = current_field_spec._replace( + root_table=table, + root_sql_table=ancestor, + join_path=next_join_path, + ) + query, column, field, result_table = field_spec.add_spec_to_query(query) + query = query._replace(internal_filters=[ + *query.internal_filters, + or_( + getattr(node, treedef_column).in_([def_id for def_id, _ in treedefs_with_ranks]), + getattr(node, treedef_column).is_(None), + ), + ]) + return query, column, field, result_table + cache_key = (node, 'TreeRanks') if cache_key in query.join_cache: logger.debug("using join cache for %r tree ranks.", node) diff --git a/specifyweb/backend/stored_queries/queryfield.py b/specifyweb/backend/stored_queries/queryfield.py index 09886bb93fe..bcf0ba2be14 100644 --- a/specifyweb/backend/stored_queries/queryfield.py +++ b/specifyweb/backend/stored_queries/queryfield.py @@ -4,7 +4,7 @@ from typing import Any, NamedTuple, Literal from .query_ops import QueryOps, QUERYFIELD_OPERATION_NUMBER -from .queryfieldspec import QueryFieldSpec +from .queryfieldspec import QueryFieldSpec, TreeRankQuery logger = logging.getLogger(__name__) @@ -78,7 +78,7 @@ def from_spqueryfield(cls, field: EphemeralField, value: str | None=None): strict=field.isStrict, ) - def add_to_query(self, query, no_filter=False, formatauditobjs=False, collection=None, user=None): + def add_to_query(self, query, no_filter=False, formatauditobjs=False, collection=None, user=None, optimize_tree=True): logger.info("adding field %s", self) value_required_for_filter = QueryOps.OPERATIONS[self.op_num] not in ( "op_true", # 6 @@ -92,6 +92,24 @@ def add_to_query(self, query, no_filter=False, formatauditobjs=False, collection self.value == "" and value_required_for_filter and not self.negate ) + # Positive scalar filters reject missing ancestors, allowing MariaDB to + # start at the matching rank instead of walking up from every specimen. + # Empty/negated filters need the existing missing-ancestor semantics; + # relationship paths and date transformations retain their usual joins. + path = self.fieldspec.join_path + use_tree_range = ( + optimize_tree + and not no_filter + and not self.negate + and (not value_required_for_filter or isinstance(self.value, str)) + and self.op_num in {0, 1, 2, 3, 4, 5, 6, 7, 9, 10, 11, 15, 18} + and len(path) >= 2 + and isinstance(path[-2], TreeRankQuery) + and not path[-1].is_relationship + and sum(isinstance(part, TreeRankQuery) for part in path) == 1 + and self.fieldspec.date_part is None + ) + return self.fieldspec.add_to_query( query, value=self.value, @@ -102,4 +120,9 @@ def add_to_query(self, query, no_filter=False, formatauditobjs=False, collection strict=self.strict, collection=collection, user=user, + use_tree_range=use_tree_range, + use_rank_lookup=use_tree_range and not ( + self.op_num == 1 + and self.fieldspec.get_field().name.lower() == self.fieldspec.table.idFieldName.lower() + ), ) diff --git a/specifyweb/backend/stored_queries/queryfieldspec.py b/specifyweb/backend/stored_queries/queryfieldspec.py index 74fc05350f4..5c943d3da03 100644 --- a/specifyweb/backend/stored_queries/queryfieldspec.py +++ b/specifyweb/backend/stored_queries/queryfieldspec.py @@ -526,6 +526,8 @@ def add_to_query( strict=False, collection=None, user=None, + use_tree_range=False, + use_rank_lookup=False, ): # print "############################################################################" # print "formatauditobjs " + str(formatauditobjs) @@ -533,7 +535,9 @@ def add_to_query( # print "field name " + self.get_field().name # print "is auditlog obj format field = " + str(self.is_auditlog_obj_format_field(formatauditobjs)) # print "############################################################################" - query, orm_field, field, table = self.add_spec_to_query(query, formatter) + query, orm_field, field, table = self.add_spec_to_query( + query, formatter, use_tree_range=use_tree_range, use_rank_lookup=use_rank_lookup + ) if orm_field is None: return query, None, None return self.apply_filter( @@ -551,7 +555,7 @@ def add_to_query( ) def add_spec_to_query( - self, query, formatter=None, aggregator=None, cycle_detector=[] + self, query, formatter=None, aggregator=None, cycle_detector=[], use_tree_range=False, use_rank_lookup=False ): if self.get_field() is None: @@ -589,6 +593,8 @@ def add_spec_to_query( field, self.join_path[tree_rank_idx + 1 :], self, + use_range=use_tree_range, + use_rank_lookup=use_rank_lookup, ) else: try: diff --git a/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py b/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py index 7f8b9274cb6..0bec8434b92 100644 --- a/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py +++ b/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py @@ -45,3 +45,213 @@ def test_explicit_tree_definition_has_separate_cache_entry(self): scoped_rank.treedef_id = -1 _, _, ranks = query.tree_rank_metadata(table, scoped_rank) self.assertEqual(ranks, []) + + +class TreeRangeFilterTests(SqlTreeSetup): + def setUp(self): + super().setUp() + self.root = self.make_taxontree('Life', 'Taxonomy Root') + self.kingdom = self.make_taxontree('Animalia', 'Kingdom', parent=self.root) + self.genus = self.make_taxontree('Alpha', 'Genus', parent=self.kingdom) + self.first = self.make_taxontree('one', 'Species', parent=self.genus) + self.second = self.make_taxontree('two', 'Species', parent=self.genus) + self.other = self.make_taxontree('Beta', 'Genus', parent=self.kingdom) + self.outside = self.make_taxontree('three', 'Species', parent=self.other) + self.skipped = self.make_taxontree('skipped', 'Species', parent=self.root) + + def field(self, stringid, op=8, value='', negate=False, display=True): + from specifyweb.backend.stored_queries.queryfield import QueryField + from specifyweb.backend.stored_queries.queryfieldspec import QueryFieldSpec + return QueryField( + QueryFieldSpec.from_stringid(stringid, False), + op, value, negate, display, None, 0, + ) + + def run_query(self, fields, legacy=False, **props): + from django.db import connection + from sqlalchemy.dialects import mysql + from specifyweb.backend.stored_queries.execution import build_query, BuildQueryProps + handle = QueryConstruct.handle_tree_field + + def parent_walk(query, *args, **kwargs): + kwargs['use_range'] = False + return handle(query, *args, **kwargs) + + with self.__class__.test_session_context() as session, patch.object( + QueryConstruct, 'handle_tree_field', parent_walk if legacy else handle + ): + query, _ = build_query( + session, self.collection, self.specifyuser, + fields[0].fieldspec.root_table.tableId, fields, BuildQueryProps(**props), + ) + compiled = query.statement.compile(dialect=mysql.dialect(), compile_kwargs={'literal_binds': True}) + with connection.cursor() as cursor: + cursor.execute(str(compiled)) + return cursor.fetchall(), str(compiled) + + def assert_equivalent(self, fields, **props): + actual, statement = self.run_query(fields, **props) + expected, _ = self.run_query(fields, legacy=True, **props) + self.assertCountEqual(actual, expected) + return actual, statement + + def test_id_filter_includes_self_and_descendants_only(self): + rows, statement = self.assert_equivalent([ + self.field('4.taxon.name'), + self.field('4.taxon.Genus ID', 1, str(self.genus.id), display=False), + ]) + self.assertEqual({row[0] for row in rows}, {self.genus.id, self.first.id, self.second.id}) + self.assertIn('BETWEEN', statement) + self.assertNotIn('ParentID', statement) + + def test_explicit_record_ids_keep_parent_walk(self): + rows, statement = self.assert_equivalent([ + self.field('4.taxon.name'), + self.field('4.taxon.taxonId', 10, str(self.first.id), display=False), + self.field('4.taxon.Genus', 11, 'Al', display=False), + ]) + self.assertEqual({row[0] for row in rows}, {self.first.id}) + self.assertNotIn('WITH RECURSIVE', statement) + self.assertIn('ParentID', statement) + + def test_recordset_keeps_parent_walk(self): + from specifyweb.specify.models import Recordset, Recordsetitem + recordset = Recordset.objects.create( + collectionmemberid=self.collection.id, dbtableid=4, + name='Small taxonomy selection', specifyuser=self.specifyuser, type=0, + ) + Recordsetitem.objects.create(recordset=recordset, recordid=self.first.id) + rows, statement = self.assert_equivalent([ + self.field('4.taxon.name'), self.field('4.taxon.Genus', 11, 'Al'), + ], recordsetid=recordset.id) + self.assertEqual({row[0] for row in rows}, {self.first.id}) + self.assertNotIn('WITH RECURSIVE', statement) + self.assertIn('ParentID', statement) + + def test_positive_scalar_operators(self): + cases = [(0, 'A%'), (1, 'Alpha'), (2, 'A'), (3, 'Z'), (4, 'Alpha'), + (5, 'Beta'), (9, 'Alpha,Beta'), (10, 'Alpha,Beta'), + (11, 'lph'), (15, 'Al'), (18, 'pha')] + for op, value in cases: + with self.subTest(op=op): + rows, statement = self.assert_equivalent([ + self.field('4.taxon.name'), self.field('4.taxon.Genus', op, value), + ]) + self.assertTrue(rows) + self.assertIn('WITH RECURSIVE', statement) + self.assertEqual(statement.count('LEFT OUTER JOIN taxon '), 1) + + def test_missing_rank_and_negation_keep_existing_null_semantics(self): + for op, value, negate in [(12, '', False), (1, None, False), + (1, 'Alpha', True), (10, 'Alpha,Beta', True)]: + with self.subTest(op=op): + rows, statement = self.assert_equivalent([ + self.field('4.taxon.name'), self.field('4.taxon.Genus', op, value, negate), + ]) + self.assertIn(self.skipped.id, {row[0] for row in rows}) + self.assertIn('ParentID', statement) + + def test_repeated_filters_and_multiple_ranks(self): + for implicit_or in [True, False]: + self.assert_equivalent([ + self.field('4.taxon.name'), + self.field('4.taxon.Genus', 1, 'Alpha', display=False), + self.field('4.taxon.Genus', 1, 'Beta', display=False), + self.field('4.taxon.Kingdom', 1, 'Animalia'), + ], implicit_or=implicit_or) + + def test_geography_filter(self): + rows, statement = self.assert_equivalent([ + self.field('3.geography.name'), + self.field('3.geography.Country', 1, 'USA'), + ]) + self.assertTrue(rows) + self.assertIn('WITH RECURSIVE', statement) + self.assertEqual(statement.count('LEFT OUTER JOIN geography '), 1) + + def test_missing_or_reversed_numbering_falls_back_without_writes(self): + from specifyweb.specify.models import Taxon + for values in [dict(nodenumber=None), dict(highestchildnodenumber=None), + dict(nodenumber=50, highestchildnodenumber=1)]: + with self.subTest(values=values): + Taxon.objects.filter(pk=self.first.id).update(**values) + rows, statement = self.assert_equivalent([ + self.field('4.taxon.name'), + self.field('4.taxon.Genus ID', 1, str(self.genus.id), display=False), + ]) + self.assertIn(self.first.id, {row[0] for row in rows}) + self.assertIn('ParentID', statement) + self.first.refresh_from_db() + for key, value in values.items(): + self.assertEqual(getattr(self.first, key), value) + + def test_rank_id_from_wrong_rank_does_not_match(self): + rows, _ = self.assert_equivalent([ + self.field('4.taxon.name'), + self.field('4.taxon.Genus ID', 1, str(self.kingdom.id), display=False), + ]) + self.assertEqual(rows, ()) + + def test_numbering_is_resolved_again_after_tree_move(self): + fields = [self.field('4.taxon.name'), + self.field('4.taxon.Genus ID', 1, str(self.genus.id), display=False)] + before, _ = self.assert_equivalent(fields) + self.assertIn(self.first.id, {row[0] for row in before}) + self.first.parent = self.other + self.first.save() + after, _ = self.assert_equivalent(fields) + self.assertNotIn(self.first.id, {row[0] for row in after}) + + def test_preferred_taxon_path_and_distinct(self): + from specifyweb.specify.models import Determination + co = self.collectionobjects[0] + determination = Determination.objects.create( + collectionobject=co, taxon=self.outside, iscurrent=True, + ) + Determination.objects.filter(pk=determination.id).update(preferredtaxon=self.first) + fields = [self.field('1.collectionobject.catalogNumber'), + self.field('1,9-determinations,4-preferredTaxon.taxon.Genus ID', + 1, str(self.genus.id), display=False)] + rows, statement = self.assert_equivalent(fields) + self.assertEqual({row[0] for row in rows}, {co.id}) + self.assertIn('PreferredTaxonID', statement) + self.assert_equivalent(fields, distinct=True) + + def test_multiple_tree_definitions_and_explicit_scope(self): + from specifyweb.specify.models import Taxontreedef + second_def = Taxontreedef.objects.create(name='Second taxonomy', discipline=self.discipline) + self.make_taxon_ranks(second_def) + root = self.make_taxontree('Other life', 'Taxonomy Root', treedef=second_def) + genus = self.make_taxontree('Alpha', 'Genus', parent=root, treedef=second_def) + leaf = self.make_taxontree('other species', 'Species', parent=genus, treedef=second_def) + fields = [self.field('4.taxon.name'), self.field('4.taxon.Genus', 1, 'Alpha')] + rows, _ = self.assert_equivalent(fields) + self.assertEqual({row[0] for row in rows}, + {self.genus.id, self.first.id, self.second.id, genus.id, leaf.id}) + + scoped_rank = TreeRankQuery.create('Genus', 'Taxon', treedef_id=self.taxontreedef.id) + scoped_spec = fields[1].fieldspec._replace( + join_path=(scoped_rank, fields[1].fieldspec.join_path[-1]), + ) + rows, _ = self.assert_equivalent([fields[0], fields[1]._replace(fieldspec=scoped_spec)]) + self.assertEqual({row[0] for row in rows}, {self.genus.id, self.first.id, self.second.id}) + + second_def.discipline = None + second_def.save() + rows, _ = self.assert_equivalent(fields) + self.assertEqual({row[0] for row in rows}, {self.genus.id, self.first.id, self.second.id}) + + def test_boolean_and_synonymy_filters(self): + from specifyweb.specify.models import Taxon + Taxon.objects.filter(pk=self.genus.id).update(isaccepted=True) + Taxon.objects.filter(pk=self.other.id).update(isaccepted=False) + for op in (6, 7): + with self.subTest(op=op): + rows, statement = self.assert_equivalent([ + self.field('4.taxon.name'), self.field('4.taxon.Genus isAccepted', op), + ]) + self.assertTrue(rows) + self.assertIn('WITH RECURSIVE', statement) + self.assert_equivalent([ + self.field('4.taxon.name'), self.field('4.taxon.Genus', 1, 'Alpha'), + ], search_synonymy=True) From 9c500e7f899bfd68017c4d8ed9adf0abe34d1b48 Mon Sep 17 00:00:00 2001 From: Grant Fitzsimmons <37256050+grantfitzsimmons@users.noreply.github.com> Date: Mon, 21 Sep 2026 12:16:11 +0200 Subject: [PATCH 08/12] fix(performance): preserve early limits for shallow tree queries --- .../backend/stored_queries/execution.py | 3 ++ .../backend/stored_queries/queryfield.py | 20 ++++++--- .../tests/test_tree_query_performance.py | 45 ++++++++++++++++++- 3 files changed, 61 insertions(+), 7 deletions(-) diff --git a/specifyweb/backend/stored_queries/execution.py b/specifyweb/backend/stored_queries/execution.py index 5aa80108ad8..faf8df5e2ae 100644 --- a/specifyweb/backend/stored_queries/execution.py +++ b/specifyweb/backend/stored_queries/execution.py @@ -73,6 +73,7 @@ class BuildQueryProps(NamedTuple): search_synonymy: bool = False implicit_or: bool = True formatter_props: ObjectFormatterProps = DefaultQueryFormatterProps() + optimize_for_page: bool = False def set_group_concat_max_len(connection): @@ -884,6 +885,7 @@ def execute( series=series, search_synonymy=search_synonymy, formatter_props=formatter_props, + optimize_for_page=bool(limit) and not (count_only or distinct or series), ), ) @@ -1095,6 +1097,7 @@ def build_query( query, field, predicate = fs.add_to_query( query, formatauditobjs=props.formatauditobjs, collection=collection, user=user, optimize_tree=optimize_tree, + prefer_parent_lookup=props.optimize_for_page, ) if field is None: diff --git a/specifyweb/backend/stored_queries/queryfield.py b/specifyweb/backend/stored_queries/queryfield.py index bcf0ba2be14..134a12b65d3 100644 --- a/specifyweb/backend/stored_queries/queryfield.py +++ b/specifyweb/backend/stored_queries/queryfield.py @@ -8,6 +8,9 @@ logger = logging.getLogger(__name__) +# Prefer parent lookups for shallow trees; deeper trees are cheaper to +# materialize as a rank range. +MAX_PAGE_PARENT_LOOKUP_RANKS = 8 QUREYFIELD_SORT_T = Literal[ 0, # NONE @@ -78,7 +81,7 @@ def from_spqueryfield(cls, field: EphemeralField, value: str | None=None): strict=field.isStrict, ) - def add_to_query(self, query, no_filter=False, formatauditobjs=False, collection=None, user=None, optimize_tree=True): + def add_to_query(self, query, no_filter=False, formatauditobjs=False, collection=None, user=None, optimize_tree=True, prefer_parent_lookup=False): logger.info("adding field %s", self) value_required_for_filter = QueryOps.OPERATIONS[self.op_num] not in ( "op_true", # 6 @@ -109,6 +112,16 @@ def add_to_query(self, query, no_filter=False, formatauditobjs=False, collection and sum(isinstance(part, TreeRankQuery) for part in path) == 1 and self.fieldspec.date_part is None ) + use_rank_lookup = use_tree_range and not ( + self.op_num == 1 + and self.fieldspec.get_field().name.lower() == self.fieldspec.table.idFieldName.lower() + ) + if use_rank_lookup and prefer_parent_lookup and query.collection is not None: + query, treedefs, _ = query.tree_rank_metadata(self.fieldspec.table, path[-2]) + # A bounded page can stop after a handful of parent lookups in a + # shallow tree. Materializing the entire rank delays that first page. + if max(depth for _, depth in treedefs) <= MAX_PAGE_PARENT_LOOKUP_RANKS: + use_tree_range = use_rank_lookup = False return self.fieldspec.add_to_query( query, @@ -121,8 +134,5 @@ def add_to_query(self, query, no_filter=False, formatauditobjs=False, collection collection=collection, user=user, use_tree_range=use_tree_range, - use_rank_lookup=use_tree_range and not ( - self.op_num == 1 - and self.fieldspec.get_field().name.lower() == self.fieldspec.table.idFieldName.lower() - ), + use_rank_lookup=use_rank_lookup, ) diff --git a/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py b/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py index 0bec8434b92..340b49c0115 100644 --- a/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py +++ b/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py @@ -155,8 +155,8 @@ def test_repeated_filters_and_multiple_ranks(self): for implicit_or in [True, False]: self.assert_equivalent([ self.field('4.taxon.name'), - self.field('4.taxon.Genus', 1, 'Alpha', display=False), - self.field('4.taxon.Genus', 1, 'Beta', display=False), + self.field('4.taxon.Genus', 1, 'Alpha'), + self.field('4.taxon.Genus', 1, 'Beta'), self.field('4.taxon.Kingdom', 1, 'Animalia'), ], implicit_or=implicit_or) @@ -169,6 +169,47 @@ def test_geography_filter(self): self.assertIn('WITH RECURSIVE', statement) self.assertEqual(statement.count('LEFT OUTER JOIN geography '), 1) + def test_shallow_pages_keep_parent_lookups_but_deep_pages_use_rank_lookup(self): + _, shallow_sql = self.assert_equivalent([ + self.field('3.geography.name'), self.field('3.geography.Country', 1, 'USA'), + ], optimize_for_page=True) + self.assertNotIn('WITH RECURSIVE', shallow_sql) + self.assertIn('ParentID', shallow_sql) + _, deep_sql = self.assert_equivalent([ + self.field('4.taxon.name'), self.field('4.taxon.Genus', 1, 'Alpha'), + ], optimize_for_page=True) + self.assertIn('WITH RECURSIVE', deep_sql) + for depth, uses_lookup in [(8, False), (9, True)]: + with self.subTest(depth=depth), patch( + 'specifyweb.backend.stored_queries.query_construct.get_treedefs', + return_value=[(self.taxontreedef.id, depth)], + ): + _, statement = self.assert_equivalent([ + self.field('4.taxon.name'), self.field('4.taxon.Genus', 1, 'Alpha'), + ], optimize_for_page=True) + self.assertEqual('WITH RECURSIVE' in statement, uses_lookup) + + def test_execute_only_prefers_parent_lookup_for_ungrouped_pages(self): + from unittest.mock import Mock + from specifyweb.backend.stored_queries.execution import execute + for count_only, limit, distinct, series, expected in [ + (False, 40, False, False, True), + (True, 40, False, False, False), + (False, 0, False, False, False), + (False, None, False, False, False), + (False, 40, True, False, False), + (False, 40, False, True, False), + ]: + with self.subTest(count_only=count_only, limit=limit, distinct=distinct, series=series): + with patch('specifyweb.backend.stored_queries.execution.set_group_concat_max_len'), patch( + 'specifyweb.backend.stored_queries.execution.build_query', + side_effect=RuntimeError('query built'), + ) as build: + with self.assertRaisesMessage(RuntimeError, 'query built'): + execute(Mock(info={'connection': Mock()}), self.collection, self.specifyuser, + 3, distinct, series, False, count_only, [], limit, 0) + self.assertEqual(build.call_args.args[5].optimize_for_page, expected) + def test_missing_or_reversed_numbering_falls_back_without_writes(self): from specifyweb.specify.models import Taxon for values in [dict(nodenumber=None), dict(highestchildnodenumber=None), From de195fb4d11d0a8ea5172190cb72f937c6d708e4 Mon Sep 17 00:00:00 2001 From: Grant Fitzsimmons <37256050+grantfitzsimmons@users.noreply.github.com> Date: Mon, 21 Sep 2026 14:34:46 +0200 Subject: [PATCH 09/12] fix(performance): remove fixed tree lookup cutoff --- specifyweb/backend/stored_queries/execution.py | 3 +++ specifyweb/backend/stored_queries/queryfield.py | 15 ++++++--------- .../tests/test_tree_query_performance.py | 9 +++++---- 3 files changed, 14 insertions(+), 13 deletions(-) diff --git a/specifyweb/backend/stored_queries/execution.py b/specifyweb/backend/stored_queries/execution.py index faf8df5e2ae..4f18aa32df4 100644 --- a/specifyweb/backend/stored_queries/execution.py +++ b/specifyweb/backend/stored_queries/execution.py @@ -74,6 +74,7 @@ class BuildQueryProps(NamedTuple): implicit_or: bool = True formatter_props: ObjectFormatterProps = DefaultQueryFormatterProps() optimize_for_page: bool = False + page_size: int | None = None def set_group_concat_max_len(connection): @@ -886,6 +887,7 @@ def execute( search_synonymy=search_synonymy, formatter_props=formatter_props, optimize_for_page=bool(limit) and not (count_only or distinct or series), + page_size=limit if limit and not (count_only or distinct or series) else None, ), ) @@ -1098,6 +1100,7 @@ def build_query( query, formatauditobjs=props.formatauditobjs, collection=collection, user=user, optimize_tree=optimize_tree, prefer_parent_lookup=props.optimize_for_page, + page_size=props.page_size, ) if field is None: diff --git a/specifyweb/backend/stored_queries/queryfield.py b/specifyweb/backend/stored_queries/queryfield.py index 134a12b65d3..7357ef94f94 100644 --- a/specifyweb/backend/stored_queries/queryfield.py +++ b/specifyweb/backend/stored_queries/queryfield.py @@ -8,10 +8,6 @@ logger = logging.getLogger(__name__) -# Prefer parent lookups for shallow trees; deeper trees are cheaper to -# materialize as a rank range. -MAX_PAGE_PARENT_LOOKUP_RANKS = 8 - QUREYFIELD_SORT_T = Literal[ 0, # NONE 1, # Ascending @@ -81,7 +77,7 @@ def from_spqueryfield(cls, field: EphemeralField, value: str | None=None): strict=field.isStrict, ) - def add_to_query(self, query, no_filter=False, formatauditobjs=False, collection=None, user=None, optimize_tree=True, prefer_parent_lookup=False): + def add_to_query(self, query, no_filter=False, formatauditobjs=False, collection=None, user=None, optimize_tree=True, prefer_parent_lookup=False, page_size=None): logger.info("adding field %s", self) value_required_for_filter = QueryOps.OPERATIONS[self.op_num] not in ( "op_true", # 6 @@ -116,11 +112,12 @@ def add_to_query(self, query, no_filter=False, formatauditobjs=False, collection self.op_num == 1 and self.fieldspec.get_field().name.lower() == self.fieldspec.table.idFieldName.lower() ) - if use_rank_lookup and prefer_parent_lookup and query.collection is not None: + if use_rank_lookup and prefer_parent_lookup and page_size and query.collection is not None: query, treedefs, _ = query.tree_rank_metadata(self.fieldspec.table, path[-2]) - # A bounded page can stop after a handful of parent lookups in a - # shallow tree. Materializing the entire rank delays that first page. - if max(depth for _, depth in treedefs) <= MAX_PAGE_PARENT_LOOKUP_RANKS: + # A parent walk touches one ancestor per rank for every page row. + # Build the shared rank lookup once when that walk exceeds the + # requested page's work budget. + if max(depth for _, depth in treedefs) <= page_size: use_tree_range = use_rank_lookup = False return self.fieldspec.add_to_query( diff --git a/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py b/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py index 340b49c0115..936aecc0d87 100644 --- a/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py +++ b/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py @@ -169,15 +169,15 @@ def test_geography_filter(self): self.assertIn('WITH RECURSIVE', statement) self.assertEqual(statement.count('LEFT OUTER JOIN geography '), 1) - def test_shallow_pages_keep_parent_lookups_but_deep_pages_use_rank_lookup(self): + def test_pages_choose_lookup_from_tree_depth_and_page_size(self): _, shallow_sql = self.assert_equivalent([ self.field('3.geography.name'), self.field('3.geography.Country', 1, 'USA'), - ], optimize_for_page=True) + ], optimize_for_page=True, page_size=20) self.assertNotIn('WITH RECURSIVE', shallow_sql) self.assertIn('ParentID', shallow_sql) _, deep_sql = self.assert_equivalent([ self.field('4.taxon.name'), self.field('4.taxon.Genus', 1, 'Alpha'), - ], optimize_for_page=True) + ], optimize_for_page=True, page_size=10) self.assertIn('WITH RECURSIVE', deep_sql) for depth, uses_lookup in [(8, False), (9, True)]: with self.subTest(depth=depth), patch( @@ -186,7 +186,7 @@ def test_shallow_pages_keep_parent_lookups_but_deep_pages_use_rank_lookup(self): ): _, statement = self.assert_equivalent([ self.field('4.taxon.name'), self.field('4.taxon.Genus', 1, 'Alpha'), - ], optimize_for_page=True) + ], optimize_for_page=True, page_size=8) self.assertEqual('WITH RECURSIVE' in statement, uses_lookup) def test_execute_only_prefers_parent_lookup_for_ungrouped_pages(self): @@ -209,6 +209,7 @@ def test_execute_only_prefers_parent_lookup_for_ungrouped_pages(self): execute(Mock(info={'connection': Mock()}), self.collection, self.specifyuser, 3, distinct, series, False, count_only, [], limit, 0) self.assertEqual(build.call_args.args[5].optimize_for_page, expected) + self.assertEqual(build.call_args.args[5].page_size, limit if expected else None) def test_missing_or_reversed_numbering_falls_back_without_writes(self): from specifyweb.specify.models import Taxon From b2281d15bdb61263d548dbef8d9eef18c86e59b1 Mon Sep 17 00:00:00 2001 From: Grant Fitzsimmons <37256050+grantfitzsimmons@users.noreply.github.com> Date: Mon, 21 Sep 2026 14:53:02 +0200 Subject: [PATCH 10/12] fix(performance): choose tree strategy by operator --- .../backend/stored_queries/execution.py | 6 --- .../backend/stored_queries/queryfield.py | 17 ++----- .../tests/test_tree_query_performance.py | 51 +++---------------- 3 files changed, 12 insertions(+), 62 deletions(-) diff --git a/specifyweb/backend/stored_queries/execution.py b/specifyweb/backend/stored_queries/execution.py index 4f18aa32df4..5aa80108ad8 100644 --- a/specifyweb/backend/stored_queries/execution.py +++ b/specifyweb/backend/stored_queries/execution.py @@ -73,8 +73,6 @@ class BuildQueryProps(NamedTuple): search_synonymy: bool = False implicit_or: bool = True formatter_props: ObjectFormatterProps = DefaultQueryFormatterProps() - optimize_for_page: bool = False - page_size: int | None = None def set_group_concat_max_len(connection): @@ -886,8 +884,6 @@ def execute( series=series, search_synonymy=search_synonymy, formatter_props=formatter_props, - optimize_for_page=bool(limit) and not (count_only or distinct or series), - page_size=limit if limit and not (count_only or distinct or series) else None, ), ) @@ -1099,8 +1095,6 @@ def build_query( query, field, predicate = fs.add_to_query( query, formatauditobjs=props.formatauditobjs, collection=collection, user=user, optimize_tree=optimize_tree, - prefer_parent_lookup=props.optimize_for_page, - page_size=props.page_size, ) if field is None: diff --git a/specifyweb/backend/stored_queries/queryfield.py b/specifyweb/backend/stored_queries/queryfield.py index 7357ef94f94..f6f4d2bc09a 100644 --- a/specifyweb/backend/stored_queries/queryfield.py +++ b/specifyweb/backend/stored_queries/queryfield.py @@ -77,7 +77,7 @@ def from_spqueryfield(cls, field: EphemeralField, value: str | None=None): strict=field.isStrict, ) - def add_to_query(self, query, no_filter=False, formatauditobjs=False, collection=None, user=None, optimize_tree=True, prefer_parent_lookup=False, page_size=None): + def add_to_query(self, query, no_filter=False, formatauditobjs=False, collection=None, user=None, optimize_tree=True): logger.info("adding field %s", self) value_required_for_filter = QueryOps.OPERATIONS[self.op_num] not in ( "op_true", # 6 @@ -108,17 +108,10 @@ def add_to_query(self, query, no_filter=False, formatauditobjs=False, collection and sum(isinstance(part, TreeRankQuery) for part in path) == 1 and self.fieldspec.date_part is None ) - use_rank_lookup = use_tree_range and not ( - self.op_num == 1 - and self.fieldspec.get_field().name.lower() == self.fieldspec.table.idFieldName.lower() - ) - if use_rank_lookup and prefer_parent_lookup and page_size and query.collection is not None: - query, treedefs, _ = query.tree_rank_metadata(self.fieldspec.table, path[-2]) - # A parent walk touches one ancestor per rank for every page row. - # Build the shared rank lookup once when that walk exceeds the - # requested page's work budget. - if max(depth for _, depth in treedefs) <= page_size: - use_tree_range = use_rank_lookup = False + # Exact matches can start at the matching ancestor and range over its + # descendants. Other operators share a descendant-to-rank lookup so + # they do not repeatedly walk every node's parent chain. + use_rank_lookup = use_tree_range and self.op_num != 1 return self.fieldspec.add_to_query( query, diff --git a/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py b/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py index 936aecc0d87..7a53ac1966e 100644 --- a/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py +++ b/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py @@ -138,7 +138,11 @@ def test_positive_scalar_operators(self): self.field('4.taxon.name'), self.field('4.taxon.Genus', op, value), ]) self.assertTrue(rows) - self.assertIn('WITH RECURSIVE', statement) + if op == 1: + self.assertNotIn('WITH RECURSIVE', statement) + self.assertIn('BETWEEN', statement) + else: + self.assertIn('WITH RECURSIVE', statement) self.assertEqual(statement.count('LEFT OUTER JOIN taxon '), 1) def test_missing_rank_and_negation_keep_existing_null_semantics(self): @@ -166,51 +170,10 @@ def test_geography_filter(self): self.field('3.geography.Country', 1, 'USA'), ]) self.assertTrue(rows) - self.assertIn('WITH RECURSIVE', statement) + self.assertNotIn('WITH RECURSIVE', statement) + self.assertIn('BETWEEN', statement) self.assertEqual(statement.count('LEFT OUTER JOIN geography '), 1) - def test_pages_choose_lookup_from_tree_depth_and_page_size(self): - _, shallow_sql = self.assert_equivalent([ - self.field('3.geography.name'), self.field('3.geography.Country', 1, 'USA'), - ], optimize_for_page=True, page_size=20) - self.assertNotIn('WITH RECURSIVE', shallow_sql) - self.assertIn('ParentID', shallow_sql) - _, deep_sql = self.assert_equivalent([ - self.field('4.taxon.name'), self.field('4.taxon.Genus', 1, 'Alpha'), - ], optimize_for_page=True, page_size=10) - self.assertIn('WITH RECURSIVE', deep_sql) - for depth, uses_lookup in [(8, False), (9, True)]: - with self.subTest(depth=depth), patch( - 'specifyweb.backend.stored_queries.query_construct.get_treedefs', - return_value=[(self.taxontreedef.id, depth)], - ): - _, statement = self.assert_equivalent([ - self.field('4.taxon.name'), self.field('4.taxon.Genus', 1, 'Alpha'), - ], optimize_for_page=True, page_size=8) - self.assertEqual('WITH RECURSIVE' in statement, uses_lookup) - - def test_execute_only_prefers_parent_lookup_for_ungrouped_pages(self): - from unittest.mock import Mock - from specifyweb.backend.stored_queries.execution import execute - for count_only, limit, distinct, series, expected in [ - (False, 40, False, False, True), - (True, 40, False, False, False), - (False, 0, False, False, False), - (False, None, False, False, False), - (False, 40, True, False, False), - (False, 40, False, True, False), - ]: - with self.subTest(count_only=count_only, limit=limit, distinct=distinct, series=series): - with patch('specifyweb.backend.stored_queries.execution.set_group_concat_max_len'), patch( - 'specifyweb.backend.stored_queries.execution.build_query', - side_effect=RuntimeError('query built'), - ) as build: - with self.assertRaisesMessage(RuntimeError, 'query built'): - execute(Mock(info={'connection': Mock()}), self.collection, self.specifyuser, - 3, distinct, series, False, count_only, [], limit, 0) - self.assertEqual(build.call_args.args[5].optimize_for_page, expected) - self.assertEqual(build.call_args.args[5].page_size, limit if expected else None) - def test_missing_or_reversed_numbering_falls_back_without_writes(self): from specifyweb.specify.models import Taxon for values in [dict(nodenumber=None), dict(highestchildnodenumber=None), From 5dd36697aa0fe70e6dc23ce5f25ae9da8ae186f9 Mon Sep 17 00:00:00 2001 From: Grant Fitzsimmons <37256050+grantfitzsimmons@users.noreply.github.com> Date: Mon, 21 Sep 2026 15:26:25 +0200 Subject: [PATCH 11/12] test(queries): cover removed tree ranks --- .../tests/test_tree_query_performance.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py b/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py index 7a53ac1966e..863356fb468 100644 --- a/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py +++ b/specifyweb/backend/stored_queries/tests/test_tree_query_performance.py @@ -155,6 +155,24 @@ def test_missing_rank_and_negation_keep_existing_null_semantics(self): self.assertIn(self.skipped.id, {row[0] for row in rows}) self.assertIn('ParentID', statement) + def test_removed_rank_is_skipped_without_tree_joins(self): + displayed_rank = self.field('4.taxon.Genus') + removed_rank = TreeRankQuery.create('Removed Rank', 'Taxon') + missing_field = displayed_rank._replace( + fieldspec=displayed_rank.fieldspec._replace( + join_path=(removed_rank, displayed_rank.fieldspec.join_path[-1]), + ), + ) + + rows, statement = self.run_query([ + self.field('4.taxon.name'), + missing_field, + ]) + + self.assertTrue(rows) + self.assertTrue(all(row[2] is None for row in rows)) + self.assertNotIn('LEFT OUTER JOIN taxon', statement) + def test_repeated_filters_and_multiple_ranks(self): for implicit_or in [True, False]: self.assert_equivalent([ From e090a09145245c644d35b5ce2adcaacb97398116 Mon Sep 17 00:00:00 2001 From: Grant Fitzsimmons <37256050+grantfitzsimmons@users.noreply.github.com> Date: Mon, 21 Sep 2026 15:51:26 +0200 Subject: [PATCH 12/12] fix(queries): break query field import cycle --- specifyweb/backend/stored_queries/queryfield.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/specifyweb/backend/stored_queries/queryfield.py b/specifyweb/backend/stored_queries/queryfield.py index f6f4d2bc09a..e7e625fae98 100644 --- a/specifyweb/backend/stored_queries/queryfield.py +++ b/specifyweb/backend/stored_queries/queryfield.py @@ -1,10 +1,14 @@ +from __future__ import annotations + from email.policy import strict import logging from collections import namedtuple -from typing import Any, NamedTuple, Literal +from typing import Any, NamedTuple, Literal, TYPE_CHECKING from .query_ops import QueryOps, QUERYFIELD_OPERATION_NUMBER -from .queryfieldspec import QueryFieldSpec, TreeRankQuery + +if TYPE_CHECKING: + from .queryfieldspec import QueryFieldSpec logger = logging.getLogger(__name__) @@ -58,6 +62,8 @@ class QueryField(NamedTuple): @classmethod def from_spqueryfield(cls, field: EphemeralField, value: str | None=None): + from .queryfieldspec import QueryFieldSpec + logger.info("processing field from %r", field) fieldspec = QueryFieldSpec.from_stringid( field.stringId, field.isRelFld) @@ -78,6 +84,8 @@ def from_spqueryfield(cls, field: EphemeralField, value: str | None=None): ) def add_to_query(self, query, no_filter=False, formatauditobjs=False, collection=None, user=None, optimize_tree=True): + from .queryfieldspec import TreeRankQuery + logger.info("adding field %s", self) value_required_for_filter = QueryOps.OPERATIONS[self.op_num] not in ( "op_true", # 6