DefaultQuerySqlGenerator not flexible enough for provider override

[roji]

As part of #9126, I set about adding a PostgreSQL-specific ILikeExpression and modifying NpgsqlQuerySqlGenerator to render it correctly. I hit several limitations with DefaultQuerySqlGenerator not exposing enough functionality to subclasses:

• _typeMapping is private, and so I can't set it properly (as VisitLike() does)
• isSearchCondition is a private static method, so there's no way to introduce a new expression type that is identified as a search condition. This means that a useless = TRUE is rendered out, and may have other consequences I'm not aware of as well.

I can submit PRs for these two specific problems, but I've run into similar issues previously, so you guys may want to do a proper review of DefaultQuerySqlGenerator keeping providers in mind.

[divega]

@roji Thanks for reporting this.

I can submit PRs for these two specific problems, but I've run into similar issues previously, so you guys may want to do a proper review of DefaultQuerySqlGenerator keeping providers in mind.

Our general approach is intentionally to avoid overdesigning the provider model and wait for feedback like yours to help us triangulate where the extensibility and additional flexibility is needed. This obviously works much better if we have time to react to feedback like this... So the second point from my list of regrets at #9126 (comment) applies here too.

Fortunately in this case I believe we can take changes like the ones you suggest later, e.g. in 2.1, without them being breaking changes. So it would be great if you can send a PR.

I also hope you can either do something that works (even if it is suboptimal) for 2.0 or wait until 2.1 to add the new Like variant.

[smitpatel]

1. We should expose type mapping allowing providers to set it. Not sure if access as property or functions.
2. There should not be side effect other than extra = True. Though the whole class BooleanConditionTranslatingExpressionVisitor is private. There is no way for providers to influence it at present. For custom nodes, providers can override VisitChildren but at present it allows no control over if Child are supposed to be condition or not. And there is no way to extend the set of condition nodes as you figured. At one point @anpete & me were thinking of a base class for all the custom expression defined in EF (& providers) such a base class would allow to add properties like that which we can directly evaluate. Give us some time to discuss 2nd point in team. The harm of that would be none apart from bit longer SQL.

[roji]

Thanks (as always) for your reactions and for caring :)

The good news on this is that the lacking extensibility in DefaultSqlQueryGenerator doesn't have a big on me at the moment - the only tangible problem is the extra = TRUE (which is really unimportant). And in addition, it seems possible to open up more aspects for 2.1 without breaking any compatibility, so there's no big problem here.

@smitpatel no problem, I'll let you guys take a look at the two issues and fix them as you see fit. If you prefer to receive a PR from me on one of them (or both) let me know.