-
Notifications
You must be signed in to change notification settings - Fork 933
Support LINQ GroupBy subquery #2210
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
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -365,7 +365,7 @@ public override void VisitNhJoinClause(NhJoinClause joinClause, QueryModel query | |
public override void VisitResultOperator(ResultOperatorBase resultOperator, QueryModel queryModel, int index) | ||
{ | ||
PreviousEvaluationType = CurrentEvaluationType; | ||
CurrentEvaluationType = resultOperator.GetOutputDataInfo(PreviousEvaluationType); | ||
CurrentEvaluationType = GetOutputDataInfo(resultOperator, PreviousEvaluationType); | ||
|
||
if (resultOperator is ClientSideTransformOperator) | ||
{ | ||
|
@@ -382,6 +382,26 @@ public override void VisitResultOperator(ResultOperatorBase resultOperator, Quer | |
ResultOperatorMap.Process(resultOperator, this, _hqlTree); | ||
} | ||
|
||
private static IStreamedDataInfo GetOutputDataInfo(ResultOperatorBase resultOperator, IStreamedDataInfo evaluationType) | ||
{ | ||
//ContainsResultOperator contains data integrity check so for `values.Contains(x)` it checks that 'x' is proper type to be used inside 'values.Contains()' | ||
//Due to some reasons (possibly NH expression rewritings) those types might be incompatible (known case NH-3155 - group by subquery). So resultOperator.GetOutputDataInfo throws something like: | ||
//System.ArgumentException : The items of the input sequence of type 'System.Linq.IGrouping`2[System.Object[],EntityType]' are not compatible with the item expression of type 'System.Int32'. | ||
//Parameter name: inputInfo | ||
//at Remotion.Linq.Clauses.ResultOperators.ContainsResultOperator.GetOutputDataInfo(StreamedSequenceInfo inputInfo) | ||
//But in this place we don't really care about types involving inside expression, all we need to know is operation result which is bool for Contains | ||
//So let's skip possible type exception mismatch if it allows to generate proper SQL | ||
switch (resultOperator) | ||
{ | ||
case ContainsResultOperator _: | ||
case AnyResultOperator _: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
case AllResultOperator _: | ||
return new StreamedScalarValueInfo(typeof(bool)); | ||
} | ||
|
||
return resultOperator.GetOutputDataInfo(evaluationType); | ||
} | ||
|
||
public override void VisitSelectClause(SelectClause selectClause, QueryModel queryModel) | ||
{ | ||
CurrentEvaluationType = selectClause.GetOutputDataInfo(); | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
Instead of doing it here, can we fix the condition at its source?
Uh oh!
There was an error while loading. Please reload this page.
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.
I don't really understand the real source of the issue. You should know this parts much better than me. And as this issue hangs since 2012 I think it's worth to implement such simple workaround.
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.
The issue is removing
MaxResultOperator
and replacing it withNhMaxExpression
.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.
What about
Min
and others aggregate functions? It's Group By specific notMax
specific.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.
All of them.
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.
That's exactly the reason not to do it. To keep workaround in one place and it doesn't affect perf
(instead of type integrity check inside we do this
resultOperator
check)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.
If you are suggesting that changing
NhMaxExpression
and etc. will fix issue at its core and it won't be workaround than please make those changes yourself and I will close this PR. As I still don't understand what fix exactly do you have in mind.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.
I'm probably fine with the proposed change.