Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Internal Query: Fixes ToList work #2060

Merged
merged 3 commits into from
Dec 16, 2020
Merged

Internal Query: Fixes ToList work #2060

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ac8aca3
cleaned up epkrange comparer code in parallel query
bchong95 Dec 10, 2020
ca11901
Merge branch 'master' into users/brchon/Query/RemoveToList
bchong95 Dec 14, 2020
79b43d1
Merge branch 'master' into users/brchon/Query/RemoveToList
bchong95 Dec 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 0 additions & 43 deletions Microsoft.Azure.Cosmos/src/Query/Core/Pipeline/CrossPartition/Parallel/EpkRangeComparer.cs
View file
Open in desktop

This file was deleted.

20 changes: 14 additions & 6 deletions ...c/Query/Core/Pipeline/CrossPartition/Parallel/ParallelCrossPartitionQueryPipelineStage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,18 +83,26 @@ public async ValueTask<bool> MoveNextAsync(ITrace trace)
else
{
// left most and any non null continuations
List<FeedRangeState<QueryState>> feedRangeStates = crossPartitionState
IOrderedEnumerable<FeedRangeState<QueryState>> feedRangeStates = crossPartitionState
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add tests to prevent future regressions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't even have a reproduce, so there is not test. It also never affected correctness, so what kind of test are you looking for?

Copy link
Member

Choose a reason for hiding this comment

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

How do we know the issue is fixed then? Can't we have a test that defines the expected behavior and verifies this change is actually fixing the issue?

Copy link
Member

Choose a reason for hiding this comment

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

Or at least have a test that defines the behavior that we had before the code change to assert that the new code does not regress/change it?

Copy link
Contributor

Choose a reason for hiding this comment

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

We know that the problem container only had 2 physical partitions. It should be easy to unit test all the possible combinations with 2 items [(a,b)(b,a)(null,a)(null,null)(b,null)(a,a)], and verify they all complete in reasonable time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I enumerated the combinations. Nothing reproduced. Send a reproduce and create a work item. This PR is just to take query code out of the stack. If the high CPU is caused by doing string comparison, then we know to route to dotnet team.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the diagnostics does not include the necessary information to reproduce the issue please update it to include it. Then capture the information to reproduce the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The user didn't even have diagnostics for the reported issue. Im sure tracing would show time being spent in move next async if that were the case.

.Value
.ToArray()
.OrderBy(tuple => (FeedRangeEpk)tuple.FeedRange, EpkRangeComparer.Singleton)
.ToList();
.OrderBy(tuple => ((FeedRangeEpk)tuple.FeedRange).Range.Min);

List<ParallelContinuationToken> activeParallelContinuationTokens = new List<ParallelContinuationToken>();
for (int i = 0; i < feedRangeStates.Count; i++)
{
FeedRangeState<QueryState> firstState = feedRangeStates.First();
Copy link
Contributor

@j82w j82w Dec 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Remove the internal from the PR title. This can impact external customers.

Query: Fixes high CPU caused by LINQ orderBy logic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't even know if it fixes anything. No reproduce means no validation.

Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't make a change unless there is a reason. How do you know this will not make it worse?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's removing instructions from the compare operator thus faster.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no benchmark so how do you know it is faster?

ParallelContinuationToken firstParallelContinuationToken = new ParallelContinuationToken(
token: firstState.State != null ? ((CosmosString)firstState.State.Value).Value : null,
range: ((FeedRangeEpk)firstState.FeedRange).Range);

activeParallelContinuationTokens.Add(firstParallelContinuationToken);
}

foreach (FeedRangeState<QueryState> feedRangeState in feedRangeStates.Skip(1))
{
this.cancellationToken.ThrowIfCancellationRequested();

FeedRangeState<QueryState> feedRangeState = feedRangeStates[i];
if ((i == 0) || (feedRangeState.State != null))
if (feedRangeState.State != null)
{
ParallelContinuationToken parallelContinuationToken = new ParallelContinuationToken(
token: feedRangeState.State != null ? ((CosmosString)feedRangeState.State.Value).Value : null,
Expand Down