Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Make collation part of type mapping #29620

Open
ajcvickers opened this issue Nov 19, 2022 · 3 comments
Open

Make collation part of type mapping #29620

ajcvickers opened this issue Nov 19, 2022 · 3 comments

Comments

@ajcvickers
Copy link
Member

See #27624, #29518, and https://github.com/dotnet/efcore/issues?q=is%3Aissue+is%3Aopen+collation

@roji
Copy link
Member

roji commented Nov 21, 2022

Not necessarily pushing back, but some thoughts:

  • Type mapping facets usually affect the parameter/literal we send to the database (e.g. size/length/precision get set on SqlParameter); that's not the case with collation AFAIK (we never need to specify the collation on DbParameter).
  • It's true that every (textual) expression conceptually has a collation, and that the collation bubbles up the expression tree. But since the collation doesn't affect parameters/literals, I don't think EF needs to track this in any way or infer collations across e.g. binary expressions, as we do with type mappings (what would be the use?).
  • Type mapping facets usually affect the actual type name, but the collation doesn't exactly - it's an additional thing in the column definition. For example, you can't use CAST to cast something to nvarchar of a specific collation (though you can apply a collation to the result via the COLLATE operator (like you can to any expression).
  • We decided to not make nullability part of the type mapping (BTW doing so could help us know in the SQL tree if every node is nullable, which could help various null semantics stuff, /cc @maumar). There are various other column facets which aren't in the type mapping (default/compuetd value/sql, stored/not stored, sparse columns...); we may want to clarify exactly why something belongs in the type mapping as opposed to just being a column facet.

We should also figure out if doing this would solve any concrete bugs (or enable features), or whether it's a pure cleanup/refactor.

@ajcvickers ajcvickers self-assigned this Dec 1, 2023
@roji
Copy link
Member

roji commented Dec 2, 2023

We now have a real use-case for tracking/bubbling up collations in the query tree. With the new primitive collection support in 8.0, we do much wider type mapping inference as before. For example:

 var query = db.Blogs
    .Where(b => ids.Contains(b.Id))
    .ToList();

... now generates:

SELECT [b].[Id]
FROM [Blogs] AS [b]
WHERE [b].[Id] IN (
    SELECT [i].[value]
    FROM OPENJSON(@__ids_0) WITH ([value] nvarchar(450) '$') AS [i]
)

Note that in this query, the collation of b.Id is inferred and applied to the output column of the OPENJSON function inside the subquery.

The same thing needs to happen with the collation of the b.Id column: OPENJSON outputs columns with the collation of its input expression; since that's a parameter above (@__ids_0), these columns get the default database collation, and if b.Id has a non-default collation, the query fails because of conflicting collations (see #32147).

To properly fix this, we should make the collation part of the type mapping, so that it can bubble up (in case there's a complex expression tree instead of just a single column), and then have it inferred and applied to the OPENJSON parameter.

@roji
Copy link
Member

roji commented Dec 4, 2023

There's another alternative to integrating the collation within the type mapping - we can also add it to SqlExpression instead (so alongside the type mapping). That would allow it to be bubbled up by inference code, without forcing it through our type mapping system, where I'm still not sure whether it belongs.

Just a thought for when we get around to doing this.

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

No branches or pull requests

2 participants