Skip to content

MINIFICPP-1293 Fix the failing PropertyTests#840

Closed
fgerlits wants to merge 2 commits intoapache:mainfrom
fgerlits:MINIFICPP-1293
Closed

MINIFICPP-1293 Fix the failing PropertyTests#840
fgerlits wants to merge 2 commits intoapache:mainfrom
fgerlits:MINIFICPP-1293

Conversation

@fgerlits
Copy link
Copy Markdown
Contributor

@fgerlits fgerlits commented Jul 15, 2020

See https://issues.apache.org/jira/browse/MINIFICPP-1293 for the description of the bug.

Description of the change: instead of using mktime and adjusting by the time zone offset, use _mkgmtime on Windows where it is available, and use a hand-written version of mkgmtime elsewhere.


Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

  • Is there a JIRA ticket associated with this PR? Is it referenced
    in the commit message?

  • Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.

  • Has your PR been rebased against the latest commit within the target branch (typically master)?

  • Is your initial contribution a single, squashed commit?

For code changes:

  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
  • If applicable, have you updated the LICENSE file?
  • If applicable, have you updated the NOTICE file?

For documentation related changes:

  • Have you ensured that format looks appropriate for the output in which it is rendered?

Note:

Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.

Instead of using mktime and adjusting by the time zone offset, use
_mkgmtime on Windows where it is available, and a hand-written version
of mkgmtime elsewhere.
Copy link
Copy Markdown
Contributor

@arpadboda arpadboda left a comment

Choose a reason for hiding this comment

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

LGTM!

Remove some complexity caused by copy-pasting.
return -1;
}
return time + timezone_offset;
return static_cast<int64_t>(mkgmtime(&timeinfo));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Instead of providing mkgmtime on posix, I would opt for converting parseDateTimeStr to use strptime on posix and use the current implementation with a direct call to _mkgmtime on windows. This way we can avoid maintaining date/time code, which is a pain.

Copy link
Copy Markdown
Contributor Author

@fgerlits fgerlits Jul 16, 2020

Choose a reason for hiding this comment

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

strptime is orthogonal to this change, as strptime converts string -> struct tm, and the problem is with the struct tm -> UTC time_t conversion.

Date/time functions are a pain because of time zones, DST, and leap seconds; mkgmtime (mercifully) does not need to deal with any of these. The formula set by Pope Gregory XIII in 1582 is unlikely to change anytime soon.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In C++20, there will (finally) be some usable date-time parsing functions; something like

stringstream stream{"2020-08-01T01:00:00Z"};
utc_time<seconds> date_time;
stream >> parse("%Y-%m-%dT%H:%M:%SZ", date_time);
return date_time.time_since_epoch().count();

but I haven't been able to find a compiler which can compile this, yet.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  1. Sorry, I got too carried away with the UTC/localtime issue that I didn't even look at the signature of strptime.
  2. I believe mkgmtime / timegm (code, don't use this) have to deal with some of these issues, too, e.g. leap seconds occur in UTC.
  3. You might find parse interesting from HowardHinnant/date, the basis of C++20 date extensions.

Copy link
Copy Markdown
Contributor Author

@fgerlits fgerlits Jul 16, 2020

Choose a reason for hiding this comment

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

I didn't know about timegm; we could use that on Linux, but we'd still need the rest of the code for non-Linux non-Windows compiles (I guess that's why you said don't use it).

Leap seconds occur in UTC, but not in Unix time. This means that arguably parseDateTimeStr("2016-12-31T23:59:60Z") should return 1483228799 (ie. the same as 2016-12-31T23:59:59Z) instead of 1483228800 (ie. the same as 2017-01-01T00:00:00Z). But mktime doesn't do this, either, so the old parseDateTimeStr before this pull request also returned 1483228800.

I would not bring in a new library just to fix a bug that only affects dates on 1 January 1970. If you hate this change too much, my preferred second best solution would be to leave parseDateTimeStr as it is, and document this limitation ("doesn't work for some times on 1 January 1970 in some time zones on some architectures") in the unit test.

I understand your point, and agree that MiNiFi code should not contain an implementation of the Gregorian calendar, as it is not its responsibility, but I think having it temporarily until we can upgrade to C++20 is not terrible.

@arpadboda arpadboda closed this in 75220e2 Jul 16, 2020
@fgerlits fgerlits deleted the MINIFICPP-1293 branch July 17, 2020 07:35
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.

3 participants