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

Remove SqlServer TypeMapping for CLR type char #8656

Closed
lajones opened this issue May 31, 2017 · 8 comments
Closed

Remove SqlServer TypeMapping for CLR type char #8656

lajones opened this issue May 31, 2017 · 8 comments
Assignees
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Milestone

Comments

@lajones
Copy link
Contributor

lajones commented May 31, 2017

An EntityType with property char is not supported by the SQL Server provider. However SqlServerTypeMapper does have a mapping in _clrTypeMappings for char (mapping it to a SQL Server int column).

The reason for this is to support generation of single character literals in the Query Pipeline e.g. as arguments to functions, or LIKE can take an ESCAPE argument which is a single character - see this test. Query checks that it can interpret each argument (in this case a single character) - if it fails to find a type mapping for that type then it assumes it cannot generate literals for that type.

We should remove the entry in _clrTypeMappings for SqlServerTypeMapper for char so that we will generate the correct error if someone tries to create a property of that type. But we need to provide an alternate means to translate the literal above. We discussed:

  1. Just changing how we deal with LIKE to enforce that the ESCAPE argument is a string, or
  2. Changing the way Query treats arguments in general to specifically allow it to try a string TypeMapping if a char one does not exist.
  3. Updating the TypeMapper to allow a set of types which it can interpret for generation of literals even if it does not allow that type as a property type.
@divega
Copy link
Contributor

divega commented Jun 1, 2017

Additional data points:

  • GetChar is not supported in either SQL Server or Oracle's ADO.NET provider

  • Currently it is necessary for the provider to know how to generate a parameter, and not just a literal for the escapeCharacter argument.

  • Another potential approach (similar to # 2 above, but more generalized) would be to create some sort of fallback mapping mechanism for types like char and byte:

    • if char is not explicitly supported by the provider, it could fall back to a 'string' of size 1
    • if byte is not explicitly supported by the provider, it could fall back to a byte[] of size 1

    This feels like it is slowly turning into a short-range version of the type conversions feature, but maybe that is ok.

@lajones
Copy link
Contributor Author

lajones commented Jun 1, 2017

Note: this came out of the work for #8631.

@divega
Copy link
Contributor

divega commented Jun 1, 2017

cc @anpete

@lajones lajones self-assigned this Jun 1, 2017
@lajones lajones added this to the 2.0.0-preview2 milestone Jun 1, 2017
@lajones
Copy link
Contributor Author

lajones commented Jun 1, 2017

Note: decided for preview2 to just to update the way we deal with LIKE to pass in the single character as a string. And remove the char SQL Server TypeMapping.

@ajcvickers
Copy link
Member

@lajones Can this issue be closed?

@lajones
Copy link
Contributor Author

lajones commented Jun 2, 2017

@ajcvickers @divega @anpete @smitpatel I thought the solution to just change the LIKE treatment was for preview2 only and that we wanted to discuss further on how, in general, to treat e.g. DbFunctions which had char parameters, or the issues that @divega raised above, for 2.0.0. So in short I thought we would be moving this to the 2.0.0 milestone but keeping it open.

@ajcvickers
Copy link
Member

I'll move it out of a milestone so we can re-triage. @divega Feel free to re-title/re-purpose this issue to cover the type conversion work we talked about, or open a new issue if that's better, or just combine the work into #242.

@divega
Copy link
Contributor

divega commented Jun 3, 2017

I have added text in #242 which I think covers the follow up for this (@ajcvickers feel free to review it). Closing for now.

@divega divega closed this as completed Jun 3, 2017
@divega divega added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jun 3, 2017
@divega divega added this to the 2.0.0-preview2 milestone Jun 3, 2017
@ajcvickers ajcvickers modified the milestones: 2.0.0-preview2, 2.0.0 Oct 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Projects
None yet
Development

No branches or pull requests

3 participants