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.

Sign up for GitHub

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

Jump to bottom

NH-3787 - Decimal truncation in Linq ternary expression #707

Merged
merged 5 commits into from
Dec 7, 2017
Merged

NH-3787 - Decimal truncation in Linq ternary expression #707

merged 5 commits into from
Dec 7, 2017

Conversation

fredericDelaporte
Copy link
Member

@fredericDelaporte fredericDelaporte commented Oct 3, 2017 •
edited
Loading

Test and fix #1335 (NH-3787), with the one still failing set as ignored.

I will not work on that linq casting trouble soon, so better merge the tests alone.

@fredericDelaporte
Copy link
Member Author

fredericDelaporte commented Oct 3, 2017 •
edited
Loading

I shouldn't had a look in this. Now I have added a fix commit.

The trouble is due to a hack in IIf expression handling, which translates to a case, for which HQL may be unable to determine the type if all its inner expressions are parameters. In such case the query preparation fails when the case type is sought for typing a result column. Adding an extra cast around the case solves the trouble, but this is just a workaround for the HQL trouble, and is unneeded for the SQL query. It pollutes it, and may corrupt the data as showcased by this NH-3787 bug.

Since I have not found any easy way to transmit the type to the hql case (or to its inner parameters, which in HQL are un-typed till binding with actual parameters, which occurs too late for the column typing), I have introduced a transparentcast hql function which is here just for supplying the type to HQL, without actually casting in SQL.

@fredericDelaporte fredericDelaporte force-pushed the NH-3787 branch 2 times, most recently from 1a89b1e to 2208094 Compare October 3, 2017 23:12
@fredericDelaporte
Copy link
Member Author

Going to fail, I have forgot I have tested with a modified driver from #708, which is required due to NH-4087 being another cause of this bug. So it needs #708 to be merged first.

fredericDelaporte
fredericDelaporte commented Oct 5, 2017
View reviewed changes
src/NHibernate/Linq/Visitors/HqlGeneratorExpressionVisitor.cs Outdated
@@ -540,7 +540,7 @@ protected HqlTreeNode VisitConditionalExpression(ConditionalExpression expressio

return (expression.Type == typeof (bool) || expression.Type == (typeof (bool?)))
? @case
: _hqlTreeBuilder.Cast(@case, expression.Type);
: _hqlTreeBuilder.TransparentCast(@case, expression.Type);
Copy link
Member Author

@fredericDelaporte fredericDelaporte Oct 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Still a crutch, but at least one that causes less bugs.

I wonder if some other cast introduced by other methods in this file should be switched to transparent too or not. Maybe the Avg by example, for it output cast (input casts should clearly stay actual casts). Or the Sum cast.

@hazzik hazzik modified the milestones: 5.0, 5.1 Oct 5, 2017
@fredericDelaporte fredericDelaporte force-pushed the NH-3787 branch 2 times, most recently from b658019 to d61fbb7 Compare November 27, 2017 08:32
@fredericDelaporte fredericDelaporte mentioned this pull request Nov 27, 2017
Closed
@fredericDelaporte
Copy link
Member Author

fredericDelaporte commented Nov 28, 2017 •
edited
Loading

Rebased and finalized.

This issue looks like #708 and #709, but this yet another case. Linq expression translated to a case when statement currently introduces a hql cast for helping the parser determining the type. But this also causes a sql cast which is unnecessary for most databases and causes decimal to eventually lose the adequate scale.

This PR avoids this additional cast in SQL, excepted for SQLite which requires it.

@fredericDelaporte
NH-3787 - Decimal truncation in some Linq queries
275f4ba 
 * Fixes nhibernate#1335
@fredericDelaporte
NH-3787 - SQLite requires the cast in SQL
b82fbf4 
And SQLite cannot perform the NH-3787 test with so many digits (it does not have a true decimal type anyway).
To be squashed
@fredericDelaporte
Copy link
Member Author

This thing is good to go in my opinion. This should be the last fix of the "decimal fix" series.

@hazzik
Copy link
Member

hazzik commented Dec 7, 2017

@fredericDelaporte need to rename the PR as it is not just "tests" anymore.

@fredericDelaporte fredericDelaporte changed the title NH-3787 - Decimal with scale above 5 tests NH-3787 - Decimal truncation in Linq ternary expression Dec 7, 2017
@fredericDelaporte fredericDelaporte merged commit 293c2eb into nhibernate:master Dec 7, 2017
@fredericDelaporte fredericDelaporte deleted the NH-3787 branch December 7, 2017 10:58
@maca88 maca88 mentioned this pull request Aug 29, 2020
Merged
This was referenced Mar 14, 2023
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hazzik hazzik hazzik approved these changes

Assignees
No one assigned
Labels
r: Fixed t: Fix
Projects
None yet
Milestone
5.1
Development

Successfully merging this pull request may close these issues.

NH-3787 - Decimal truncation in Linq ternary expression
2 participants
@fredericDelaporte @hazzik