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

Include Expression could not handle extra join #13092

Closed
wants to merge 1 commit into from
Closed

Include Expression could not handle extra join #13092

wants to merge 1 commit into from

Conversation

Condra963
Copy link

  • The code builds and tests pass (verified by our automated build checks)

  • Commit messages follow this format

    Summary of the changes
    If you look into following issues, Expressions: Create Navigation Property #12965 and Should we be treating objects created by the user in a query differently from the ones we materialize? #12668, you can see i want to create an extra join on
    properties that i marked for it. Everytime i do a query i want to Coalesce these column with a
    Joined Table. When i did this i ran into multiple issues, as example when you have a Navigation
    Property
    it was always filled in, EFCore saw this as an object when you use Expression.New and not
    anymore as an EntityType. The reason for this is explained in the issues and that's because they
    make use of Tracking.
    As you can see in the issue Should we be treating objects created by the user in a query differently from the ones we materialize? #12668 i asked if it is a good idea to keep creating an EntityType if
    AsNoTracking Expression is added. While i am waiting i created a work around in my code that make
    this work on EFCore.Relational level, i made an overwrite of the SelectExpression class. While i was
    creating this work around i bumped into a problem and that problem is when i add an Include
    Expression EFCore first creates a SelectExpression and than convert it to an InnerJoin. My
    modification adds to the SelectExpression of the Include Expression also a LeftOuterJoin. Because
    of this the innerSelectExpression had 2 tables instead of 1. So what i did is use the First table for the
    InnerJoin and add my LeftOuterJoin also as a Table to the outerSelectExpression.

    This solved my issue, so now i hope this could be implemented in EFCore so i can keep working with
    this final work around until there comes a better solution.

  • Tests for the changes have been added (for bug fixes / features)

  • Code meets the expectations our engineering guidelines.

@smitpatel
Copy link
Contributor

This will not work in all scenarios.
TryFlattenJoin requires both outer & inner sequence translation to be in the form of select expression without any additional tables. If there are multiple tables then the select expression needs to be pushed down prior to coming here. That is the main reason for using Single().
Adding additional tables may have worked for you but it is breaking a huge invariant in current system and can break a lot of things.
In the light of #12795 , We should not take this PR.

@Condra963
Copy link
Author

Could you give me some scenario('s) where this should not work please? Then i am able to create some unit tests and make this work properly. Because i really need a way to work this out... In the mean while i try to recognize the Include Expression and try to figure out a way to do my second join after the inner join.

Thanks for the quick feedback.

@ajcvickers
Copy link
Member

@Condra963 As @smitpatel says, we are reluctant to take this PR, but in order for us to make a better determination can you please add a test that demonstrates how the code change is useful?

@Condra963
Copy link
Author

For now i worked arround it, when i find some time i will add one or more test cases how this is useful.

@ajcvickers ajcvickers closed this Sep 4, 2018
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.

3 participants