Fixes #2293: Enable DateOnly and TimeOnly

[xuzhg]

Issues

This pull request fixes #2293.

Description

OData.Spec defines the following two primitive types:

From OData model perspective, it only knows Edm.Date and Edm.TimeOfDay.

We'd resolve the mapping between the Edm types and C# types from the real value.

OData library maps 'Edm.Date' to 'Microsoft.OData.Edm.Date' struct and 'Edm.TimeOfDay' to 'Microsoft.OData.Edm.TimeOfDay' struct.

However, neither 'Date' type nor 'TimeOfDay' is supported in database side.

Now since .NET 6, two new primitive types have introduced into the 'System" namespace as:

System.TimeOnly

System.DateOnly

They are widely used in modern database side and EF core supports them. See at: dotnet/efcore#24507
and Here: https://learn.microsoft.com/en-us/ef/core/what-is-new/ef-core-8.0/breaking-changes

At OData side, we should support those two types as well.

1. At OData.Edm side, typically we don't need to change anything. Because it only knows the 'Edm.Date' and 'Edm.TimeOfDay' primitive types. But for conversion, we'd update 'Date' and 'TimeOfDay' struct type to support convert to 'System.DateOnly' and 'System.TimeOnly' respectively, implicitly.

2. At OData.Core side, we'd support to write the 'DateOnly' and 'TimeOnly' values, same as 'Date' and 'TimeOfDay' respectively.
   For reading, suppose we have a property at model side as:

    <Property Name='ValidDate' Type='Edm.Date' />

   So, if ODL reads the value of 'ValidDate' property, we can continue to read it as 'Date' instance and it should be able to convert it to 'System.DateOnly' implicitly.

3. At OData.Client side, we should support to create the property using the correct C# types (Microsoft.OData.Edm.Date, or System.DateOnly) during code generation. I'd suggest to add a flag/configuration for the OData Connected service to switch between them. This part is not included in this PR.

Checklist (Uncheck if it is not completed)

• Test cases added
• Build and test with one-click build and test script passed

Additional work necessary

If documentation update is needed, please add "Docs Needed" label to the issue and provide details about the required document change in the issue.

[JamesTerwilliger]

@xuzhg I left a few comments, but by and large this looks great, and I'm really grateful to see this.

[julealgon]

I confess that while reading this PR, it stuck out to me how many different places that need updating to introduce a new type. This feels less than ideal to me.

Of course, this has nothing to do with the changes themselves but with the overall design.

Added a few minor comments and suggestions, but don't see any major problems here.

[habbes]

I wonder whether it's necessary to cast to Date here? Could we consider creating Date-to-string helper function that could be used to ensure consistency for both Date and DateOnly but could allow us to work with DateOnly without having to cast to Date? I think if DateOnly is used often or the preferred choice in the future, we should consider having first-class support for it and avoid the overhead of casting to Date when possible. If that's a lot of work, it could be done in a follow-up PR.

[xuzhg]

In the next breaking change version (9 version), we should remove 'Date' 'TimeOfDay' and use 'DateOnly' and 'TimeOnly' types introduced in .NET.
In current 8 version, I try to avoid breaking change. And most important, we should make sure to get the same 'literal' string no matter whether customers are using 'Date' or 'DateOnly'.

DateOnly has several 'ToString()' overloads, the string literal varys along with the formatter provider. 'Cast' to Date then call 'ToString' on 'Date' is a 'center' method and make sure we get the same literal string no matter what type is. And to call 'ToString()' is a straightforward. There's no need to create a helper method to wrapper 'ToString()' again. What do you think?

[habbes]

I was curious about the potential overhead of casting to Edm.Date so I ran some tests to measure the ToString() scenario. To my surprise, casting to Edm.Date then call ToString() (i.e. ((Date)dateOnly).ToString() appears to be faster than dateOnly.ToString("yyyy-MM-dd", InvariantCulture).

This issue appears to be specific case of DateOnly.ToString(format, provider) overload which verifies that format is a valid date-only format, and this verification happens in O(n). You can bypass this overhead by converting the DateOnly to DateTime then calling DateTime.ToString("yyyy-MM-dd", InvariantCulture). This happens to be faster than casting to Edm.Date.

Here's the benchmark code.

This was meant to verify whether there's an actual overhead of casting DateOnly to Edm.Date and that we can avoid that perf overhead by handling DateOnly directly

[habbes]

Similar concerns as https://github.com/OData/odata.net/pull/3078/files#r1792817270 on whether we can avoid casting to TimeOfDay and handle TimeOnly directly.

[habbes]

Generally looks good to me. I would recommend creating a follow-up item to investigate whether we can get better performance by handling DateOnly and TimeOnly directly in OData.Core instead of casting to and from Edm.Date and Edm.TimeOfDay respectively.

[robbaman]

What does the release schedule for merged PR's look like? Can we expect this change to be included in the versions distributed via NuGet any time soon? Or is this part of a periodic release with a longer release window?

[xuzhg]

What does the release schedule for merged PR's look like? Can we expect this change to be included in the versions distributed via NuGet any time soon? Or is this part of a periodic release with a longer release window?

I think we will have a new version on nuget.org soon. We will try our best to unblock any customer.