Add extensibility point for IQueryOptimizer

[austindrenski]

This work supports #431 by providing extensibility points for query rewriting expression visitors.

[austindrenski]

@roji this is ready for review.

[roji]

@austindrenski the general approach and goals look good. However, the two services you're replacing (IExpressionFragmentTranslator, IQueryOptimizer) are from an EF Core internal namespace, which is something we should be wary of doing.

How about submitting an issue/PR to EF Core itself, for moving these services out of Internal and providing the extensibility point? If you explain the Npgsql needs clearly I don't think they'll object to it. Many services were defined as Internal because they didn't want to commit to a stable interface for them early on, but it seems that things have matured enough for these to be nailed down.

Let me know what you think, and please cc me from any issue/PR.

[roji]

However, before working on an EF Core issue/PR, it seems like we need to decide whether the null element discrepancy (described here) prevents us from translating constructs such as array1.All(x => array2.Contains(x)) altogether, in which case this PR would have no real use.

[roji]

@austindrenski is this still actually useful for anything?

[austindrenski]

@roji Ehh.. It's on the 3.0 milestone for EF Core, but that's a long ways off still. I'll close this, and reopen in the future if the upstream changes are made.

[roji]

@austindrenski to be clear, I'm actually not 100% against us overriding internal EF Core types - I've done this several times in the past (not sure if there are any currently). The only consequence is having to be reactive to EF Core minor version releases, since breakage may be introduced to these types. But as a general rule we've always released in lock-step with EF Core anyway, so it's not a big deal.

The question is whether there's any value in this, in light of the null behavior discrepancy of array1.All(x => array2.Contains(x)). If there's real value I'm fine with providing our own implementation of IQueryOptimizer, otherwise we can forget about it...

[austindrenski]

The question is whether there's any value in this, in light of the null behavior discrepancy of array1.All(x => array2.Contains(x)). If there's real value I'm fine with providing our own implementation of IQueryOptimizer, otherwise we can forget about it...

So right now, I don't think it adds any value...

As for the null discrepancy, I went ahead and dropped the translation of array1.All(x => array2.Contains(x)) in favor of EF.Functions.Contains(array1, array2).

I think it would still be cool to translate the former if the discrepancy could be handled through the UseRelationalNulls style offsetting. But, I haven't had time to look into whether the array operators could be offset in the same way.

The only consequence is having to be reactive to EF Core minor version releases, since breakage may be introduced to these types. But as a general rule we've always released in lock-step with EF Core anyway, so it's not a big deal.

As mentioned in #541, there are some functions for which translation will involve sub-query manipulation. (The added complexity is why I'm splitting them into another PR.)

I've got a feeling that IQueryOptimizer might tame some of that complexity, but I'm okay waiting to introduce an implementation until I've had some time to look specifically at unnest and array_agg.

Since there could be a cost to introducing it (e.g. minor version breakages), we might as well wait until there's a well-defined use case for it.

[roji]

Makes sense, thanks!