Octonica / ClickHouseClient

ClickHouse .NET Core driver
Apache License 2.0
138 stars 23 forks source link

Fixed handling of DateTime64 dates on Windows systems #88

Closed MrDoe closed 6 months ago

MrDoe commented 6 months ago
victor-sushko commented 6 months ago

Thank you for your PR.

  1. The task of configuring ICU for timezones is out of scope of ClickHouseClient library. The package Microsoft.ICU.ICU4C.Runtime is a an application level level dependency. Thus, ClickHouseClient itself should not have a reference to this package;
  2. Also, the condition [MSBuild]::IsOSPlatform('Windows') checks an OS of a build machine, not the target runtime;
  3. netcoreapp3.1, net6.0, net8.0 are still supported;
  4. The actual fix works only if the precision of DateTime64 is 3;
  5. Tests don't pass.

For the above reasons, I can't accept your pull request, except for the typo fix.

MrDoe commented 6 months ago

Okay, never mind! Meanwhile I've found more issues with DateTime64 columns, so I switched over using NodaTime for handling dates in my fork, because the .NET functions are faulty when converting DateTimeOffsets before 1970 (I need this for birth dates) from UTC to a time zone with DST. Please take a look at my newest pull request which fixes that issue.