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

Implement SqlDateTime2 #46127

Closed
bjornbouetsmith opened this issue Dec 16, 2020 · 5 comments
Closed

Implement SqlDateTime2 #46127

bjornbouetsmith opened this issue Dec 16, 2020 · 5 comments

Comments

@bjornbouetsmith
Copy link

bjornbouetsmith commented Dec 16, 2020

Description

My own and other people's performance tests show that that SqlTypes can give better performance than using the built in .NET types. dotnet/SqlClient#846

In sql client specifically, calling GetDateTime vs GetSqlDateTime and similar "pairs" of methods.

The performance gain from using SqlTypes and their GetSqlWhatever method comes from not having to call IsDbNull on SqlDataReader.

I found this issue when profiling and IsDbNull stuck its head out, then I tried using GetSqlDateTime, since it handles null internally - but that struct does not support DATETIME2 - so I am stuck with using IsDbNull+GetDateTime.

So I propose that you add a SqlDateTime2 struct to make the "API" complete. It seems like this was forgotten when DATETIME2 was introduced into SQL Server - unless the SqlTypes are legacy and not meant for new development?

Having a SqlDateTime2 struct makes it possible to implement a GetSqlDateTime2 method in "sql client".

An alternative would also be to "fix" SqlDateTime - so it also handles the extended range of DATETIME2 - but that would potentially be a breaking change for some applications.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Data.SqlClient untriaged New issue has not been triaged by the area owner labels Dec 16, 2020
@ghost
Copy link

ghost commented Dec 16, 2020

Tagging subscribers to this area: @cheenamalhotra, @David-Engel
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

My own and other people's performance tests show that that SqlTypes can give better performance that using the built in .NET types. dotnet/SqlClient#846

In sql client specifically, calling GetDateTime vs GetSqlDateTime and similar "pairs" of methods.

The performance gain from using SqlTypes and their GetSqlWhatever method comes from not having to call IsDbNull on SqlDataReader.

I found this issue when profiling and IsDbNull stuck its head out, then I tried using GetSqlDateTime, since it handles null internally - but that struct does not support DATETIME2 - so I am stuck with using IsDbNull+GetDateTime.

So I propose that you add a SqlDateTime2 struct to make the "API" complete. It seems like this was forgotten when DATETIME2 was introduced into SQL Server - unless the SqlTypes are legacy and not meant for new development?

Having a SqlDateTime2 struct makes it possible to implement a GetSqlDateTime2 method in "sql client".

An alternative would also be to "fix" SqlDateTime - so it also handles the extended range of DATETIME2 - but that would potentially be a breaking change for some applications.

Author: bjornbouetsmith
Assignees: -
Labels:

area-System.Data.SqlClient, untriaged

Milestone: -

@David-Engel
Copy link
Contributor

This would be in System.Data.

@roji - Do you know any of the history about why some data types like datetime2, introduced in SQL Server 2008, did not get their own SqlType while they did get their own DbType and SqlDbType? My guess is it may have to do with other ADO.NET data providers and backwards compatibility...

@Wraith2
Copy link
Contributor

Wraith2 commented Dec 16, 2020

This would be in System.Data.

Or, it could be put into Micsoft.Data.SqlClient if that's the only place that would use it. As far as i know the reason that the other SqlTypes are still in runtime is that DataTable uses them so if it were put into runtime would that also include adding these types to DataTable DataAdapter etc? @roji and I discussed this somewhat at dotnet/SqlClient#846 and I thought that those types were supported but not actively being considered for new development, EF and other ORM's have largely replaced them.

@ghost
Copy link

ghost commented Dec 17, 2020

Tagging subscribers to this area: @roji, @ajcvickers
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

My own and other people's performance tests show that that SqlTypes can give better performance than using the built in .NET types. dotnet/SqlClient#846

In sql client specifically, calling GetDateTime vs GetSqlDateTime and similar "pairs" of methods.

The performance gain from using SqlTypes and their GetSqlWhatever method comes from not having to call IsDbNull on SqlDataReader.

I found this issue when profiling and IsDbNull stuck its head out, then I tried using GetSqlDateTime, since it handles null internally - but that struct does not support DATETIME2 - so I am stuck with using IsDbNull+GetDateTime.

So I propose that you add a SqlDateTime2 struct to make the "API" complete. It seems like this was forgotten when DATETIME2 was introduced into SQL Server - unless the SqlTypes are legacy and not meant for new development?

Having a SqlDateTime2 struct makes it possible to implement a GetSqlDateTime2 method in "sql client".

An alternative would also be to "fix" SqlDateTime - so it also handles the extended range of DATETIME2 - but that would potentially be a breaking change for some applications.

Author: bjornbouetsmith
Assignees: -
Labels:

area-System.Data, untriaged

Milestone: -

@roji
Copy link
Member

roji commented Jul 13, 2021

Closing in favor of dotnet/SqlClient#846, and the move to implement there instead of in the runtime. To summarize the current situation:

  • SQL Server datetime2 is already fully usable via SqlDataReader.GetDateTime (but not via SqlDataReader.GetSqlDateTime - by design)
  • The motivation behind introducing a GetSqlDateTime2 (this issue) is that GetDateTime requires a null check with IsDBNull before invocation. The argument is that the perf impact of the extra IsDBNull check is worth introducing this new API.

@roji roji closed this as completed Jul 13, 2021
@roji roji removed the untriaged New issue has not been triaged by the area owner label Jul 13, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants