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

feat: add support to filter by Instant, ZonedDateTime, OffsetDateTime, ... in LINQ #605

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Sylvain2703
Copy link

@Sylvain2703 Sylvain2703 commented Jan 8, 2024

Proposed Changes

Features

  • Add nanosecond precision support to the DateTimeLiteral AST type (thanks to NodaTime's Instant).
  • Add support in LINQ queries for:
    • NodaTime's Instant, ZonedDateTime, OffsetDateTime, OffsetDate, LocalDateTime and LocalDate,
    • .NET 6+ DateOnly (adding net6.0 to InfluxDB.Client.Linq's TargetFrameworks),
    • NodaTime's Duration and Period,
    • Tick precision for TimeSpan (only microsecond precision was supported),
    • Unsigned integers, sbyte, short and decimal.

Refactor

  • Use pattern matching when creating AST Expression (in VariableAggregator.CreateExpression()).
  • Remove a useless function call for DateTime conversion (in FluxRecord.GetTimeInDateTime()).

Tests

  • Add tests to cover new supported types and adjust existing ones for TimeSpan precision.

Checklist

  • CHANGELOG.md updated
  • Rebased/mergeable
  • A test has been added if appropriate
  • dotnet test completes successfully
  • Commit messages are conventional
  • Sign CLA (if not already signed)

@Sylvain2703 Sylvain2703 force-pushed the feat/better-datetime-and-duration-precision branch from 6645a51 to b57a9d2 Compare January 8, 2024 20:14
Copy link
Contributor

@bednar bednar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Sylvain2703,

First off, a big thank you for your pull request and the fantastic improvements you've made to this client! Your contribution is greatly appreciated, and it's wonderful to see such valuable enhancements. 👍

I do have one concern regarding the support for older .NET versions. Would it be possible to retain the <LangVersion> as 8 in the project configuration? We aim to continue supporting .NET Core 3.1, and keeping the language version to 8 would help ensure compatibility with this version of .NET Core.

Thanks again for your excellent work and looking forward to your input on this matter.

Best regards

@Sylvain2703
Copy link
Author

Sylvain2703 commented Jan 10, 2024

Hi @bednar,

Thank you very much for your feedback 😀

Regarding the C# version, I was surprised that this lib still uses C# 8 only... but I understood the reason when I saw the CI errors 😉
Fortunately, we can use newer versions of C# with older versions of .NET as soon as the .NET SDK provides support for the specified C# version.
All new C# features that don't require runtime changes will be supported (because the compiler produces compatible IL code with older .NET versions). Unsupported C# features will simply produce compilation errors.

In our case, C# 12 requires the .NET 8.0 SDK but can compile for .NET Core 3.1 (or even .NET Framework 4.*).
My main reason for using a newer C# version is the switch expression with logical operators and, or and not (used here) requires C# 9.

After checking the CI pipeline and trying to use the .NET 8 SDK, I realized that the tests cannot run because the .NET SDK Docker image only contains the runtime for the same version.
A solution will be to install .NET Core 3.1/5/6/7 runtimes on the .NET 8 SDK Docker image, as mentioned here.
What do you think about this approach?

Best regards

@Sylvain2703 Sylvain2703 force-pushed the feat/better-datetime-and-duration-precision branch 2 times, most recently from f098a8e to 34facc5 Compare January 11, 2024 10:27
@Sylvain2703 Sylvain2703 force-pushed the feat/better-datetime-and-duration-precision branch from 34facc5 to 427de6b Compare January 11, 2024 10:37
@bednar bednar changed the title Better time and duration precision feat: add support to filter by Instant, ZonedDateTime, OffsetDateTime, ... in LINQ Jan 24, 2024
@bednar
Copy link
Contributor

bednar commented Feb 1, 2024

Hi @Sylvain2703,

Apologies for my late response.

Thank you for thoroughly checking the CI pipeline and experimenting with the .NET 8 SDK. Your insights into the issue with the .NET SDK Docker image and the inability to run tests due to version-specific runtime constraints are invaluable.

Your proposed solution to install multiple .NET Core runtimes (3.1/5/6/7) on the .NET 8 SDK Docker image, as discussed in this StackOverflow answer, sounds like an excellent approach. It not only resolves our current issue but also simplifies future maintenance by ensuring compatibility across different .NET versions. This foresight is greatly appreciated and aligns well with our goals for the client's long-term usability and robustness.

We are more than happy to incorporate these changes into the client. Your initiative in finding and suggesting a viable solution is a significant contribution to the project. Thank you for your dedication and valuable input.

Best Regards

@Sylvain2703
Copy link
Author

Sylvain2703 commented Feb 4, 2024

Hi @bednar,

Thank you for your answer!

After experimenting a lot with CircleCI, I propose a slightly different but cleaner solution than the one mentioned previously.

Instead of installing additional .NET runtimes on the .NET 8 SDK Docker image (which was causing dependencies incompatibilities with the base image), I opened PR #613 to:

  1. Build the entire solution, in Release mode, with the .NET 8 SDK,
  2. Copy the resulting build for testing on .NET Core 3.1/5/6/7/8 Docker images and Windows execution environment,
  3. Deploy the tested build (instead of rebuilding/repacking it).

After completing PR #613, this PR should be able to pass the CI pipeline 😀

Best regards

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants