Skip to content

Infer inline collection element type mappings from each other (and onto the projection) #33438

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
roji opened this issue Mar 30, 2024 · 2 comments · Fixed by #33439
Closed

Infer inline collection element type mappings from each other (and onto the projection) #33438

roji opened this issue Mar 30, 2024 · 2 comments · Fixed by #33439

Comments

@roji
Copy link
Member

roji commented Mar 30, 2024

Our logic for handling inline collections (VALUES) in relational currently does the same thing as for parameter collections - the type mapping is left null, and is inferred based on usage in the query (e.g. if the query's projection column is compared to an actual column, that latter column's type mapping gets inferred to the VALUES expression).

However, unlike a parameter collection, an inline collection can actually contain a column with a type mapping. For example, in the following:

await context.Blogs.Where(b => new[] { b.Name }...).ToListAsync();

The type mapping of the SQL ValuesExpression projection should just bubble up from the type mapping of b.Name. Similarly, if there are some constants/parameters in the inline collection, we should infer their type mapping from the column in the same type mapping:

await context.Blogs.Where(b => new[] { b.Name, "foo" }...).ToListAsync();

This is what we already do for InExpression over an inline collection, which is conceptually very similar.

Triggered by analysis by @maumar in #33410 (comment)

@maumar
Copy link
Contributor

maumar commented Apr 6, 2024

@roji, should we patch this, given that #33410 is a regression and the fix is not too complicated?

@maumar maumar reopened this Apr 6, 2024
@roji
Copy link
Member Author

roji commented Apr 6, 2024

We could... though note that this (and #33439) don't actually fix #33410 specifically, since that uses Union and not Concat... Also I agree that these fixes are simple enough and patchable, though this isn't exactly a one-liner either.

Should we wait for more user complaints about this first?

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants