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

Query: remove nav expansion as a way to tackle the pending selector problem #32957

Open
Tracked by #31327
maumar opened this issue Jan 30, 2024 · 2 comments
Open
Tracked by #31327

Comments

@maumar
Copy link
Contributor

maumar commented Jan 30, 2024

See #20291 for additional context.

Basically the idea is to retire nav expansion as a way of dealing with navigations in our query pipeline. Instead, the work would be done in translation phase. This is good because:

  • nav expansion is located in core without means of extending it for provider specific functionality (see the hack we added for temporal tables - NavigationExpansionExtensibilityHelper),
  • during translation we have more information about the query that's being processed (e.g. we know which functions are translatable and which should be client-evaled),
  • some of the processing can potentially be streamlined, e.g we process includes right away (add JOIN and generate final client projection), rather than converting them to LeftJoin and then processing that again.

On the flipside, translation phase is already very complex, and this will add even more complexity, UNLESS some things can be simplified.
Also, nav expansion currently is is performing other tasks, e.g. reasons about primitive collections - this is because it's the only place in pre-processing where we have access to the model. So all preprocessing needing model information is performed there.

We either need to keep some pre-processing based on model where all those tasks can be performed (and maybe make it extensible for providers?) or move some of that to translation. @roji

@roji
Copy link
Member

roji commented Jan 30, 2024

Just to note that a main reason to solve the pending selector problem is performance: because we defer Select(), we end up duplicating the same SQL (e.g. subqueries) across multiple operators rather than having it just once in the query. It may be possible to keep nav expansion in its current form but to stop deferring selectors, but we know we already want to try to get rid of nav expansion as a separate phase anyway (for the above other reasons).

nav expansion is located in core without means of extending it for provider specific functionality (see the hack we added for temporal tables - NavigationExpansionExtensibilityHelper),

Examples of these include the inability to properly process ExecuteUpdate/Delete since they're relational (see #32493). Similarly, provider-specific LINQ operators such as DistinctBy (directly translatable on PostgreSQL) cannot be handled.

Finally, it's worth noting that nav expansion was written in a perf-suboptimal way: in order to avoid all visitor state, it visits the tree multiple times, first wrapping everything in "state nodes", doing its job, and then unwrapping them back again to get a normal LINQ expression tree. This adversely affects query compilation performance, which we're starting to pay a bit more attention to.

On the flipside, translation phase is already very complex, and this will add even more complexity, UNLESS some things can be simplified.

I'm personally not really worried about this - I think it's a question of factoring the logic correctly into the translation phase. In fact, I believe that the splitting up of navigation handling into the separate pre-processing phase adds much more complexity than it saves, and doing it in the right way inside translation could potentially make it much simpler. Breaking a thing into two passes really isn't necessarily a great way to make that thing simpler.

We either need to keep some pre-processing based on model where all those tasks can be performed (and maybe make it extensible for providers?) or move some of that to translation.

IMHO we should not have any sort of model awareness in preprocessing - preprocessing really should be concerned with basic normalization and operations working only the LINQ expression tree shape; all model knowledge should happen at the translation phase only. This is because of provider extensibility, and also because tracking which node corresponds to which model thing (e.g. binding properties) is very non-trivial, and we shouldn't need to do it twice (both in preprocessing and translation).

So at least in my ideal mental model, any processing that needs to be aware of the model should move to translation, just like nav expansion - compared to the complexity of nav expansion, I don't necessarily foresee a huge amount of complexity there.

One motivating factor for having nav expansion in pre-processing, was that providers get this logic for free rather than needing to implement it (e.g. conversion of enumerable LINQ operators to queryable, query filter integration...). We should try to move this universal logic to core via other means during translation (e.g. do enumerable->queryable in QueryableRelationalExpressionVisitor), rather than as a preprocessing pass as we currently do.

@maumar
Copy link
Contributor Author

maumar commented May 8, 2024

When this is done, see if #33621 is also solved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants