-
-
Notifications
You must be signed in to change notification settings - Fork 786
Fixed relative cursors to handle nullable types. #8230
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Fixed relative cursors to handle nullable types. #8230
Conversation
…ly fails to highlight a flaw in the implementation. The algorithm expects that null values are smaller than any other value when ordering.
The PR looks very good.
This is another aspect we were also looking into ... especially as with postgres null order can be configured to be at the beginning of a set or at the and of a set. I will copy over some new tests that we had in our work branch for nullable cursor values. Ping me on slack once you have solved the null ordering issue and we can see to get this one merged. |
…ll values are ordered first or last.
Ping me once you need help, want another review or are ready to merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 34 out of 34 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
src/GreenDonut/src/GreenDonut.Data.EntityFramework/Expressions/ExpressionHelpers.cs:73
- The variable 'forward' is not defined in the current scope; please define it or pass it as a parameter to ensure correct directional logic in the expression building.
var greaterThan = forward ? key.Direction == CursorKeyDirection.Ascending : key.Direction == CursorKeyDirection.Descending;
…llable keys." This reverts commit 0fba235.
There was a problem hiding this 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 enhances cursor-based pagination by introducing nullable key support and properly handling null ordering in the underlying SQL operations. Key changes include updating the serializer interfaces and implementations to support nullable types, revising the page info and formatter to propagate a new nulls-first flag, and modifying the expression builders and query extensions to correctly apply these new semantics.
Reviewed Changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/GreenDonut/src/GreenDonut.Data/Cursors/Serializers/LongCursorKeySerializer.cs | Updated to support nullable long values and return CursorKeyCompareMethod. |
src/GreenDonut/src/GreenDonut.Data/Cursors/Serializers/IntCursorKeySerializer.cs | Updated to support nullable int values and return CursorKeyCompareMethod. |
src/GreenDonut/src/GreenDonut.Data/Cursors/Serializers/ICursorKeySerializer.cs | Modified interface to include IsNullable and use CursorKeyCompareMethod. |
src/GreenDonut/src/GreenDonut.Data/Cursors/Serializers/GuidCursorKeySerializer.cs | Updated to support nullable Guid values and return CursorKeyCompareMethod. |
src/GreenDonut/src/GreenDonut.Data/Cursors/Serializers/FloatCursorKeySerializer.cs | Updated to support nullable float values and return CursorKeyCompareMethod. |
src/GreenDonut/src/GreenDonut.Data/Cursors/Serializers/DoubleCursorKeySerializer.cs | Updated to support nullable double values and return CursorKeyCompareMethod. |
src/GreenDonut/src/GreenDonut.Data/Cursors/Serializers/DecimalCursorKeySerializer.cs | Updated to support nullable decimal values and return CursorKeyCompareMethod. |
src/GreenDonut/src/GreenDonut.Data/Cursors/Serializers/DateTimeOffsetCursorKeySerializer.cs | Updated to support nullable DateTimeOffset values and return CursorKeyCompareMethod. |
src/GreenDonut/src/GreenDonut.Data/Cursors/Serializers/DateTimeCursorKeySerializer.cs | Updated to support nullable DateTime values and return CursorKeyCompareMethod. |
src/GreenDonut/src/GreenDonut.Data/Cursors/Serializers/DateOnlyCursorKeySerializer.cs | Updated to support nullable DateOnly values and return CursorKeyCompareMethod. |
src/GreenDonut/src/GreenDonut.Data/Cursors/Serializers/CompareToResolver.cs | Altered to return a CursorKeyCompareMethod with new array initialization syntax. |
src/GreenDonut/src/GreenDonut.Data/Cursors/Serializers/BoolCursorKeySerializer.cs | Updated to support nullable bool values and return CursorKeyCompareMethod. |
src/GreenDonut/src/GreenDonut.Data/Cursors/CursorParser.cs | Adjusted parsing to extract the nulls-first flag from formatted page info. |
src/GreenDonut/src/GreenDonut.Data/Cursors/CursorPageInfo.cs | Modified to include a nulls-first property throughout construction and deconstruction. |
src/GreenDonut/src/GreenDonut.Data/Cursors/CursorKeyCompareMethod.cs | New class encapsulating the compare method and its owner type. |
src/GreenDonut/src/GreenDonut.Data/Cursors/CursorKey.cs | Changed to use CursorKeyCompareMethod and added an IsNullable property. |
src/GreenDonut/src/GreenDonut.Data/Cursors/CursorFormatter.cs | Updated to format the new nulls-first flag in cursor strings. |
src/GreenDonut/src/GreenDonut.Data/Cursors/Cursor.cs | Extended cursor record to carry the nulls-first flag. |
src/GreenDonut/src/GreenDonut.Data.EntityFramework/Extensions/PagingQueryableExtensions.cs | Adjusted page creation logic to propagate and compute the nulls-first flag. |
src/GreenDonut/src/GreenDonut.Data.EntityFramework/Expressions/ExpressionHelpers.cs | Revised expression construction logic to account for nullable keys and null ordering. |
Comments suppressed due to low confidence (1)
src/GreenDonut/src/GreenDonut.Data.EntityFramework/Expressions/ExpressionHelpers.cs:73
- The variable 'forward' is used here without a prior declaration, which will result in a runtime error. Please declare and initialize 'forward' appropriately or pass it as a parameter to determine the direction.
var greaterThan = forward ? key.Direction == CursorKeyDirection.Ascending : key.Direction == CursorKeyDirection.Descending;
Hey @sleepertassly – just checking in to see if you're planning on getting this PR over the line? |
Hi @glen-84 — yes, I'm working on it. I estimate it will take about 5–6 more hours to finish, but I’m not sure when I’ll have that much free time this week or next. For the cursor to work properly across all databases, we need additional information: whether the database places nulls at the beginning or end of the sorted list. I’m now working on a solution where we explicitly force nulls to appear at either the beginning or end by adding an extra ordering condition for all nullable fields. |
@sleepertassly we gonna take over the work on this issue if its OK with you as we want to move this in one of the next patch releases. |
No problem. Tomorrow, I will clean up and push all my local changes so they’re accessible to you. |
@michaelstaib I’ve pushed my local changes. Currently, the The goal is to remove all original ordering conditions when generating the cursor keys, and then rebuild the ordering from those cursor keys after the pagination |
🧭 Paging Cursor Enhancements: Nullable Keys and Null Ordering Support
Summary
This PR introduces enhancements to the paging logic to correctly handle sorting and filtering when cursor keys are nullable. The main change is that the
WHERE
condition used in pagination must now adapt based on three factors:null
or not.🔍 Why This Is Needed
When performing cursor-based pagination on fields that can be
null
, it's essential to construct theWHERE
clause and ordering logic correctly. Incorrect handling ofnull
values can result in missing or duplicated records across pages.By explicitly controlling null ordering, we avoid relying on database-specific behavior and ensure consistent, predictable results.
🔧 Implementation Details
To support this behavior, the following changes were made:
Cursor Key Metadata
Cursor keys now include a property (
IsNullable
) to indicate whether the field is nullable.Null Ordering in Cursor
Cursors carry a
NullsFirst
flag to indicate hownull
values should be ordered.Null ordering is enforced by appending a secondary SQL clause for all nullable cursor keys:
For nulls last:
ORDER BY column IS NULL ASC
For nulls first:
ORDER BY column IS NULL DESC
Ordering Extraction and Rewriting
When cursors are constructed from the expression's
OrderBy
methods, all originalOrderBy
conditions are removed.These are later reconstructed from the cursor keys, ensuring that the resulting order is explicit and correct for the generated pagination
WHERE
clause.✅ Benefits
null
records might be skipped or duplicatednull
values in cursor logicThe use of an additional sort condition for null handling may slightly affect query performance.
However, it ensures correctness and removes ambiguity when dealing with nullable fields.
If the original expression contains an
OrderBy
that cannot be transformed into a cursor key (e.g., because it uses an unsupported expression in Green Donut), that ordering will be skipped.As a result, pagination may not follow the full original sort semantics in such cases.