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

Set operation support (except navigation/include) #16127

Merged
merged 1 commit into from
Jun 25, 2019
Merged

Conversation

roji
Copy link
Member

@roji roji commented Jun 17, 2019

This is a first draft for set operation support (UNION/UNION ALL/INTERSECT/EXCEPT). @smitpatel I'm looking for feedback on the general direction before I go any further etc.

Some important notes on database-specific quirks/capabilities (which have informed the design) have been posted in #6812 (comment).

Design notes

Set operations are currently represented as a special kind of SelectExpression, with an enum marker has been added (SetOperationType). Each such SelectExpression represents a single set operation with exactly two operands.

RelationalQueryableMethodTranslatingExpressionVisitor performs pushdown of set operations into a subquery on most LINQ operations, in one place. As part of this, I have refactored translation of queryable operators into their own method.

Copy link
Contributor

@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.

Need a lot more testing on combination of shapes which are not same in SelectExpression.
Also non-entity projections combined.

@roji
Copy link
Member Author

roji commented Jun 20, 2019

@smitpatel @maumar here's a new version in a much more advanced state. Various projections and hierarchy edge cases are now working, take a look at the tests.

Some notes on siblings/hierarchy:

  • Performing a set operation over siblings is complicated (especially when there are other unrelated siblings). Because the result shape needs to be identical on both sides, I generate null constant projections for properties of the other entity type.
  • More complicated; the entity type coming out of the operation is (currently) the closest common ancestor. In order for that to work I generate null constant projections even for properties which neither sibling actually requires (the shaper doesn't know only sibling instances will be coming from the database).
  • Usually projections are duplicated (why load the same thing twice), but we can't deduplicate nulls, since their point is to make the query shape identical on both sides.
  • Most importantly, since the set operation changes the result shape (from sibling to common ancestor), RelationalQueryableMethodTranslatingExpressionVisitor.TranslateSetOperation() sets a new ShapedQueryExpression (a bit like CombineShapers for join). This feels hacky/wrong, please provide feedback...

Some other notes:

  • The core logic is SelectExpression.ApplySetOperation.
  • Factoring of QueryableMethodTranslatingExpressionVisitor and SelectExpression, i.e. how/where to implement the pushdown of set operations.
  • Have scoped out all nav rewriting support (but intend to work on that as soon as this is done).

@roji
Copy link
Member Author

roji commented Jun 24, 2019

@smitpatel, regarding application of set operations on SelectExpression with already-populated projections (this is the corresponding code in PushdownIntoSubquery), I couldn't repro this with the test below. For now I will simply place a NotImplementedExpression, we can always do this in a separate PR.

public virtual Task Union_client_eval_FirstOrDefault(bool isAsync)
    => AssertFirstOrDefault<Customer>(isAsync, cs => cs
        .Where(c => c.City == "Berlin")
        .Union(cs.Where(c => c.City == "London"))
        .OrderBy(c => c.Address)
        .Select(c => Dummy.GetCustomerAddress(c)));

@roji roji marked this pull request as ready for review June 24, 2019 21:17
@roji
Copy link
Member Author

roji commented Jun 24, 2019

OK guys, this seems more or less ready to me. It doesn't contain navigation/include support; @maumar if you want to take a look at this, go ahead, otherwise tomorrow morning I'll be looking at it - please let me know if you're on it so we don't do double work.

Hopefully we can get this in for preview7...

@roji roji changed the title Draft work on set operation support Set operation support (except navigation/include) Jun 24, 2019
@@ -79,6 +80,28 @@ protected override Expression VisitSelect(SelectExpression selectExpression)
subQueryIndent = _relationalCommandBuilder.Indent();
}

if (selectExpression.IsSetOperation)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are not doing a lot of process in between, just use using syntax for Indent

Copy link
Member Author

Choose a reason for hiding this comment

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

We only want to indent for subqueries (when alias != null)...

Navigation/include support not included.

Fixes #6812
Fixes #12549
@roji roji merged commit 16c9a09 into master Jun 25, 2019
@ghost ghost deleted the StateOfTheUnion branch June 25, 2019 11:06
@roji
Copy link
Member Author

roji commented Jun 25, 2019

@smitpatel merged although there are some open questions, in order to make sure this gets into preview7. Let's continue the conversations on this issue and I'll fix as necessary.

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.

5 participants