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

SqlClient fails to convert decimal(38,20) to .Net decimal for values within the range of .Net's decimal type #95

Closed
dbrownems opened this issue May 10, 2019 · 15 comments · Fixed by #1179

Comments

@dbrownems
Copy link

This program

        static void Main(string[] args)
        {
            using (var con = new SqlConnection("server=.;database=tempdb;Integrated Security=true"))
            {
                con.Open();
                var cmd = new SqlCommand("select cast(4210862852.86 as decimal(38,20))  val", con);
                using (SqlDataReader rdr = cmd.ExecuteReader())
                {
                    rdr.Read();

                    var val = rdr.GetDecimal(0);
                    Console.WriteLine(val);
                }
            }
        }

fails with

System.OverflowException: 'Conversion overflows.'

at SqlBuffer.cs line 254:

                if (StorageType.Decimal == _type)
                {
                    if (_value._numericInfo.data4 != 0 || _value._numericInfo.scale > 28)
                    {
                        throw new OverflowException(SQLResource.ConversionOverflowMessage);
                    }

This value is well within the range of a .NET decimal, and it's a value that the SqlClient can write using a SqlParameter. So it should be converted to .NET decimal.

The byte pattern in SQL and TDS for this value is:

0x261400010000D877FB4DEE8B51699A5005000000

And it looks like SqlBuffer refuses to convert any value with a non-zero value in the last 4 bytes. But a .NET Decimal is stored as a 12-byte integer, along with a sign and a scaling factor.

So the apparent intent is to determine if the SqlDecimal uses a larger-than-12 byte integer and trigger the overflow.

So this conversion has a bug. It looks like the SqlClient is confused on the byte ordering SQL Server is using, and the last 4 bytes of this buffer are not the 4 least-significant bytes of the 16-byte integer embedded in the decimal.

This number requires uses 12 (base-10) decimal digits to store, so it requires fewer than 12 (base-256) bytes to store.

https://docs.microsoft.com/en-us/dotnet/api/system.decimal?redirectedfrom=MSDN&view=netframework-4.8

@Wraith2
Copy link
Contributor

Wraith2 commented May 11, 2019

I've got a debuggable reproduction but I don't really understand what the problem is even though I can see it occurring. The sql decimal requires 16 bytes to represent because that 5 spills into the last uint, and I think you're saying that even though it's 16 bytes in sql the actual value can be represented in a decimal because they use different scales. Is that right? and if so how do you convert from one representation to the other?

@dbrownems
Copy link
Author

I'll ask around for an authoritative layout for the decimal(38,20).

@Wraith2
Copy link
Contributor

Wraith2 commented May 11, 2019

The layout seems fine and I agree with your assertion that the value it is representing is in range of a .net decimal type, so there needs to be some sort of conversion I just don't know that isn't being done or how it would be.

@Wraith2
Copy link
Contributor

Wraith2 commented May 11, 2019

I think the problem is that the scale is excessive for that value. If you lower the scale it'll work so the value itself is fine but forcing that precision and scale makes the TDS representation use 16 bytes to represent it which hits the check that you point out fails.

@dbrownems
Copy link
Author

dbrownems commented May 12, 2019

Ok, I think I know what the bug is.

This
select cast(cast(4210862852.86 as decimal(38,20)) as varbinary(40))
outputs
0x261400010000D877FB4DEE8B51699A5005000000

0x261400 <- metadata (precision=0x26 scale= 0x14, ??=0x00) may not be present in TDS

                01 <- sign byte

                    0000D877FB4DEE8B51699A5005000000 <-16-byte Little Endian Integer

You can see the byte order like this:

set nocount on
go
declare @d decimal(38,0) = 1

while @d <= 999999999999999999999999999
begin
   print cast(@d as varbinary(40))
   set @d = @d * 16
end

outputs

0x2600000101000000
0x2600000110000000
0x2600000100010000
0x2600000100100000
0x2600000100000100
0x2600000100001000
0x2600000100000001
0x2600000100000010
0x260000010000000001000000
0x260000010000000010000000
0x260000010000000000010000
0x260000010000000000100000
0x260000010000000000000100
0x260000010000000000001000
0x260000010000000000000001
0x260000010000000000000010
0x26000001000000000000000001000000
0x26000001000000000000000010000000
0x26000001000000000000000000010000
0x26000001000000000000000000100000
0x26000001000000000000000000000100
0x26000001000000000000000000001000
0x26000001000000000000000000000001

So .Net is failing whenever any of most-significant bytes is non-zero. That is the correct behavior for decimal(38,0) as that would indicate a number larger than can fit in a 12-byte integer. However this 16-byte integer stores the numeric value multiplied by 10^s for decimal(p,s). And for non-zero s, the non-zero most-significant bytes might not represent an overflow value.

The 16-byte integer has to be divided by 10^s to determine if it's out of bounds.

IE the position of the non-zero bytes in the 16-byte integer will change depending on the scale. eg

print cast(cast(4210862852.86 as decimal(38,20)) as varbinary(40))
print cast(cast(4210862852.86 as decimal(38,22)) as varbinary(40))
print cast(cast(4210862852.86 as decimal(38,24)) as varbinary(40))
print cast(cast(4210862852.86 as decimal(38,26)) as varbinary(40))
print cast(cast(4210862852.86 as decimal(38,28)) as varbinary(40))

outputs

0x261400010000D877FB4DEE8B51699A5005000000
0x26160001000060D03A7616A9DA23517C13020000
0x2618000100008065F92EC60C6A01B28F9CCF0000
0x261A0001000000A66B596AFD6C8D882128195100
0x261C0001000000D80CEE8AFD923E5719ADD3AD1F

In other words if the 4 most-significant bytes are 0s, then there can be no overflow. But if the most-significant bytes are non-zero, and the declared scale is high, and some of the least-significant bytes are zero then there is also no overflow.

@divega divega transferred this issue from dotnet/corefx May 16, 2019
@divega
Copy link

divega commented May 16, 2019

As recently announced in the .NET Blog, focus on new SqlClient features an improvements is moving to the new Microsoft.Data.SqlClient package. For this reason, we are moving this issue to the new repo at https://github.com/dotnet/SqlClient. We will still use https://github.com/dotnet/corefx to track issues on other providers like System.Data.Odbc and System.Data.OleDB, and general ADO.NET and .NET data access issues.

@David-Engel
Copy link
Contributor

This same exception occurs in .NET Framework and Core. It seems like a fairly basic bug but I'm guessing there is a good reason it hasn't been discovered yet. Priority wise, I'm going to consider this an edge case for now unless there is more information that is occurs in a more general use case.

@David-Engel David-Engel added this to the Future milestone May 17, 2019
pashokchakravarthi added a commit to pashokchakravarthi/SqlClient that referenced this issue Jan 7, 2020
When mapping data from SQL Decimal(38,15) to .net decimal, this driver is throwing Conversion Overflow exception. Bug is already tracked under dotnet#95. 
This fix tries to alter this functionality, by rounding to nearest allowable decimal value.
pashokchakravarthi added a commit to pashokchakravarthi/SqlClient that referenced this issue Jan 7, 2020
When mapping data from sql Decimal(38,15) to .net decimal, this driver is throwing CoNversion overflow exception. Bug is already tracked under dotnet#95. 
This fix tries to alter this functionality, by rounding to nearest allowable decimal value.
@NathanJonker
Copy link

This same exception occurs in .NET Framework and Core. It seems like a fairly basic bug but I'm guessing there is a good reason it hasn't been discovered yet. Priority wise, I'm going to consider this an edge case for now unless there is more information that is occurs in a more general use case.

@David-Engel, I have the same issue, I am trying to retrieve value: 1234567891234.56789 stored in SQL as: 1234567891234.56789000000000000, because of a column data type decimal(35,17), as you can see it does not exceed the limit of the .Net decimal type if you ignore the 0's. Can you recommend an interim solution while this is set as low priority?

@dbrownems
Copy link
Author

As a workaround, use an intermediate conversion to string. Either on SQL Server with a CAST or on the client like this:

var val = Decimal.Parse(rdr.GetSqlDecimal(0).ToString());

@NathanJonker
Copy link

As a workaround, use an intermediate conversion to string. Either on SQL Server with a CAST or on the client like this:

var val = Decimal.Parse(rdr.GetSqlDecimal(0).ToString());

Thank you for the reply @dbrownems, much appreciated. Note that I am using Entity Framework Core 2.1, any suggestion would be great?

@NathanJonker
Copy link

Would it be possible to remove or exclude the trailing zeros when the _value._numericInfo.scale exceed 28 in the below extract, in that way you keep your precision there is no rounding issue, and throwing the conversion overflow is more valid?

if (StorageType.Decimal == _type)
{
if (_value._numericInfo.data4 != 0 || _value._numericInfo.scale > 28)
{
throw new OverflowException(SQLResource.ConversionOverflowMessage);
}

@branko-d
Copy link

I'm hitting the same issue. We have a test which writes a random decimal value to the database, and then ensures that the same value can be retrieved from the database intact. Which is failing intermittently because of this.

@scidec
Copy link

scidec commented Apr 13, 2021

Same problem here. A simple entity project generated from a sql server database cannot write decimals from c# back to the database, even when they are both of same type decimal.

@klemmchr
Copy link

klemmchr commented Jul 6, 2021

Any updates on this? This issue is present since 3 years.

@DavoudEshtehari
Copy link
Contributor

It is on the to-do list this semester.

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 a pull request may close this issue.

10 participants