Skip to content

Conversation

@KurtThiemann
Copy link
Member

Currently, the TestDriver will go through all entries, add entries that match the WHERE condition to the results until the LIMIT is reached, and then sort the result. This produces incorrect results, since the matching models should be sorted before the limit is applied.

Old behavior:
[0, 1, 2, 3, 4, 5, 6, 7] -> SELECT * FROM ... LIMIT 3 ORDER BY num DESC -> [2, 1, 0]

Correct behavior:
[0, 1, 2, 3, 4, 5, 6, 7] -> SELECT * FROM ... LIMIT 3 ORDER BY num DESC -> [7, 6, 5]

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes the order of operations in the TestDriver's query execution. Previously, the driver would filter results, apply the limit, and then sort - producing incorrect results. The fix ensures results are filtered, sorted, and then limited, matching standard SQL query execution order.

Key Changes:

  • Refactored findEntries() to collect all matching entries before applying order and limit
  • Moved ordering logic inside findEntries() to execute before offset/limit application
  • Added test case to verify correct behavior with ORDER BY and LIMIT

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/Driver/Test/TestTable.php Refactored query execution to apply WHERE filter, ORDER BY sort, then OFFSET/LIMIT in correct sequence
test/tests/TestDriverTest.php Added test case validating that ORDER BY is applied before LIMIT

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@KurtThiemann KurtThiemann merged commit 84bcd0f into master Nov 26, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants