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

SqlServer: Char with converter does not work well in query #15330

Closed
smitpatel opened this issue Apr 11, 2019 · 3 comments · Fixed by #15954
Closed

SqlServer: Char with converter does not work well in query #15330

smitpatel opened this issue Apr 11, 2019 · 3 comments · Fixed by #15954
Assignees
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-bug
Milestone

Comments

@smitpatel
Copy link
Contributor

SqlServer does not support char so we use char to string converter. When comparing character to constant/parameter, we run the value through converter so we get a string constant/parameter. But char is always wrapped in conversion to int on client side. So we also generate cast in SQL query. Which works well for SQLite but on SqlServer it fails to convert "varchar" to "int" and throws.

  • Funcletizers leaves convert around char because of Char support broken in 3.0.0-preview1 #14159
  • Convert nodes cannot be identified if implicit or explicit. We could just make a guess that char to int convert is always implicit and we can remove convert on both side while translating to SQL. @roji with regards to Char support broken in 3.0.0-preview1 #14159 do you see any issue with this approach?
  • SqlServer can intercept such conversions and generate parameter/constant of required type directly. May have to constraint on char to int since some explicit conversions may be used for truncation by user.
@smitpatel smitpatel self-assigned this Apr 11, 2019
@smitpatel smitpatel added this to the 3.0.0 milestone Apr 11, 2019
@roji
Copy link
Member

roji commented Apr 11, 2019

@smitpatel always removing char->int conversion nodes would certainly fix #14159 for me. I'm a little concerned that this would silently disallow explicit casts which may be legitimately used by the user in some cases - but I don't have a good scenario in mind. On the other hand I'm not sure there's a database out there which supports ASCII casting char->int (when text of any kind is cast to int, PostgreSQL attempts to parse it numerically and throws if a non-digit is found).

So removing the cast node seems to make sense... Assuming I've understood everything correctly :)

@smitpatel
Copy link
Contributor Author

Same for ushort (and probably all types which upcast to int for equality operator)

@dmitry-lipetsk
Copy link
Contributor

So removing the cast node seems to make sense... Assuming I've understood everything correctly :)

I remove all impicit cast (Convert nodes) in my data provider.

One problem - need rebuild the functions and binary operations nodes. I say about local calculations.

For example, the call of datetime.AddYears(int) may transformed into extension method AddYears(this DateTime, int?) or vice versa.

Plus, I'm losing the call "NullableValue.Value" - EFCore (or linq?) translates this call to Convert node.

smitpatel added a commit that referenced this issue Jun 5, 2019
…st in generated SQL

Enable BuiltInDataTypeTests
- Preserve convert nodes around parameters/constants in parameter extracting.
- While translating to Sql, remove implicit convert nodes. Implicit convert nodes appear only for binary expressions since equality operators are not defined for all types
- Apply explicit cast in SQL only when converted type is mapped.
- Convert int value to enum value before printing literal (how old pipeline did it)

Resolves #14159
Resolves #15330
Resolves #15948
@smitpatel smitpatel added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jun 5, 2019
smitpatel added a commit that referenced this issue Jun 5, 2019
…st in generated SQL

Enable BuiltInDataTypeTests
- Preserve convert nodes around parameters/constants in parameter extracting.
- While translating to Sql, remove implicit convert nodes. Implicit convert nodes appear only for binary expressions since equality operators are not defined for all types
- Apply explicit cast in SQL only when converted type is mapped.
- Convert int value to enum value before printing literal (how old pipeline did it)

Resolves #14159
Resolves #15330
Resolves #15948
@ajcvickers ajcvickers modified the milestones: 3.0.0, 3.0.0-preview7 Jul 2, 2019
@ajcvickers ajcvickers modified the milestones: 3.0.0-preview7, 3.0.0 Nov 11, 2019
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. customer-reported type-bug
Projects
None yet
4 participants