Implementation of DateTime2 + accompanying GetSqlDateTime2 method (#846)

[bjornbouetsmith]

Initial implementation - let me know what you think so far.
I have tried to do everything I could think of and have tried to follow the "style"
I don't know why VS insists on changing the .csproj for SqlCommand etc - I have done nothing, but will try to revert the changes out side of VS

And I need some help understanding why VS can build my stuff when I use the ADP class, but I cannot build it outside of VS - possibly some "tweaks" needed - which is why I am throwing exceptions directly and not going through the ADP class like the other classes do.

[dnfadmin]

All CLA requirements met.

[JRahnama]

Some of the test failures are not due to your changes. You can address the ones are related to your changes (if there is any) and push the changes again.

[bjornbouetsmith]

Some of the test failures are not due to your changes. You can address the ones are related to your changes (if there is any) and push the changes again.

Yeah I looked at one of the test failues and that has definately not anything to do with my additions - but will go through them all just to be sure when they are all finished.

[Wraith2]

Some comments added inline but overall looks good to me.

[bjornbouetsmith]

Checked the tests and it is all relating to encryption in the ManualTests - so has nothing to do with the changes I have made.

[bjornbouetsmith]

Changed the internals of SqlDateTime2 to be inline with the https://github.com/dotnet/SqlClient/blob/master/coding-style.md - and since its a new file I changed some of the fieldnames.

[bjornbouetsmith]

squashed everything into one commit

[bjornbouetsmith]

Any chance to get this reviewed and merged so I don't have to handle any more conflicts?

[cheenamalhotra]

Hi @bjornbouetsmith

We will be reviewing this PR soon, currently we're occupied with a bundle of support issues that have much higher priority.
We will definitely try to come up some update by end of this month.

Thanks for your patience!

[cheenamalhotra]

Partially reviewed.

We need much more tests I think. e.g. Async Readers, and anything else that the new type is going to add support for.

Also as documented here: https://docs.microsoft.com/en-us/sql/connect/ado-net/sql-server-data-type-mappings?view=sql-server-ver15, DateTime2 corresponds to DateTime for .NET Framework type, and is supported with for DbType accessor with SqlDataReader.GetDateTime API.

Do we have a solution without causing a breaking change that we can introduce this type, and also ensure DateTime2 continues to work for existing apps with DateTime but does correspond to DateTime2 by default in all cases to avoid confusions to new developers?

[bjornbouetsmith]

I am not sure why you need many more tests.

The SqlDateTime2 struct is only being exposed in one single method: GetSqlDateTime2 - and does not interface with any of the existing methods.

So its a non breaking change - available only in SqlDataReader - if support had been added in DbDataReader and all over the place, it would be a different change.

But this change was a needle point expansion of SqlDataReader only.

I had no plans of introducing usage of SqlDateTime2 anywhere else, but if anyone else wants to do that, then yes more tests are needed - but I think the tests I made for GetSqlDateTime2 is more than sufficient and should handle the possible scenarios.

If not, I am happy to write more tests.

My change was made, so I could stop using GetDateTime and use GetSqlDateTime2 instead - similar to GetSqlDateTime - which unfortunately does not support the DATETIME2 type.

[bjornbouetsmith]

Hi,

I am going to abandon this.

It has now been more than 3 months to get a tiny patch merged into this repository and if you are not interested enough to allocate the very few resources to get this pull request merged then so be it.

I will not be spending more time on updating the pull request as other merge into the repository and I have to handle conflicts.

So what is there now is it.

Take it or leave it - I will not be working to move this further along your apparenly very long road into the repository.

Thank you for your time so far.

[cheenamalhotra]

Hi @bjornbouetsmith

Apologies I missed your earlier message.

I'd just like to state, in order to make a public API introduction like this one, we need a stronger case and not enough people have shown interest in this PR so far, neither in issue #846. We don't want to discourage customers, but this is the reason why this feature loses priority over other customer issues. If there had been a lot of interest from community, this PR would have got desired attention as it would have bubbled up priority score.

Thanks for your suggestion, if we get more interest on issue #846 , we will consider re-opening this again!

[Wraith2]

How much external attention is required to get a new function added? There are a few things I'd like to be able to add or support adding (like these functions) but the number of people involved in this repository for anything other than bug reporting is extremely low so getting more than one of two upvotes is very unlikely.

Just how big a crowd do I need to hire to get a new feature added?

[cheenamalhotra]

@Wraith2 it depends on various factors.

This PR introduces a new SqlDbType and also a new API to append the core driver usability on DataReader where no one else has yet raised a concern and alternate solutions already exist, so it does require strong support in order to move it forward. Our customer base also includes SSMS, Azure Data Factory, etc, and we have not heard any such need, so it's very unlikely there's a requirement for a new API.

Also if you see in issue #1009 (comment) discussions, the trends do not favor adding new methods/APIs for each and every type that don't add considerable benefit and are proceeding to generic behaviors. So unless and until there's a huge roadblock impacting a major customer base, we'd also know from within, and only then such major decisions can be made. cc @David-Engel @saurabh500

[Wraith2]

I think that many consumers assume that the api's for dealing with extra types won't be added because it's been so long since they were made available in sql server and they're still not in this driver. There are very few people interested in ADO advancements because they've been ignored for so long so for any api proposal outside the MS teams associated with the product there are very few people who are going to see it. Realistically how many frequent users are there on this particular product? half a dozen at most and I'd struggle to name many beyond Erik and myself.

I intend to implement the additional api we recently investigated with named parameters in large parameter sets. Do I need to get signoff on that before starting? And if so how do i go about doing that and how much public support do I ned to demonstrate that it is worth adding? The report we investigated was the only one that I've seen on the topic so it's low use but it's a clear improvement to the driver and allows big data users to be more efficient.

To return to the datetime2 api in this PR it isn't feasible to add it to the GetFieldValue members because it returns a specialized structure due to the range differences. If it were possible it would have been suggested. I see this approach as the only way to add it and it's disappointing that we can't get it included.

[cheenamalhotra]

I see this approach as the only way to add it and it's disappointing that we can't get it included.

I'd like to re-iterate, I didn't mean to discard the PR or API neither did I say it cannot be included.

As I said before, this wasn't in our priority list due to not enough demand and already available workarounds, but it has been reviewed for sanitization. We have lots of priority customer issues that eventually push this behind to be polished and considered for a release milestone.