Conversation
16b0067 to
ce977d7
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #431 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 108 109 +1
Lines 15606 16045 +439
Branches 812 1410 +598
==========================================
+ Hits 15606 16045 +439
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
50a20a7 to
e8b3570
Compare
4d3b6d9 to
2159470
Compare
e8b3570 to
ee53a9f
Compare
2159470 to
ec9eae3
Compare
2b437e1 to
4439b05
Compare
ab5e72b to
1ba6320
Compare
4439b05 to
9a0ae15
Compare
b7927ab to
3a3f704
Compare
3ca4856 to
3188915
Compare
6a2d709 to
28b5ab9
Compare
|
(for reviewers that are extra curious, you can see the evolution of this over time by seeing the versions on graphite. claude helped quite a lot but required a massive amount of manual intervention) |
packages/entity-database-adapter-knex/src/internal/EntityKnexDataManager.ts
Outdated
Show resolved
Hide resolved
| return sql`CASE WHEN ${SQLFragment.join(ilikeConditions, ' OR ')} THEN ${raw( | ||
| direction === PaginationDirection.FORWARD ? '1' : '0', | ||
| )} ELSE ${raw(direction === PaginationDirection.FORWARD ? '0' : '1')} END`; |
There was a problem hiding this comment.
Why does the pagination direction affect the matching score of each row? To me it seems like ASC/DESC should take care of reversing the order without flipping 1 and 0 here.
There was a problem hiding this comment.
Good catch. Technically it worked because the order was also flipped in buildTrigramExactMatchPriority based on direction, but the flip wasn't necessary. Updated and existing tests still pass.
From claude:
You're right. Look at buildTrigramExactMatchPriority (line 672): it already flips both the CASE
values and the sort direction. That's redundant — CASE WHEN ... THEN 1 ELSE 0 END DESC and CASE WHEN
... THEN 0 ELSE 1 END ASC produce the same ordering.
And buildTrigramExactMatchCaseExpression (used in the cursor comparison) also flips the values based
on direction, but the cursor comparison operator is already flipped at line 549 (< vs >).
The direction-aware value flipping is doing nothing in both places. The sort direction / comparison
operator already handles it. You should just always use 1 for match and 0 for no-match, and let
DESC/ASC and </> do the work.
There was a problem hiding this comment.
My concern was that it flipped the scores for matching but not any other sorting criteria, so the "backward" option wouldn't always be the reverse of "forward".
28b5ab9 to
89f4ba6
Compare
Merge activity
|
…#453) # Why The one downside of using id-based cursor pagination and subqueries (added in #422, #431) is that it risks the row referenced by the cursor being deleted between pagination requests. It's an edge case though to be clear. Encoding the full cursor alleviates this, but has it's own downsides since trigram similarity isn't encodable and neither are things like Date objects. It is still preferable to use ID-based cursors for all pagination. But it becomes the library's responsibility to explicitly document what the behavior is when a row referenced by the cursor is no longer present. This PR does this. # How When the cursor row is no longer present, the tuple evaluates to `NULL`, which produces a empty page. The alternative to this behavior is to run an id check query ahead of the pagination query, and throw an error if it doesn't exist, or even return the first page of data if it doesn't exist. Both of these are less optimal since they cause unexpected behavior during results consumption. So we keep the behavior as is and explicitly document it. # Test Plan Run new tests.

Why
This PR adds search capabilities to the existing cursor-based pagination system added in #422 which only supported orderBy ordering.
This adds:
This enables building user-facing search features while maintaining the benefits of cursor-based pagination (stable results, efficient queries).
How
The implementation introduces a unified PaginationSpecification that supports three strategies:
Key implementation details:
Test Plan
The PR includes comprehensive test coverage:
EnforcingKnexEntityLoader
All existing tests pass, confirming backward compatibility for standard pagination when using the new API.