Fix parsing of fractional seconds. by igorpeshansky · Pull Request #65 · Stackdriver/metadata-agent

igorpeshansky · 2018-03-02T23:57:55Z

This addresses cases when fractional seconds aren't present, or when fractional seconds don't have exactly 9 digits.

bmoyles0117 · 2018-03-03T00:48:51Z

What do you think about failing over to a secondary parser instead? If we can parse with nanoseconds, great, otherwise failover to without nanoseconds, or vice versa. It'd be nice to stick with the strptime utility instead of custom logic.

igorpeshansky · 2018-03-03T01:24:54Z

There's no secondary parser — strptime does not support fractional seconds.
We could exclude seconds from the strptime format and then parse seconds and nanoseconds together as a double using strtod, which would avoid the whole length computation, but then we'd have to worry about funky formats like hex and floating point values like "+inf", so the tradeoff is not obvious. I could try it and see, I suppose...

igorpeshansky · 2018-03-04T05:52:10Z

I've tried it, and it would be fairly hard to detect cases like "2018-03-03T01:23:00045.6789" (i.e., 5 digits for the seconds) with the new approach (see the failing test). I'll leave the commit around in case you want to look at it, but I would lean towards removing it and sticking with the original implementation for now.

supriyagarg

The original implementation LGTM. Minor comment.

supriyagarg · 2018-03-05T20:26:55Z

+    if (length > 9) {
+      // More digits than can be stored as nanoseconds.
+      // TODO: Should this round (std::lround)?
+      ns /= Exp10(length - 9);


round the value, rather than take floor?

That would require changing the test as well. Which semantics do we want?

igorpeshansky

Turns out it wasn't that hard to catch the cases of more than 2 digits in seconds. So the new implementation is viable as well. Would really like your opinion on which one is better/more readable. PTAL.

igorpeshansky · 2018-03-06T04:35:04Z

+    if (length > 9) {
+      // More digits than can be stored as nanoseconds.
+      // TODO: Should this round (std::lround)?
+      ns /= Exp10(length - 9);


That would require changing the test as well. Which semantics do we want?

supriyagarg · 2018-03-06T18:43:40Z

    return std::chrono::system_clock::time_point();
  }
+  tm.tm_sec = sec_i;
+  long ns = std::lround((seconds - sec_i) * 10000000000) / 10;


The new implementation looks good - much simpler.
Please add a test for the rounding logic

This is actually truncation logic. There just isn't an std::ltrunc, so I use std::lround and then truncate the last int digit. Added a comment.

bmoyles0117

LGTM, one comment / concern that may not be valid.

bmoyles0117 · 2018-03-06T21:01:19Z

-  if (zone <= end + 1 || *zone != 'Z' || *(zone+1) != '\0') {
-    // TODO
+  double seconds = std::strtod(end, &zone);
+  if (sec_i < 0 || sec_i != static_cast<long>(seconds)) {


Only minor note, I couldn't find a correlating unit test to this conditional statement. Every time there was a strange value in the place of the seconds, it was longer than 2 characters, that would have been caught by the previous condition. Might be overlooking the unit test that covers this case, just wanted to make a note.

Good catch. A negative value would be caught by the earlier isdigit check, so the first test is useless.
The second test was intended to catch a double in the scientific notation. However, adding e+0 to an integer produces a double that is equal to that integer. I've added a test, but if fails with the new implementation. Unfortunately, there are way too many valid formats that strtod accepts, so it seems like I'm blacklisting them one-by-one, rather than whitelisting the one format I know I'll want.
The new test passes with the original implementation, so let's just go with that one and clean it up later. WDYT?

Managed to get it to work. Back to the question of which one's more readable.

igorpeshansky

Thanks for the reviews. I'm still struggling with the new implementation. I'll take one more stab at it later today, but we can always fall back to the original one.

igorpeshansky · 2018-03-07T21:11:25Z

-  if (zone <= end + 1 || *zone != 'Z' || *(zone+1) != '\0') {
-    // TODO
+  double seconds = std::strtod(end, &zone);
+  if (sec_i < 0 || sec_i != static_cast<long>(seconds)) {


Good catch. A negative value would be caught by the earlier isdigit check, so the first test is useless.
The second test was intended to catch a double in the scientific notation. However, adding e+0 to an integer produces a double that is equal to that integer. I've added a test, but if fails with the new implementation. Unfortunately, there are way too many valid formats that strtod accepts, so it seems like I'm blacklisting them one-by-one, rather than whitelisting the one format I know I'll want.
The new test passes with the original implementation, so let's just go with that one and clean it up later. WDYT?

igorpeshansky · 2018-03-07T21:34:41Z

    return std::chrono::system_clock::time_point();
  }
+  tm.tm_sec = sec_i;
+  long ns = std::lround((seconds - sec_i) * 10000000000) / 10;


This is actually truncation logic. There just isn't an std::ltrunc, so I use std::lround and then truncate the last int digit. Added a comment.

igorpeshansky

The new implementation now passes the tests again. Let's figure out which one's more readable and use that as the baseline -- we can tweak it going forward.

igorpeshansky · 2018-03-08T01:07:59Z

-  if (zone <= end + 1 || *zone != 'Z' || *(zone+1) != '\0') {
-    // TODO
+  double seconds = std::strtod(end, &zone);
+  if (sec_i < 0 || sec_i != static_cast<long>(seconds)) {


Managed to get it to work. Back to the question of which one's more readable.

igorpeshansky · 2018-03-09T23:18:51Z

I've updated the new implementation to actually use the full time spec as well -- my one concern with the earlier implementation was that we cut off the seconds and parse them separately, which is now fixed. @bmoyles0117 @supriyagarg PTAL.

supriyagarg

Thanks - this looks much better! The commit title was a bit confusing, but using the full time spec sounds good.

bmoyles0117 · 2018-03-09T23:42:06Z

+    (void)std::strtol(end + 1, &zone, 10);
+    if (zone <= end + 1) {
+      // TODO: Missing nanoseconds.
+      return std::chrono::system_clock::time_point();


Is it not safe to assume 0 for nanoseconds?

It depends on whether we treat a trailing decimal point with no digits after as valid input or an error. As I read the spec, a decimal point must be followed by at least one digit (search for time-fraction).

bmoyles0117 · 2018-03-09T23:43:01Z

+    // TODO: Internal error.
    return std::chrono::system_clock::time_point();
  }
+  static_assert(sizeof(long) == 8, "long is too small");


When would this happen?

For example with a 32-bit compiler. I'd rather fail compilation if someone ever tries to build for Amazon's 32-bit AMI or Windows than silently overflow.

igorpeshansky

PTAL.

igorpeshansky · 2018-03-10T00:19:45Z

+    (void)std::strtol(end + 1, &zone, 10);
+    if (zone <= end + 1) {
+      // TODO: Missing nanoseconds.
+      return std::chrono::system_clock::time_point();


It depends on whether we treat a trailing decimal point with no digits after as valid input or an error. As I read the spec, a decimal point must be followed by at least one digit (search for time-fraction).

igorpeshansky · 2018-03-10T00:39:52Z

+    // TODO: Internal error.
    return std::chrono::system_clock::time_point();
  }
+  static_assert(sizeof(long) == 8, "long is too small");


For example with a 32-bit compiler. I'd rather fail compilation if someone ever tries to build for Amazon's 32-bit AMI or Windows than silently overflow.

bmoyles0117

LGTM

…double.

…uble.

igorpeshansky · 2018-03-13T15:11:07Z

Rebased off master. Merging.

igorpeshansky requested review from bmoyles0117 and supriyagarg March 2, 2018 23:57

igorpeshansky force-pushed the igorp-fix-timestamp-parsing branch 4 times, most recently from 4fcfa4c to ac5966e Compare March 4, 2018 05:48

supriyagarg reviewed Mar 5, 2018

View reviewed changes

igorpeshansky force-pushed the igorp-fix-timestamp-parsing branch from 3a267c6 to ac3797d Compare March 6, 2018 04:34

igorpeshansky commented Mar 6, 2018

View reviewed changes

supriyagarg reviewed Mar 6, 2018

View reviewed changes

bmoyles0117 approved these changes Mar 6, 2018

View reviewed changes

igorpeshansky force-pushed the igorp-fix-timestamp-parsing branch from 288d579 to fa90d5a Compare March 7, 2018 23:31

igorpeshansky commented Mar 7, 2018

View reviewed changes

igorpeshansky commented Mar 8, 2018

View reviewed changes

supriyagarg approved these changes Mar 9, 2018

View reviewed changes

bmoyles0117 reviewed Mar 9, 2018

View reviewed changes

igorpeshansky commented Mar 10, 2018

View reviewed changes

bmoyles0117 approved these changes Mar 13, 2018

View reviewed changes

igorpeshansky added 8 commits March 13, 2018 11:04

Add initial unit test for time.cc. Fix a subtle bug exposed by the test.

4091724

Add test/.gitignore; normalize src/.gitignore.

6eaf96a

Allow running "make test" in src/.

35701a5

Handle timestamps without nanoseconds.

615e6c6

Ensure fractional seconds are converted to nanoseconds.

b0501e0

Ensure nanos start with a digit.

bd90968

Add more tests for timestamp parsing.

d0d054a

Even more tests.

f715f2d

igorpeshansky added 7 commits March 13, 2018 11:04

Add testing instructions to README.md.

a45f57a

An alternate implementation that parses seconds and nanoseconds as a …

51ba2f3

…double.

Ensure that seconds is exactly 2 digits.

9a46457

Add more tests.

256eb2b

Remove useless check and add a clarifying comment.

bcc75b6

Ensure that the time is in the right format before parsing it as a do…

603e1ac

…uble.

Keep the original time format.

e70bb6c

igorpeshansky force-pushed the igorp-fix-timestamp-parsing branch from bd8316f to e70bb6c Compare March 13, 2018 15:04

igorpeshansky merged commit 1e29f74 into master Mar 13, 2018

igorpeshansky deleted the igorp-fix-timestamp-parsing branch March 13, 2018 15:15

Conversation

igorpeshansky commented Mar 2, 2018

Uh oh!

bmoyles0117 commented Mar 3, 2018

Uh oh!

igorpeshansky commented Mar 3, 2018

Uh oh!

igorpeshansky commented Mar 4, 2018

Uh oh!

supriyagarg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorpeshansky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmoyles0117 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorpeshansky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorpeshansky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorpeshansky commented Mar 9, 2018

Uh oh!

supriyagarg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorpeshansky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmoyles0117 left a comment

Choose a reason for hiding this comment

Uh oh!

igorpeshansky commented Mar 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants