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.

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

Update single/split query docs #2606

Merged
merged 1 commit into from
Sep 5, 2020
Merged

Update single/split query docs #2606

merged 1 commit into from
Sep 5, 2020

Conversation

roji
Copy link
Member

@roji roji commented Sep 3, 2020

  • Document the new global split query configuration and AsSingleQuery
  • Refactoring of the section for better readability and some updates

Continues #2479

@roji roji marked this pull request as ready for review September 3, 2020 15:27
@roji roji requested a review from a team September 3, 2020 15:27
@roji roji requested a review from smitpatel September 4, 2020 06:57
[!code-csharp[Main](../../../samples/core/Querying/RelatedData/Sample.cs?name=AsSplitQuery&highlight=5)]
[!code-csharp[Main](../../../samples/core/Querying/RelatedData/Sample.cs?name=AsSplitQuery&highlight=5)]

By default, EF Core emits a warning if it detects single queries loading collection includes that may cause a performance issue. If you're confident that there are no related performance issue, you can explicitly configure the splitting behavior to SplitQuery to prevent these warnings from being generated.
Copy link
Member

Choose a reason for hiding this comment

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

Warning is not working that way.
We issue a warning if behavior is not specified globally or in the query using operator.
No warning if user explicitly asks for AsSingleQuery.
And to suppress a warning, we need to show the warning configuration in OnConfiguring.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, wrote SplitQuery instead of SingleQuery. Is it OK after this fix? If not do you want to make a suggestion?

Copy link
Member

Choose a reason for hiding this comment

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

This still does not clarify what is the warning, why it is issued in first place and how to suppress the warning.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've reworded the note, hopefully you'll find it better. If not, please make another suggestion.

And to suppress a warning, we need to show the warning configuration in OnConfiguring.

I don't think it's necessary to show multiple ways to make the warning go away (set SingleQuery globally/on the query, warning config in OnConfiguring), from the user's perspective it's the same. In addition, configuring warnings in OnConfiguring is a global matter that should be documented elsewhere, rather than repeating this information everywhere where a warning is discussed.

Copy link
Member

Choose a reason for hiding this comment

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

You can add a global docs somewhere about how to suppress warnings. The unrepeated part which is most crucial for user is what EventId to use to suppress it and that is going to be varying between different scenarios.
Also setting SingleQuery mode is not the (right or only) way to suppress the warning.

I will update this page myself later. You can merge this.

Copy link
Member Author

Choose a reason for hiding this comment

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

You can add a global docs somewhere about how to suppress warnings

Opened #2615 to track.

The unrepeated part which is most crucial for user is what EventId to use to suppress it and that is going to be varying between different scenarios

Shouldn't the EventIds be discoverable from the warning itself, rather than having to make the docs more verbose/heavy with them? Typically a user will want to suppress a warning after they see it in the log, that should contain the EventId etc.

Also setting SingleQuery mode is not the (right or only) way to suppress the warning.

Looking forward to seeing what you propose.

@@ -63,7 +62,12 @@ LEFT JOIN [Post] AS [p] ON [b].[BlogId] = [p].[BlogId]
ORDER BY [b].[BlogId], [p].[PostId]
```

If a typical blog has multiple related posts, rows for these posts will duplicate the blog's information, leading to the so-called "cartesian explosion" problem. As more one-to-many relationships are loaded, the amount of duplicated data may grow and adversely affect the performance of your application.
If a typical blog has multiple related posts, rows for these posts will duplicate the blog's information, leading to the so-called "cartesian explosion" problem. As more one-to-many relationships are loaded, the amount of duplicated data may grow and adversely affect the performance of your application. By default, EF Core emits a warning if it detects queries loading collection includes that may cause performance issues.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If a typical blog has multiple related posts, rows for these posts will duplicate the blog's information, leading to the so-called "cartesian explosion" problem. As more one-to-many relationships are loaded, the amount of duplicated data may grow and adversely affect the performance of your application. By default, EF Core emits a warning if it detects queries loading collection includes that may cause performance issues.
If a typical blog has multiple related posts, rows for these posts will duplicate the blog's information, leading to the so-called "cartesian explosion" problem. As more one-to-many relationships are loaded, the amount of duplicated data may grow and adversely affect the performance of your application.

Copy link
Member

@smitpatel smitpatel left a comment

Choose a reason for hiding this comment

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

Too much to and fro, I will update it later.

* Document the new global split query configuration and AsSingleQuery
* Refactoring of the section for better readability and some updates

Continues #2479
@roji roji merged commit c0a6cbc into master Sep 5, 2020
@smitpatel smitpatel deleted the SplitQueriesEverywhere branch September 5, 2020 22:00
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.

2 participants