Infer type mapping suitable for concatenating two results #32510
Conversation
There was a problem hiding this comment.
Looks good as a targeted fix specifically for #32325 (see comments).
The question in my mind is whether we should also try to handle other (binary?) operators, which still have the simple left-mapping-wins behavior. I'm not sure there's something relevant for string, but stuff like math on decimals with varying precision/scale...
Of course, we can wait and see.
test/EFCore.SqlServer.FunctionalTests/Query/CompositeKeysQuerySqlServerTest.cs
Outdated
Show resolved
Hide resolved
|
|
||
| [ConditionalTheory] | ||
| [MemberData(nameof(IsAsyncData))] | ||
| public virtual Task Contains_over_concatenated_column_and_constant(bool async) |
There was a problem hiding this comment.
For completeness, would be good to also have tests for column+parameter, and also parameter+constant (these are evaluated as a single parameter in regular queries, but the tree is preserved for compiled queries).
| public class Scrathcy | ||
| { | ||
| [ConditionalFact] | ||
| public async Task Main() |
There was a problem hiding this comment.
Seems a bit much, does this add coverage that we don't have in the other tests?
There was a problem hiding this comment.
Will remove this. (This is just the customer's repo as a test. I didn't mean to include it in the PR; I use it while working to double check that the customer issue is still fixed after having written the other tests and implemented the fix.)
There was a problem hiding this comment.
Ah I see, I've got a separate console program (with ProjectReferences into the EF repo) for doing that stuff.
test/EFCore.Specification.Tests/Query/NorthwindMiscellaneousQueryTestBase.cs
Outdated
Show resolved
Hide resolved
I will look into this for 9+. |
…ts (#32510) (#32520) * Infer type mapping suitable for concatenating two results (#32510) * Work on type mapping inference for string concatenation Fixes #32325, see also #32333 * Tweaks, update baselines add more tests and cleanup * Update baselines, add more tests, some cleanup. --------- Co-authored-by: Shay Rojansky <roji@roji.org> * Qwerk * Fix quirk. --------- Co-authored-by: Shay Rojansky <roji@roji.org>
Fixes #32325
The issue here is that the concatenation of strings requires a max length on the resulting SQL type that can handle the max length of both sides combined. We should make the type mapping be able to participate in this directly, as indicated in #32333, but this is a smaller fix just to address the regression.