Repository navigation
fix(query): emit DISTINCT only where rows can repeat; keep it through composition - #1662
Open
dimitri-yatsenko wants to merge 1 commit into
Open
dimitri-yatsenko wants to merge 1 commit into
dimitri-yatsenko wants to merge 1 commit into
Conversation
… composition Every query has a primary key that identifies its rows, so deduplication is needed only where dj.U(...) & expr projects onto attributes that do not include expr's primary key, and in the union of key sets that may overlap. - len() counts rows with count(*) unless the query deduplicates; counting distinct primary keys was redundant and forced a DISTINCT pass. - Aggregations no longer add DISTINCT when the heading equals the primary key: GROUP BY already yields one row per key. DISTINCT is emitted only for dj.U(...) & aggregation. - The outer union (operands with secondary attributes) uses UNION ALL: its branches, arg1 LEFT JOIN arg2 and arg2 - arg1, are disjoint. - dj.U(...) & expr skips DISTINCT when U's attributes include expr's primary key. Where DISTINCT is needed, it now survives composition: - join, extend and aggr wrap a dj.U(...) & expr operand in a subquery, so it contributes one row per key instead of one row per underlying row (#1660). len() previously masked this by counting distinct keys. - dj.U(...).aggr deduplicates a dj.U(...) & expr group before GROUP BY. - dj.U(...) & union sets DISTINCT after proj(), which wraps the union in a subquery; it was previously lost inside the union. Fixes #1660
dimitri-yatsenko
force-pushed
the
fix/redundant-distinct
branch
from
November 7, 2026 23:52
7cb7f81 to
3f67417
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1660.
Background
Every DataJoint query has a primary key that identifies its rows. The SQL therefore needs deduplication in only two places:
dj.U(...) & expr, when U's attributes do not includeexpr's primary key.Everywhere else,
DISTINCT(orUNIONwithoutALL) only adds work for the database. WhereDISTINCTis needed, some operators dropped it when the expression was used inside another query.Redundant deduplication removed
len()counts rows withcount(*)unless the query deduplicates. It used to count distinct primary keys, which for multi-column keys meantcount(*) FROM (SELECT DISTINCT pk ...)on every call.DISTINCTwhen the heading equals the primary key, becauseGROUP BYon that key already returns one row per key.DISTINCTis now emitted only fordj.U(...) & aggregation, driven by_distinctinstead of theheading == PKheuristic, which had been doing that job implicitly.UNION ALL. Its branches,arg1 LEFT JOIN arg2andarg2 - arg1, are disjoint.dj.U(...) & exprskipsDISTINCTwhen U's attributes includeexpr's primary key.Deduplication kept through composition
join(and so*,extend,aggr) wraps adj.U(...) & exproperand in a subquery, so it contributes one row per key. It used to be joined as the underlying table, which gave repeated rows and inflatedaggrcounts.len()hid this by counting distinct keys. Without this fix, thelen()change above would have exposed the wrong count inlen()too.dj.U(...).aggrdeduplicates adj.U(...) & exprgroup beforeGROUP BY.dj.U(...) & unionsetsDISTINCTafterproj().proj()wraps a union in a subquery, and the flag used to be set on the inner copy, whereUnion.make_sqlignores it, so duplicates came through.Why it matters for performance
Measured outside the test suite on MySQL 8.0/8.4 and PostgreSQL 15/18:
UNIONhashes every column, blobs included, and the planner cannot drop unused columns under it. WithUNION ALL, the planner flattens the union, drops unused columns, and removes the redundant left join: 2.5 ms instead of 167 ms on a blob table.DISTINCTprevents MySQL from merging a derived table into the outer query and PostgreSQL from dropping unused columns. Removing the redundant ones avoids both problems.Tests
tests/integration/test_distinct.pyhas 10 tests, each run on MySQL and PostgreSQL. They cover:DISTINCTandUNION ALLappear in the generated SQL only where needed.len()issues noDISTINCTfor non-deduplicating queries.len()equals the number of fetched rows across restriction, anti-restriction, join,extend,proj,dj.Top, union anddj.U.dj.Uoperands inside joins,extend,aggranddj.U(...).aggr.dj.U(...) & aggregationanddj.U(...) & union.On
master, 16 of the 20 runs fail. With this change all pass. The full suite passes locally on both backends; the only failures are the diagram-rendering tests, which need graphviz'sdotprogram and fail identically onmaster.Overlap with other v2.3.7 PRs
These PRs edit the same lines in
expression.py. Whichever merges second needs small conflict fixes:__len__and rewritesAggregation.make_sql. Keep itsGROUP BY/HAVINGrestructuring and usedistinct = self._distinct._topoperands injoin,U.__and__andU.aggr, next to the_distinctchecks added here. It also adds a secondUNIONfor a limited outer union, which should becomeUNION ALLas well.Behavior change
dj.U(...) & exproperand return one row per key instead of repeated rows.aggrover such a group counts each key once.dj.U(...) & unionreturns distinct rows.All three match the documented semantics.