Repository navigation
fix: dj.Top is honored when a limited expression is used in another query - #1657
Conversation
…uery An expression restricted by dj.Top keeps its ORDER BY/LIMIT in _top, and several operations ignored it or broke on it: - join() joined a limited operand directly, on either side, so the limit was dropped from *, aggr, and extend. A limited operand is now a subquery. - Restriction by a limited expression put the LIMIT inside IN (...), which MySQL rejects (error 1235). The expression is now a subquery there. - dj.U.aggr ignored the group's limit, and dj.U & expr applied DISTINCT before the limit. Both now apply the limit first. - Union put its ORDER BY/LIMIT inside the quoted alias name (a syntax error), and ignored it for operands with secondary attributes. - proj() kept the limit while dropping or redefining an attribute its ORDER BY names, so keys() and similar calls failed or selected other rows. It now adds a subquery in exactly those cases. - Top.merge let a second Top with an offset reach past the first Top's limit. - bool() ignored the limit. - Diagram.restrict() ignored a dj.Top on its seed; it now builds the seed as cascade and trace do. - A subquery heading dropped table_info, so a later rename quoted with backticks on PostgreSQL. dj.Top still cannot be negated or nested in a list or AndList; the error now says so and names the complement, dj.Top(None, order_by=..., offset=n). Fixes #1656.
MilagrosMarin
left a comment
There was a problem hiding this comment.
Thanks @dimitri-yatsenko, this is a careful fix, and the table of reproductions made it easy to check. I read each change in context:
✅ join() / make_condition / dj.U: a limited operand becomes a subquery, and the wrapped expression carries no _top, so the limit can't leak outward
✅ Top.merge: checked the window arithmetic, including the offset-only placeholder limit and the remaining == 0 case
✅ proj(): subquery only when the ORDER BY names a redefined or dropped computed attribute; KEY ordering after a key rename still resolves on both backends
✅ make_subquery_heading keeping table_info: it doesn't trigger extra lookups (table_status is only read for real tables), and select() already copies table_info the same way
A few notes, none blocking:
- Docs follow-up:
datajoint-docssrc/reference/specs/query-algebra.md§12.5 still says the mergedlimitis "minimum of both limits". The examples there still hold, but the rule should read "the second Top's window within the first". The newdj.U & (expr & dj.Top(...))meaning and the "no nested/negated Top" rule should probably go in §12 andreference/operators.mdtoo. Happy to open that docs PR. Diagram.restrict(): besidesdj.Top, any seed restricted by another query is now read into keys, ascascadedoes. Worth a line in the v2.3.7 notes, since a very large seed becomes a largeINlist.- Nit:
proj()buildsself.heading.select(...)twice (once for the check, once for the result); it could be built once and reused.
Approving. Let me know if you'd like me to take the docs update!
|
Thanks @MilagrosMarin.
I also added #1666 to the issues this closes; it reports the same bug as #1656. GitHub Actions has no run for this branch (no workflows ran between Oct 31 and Nov 9). Merged onto current |
Fixes #1656. Fixes #1666 (a duplicate of #1656).
The bug
An expression restricted by
dj.Topkeeps itsORDER BY/LIMITin_top, separately from itsWHEREconditions. Several operations that take an expression as an operand ignored_top, or broke on it. They returned too many rows without a warning, or failed with a SQL error. All of the following reproduce onmasteron MySQL 8.0 and PostgreSQL.top_childselects one child, andtop_parentselects parent 1.Parent * (Child & top_child),(Parent & top_parent) * Child, both, ordj.Toptogether with a conditionParent.aggr(Child & top_child, ..., exclude_nonmatching=True),(Parent & top_parent).aggr(Child, ...),(Child & top_child).extend(Parent)dj.U("color").aggr(Child & top_child, ...)Parent & (Child & top_child),Parent - (Child & top_child)LIMIT & INsubquery); PostgreSQL: correctdj.U("parent_id") & (Child & dj.Top(2, ...))DISTINCT/ORDER BYerror when the order is on another attribute; otherwise the limit is applied afterDISTINCT(A + B) & dj.Top(...)ORDER BY … LIMITsat inside the quoted alias name(Parent & top_parent) + (ParentInfo & dj.Top(...))(Parent.aggr(Child, n="count(*)") & dj.Top(1, order_by="n DESC")).keys()Unknown column 'n'(proj()dropped the ordering attribute)(Parent & dj.Top(1, order_by="label DESC")).proj(label="-parent_id")ORDER BY labelbound to the new column(Parent & dj.Top(2)) & dj.Top(None, offset=1)and.to_dicts(limit=5, offset=1)Topbool(Parent & dj.Top(None, order_by="parent_id", offset=3))Truefor an empty resultdj.Diagram(schema).restrict(Parent & top_parent)Parentand its descendantsThe fix
The rule is the one #1650 applies to a restricted left-join operand: a limited operand is wrapped in a subquery, so its
ORDER BY/LIMITapplies to that operand on its own.join(): an operand with_top, on either side, is a subquery. This covers*,aggr(inner and left), andextend.make_condition: a limited restricting expression is a subquery:IN (SELECT a FROM (SELECT … ORDER BY … LIMIT n) AS t). This is the usual MySQL workaround for 1235, and it applies the limit before the projection to the common attributes. That projection matters on PostgreSQL too, for example when the order is on a computed attribute.dj.U & expranddj.U.aggr(group): a limited operand is a subquery, so the limit applies beforeDISTINCT/GROUP BY.Union.make_sql:sorting_clauses()moves out of the alias name and follows it. The branch for operands with secondary attributes is wrapped the same way when the union is limited.proj(): adds a subquery only when theORDER BYnames an attribute that the projection redefines (a rename or computation into that name) or a computed/renamed attribute that it drops. Dropping a plain table column needs no subquery, becauseORDER BYon an unselected column is valid. Sofetch(attr, order_by=..., limit=...)keeps its ordering at the outer level.Top.merge: the secondTopskipsother.offsetrows of the firstTop's window and takes up toother.limit, never past the firstTop's limit. Previouslymin(limit1, limit2)with added offsets could reach past it.QueryExpression.__bool__: uses a subquery when_topis set, as__len__already does.Diagram.restrict(): builds its seed with_seed_restriction(), ascascadeandtracedo. Adj.Topseed, or a seed restricted by another query, becomes the primary keys it selects, and%in seed conditions is no longer escaped twice when the conditions are re-applied.Heading.make_subquery_heading(): keepstable_info, so a rename after a subquery quotes for the backend. Without it, PostgreSQL got backticks. This is the same fix fix(heading): keep table_info in set_primary_key so a later rename quotes for the backend #1643 makes inset_primary_key, and it was needed onceproj()started wrapping.A - dj.Top()stays unsupported, as discussed in #1656. A negated or nestedTop(-,dj.Not, a list, anAndList, orpopulate()/progress()restrictions) now raises:TestDjTop::test_top_errorspreviously pinned the old "Invalid restriction type Top(...)" text and is updated.Behavior changes (for the v2.3.7 notes)
All of these bring results in line with what the queries say:
dj.U(attrs) & (expr & dj.Top(n, ...))limitsexprfirst and then takes distinct values. The previous "first n distinct values" is written(dj.U(attrs) & expr) & dj.Top(n, ...).Tops andto_dicts(limit=…, offset=…)on a limited expression no longer return rows past the first limit.Diagram.restrict()seeds the same way ascascade.Tests
tests/integration/test_top_in_queries.py: 49 cases × MySQL and PostgreSQL.offsetform, and a projected operand.extendand defaultaggr, with the limit on either side.aggranddj.U.aggr, with bothTopand theoffsetform.Top,Top(2), theoffsetform, the order on a computed attribute, and a limited union.dj.Urestriction: the order on a dropped attribute and on a kept one.Top, and of limited operands, with and without secondary attributes.proj()of a limited expression:keys()of a limited aggregate, a dropped computed ordering attribute, an ordering attribute recomputed or swapped by a rename, and a dropped table column, which needs no subquery.bool()andDiagram.restrict().Toperror: five forms.dj.U &anddj.U.aggr.tests/unit/test_condition.py:Top.mergewindows.table_infochange shows only on PostgreSQL.Interaction with other PRs
This branch merges cleanly with #1655, #1643, #1653 and #1654 (checked with
git merge-tree). After #1655 merges, itsor other._topin the left-join condition is redundant with the general rule here, but harmless.Not changed here
These turned up in review. They are not about
dj.Topand predate this PR:Union.make_sql()takes nofieldsargument, butmake_conditionandTable.insertpass one. So restricting by, or inserting from, an unlimited union raisesTypeError. A limited union works after this PR, because it goes through a subquery.dj.U.aggrover aUnionor anAggregationgroup uses the group'ssupport/restrictiondirectly, dropping the second union operand or the innerGROUP BY.join()checksisinstance(self, Union)where it meansotherfor the right operand.dj.U & …restriction can return duplicate rows, because the join ignores_distinct.