Skip to content

Conversation

@Maya-Painter
Copy link
Contributor

@Maya-Painter Maya-Painter commented May 29, 2025

Description

Addresses a bug where the top value was being set > int.Max for queries with ORDER BY and SKIP clauses. This triggered an assert and caused query failures.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Closing issues

closes #5016

@adityasa
Copy link
Contributor

            Debug.Assert(queryInfo.Top.Value <= int.MaxValue, "PipelineFactory Assert!", "Top value must be <= int.MaxValue");

Do we need to repeat the original check here? What happens if query is missing ORDER BY but has only TOP (did we have coverage for that case in the original change)?


Refers to: Microsoft.Azure.Cosmos/src/Query/Core/Pipeline/PipelineFactory.cs:299 in 4d983f8. [](commit_id = 4d983f8, deletion_comment = False)

@Maya-Painter
Copy link
Contributor Author

            Debug.Assert(queryInfo.Top.Value <= int.MaxValue, "PipelineFactory Assert!", "Top value must be <= int.MaxValue");

Do we need to repeat the original check here? What happens if query is missing ORDER BY but has only TOP (did we have coverage for that case in the original change)?

Refers to: Microsoft.Azure.Cosmos/src/Query/Core/Pipeline/PipelineFactory.cs:299 in 4d983f8. [](commit_id = 4d983f8, deletion_comment = False)

With both SKIP + ORDER BY and just SKIP queryInfo.HasTop is set to false, so this assert is never hit.

@Maya-Painter Maya-Painter marked this pull request as ready for review May 30, 2025 21:53
@adityasa
Copy link
Contributor

adityasa commented Jun 3, 2025

Consider adding coverage to NegativeQueryTests::TestTopOffsetLimitClientRanges for ORDER BY + OFFSET + LIMIT


In reply to: 2923582595

adityasa
adityasa previously approved these changes Jun 3, 2025
Copy link
Contributor

@adityasa adityasa left a comment

Choose a reason for hiding this comment

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

:shipit:

@Maya-Painter Maya-Painter added the auto-merge Enables automation to merge PRs label Jun 3, 2025
Copy link
Contributor

@adityasa adityasa left a comment

Choose a reason for hiding this comment

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

:shipit:

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

Labels

auto-merge Enables automation to merge PRs LINQ

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Regression in LINQ Skip in 3.47.0

5 participants