fix(Top): allow order_by=None to inherit existing ordering (#1242)

When chaining dj.Top restrictions, the second Top can now inherit
the ordering from the first by using order_by=None:

    (Table & dj.Top(100, "score DESC")) & dj.Top(10, order_by=None)
    # Returns top 10 of top 100, ordered by score descending

This fixes an issue where preview (to_arrays with limit) would
override user-specified ordering with the default KEY ordering.

Changes:
- Top class now accepts order_by=None to mean "inherit"
- Added Top.merge() method for combining Tops with compatible ordering
- restrict() now merges Tops when order_by=None or identical
- Creates subquery only when orderings differ

Merge behavior:
- limit = min(existing_limit, new_limit)
- offset = existing_offset + new_offset
- order_by = preserved from existing Top

Co-Authored-By: Claude Opus 4.5 <[email protected]>

Author: dimitri-yatsenko

src/datajoint/condition.py

@@ -107,32 +107,73 @@ class Top:
     ----------
     limit : int, optional
         Maximum number of rows to return. Default 1.
-    order_by : str or list[str], optional
-        Attributes to order by. ``"KEY"`` for primary key. Default ``"KEY"``.
+    order_by : str or list[str] or None, optional
+        Attributes to order by. ``"KEY"`` for primary key order.
+        ``None`` means inherit ordering from an existing Top (or default to KEY).
+        Default ``"KEY"``.
     offset : int, optional
         Number of rows to skip. Default 0.
+
+    Examples
+    --------
+    >>> query & dj.Top(5)                    # Top 5 by primary key
+    >>> query & dj.Top(10, 'score DESC')     # Top 10 by score descending
+    >>> query & dj.Top(10, order_by=None)    # Top 10, inherit existing order
+    >>> query & dj.Top(5, offset=10)         # Skip 10, take 5
     """
 
     limit: int | None = 1
-    order_by: str | list[str] = "KEY"
+    order_by: str | list[str] | None = "KEY"
     offset: int = 0
 
     def __post_init__(self) -> None:
-        self.order_by = self.order_by or ["KEY"]
         self.offset = self.offset or 0
 
         if self.limit is not None and not isinstance(self.limit, int):
             raise TypeError("Top limit must be an integer")
-        if not isinstance(self.order_by, (str, collections.abc.Sequence)) or not all(
-            isinstance(r, str) for r in self.order_by
-        ):
-            raise TypeError("Top order_by attributes must all be strings")
+        if self.order_by is not None:
+            if not isinstance(self.order_by, (str, collections.abc.Sequence)) or not all(
+                isinstance(r, str) for r in self.order_by
+            ):
+                raise TypeError("Top order_by attributes must all be strings")
+            if isinstance(self.order_by, str):
+                self.order_by = [self.order_by]
         if not isinstance(self.offset, int):
             raise TypeError("The offset argument must be an integer")
         if self.offset and self.limit is None:
             self.limit = 999999999999  # arbitrary large number to allow query
-        if isinstance(self.order_by, str):
-            self.order_by = [self.order_by]
+
+    def merge(self, other: "Top") -> "Top":
+        """
+        Merge another Top into this one (when other inherits ordering).
+
+        Used when ``other.order_by`` is None or matches ``self.order_by``.
+
+        Parameters
+        ----------
+        other : Top
+            The Top to merge. Its order_by should be None or equal to self.order_by.
+
+        Returns
+        -------
+        Top
+            New Top with merged limit/offset and preserved ordering.
+        """
+        # Compute effective limit (minimum of defined limits)
+        if self.limit is None and other.limit is None:
+            new_limit = None
+        elif self.limit is None:
+            new_limit = other.limit
+        elif other.limit is None:
+            new_limit = self.limit
+        else:
+            new_limit = min(self.limit, other.limit)
+
+        return Top(
+            limit=new_limit,
+            order_by=self.order_by,  # preserve existing ordering
+            offset=self.offset + other.offset,  # offsets add
+        )
 
 
 class Not:

src/datajoint/expression.py

@@ -132,7 +132,9 @@ def where_clause(self):
     def sorting_clauses(self):
         if not self._top:
             return ""
-        clause = ", ".join(_wrap_attributes(_flatten_attribute_list(self.primary_key, self._top.order_by)))
+        # Default to KEY ordering if order_by is None (inherit with no existing order)
+        order_by = self._top.order_by if self._top.order_by is not None else ["KEY"]
+        clause = ", ".join(_wrap_attributes(_flatten_attribute_list(self.primary_key, order_by)))
         if clause:
             clause = f" ORDER BY {clause}"
         if self._top.limit is not None:
@@ -216,10 +218,18 @@ def restrict(self, restriction, semantic_check=True):
         """
         attributes = set()
         if isinstance(restriction, Top):
-            result = (
-                self.make_subquery() if self._top and not self._top.__eq__(restriction) else copy.copy(self)
-            )  # make subquery to avoid overwriting existing Top
-            result._top = restriction
+            if self._top is None:
+                # No existing Top — apply new one directly
+                result = copy.copy(self)
+                result._top = restriction
+            elif restriction.order_by is None or restriction.order_by == self._top.order_by:
+                # Merge: new Top inherits or matches existing ordering
+                result = copy.copy(self)
+                result._top = self._top.merge(restriction)
+            else:
+                # Different ordering — need subquery
+                result = self.make_subquery()
+                result._top = restriction
             return result
         new_condition = make_condition(self, restriction, attributes, semantic_check=semantic_check)
         if new_condition is True:

tests/integration/test_relational_operand.py

@@ -627,3 +627,49 @@ def test_top_errors(self, schema_simp_pop):
         assert "TypeError: Top limit must be an integer" == str(err3.exconly())
         assert "TypeError: Top order_by attributes must all be strings" == str(err4.exconly())
         assert "TypeError: The offset argument must be an integer" == str(err5.exconly())
+
+    def test_top_inherit_order(self, schema_simp_pop):
+        """Test that dj.Top(order_by=None) inherits existing ordering."""
+        # First Top sets descending order, second Top inherits it
+        query = L() & dj.Top(10, "id_l desc") & dj.Top(3, order_by=None)
+        result = query.to_dicts()
+        assert len(result) == 3
+        # Should be top 3 by descending id_l (L has ids 0-29)
+        assert result[0]["id_l"] > result[1]["id_l"] > result[2]["id_l"]
+        assert [r["id_l"] for r in result] == [29, 28, 27]
+
+    def test_top_merge_identical_order(self, schema_simp_pop):
+        """Test that Tops with identical order_by are merged."""
+        # Both Tops specify same ordering - should merge
+        query = L() & dj.Top(10, "id_l desc") & dj.Top(5, "id_l desc")
+        result = query.to_dicts()
+        # Merged limit is min(10, 5) = 5
+        assert len(result) == 5
+        assert [r["id_l"] for r in result] == [29, 28, 27, 26, 25]
+
+    def test_top_merge_offsets_add(self, schema_simp_pop):
+        """Test that offsets are added when merging Tops."""
+        # First Top: offset 2, second Top: offset 3, inherited order
+        query = L() & dj.Top(10, "id_l desc", offset=2) & dj.Top(3, order_by=None, offset=3)
+        result = query.to_dicts()
+        # Total offset = 2 + 3 = 5, so starts at 6th element (id_l=24)
+        assert len(result) == 3
+        assert [r["id_l"] for r in result] == [24, 23, 22]
+
+    def test_preview_respects_order(self, schema_simp_pop):
+        """Test that preview (to_arrays with limit) respects Top ordering (issue #1242)."""
+        # Apply descending order with no limit (None = unlimited)
+        query = L() & dj.Top(None, order_by="id_l desc")
+        # Preview should respect the ordering (single attr returns array directly)
+        id_l = query.to_arrays("id_l", limit=5)
+        assert list(id_l) == [29, 28, 27, 26, 25]
+
+    def test_top_different_order_subquery(self, schema_simp_pop):
+        """Test that different orderings create subquery."""
+        # First Top: descending, second Top: ascending - cannot merge
+        query = L() & dj.Top(10, "id_l desc") & dj.Top(3, "id_l asc")
+        result = query.to_dicts()
+        # Second Top reorders the result of first Top
+        # First Top gives ids 29-20, second Top takes lowest 3 of those
+        assert len(result) == 3
+        assert [r["id_l"] for r in result] == [20, 21, 22]

tests/unit/test_condition.py

@@ -0,0 +1,95 @@
+"""Unit tests for condition.py - Top class and merge logic."""
+
+import pytest
+from datajoint.condition import Top
+
+
+class TestTopMerge:
+    """Tests for Top.merge() method."""
+
+    def test_merge_inherits_order(self):
+        """When other.order_by is None, ordering is inherited."""
+        top1 = Top(limit=10, order_by="score desc")
+        top2 = Top(limit=5, order_by=None)
+        merged = top1.merge(top2)
+        assert merged.order_by == ["score desc"]
+        assert merged.limit == 5
+        assert merged.offset == 0
+
+    def test_merge_limits_take_min(self):
+        """Merged limit is minimum of both."""
+        top1 = Top(limit=10, order_by="id")
+        top2 = Top(limit=3, order_by=None)
+        merged = top1.merge(top2)
+        assert merged.limit == 3
+
+        # Reverse order
+        top1 = Top(limit=3, order_by="id")
+        top2 = Top(limit=10, order_by=None)
+        merged = top1.merge(top2)
+        assert merged.limit == 3
+
+    def test_merge_none_limit_preserved(self):
+        """None limit (unlimited) is handled correctly."""
+        top1 = Top(limit=None, order_by="id")
+        top2 = Top(limit=5, order_by=None)
+        merged = top1.merge(top2)
+        assert merged.limit == 5
+
+        top1 = Top(limit=5, order_by="id")
+        top2 = Top(limit=None, order_by=None)
+        merged = top1.merge(top2)
+        assert merged.limit == 5
+
+        top1 = Top(limit=None, order_by="id")
+        top2 = Top(limit=None, order_by=None)
+        merged = top1.merge(top2)
+        assert merged.limit is None
+
+    def test_merge_offsets_add(self):
+        """Offsets are added together."""
+        top1 = Top(limit=10, order_by="id", offset=5)
+        top2 = Top(limit=3, order_by=None, offset=2)
+        merged = top1.merge(top2)
+        assert merged.offset == 7
+
+    def test_merge_preserves_existing_order(self):
+        """Merged Top preserves first Top's ordering."""
+        top1 = Top(limit=10, order_by=["col1 desc", "col2 asc"])
+        top2 = Top(limit=5, order_by=None)
+        merged = top1.merge(top2)
+        assert merged.order_by == ["col1 desc", "col2 asc"]
+
+
+class TestTopValidation:
+    """Tests for Top validation."""
+
+    def test_order_by_none_allowed(self):
+        """order_by=None is valid (means inherit)."""
+        top = Top(limit=5, order_by=None)
+        assert top.order_by is None
+
+    def test_order_by_string_converted_to_list(self):
+        """Single string order_by is converted to list."""
+        top = Top(order_by="id desc")
+        assert top.order_by == ["id desc"]
+
+    def test_order_by_list_preserved(self):
+        """List order_by is preserved."""
+        top = Top(order_by=["col1", "col2 desc"])
+        assert top.order_by == ["col1", "col2 desc"]
+
+    def test_invalid_limit_type_raises(self):
+        """Non-integer limit raises TypeError."""
+        with pytest.raises(TypeError):
+            Top(limit="5")
+
+    def test_invalid_order_by_type_raises(self):
+        """Non-string order_by raises TypeError."""
+        with pytest.raises(TypeError):
+            Top(order_by=123)
+
+    def test_invalid_offset_type_raises(self):
+        """Non-integer offset raises TypeError."""
+        with pytest.raises(TypeError):
+            Top(offset="1")