Skip to content

Conversation

@sarpit2907
Copy link

Fixes #1205

Adjusts the expected row ordering in test_top_restriction_with_keywords.
The failure was caused by nondeterministic fetch ordering, not library behavior.

@github-actions github-actions bot added bug Indicates an unexpected problem or unintended behavior enhancement Indicates new improvements labels Jan 7, 2026
@dimitri-yatsenko dimitri-yatsenko self-assigned this Jan 7, 2026
@dimitri-yatsenko
Copy link
Member

This is related to #1242. The order is not deterministic. Fix in DataJoint 2.0, but we can also fix it in DataJoint 0.14.* for folks who do not upgrade right away.

@dimitri-yatsenko
Copy link
Member

This issue is addressed in DataJoint 2.0 (on the docs-2.0-migration branch).

The Fix

The test test_top_restriction_with_keywords has been rewritten to use set comparisons instead of list comparisons, making it order-independent:

def test_top_restriction_with_keywords(self, schema_simp_pop):
    # dj.Top only guarantees which elements are selected, not their order
    select = SelectPK() & dj.Top(limit=9, order_by=["select desc"])
    key = KeyPK() & dj.Top(limit=9, order_by="key desc")
    
    # Convert to sets of tuples for order-independent comparison
    select_result = {tuple(sorted(d.items())) for d in select.to_dicts()}
    select_expected = {
        tuple(sorted(d.items()))
        for d in [
            {"id": 2, "select": 8},
            {"id": 2, "select": 6},
            # ... etc
        ]
    }
    assert select_result == select_expected

Why This Approach?

When you ORDER BY key DESC, rows with the same key value can appear in any order (MySQL doesn't guarantee secondary ordering). The original test assumed a specific ordering that isn't guaranteed by SQL semantics.

Rather than swapping lines to match one particular MySQL execution, the 2.0 test correctly verifies:

  1. The correct 9 rows are selected (the top 9 by the ordering column)
  2. Without assuming any order among rows with equal values

This makes the test deterministic across different MySQL versions, configurations, and execution plans.


Suggest closing this issue as addressed in PR #1312.

@dimitri-yatsenko
Copy link
Member

Correction: This is addressed in PR #1312, not 'the branch'. The fix is part of the DataJoint 2.0 work in that PR.

@dimitri-yatsenko
Copy link
Member

Hi @sarpit2907, thank you for this contribution!

We're closing this PR because DataJoint 2.0 includes a redesigned dj.Top implementation that properly handles ordering. The new implementation:

  • Preserves order_by through query chains and preview
  • Has comprehensive test coverage including test_preview_respects_order specifically for this behavior
  • Uses a cleaner architecture with Top.merge() for combining multiple Top restrictions

The issue you addressed (#1205) is resolved by the new Top semantics in 2.0. We appreciate your effort in identifying and fixing this test ordering issue!

See PR #1312 for the 2.0 changes.

@dimitri-yatsenko
Copy link
Member

Closing in favor of DataJoint 2.0 implementation. Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Indicates an unexpected problem or unintended behavior enhancement Indicates new improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test fails in relational_operand due to test code, not functionality.

2 participants