Add plugin support to EvaluatableExpressionFilter

[roji]

2.2 is introducing plugins, which can extend type mappings and method/member translations from outside EF Core. I'm currently porting Npgsql's own plugin model (introduced in 2.1) to the new EF Core one (npgsql/efcore.pg#658), and one missing extension point is for EvaluatableExpressionFilter: our NodaTime plugin flags SystemClock.Instance.GetCurrentInstance() as non-evaluatable, to make sure it's evaluated at the server (much like DateTime.Now).

It seems important for this to make it into 2.2, otherwise NodaTime specifically won't be portable to the new plugin model (and this may also affect other plugins). I'll probably be able to submit a PR in about 2-3 weeks, is that early enough?

[bricelam]

On a related note, I've wanted hooks for the following in my TimeSpan on SQLite extension:

• SqlTranslatingExpressionVisitor
  • VisitUnary
  • VisitBinary
• RelationalConnection
  • Open
  • OpenAsync

[ajcvickers]

@roji 2-3 weeks won't make it for preview3, but it should be good for RTM. This doesn't sound like it will be a risky change, so I think we'll be good taking it directly into RTM. (There is no RC for 2.2; rightly or not, we're going straight from preview3 to RTM.)

[ajcvickers]

@smitpatel and @bricelam to follow up with @roji whether this makes sense for 2.2 given changes coming in 3.0.

[smitpatel]

@roji - Is this required to be in 2.2 to support everything for npgsql? In new query pipeline, the structure of this would be changing especially current funcletizer depends on Relinq so any work here could become throw-away work. If it is necessary then we can do it for 2.2

[roji]

I'm just completing npgsql/efcore.pg#672, which redoes Npgsql's plugins to use the new EF Core plugin model introduced in 2.2. The only place where specifying evaluatable was important is in NodaTime's SystemClock.GetCurrentInstant() (similar to DateTime.UtcNow, but for now I hacked around the lack of support by checking for this method in Npgsql's own EvaluatableExpressionFilter (the provider's, not in NodaTime plugin), using string checks to avoid taking a reference on NodaTime. This is an ugly workaround but is totally fine.

Long story short, there's no critical need for this in 2.2 - if things are radically changing in 3.0 we can forget it.

[smitpatel]

@roji - Do we still need this in new pipeline?

[roji]

@smitpatel if I understand correctly, methods/properties like DateTime.UtcNow will now always be translated (if translatable) so I don't see any further need....

[roji]

@smitpatel unless I'm mistaken this is actually needed even in the new pipeline, to make sure that ParameterExtractingExpressionVisitor doesn't evaluate invocations of methods and embed the result as a constant?