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

Fix to #8652 - InvalidCastException when casting from one value type to another in a simple select statement #8861

Merged
merged 1 commit into from
Jun 16, 2017

Conversation

maumar
Copy link
Contributor

@maumar maumar commented Jun 15, 2017

Problem was for queries with value types being projected in a select expression when also using convert. The problem was that getValue is typed as object which then was converted to an expected type. However if the value returned by SQL was not exactly the same type, exception would get thrown due to boxing/unboxing.

Fix is to detect when we apply convert on a top level projection, and in this case use explicit cast, so that type returned by SQL was exactly the same as the type that was expected after unboxing.

Also added support for translating Negate expression, which was previously evaluated on the client.

@@ -380,7 +380,7 @@ public override void Select_count_plus_sum()

AssertSql(
@"SELECT (
SELECT SUM([od].[Quantity])
SELECT SUM(CAST([od].[Quantity] AS int))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is unfortunate, cast is not needed because SUM takes care of the type discrepancy, but result operators are translated after selector (in a separate step), so when translating selector we don't know that it's wrapped in a result op. We could recognize this when handling result ops, and remove the explicit cast, but it seems like an overkill. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove for all providers?
We could do it as SQL tree optimization.

var isTopLevelProjection = _isTopLevelProjection;
_isTopLevelProjection = false;

try
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use try/finally. Use the usual pattern of storing old value locally running the code and restoring the value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we do both

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally is convenient because of return

? expression.Update(left, expression.Conversion, right)
: null;
}
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check R# settings

@maumar maumar force-pushed the fix8652 branch 2 times, most recently from 081c18b to 8bbadb6 Compare June 15, 2017 20:01
@maumar
Copy link
Contributor Author

maumar commented Jun 15, 2017

new version up @smitpatel @anpete

@smitpatel
Copy link
Contributor

Test failures on CI.

@anpete
Copy link
Contributor

anpete commented Jun 15, 2017

smitbot! :trollface:

@maumar
Copy link
Contributor Author

maumar commented Jun 15, 2017

fixed @smitpatel

@maumar
Copy link
Contributor Author

maumar commented Jun 16, 2017

ping @anpete @smitpatel

@@ -107,7 +110,22 @@ public override Expression Visit(Expression expression)
return Visit(translatedExpression);
}

return base.Visit(expression);
if ((expression is UnaryExpression || expression is NewExpression))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check NodeType instead.

…to another in a simple select statement

Problem was for queries with value types being projected in a select expression when also using convert. The problem was that getValue is typed as object which then was converted to an expected type. However if the value returned by SQL was not exactly the same type, exception would get thrown due to boxing/unboxing.

Fix is to detect when we apply convert on a top level projection, and in this case use explicit cast, so that type returned by SQL was exactly the same as the type that was expected after unboxing.

Also added support for translating Negate expression, which was previously evaluated on the client.
@maumar maumar merged commit 44e7316 into dev Jun 16, 2017
@smitpatel
Copy link
Contributor

😨

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

Successfully merging this pull request may close these issues.

4 participants