Decimal parameters are truncated when precision and scale are set

[ajcvickers]

This is the issue we discussed at the last EF/SqlClient sync. It is related to dotnet/efcore#19293

---

Consider attempting to insert the value 10.9876 into a decimal(18, 2) column.

• If DbParameter.Precision is set to 18, and DbParameter.Scale is set to 2, then
  • SqlClient truncates the value to 10.98 before inserting.
• If DbParameter.Precision and DbParameter.Scale are not set, then:
  • SqlClient passes the full 10.9876 to SQL Server
  • SQL Server rounds this to 10.99.

Historically the behavior in SqlClient could not be changed, since it would be a break for existing applications that are relying on truncation.

At the same time, EF didn't set precision and scale on decimal parameters (since before my time on the team!) which meant EF was relying on SQL Server rounding.

Initially in EF Core we set precision and scale, but people argued that truncation was a deviation from EF6 behavior and that rounding was better. In fact, a reasonable case could be made that the truncation behavior was wrong. We concurred and so we kept the rounding behavior in EF Core.

---

Enter always-encrypted. Now not setting precision and scale on a decimal parameter causes SQL Server to throw. See dotnet/efcore#19293.

But if EF starts setting precision and scale, then the behavior will change to truncation.

So, options here are:

• SqlClient introduces a rounding behavior. Note that this is breaking if it is on by default.
• EF introduces its own rounding behavior before passing the value to SqlClient
• We start throwing and require applications truncate/round as appropriate. This is also breaking.

After writing this, I'm leaning even more towards EF doing rounding. But we should first be sure that SqlClient doesn't want to change or introduce new behaviors.

---

Repro code. The code uses EF only to create a database and print results. The inserts use SqlClient directly.

public static class ThreeOne
{
    public static void Main()
    {
        // Use use EF to create a test database
        using (var context = new SomeDbContext())
        {
            context.Database.EnsureDeleted();
            context.Database.EnsureCreated();
        }

        InsertRow(1, 10.9876m, null);
        InsertRow(2, 10.9876m, (18, 2));

        // Use use EF to easily read the inserted values back
        using (var context = new SomeDbContext())
        {
            var products = context.Products.ToList();
            Console.WriteLine($"Product {products[0].Id} has decimal value {products[0].Price}");
            Console.WriteLine($"Product {products[1].Id} has decimal value {products[1].Price}");
        }

        void InsertRow(int id, decimal price, (byte, byte)? precisionAndScale)
        {
            using var connection = new SqlConnection(Your.SqlServerConnectionString);
            using var command = connection.CreateCommand();

            var idParameter = command.CreateParameter();
            idParameter.ParameterName = "p0";
            idParameter.Value = id;

            var priceParameter = command.CreateParameter();
            priceParameter.ParameterName = "p1";
            priceParameter.DbType = DbType.Decimal;

            if (precisionAndScale.HasValue)
            {
                var (precision, scale) = precisionAndScale.Value;
                priceParameter.Precision = precision;
                priceParameter.Scale = scale;
            }

            priceParameter.Value = price;

            command.Parameters.Add(idParameter);
            command.Parameters.Add(priceParameter);
            command.CommandText = "INSERT INTO [Products] ([Id], [Price]) VALUES (@p0, @p1)";
            connection.Open();
            command.ExecuteNonQuery();
            connection.Close();
        }
    }
}

public class SomeDbContext : DbContext
{
    private static readonly ILoggerFactory
        Logger = LoggerFactory.Create(x => x.AddConsole()); //.SetMinimumLevel(LogLevel.Debug));

    public DbSet<Product> Products { get; set; }
    
    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        => optionsBuilder
            //.UseLoggerFactory(Logger)
            .EnableSensitiveDataLogging()
            .UseSqlServer(Your.SqlServerConnectionString);
}

public class Product
{
    [DatabaseGenerated(DatabaseGeneratedOption.None)]
    public int Id { get; set; }
    
    [Column(TypeName = "decimal(18,2)")]
    public decimal Price { get; set; }
}

/cc @AndriySvyryd @bricelam @roji

[roji]

After writing this, I'm leaning even more towards EF doing rounding.

The disadvantage here is that the problem seems to be relevant for non-EF users as well, e.g. Dapper users. It seems problematic for the MSSQL client and server to have two completely different behaviors (the first truncates, the seconds rounds), and for Always Encrypted to basically force you into the first.

We start throwing and require applications truncate/round as appropriate. This is also breaking.

You mean explicitly detect in advance whether a floating point type would require truncation/rounding? This sounds like it could be tricky (am not a floating point expert but I'm not sure EF should be putting its nose in there), and also potentially perf-impacting to me (not sure it's trivial to detect this).

[ajcvickers]

@roji

The disadvantage here is that the problem seems to be relevant for non-EF users

Yes, but worth making a breaking change to SqlClient? The question in my mind about the split between the following groups:

1. Applications that are using truncation intentionally
   a. Have tests that will catch the break
   b. Don't have tests that will catch this and hence are going to have a bug after the change. Break!
2. Applications that are using truncation just because it's the default behavior
   a. Either truncation or rounding is okay--no real break
   b. Truncation is needed, but they don't know it, yet their testing is still good enough to catch the change
   c. Truncation is needed, but they don't know it and tests don't catch it. Break!

My feeling is that 1.b and 2.c., while very unlikely to be the majority, are still likely to be a significant minority of cases.

am not a floating point expert but I'm not sure EF should be putting its nose in there

Decimals don't have the same challenges as floating point representations do. A decimal of 0.123 always means exactly0.123, not an approximation to it, as can be the case with floats. So while we have to match the behavior of SQL Server, I don't think this is going to be dangerous or hard.

[roji]

Oh sorry, I somehow missed that this is about decimal and not float. This does make it easier.

I agree that a break is involved, I guess it's more of a general question of what kind of breaks the SqlClient people are willing to do and in which situations. I'd definitely consider this an inconsistent quirk (compared to the server behavior of rounding) and as this is a major version this could be the right moment to make things right.

[cheenamalhotra]

Hi @ajcvickers @roji

SqlClient introduces a rounding behavior. Note that this is breaking if it is on by default.

I agree on SqlClient making this change in order to provide consistent behavior that is expected from a SQL Server client driver to match SQL Server behavior.

• Will it be a breaking change?
  Yes. 2.0 brings this opportunity to set things straight.
• Will there be workaround?
  Yes. We can provide a toggle behavior for applications willing to continue to truncate for backwards compatibility.
• Does EF Core need to do something?
  No. Since this is a driver issue, wrappers/frameworks need not handle this change specifically.

Doing so should address all cases in applications, also setting things right for the driver.

/cc: @saurabh500 @David-Engel

[ajcvickers]

@cheenamalhotra Excellent!