Improve SQL Server IndexOf translation and change tests to use Where #28031
Conversation
|
OK, I see we have some duplication of testing between NorthwindWhere and NorthwindFunctions (e.g. Where_string_index_of)... Am I unaware of some reason we want to test the same function once in Where and once in top-most Select? |
No hard-line about it but as you noticed. When we want to test exact translation we do Where and when we want to test that it works on client eval we do Select. In a way doing both gives providers possibility to check out if they don't have translation available. For functions where translation is available in all providers (generally through relational layer), we just do where. |
| @@ -443,7 +443,10 @@ private SqlExpression TranslateStartsEndsWith(SqlExpression instance, SqlExpress | |||
|
|
|||
| if (startIndex is not null) | |||
| { | |||
| charIndexArguments.Add(_sqlExpressionFactory.Add(startIndex, _sqlExpressionFactory.Constant(1))); | |||
| charIndexArguments.Add( | |||
| startIndex is SqlConstantExpression { Value : int constantStartIndex } | |||
There was a problem hiding this comment.
nice, didn't know about this nested declaration.
There was a problem hiding this comment.
To avoid this kind of missing things, probably we should do this as optimization in post-translation phase (arithmetic between 2 constant operations), can you file an issue?
There was a problem hiding this comment.
Yeah pattern matching has made some nice progress :)
After the discussion in #1625, it turns out the Npgsql provider didn't implement IndexOf with starting position (see #25396), but this didn't surface because the relevant test was calling the function in the top-most projection, and not in a Where clause (so it was client-evaled).
Changed the IndexOf/Replace tests to use Where - we should avoid testing translations in top-most projections except where we specifically need to test materialization etc.
Also tweaked SQL Server IndexOf translation to be better with constants.