SqlParameter ignores scale when explicitly set to 0

[EamonHetherton]

Describe the bug

When creating a DateTime2 SqlParameter with scale explicitly set to zero, the parameter is sent to the SQL server as datetime2(7)

new SqlParameter("@p1", System.Data.SqlDbType.DateTime2) { Scale = 0}; -> @p1 datetime2(7)

Miss-matched parameter types can cause performance issues in SQL. in addition the use of datetime2(0) instead of datetime2(7) in the database saves 2 bytes per field.

My real world example of this is a table with hundred of millions of rows where the datetime data does not have sub-second precision so we are using datetime2(0) as the data type and that a specific database query does 200-300 times as much IO when using a parameter type of datetime2(7) than datetime2(0).

To reproduce

[Fact]
public async Task DemonstrateBrokenScale()
{
    var date = new DateTime(2021, 11, 01, 23, 59, 59, 999);
    var dateRoundedToSeconds = date.AddMilliseconds((Math.Round((double)date.Millisecond / 1000) * 1000) - date.Millisecond);

    var sqlConnection = new SqlConnection("Data Source=.;Integrated Security=True;");
    await sqlConnection.OpenAsync();
    await using var command = sqlConnection.CreateCommand();
    command.CommandType = System.Data.CommandType.Text;
    command.CommandText = "select @p1";
    
    var p1 = new SqlParameter("@p1", System.Data.SqlDbType.DateTime2) { Scale = 0, Value = date };
    command.Parameters.Add(p1);

    var result = await command.ExecuteScalarAsync();
    
    Assert.Equal(dateRoundedToSeconds, (DateTime)result); //should get back value rounded up to whole seconds (SQL server behaviour)
}

This will produce the following sql:
exec sp_executesql N'select @p1',N'@p1 datetime2(7)',@p1='2021-11-01 23:59:59.9990000'

Expected behavior

it should create a parameter of type datetime2(0), i'm a little unsure if it should round the value or leave that to the server:

exec sp_executesql N'select @p1',N'@p1 datetime2(0)',@p1='2021-11-01 23:59:59.9990000'
or
exec sp_executesql N'select @p1',N'@p1 datetime2(0)',@p1='2021-11-02 00:00:00'

Further technical details

Additional context
I believe this has been a problem for some time and the issue is noted in a comment in the source:

SqlClient/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlParameter.cs

Line 1448 in 78e8ebc

// issue: how could a user specify 0 as the actual scale?

May also be related to #1214

[Wraith2]

In the docs https://docs.microsoft.com/en-us/dotnet/api/system.data.sqlclient.sqlparameter.scale?view=dotnet-plat-ext-5.0#System_Data_SqlClient_SqlParameter_Scale there is a warning:

Warning

Data may be truncated if the Scale property is not explicitly specified and the data on the server does not fit in scale 0 (the default).
For the DateTime2 type, scale 0 (the default) will be passed as datetime2(7). There is currently no way to send a parameter as datetime2(0). Scales 1-7 work as expected. This problem applies to DateTimeOffset and Time as well.

[JRahnama]

@EamonHetherton as @Wraith2 has mentioned this is something due to current limitations and it has been clearly stated on System.Data.SqlClient documentation documentation and Microsfot.Data.SqlClient docs as well.

[EamonHetherton]

...this is something due to current limitations and it has been clearly stated on System.Data.SqlClient documentation as well.

The only limitation that I can see is that no one has bothered to fix the issue. Let me clarify that I am not just lobbing a bug report in an hoping someone else will pick it up and fix it for me; I created the bug report so that I would have something to tie to my pull request that fixes the problem. (see PR1381)

I understand that the problem is noted in the documentation (as it was also noted in the source code), but adding a warning to the documentation about a problem doesn't make the problem go away. The fact is that it is noted within a "Warning" section and is explicitly called a "problem" in the documentation because the behavior when setting scale to '0' is both unintuitive and inconsistent with other values. To say it doesn't abide by the principle of least surprise is somewhat of an understatement. This does highlight though that the documentation will need to be updated to remove the warning about setting the scale to zero as part of fixing the issue.

I really don't understand why there is pushback here? If it's a matter of semantics I can file it as a feature request instead: "When a user sets the scale to zero on a datetime2 parameter, the code should emit an sql parameter with a scale of zero" and we can move on to getting it fixed.

[Wraith2]

I really don't understand why there is pushback here?

Because it will change the behaviour of any existing code and will break anyone who relied on it. It's isn't a matter of if, it will break someone somewhere. If it broke you app you'd be very very annoyed by it, as would most people, so it's a serious blocking issue.

If there is a way to implement the behaviour in a way that allows you to opt into the new behaviour without the possibility of anyone else being broken then that route would probably be ok to add as a feature.

[roji]

Maybe an AppContext switch? Assuming breaking changes are being allowed in 4.0.0, this could be done there with an opt-out flag for backwards compat (it wouldn't be the first)... Assuming everyone agrees the change is good of course.

[EamonHetherton]

I'm proposing that the fix only needs to address the scenario where a user explicitly sets the scale of datetime2 type to zero. If the user does not set the scale it should continue to emit a datetime2(7) parameter. (this is consistent with SQL server as well where 'datetime2' is an alias for datetime2(7))

It is my contention that anyone explicitly setting scale to '0' and expecting a scale of '7' to be output has totally misconstrued the purpose of the scale property and has a bug in their code. Based in this contention, I think the additional complexity of an AppContext switch is unwarranted when there is a correct and easily accessible way of getting a parameter with a scale of '7' already, but if we are at the point of negotiating a way forward I'll take it over inaction.

The change I propose is this:

1. It will only affect the behavior of SqlDbType.Time, SqlDbType.DateTime2 and SqlDbType.DateTimeOffset
2. Default behaviour will remain unchanged i.e. the default scale of the emitted parameter will be '7'

• new SqlParameter("@p1", System.Data.SqlDbType.DateTime2); -> '@p1 datetime2(7)'

3. explicitly setting the scale with emit a parameter definition with a matching scale

• new SqlParameter("@p1", System.Data.SqlDbType.DateTime2) { Scale = 0}; -> @p1 datetime2(0)
• new SqlParameter("@p1", System.Data.SqlDbType.DateTime2) { Scale = 1}; -> @p1 datetime2(1)
• new SqlParameter("@p1", System.Data.SqlDbType.DateTime2) { Scale = 2}; -> @p1 datetime2(2)
• new SqlParameter("@p1", System.Data.SqlDbType.DateTime2) { Scale = 3}; -> @p1 datetime2(3)
• new SqlParameter("@p1", System.Data.SqlDbType.DateTime2) { Scale = 4}; -> @p1 datetime2(4)
• new SqlParameter("@p1", System.Data.SqlDbType.DateTime2) { Scale = 5}; -> @p1 datetime2(5)
• new SqlParameter("@p1", System.Data.SqlDbType.DateTime2) { Scale = 6}; -> @p1 datetime2(6)
• new SqlParameter("@p1", System.Data.SqlDbType.DateTime2) { Scale = 7}; -> @p1 datetime2(7)

[Wraith2]

It is my contention that anyone explicitly setting scale to '0' and expecting a scale of '7' to be output has totally misconstrued the purpose of the scale property and has a bug in their code.

I agree. There's tons of buggy code out there though and breaking it in age old projects probably well into maintenance lifetimes is a bad move. It will inhibit uptake of this library as a direct and easy replacement for the System version. Unfortunately changing this 12 year old piece of behaviour is a breaking change. I want people to change to this library because it's better, things that prevent that are a problem.

I think the additional complexity of an AppContext switch is unwarranted when there is a correct and easily accessible way of getting a parameter with a scale of '7' already,

The problem isn't that they won't be able to get the same behaviour, it's that the behaviour that they currently get will change without them knowing and in a way that may not be easy to see. If it were a crash I'd view it more favourably but it's a silent change in data format that will likely succeed. I've been on the cleanup side of this sort of thing, it wasn't fun.

If this were /runtime there would probably be a suggestion to add a warning/error analyzer on any detection of explicitly setting scale and datetime2 type together. It wouldn't be perfect but it would at least be a way to catch what I see as the likely majority of problems.

It's down to the MS team to decide.

[JRahnama]

@David-Engel, @DavoudEshtehari any opinion about this issue?

[roji]

FYI this is the root cause for dotnet/efcore#27513, where EF Core attempts to send a datetime2(0) parameter in a WHERE clause, as a concurrency check. Since SqlClient ignores the 0 Scale, the untruncated value is sent, triggering an exception since it does not match the (truncated) database column value.

[lcheunglci]

Given that this change would silently affect the data inserted, it would dangerously cause issues that existing apps that depend on this behavior, so we will not be moving forward with this change. If an app needs to insert data with 0 scale, they could truncate the data beforehand.

[EamonHetherton]

I'm proposing that the fix only needs to address the scenario where a user explicitly sets the scale of datetime2 type to zero. If the user does not set the scale it should continue to emit a datetime2(7) parameter. (this is consistent with SQL server as well where 'datetime2' is an alias for datetime2(7))

If the default type is still datetime2(7) except where the user is explicitly setting the scale to zero then I disagree that this is "Silently" affecting data.

Can we at least consider a code switch that a user can explicitly turn on to enable the client to emit datetime2(0) parameters where they also set scale to zero? As it stands, I have no way to emit a datetime2(0) parameter from the SqlClient library even being fully aware of all the effects and where it is highly desirable to do so.

The issue is less about data truncation than it is about the parameter type and what query plan the sever generates as a result. In the past I have fallen victim to horrendous performance issues with ORMs where the mapped type and hence the parameter types were "bigger" than the database column type and so the server was unable to effectively use indexes. A particularly memorable situation is where the database had a VARCHAR column but the parameter was NVARCHAR so the database actually did a table scan an "upcast" every value in the database to NVARCHAR to do the comparison even though the value was plain ascii text. The performance difference was several orders of magnitude.

The description of the bug is the same effect with datetime2(0) in the database and a parameter of datetime2(7) in the query. It may not manifest in a noticeable way on small datasets but that is not my use case.

[lcheunglci]

Thank you for your explanation. We had a meeting yesterday to discuss all options mentioned above. We did consider the fact that only the apps that explicitly set datetime2(0) would be affected by your PR and that assumes majority of the users using the default value should not be explicitly setting it in the first place, which is safe assumption; however, unlike a breaking change, if an app did end up setting datatime2(0) and expects the default behavior, it might not be noticeable right away until they do an audit on their tables or until a breakage occurs due to a dependency on the existing behavior which may be harder to detect i.e. silently affecting the data. The application context switch for enabling this behavior was highly considered; however, we don't currently have the capacity to do so at the moment, and I also understand your concern with the performance, so I'll change the status back to untriage so we can bring this issue back for discussion at our next meeting.

[Wraith2]

How complex do the team judge the implementation of this with an AppContext switch to be? If it's isn't too difficult to implement or review then it could be done as a community contribution.

[EamonHetherton]

I would like to get this looked at again. I've created a new pull request #2411 from the current main to fix the issue.

This is still a big problem for me and getting bigger as the volume of data in my database grows.

An example of how this is severely impacting me:
I have a number of tables with > 1,000,000,000 rows of data that are partitioned on a datetime2(0) column.
If I query with a parameter type matching the partition key i.e. dateatime2(0), it will do partition elimination and the query is more 100x lower cost than if the parameter type is datetime2(7).

[Charlieface]

As I mentioned on the earlier pull request:

99.999% of code that relies on the default just uses AddWithValue or similar.

It just seems crazy not to fix this when it's highly unlikely to actually affect any code. No one sets Precision explicitly unless they have a reason to, ergo all code that does so is already buggy and should be fixed.

---

I have discovered a rather horrible workaround, which would be rather complex to do in EF Core, but can be done relatively simply in bare SqlClient:

Call sp_executesql directly, passing in the command and parameter types as parameters (this is essentially the same as dynamic SQL except without injection). Note the use of CommandType.

using var conn = new SqlConnection(someConnStr);
using var comm = new SqlCommand("sp_executesql", conn) { CommandType = CommandType.StoredProcedure };
cmd.Parameters.Add("@sql", SqlDbType.NVarChar, -1).Value = "SELECT SQL_VARIANT_PROPERTY(@dt, 'Scale');";
cmd.Parameters.Add("@params", SqlDbType.NVarChar, -1).Value = "@dt datetime2(0)";
cmd.Parameters.Add("@dt", SqlDbType.DateTime2).Value = DateTime.Now;
var result = (int)cmd.ExecuteScalar();

What actually happens here is that SqlClient passes it as datetime(7) over the wire, and then sp_executesql implicitly converts it to datetime(0) before executing the statement.

[EamonHetherton]

@Charlieface I'm working on a pull request that I hope will satisfy all previous resistance to the change of behaviour.

[Wraith2]

It just seems crazy not to fix this when it's highly unlikely to actually affect any code. No one sets Precision explicitly unless they have a reason to, ergo all code that does so is already buggy and should be fixed.

EF Core was at some point in the last few years generating code which set the precision. It isn't as niche as you seem to think.

Even if existing code is already broken it is working as the owners currently intend it to. If you can make sure that their code throws an exception instead of silently changing behaviour then that's undesirable but possible to do. Changing the behaviour of their code silently is not a responsible thing to do and will not happen.

Think of the broad scope of consumption of this library and amount of code worldwide that has been written anytime in the last 20 years which could be broken. If you've ever had a library you use change underneath you and had to debug it then you should have some understanding of this situation and not want to be the one that causes issued for others.

[Charlieface]

@Wraith2

EF Core was at some point in the last few years generating code which set the precision. It isn't as niche as you seem to think.

Presumably it was generating the correct precision, not putting 0 when something else was meant, so again it's a bug that should be fixed.

Even if existing code is already broken it is working as the owners currently intend it to.

Is it? The owners most likely don't even realize that the parameter is being passed incorrectly.

If you can make sure that their code throws an exception instead of silently changing behaviour then that's undesirable but possible to do. Changing the behaviour of their code silently is not a responsible thing to do and will not happen.

Like I said, I'd love to hear of a scenario where someone used Precision = 0 and meant Precision = 7, and the results were what they intended, and would be changed by this bug fix.

• If the value has no seconds fractional component then the results are the same
• If it does have a fractional component which they didn't round before passing, then current behaviour would be to pass that in.
  • If it is being stored in a datetime2(0) column then the point is moot because it will be truncated anyway.
  • If it is used in a range comparison > < etc, then it is highly unlikely to cause different results with a different seconds fraction.
  • If it is used in an equality comparison against a rounded value, then currently they get no results at all, which is a bug (one reason to never use = on dates).

The only other difference is a widening conversion of the column side from datetime2(0) to datetime(7) to match the incorrect parameter, causing a poor execution plan. This is certainly not a desirable feature.

So what scenario are you actually worried about?

[Wraith2]

Going over all of this again is pointless. It's already been discussed and a decision made earlier in the conversation. No matter how much you want to disregard the legacy codebase concern it's it relevant and you're not going to get any change accepted which changes the current default situation, the best you can get is an opt-in behavioural change so work towards that.

[EamonHetherton]

@Charlieface I understand your frustrations and I've already been through the five stages of grief on this and have come out the other end at acceptance. As @Wraith2 said, the agreed way forward is for an opt-in context switch which is what I've implemented :)

It was in fact through EntityFramework that I encountered this bug originally as I had a mapping that was datettime2(0) and EntityFramework would correctly created the table as such but then query it with a parameter of datetime2(7) which was totally unexpected for me and causing significant performance issues.

In anycase, I'd just like to see this pull request #2411 merged so that I can use the context switch so would prefer now to focus on what it will take to make that happen.

Discussion on if/when the new behaviour should become the default and how to go about that in a way that minimised chances of subtle functionality change to existing code, I'd prefer to leave for later.

[EamonHetherton]

@Wraith2 what are your thoughts on making the existing behavior apply only if the parameter value contains sub-second precision?

i.e. If I specify a datetime2 parameter with "Scale=0" and a value that only has whole seconds ("2024-03-29 16:23:02") it would emit a datetime2(0) parameter but if the value had sub-second precision ("2024-03-29 16:23:02.123"), it would continue to emit datetime(7)? This would eliminate the need for the context switch in most cases (at least for my usage)

I believe this would resolve any potential issue with data loss in current behavior vs new behavior. (Caveat: I have not yet investigated the feasibility of doing this)

[Wraith2]

I think any heuristic based on usage is too risky. The context switch is the only way I'd support the modification. Keep in mind that I don't work here and that the MS team may still decide it's too risky even with the context switch but I'm trying to do what I can help everyone.

[EamonHetherton]

@Charlieface #2411 merged. Just waiting now for next nuget package build