feat(): add support for DateOnly and TimeOnly #680

[joefeser]

Description

Add support for DateOnly and TimeOnly that were both added in .net 6

Issues Resolved

Closes #680.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[Xtansia]

Thanks for tackling this @joefeser!
Some relatively small issues other than those already commented in particular files:

• Looks like there's a compile error with the change in SDK.
• Please add an entry to the CHANGELOG.md under the ### Added section
• The DCO check requires commits to be signed off, you can do this for your existing commits via git rebase HEAD~2 --signoff and then make sure to pass --signoff to any future commits for this repo

[Xtansia]

The github workflow setup-dotnet steps can be updated to change the sdk versions being installed.

[joefeser]

Resolved

[Xtansia]

Please remove the .ToArray(), this should be using the constructor here: https://github.com/opensearch-project/opensearch-net/blob/main/src/OpenSearch.Net/Transport/PostData.cs#L149

You'll just need to update the #ifs further down.

[joefeser]

I think I understood this comment, it is in this commit bcbd1a2

[Xtansia]

Please move these #pragmas below the license header

[joefeser]

Resolved

[joefeser]

Working on the changes as we speak. Thank you for being so proactive on my PR

[joefeser]

Thanks for tackling this @joefeser! Some relatively small issues other than those already commented in particular files:

• Looks like there's a compile error with the change in SDK.
• Please add an entry to the CHANGELOG.md under the ### Added section
• The DCO check requires commits to be signed off, you can do this for your existing commits via git rebase HEAD~2 --signoff and then make sure to pass --signoff to any future commits for this repo

All of these tasks are completed

[opensearch-trigger-bot]

The backport to 1.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.x 1.x
# Navigate to the new working tree
cd .worktrees/backport-1.x
# Create a new branch
git switch --create backport/backport-734-to-1.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 f3bae74a0e5f7146127262b566259898d8a45ebc
# Push it to GitHub
git push --set-upstream origin backport/backport-734-to-1.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.x

Then, create a pull request where the base branch is 1.x and the compare/head branch is backport/backport-734-to-1.x.

[Xtansia]

Thank you for this @joefeser, would you mind making a follow up PR adding the DateOnly and TimeOnly types into the unit test here: https://github.com/opensearch-project/opensearch-net/blob/main/tests/Tests.Reproduce/DateSerialization.cs

[joefeser]

@Xtansia I will add it in the next few days. I wasn't sure where to write the tests. I am going to do a POC also to add NodaTime support and at least get that conversation going

[nalka0]

I have played a bit with this PR and I think we still have some likely problems :

• If you don't do anything to the mappings, your TimeOnly fields are mapped as strings with

"type": "text",
"fields": {
    "keyword": {
        "type": "keyword",
        "ignore_above": 256
    }
}

instead of something with "type": "date"

• If you use openSearchClient.Indices.CreateAsync(indexName, x => x.Map(x => x.AutoMap(typeof(YourType)))) your DateOnly and TimeOnly properties have no way to be inferred and will be still be treated as "normal" objects here.

  opensearch-net/src/OpenSearch.Client/Mapping/Visitor/PropertyWalker.cs

  Line 197 in c2fdd5d

  return new ObjectProperty();

I tried fixing this second issue and for DateOnly, it seems like adding a case "DateOnly" next to the DateTime one above on the method I linked is sufficient.

On the other hand for TimeOnly it seems like we need more than that since you get a parsing error when trying to put data in your index. From my understanding, the format is responsible there but adding the following case

case "TimeOnly":
    return new DateProperty()
    {
        Format = "strict_hour_minute_second_millis"
    };

seems unsuccesful (for some reasons, doing Format = ... there doesn't seem to change anything to the resulting mappings).

PS : I pulled the value for Format = here but am unsure if strict_hour_minute_second_millis is really the best fit