Fixed wrong counting on leap days #6676

south37 · 2019-09-20T10:44:56Z

REF

Google::Protobuf::Timestamp.decode_json sometimes return incorrect timestamp values in Ruby #6616

WHY

In current implementation of protobuf/ruby, the result of counting leap days was wrong.
This caused the wrong result of Google::Protobuf::Timestamp.decode_json.

[1] pry(main)> t = Google::Protobuf::Timestamp.decode_json('"1993-02-10T00:00:00.000+00:00"'); Time.at(t.seconds)
=> 1993-02-09 00:00:00 +0000  # This is the wrong result. The input is 1993-02-10, but the decoded value is 1993-02-09.

Here are some other examples.

[2] pry(main)> [*1900..2010].each { |year| t = Google::Protobuf::Timestamp.decode_json("\"#{year}-02-10T00:00:00.000+00:00\""); print Time.at(t.seconds).to_s + "#{'  # wrong' if Time.at(t.seconds).day != 10 }\n";  }

1900-02-10 09:00:00 +0900
1901-02-10 09:00:00 +0900
1902-02-10 09:00:00 +0900
1903-02-11 09:00:00 +0900  # wrong
1904-02-11 09:00:00 +0900  # wrong
1905-02-10 09:00:00 +0900
1906-02-10 09:00:00 +0900
1907-02-11 09:00:00 +0900  # wrong
1908-02-11 09:00:00 +0900  # wrong
1909-02-10 09:00:00 +0900
1910-02-10 09:00:00 +0900
1911-02-11 09:00:00 +0900  # wrong
1912-02-11 09:00:00 +0900  # wrong
1913-02-10 09:00:00 +0900
1914-02-10 09:00:00 +0900
1915-02-11 09:00:00 +0900  # wrong
1916-02-11 09:00:00 +0900  # wrong
1917-02-10 09:00:00 +0900
1918-02-10 09:00:00 +0900
1919-02-11 09:00:00 +0900  # wrong
1920-02-11 09:00:00 +0900  # wrong
1921-02-10 09:00:00 +0900
1922-02-10 09:00:00 +0900
1923-02-11 09:00:00 +0900  # wrong
1924-02-11 09:00:00 +0900  # wrong
1925-02-10 09:00:00 +0900
1926-02-10 09:00:00 +0900
1927-02-11 09:00:00 +0900  # wrong
1928-02-11 09:00:00 +0900  # wrong
1929-02-10 09:00:00 +0900
1930-02-10 09:00:00 +0900
1931-02-11 09:00:00 +0900  # wrong
1932-02-11 09:00:00 +0900  # wrong
1933-02-10 09:00:00 +0900
1934-02-10 09:00:00 +0900
1935-02-11 09:00:00 +0900  # wrong
1936-02-11 09:00:00 +0900  # wrong
1937-02-10 09:00:00 +0900
1938-02-10 09:00:00 +0900
1939-02-11 09:00:00 +0900  # wrong
1940-02-11 09:00:00 +0900  # wrong
1941-02-10 09:00:00 +0900
1942-02-10 09:00:00 +0900
1943-02-11 09:00:00 +0900  # wrong
1944-02-11 09:00:00 +0900  # wrong
1945-02-10 09:00:00 +0900
1946-02-10 09:00:00 +0900
1947-02-11 09:00:00 +0900  # wrong
1948-02-11 09:00:00 +0900  # wrong
1949-02-10 09:00:00 +0900
1950-02-10 09:00:00 +0900
1951-02-11 09:00:00 +0900  # wrong
1952-02-11 09:00:00 +0900  # wrong
1953-02-10 09:00:00 +0900
1954-02-10 09:00:00 +0900
1955-02-11 09:00:00 +0900  # wrong
1956-02-11 09:00:00 +0900  # wrong
1957-02-10 09:00:00 +0900
1958-02-10 09:00:00 +0900
1959-02-11 09:00:00 +0900  # wrong
1960-02-11 09:00:00 +0900  # wrong
1961-02-10 09:00:00 +0900
1962-02-10 09:00:00 +0900
1963-02-11 09:00:00 +0900  # wrong
1964-02-11 09:00:00 +0900  # wrong
1965-02-10 09:00:00 +0900
1966-02-10 09:00:00 +0900
1967-02-11 09:00:00 +0900  # wrong
1968-02-11 09:00:00 +0900  # wrong
1969-02-10 09:00:00 +0900
1970-02-10 09:00:00 +0900
1971-02-10 09:00:00 +0900
1972-02-10 09:00:00 +0900
1973-02-09 09:00:00 +0900  # wrong
1974-02-10 09:00:00 +0900
1975-02-10 09:00:00 +0900
1976-02-10 09:00:00 +0900
1977-02-09 09:00:00 +0900  # wrong
1978-02-10 09:00:00 +0900
1979-02-10 09:00:00 +0900
1980-02-10 09:00:00 +0900
1981-02-09 09:00:00 +0900  # wrong
1982-02-10 09:00:00 +0900
1983-02-10 09:00:00 +0900
1984-02-10 09:00:00 +0900
1985-02-09 09:00:00 +0900  # wrong
1986-02-10 09:00:00 +0900
1987-02-10 09:00:00 +0900
1988-02-10 09:00:00 +0900
1989-02-09 09:00:00 +0900  # wrong
1990-02-10 09:00:00 +0900
1991-02-10 09:00:00 +0900
1992-02-10 09:00:00 +0900
1993-02-09 09:00:00 +0900  # wrong
1994-02-10 09:00:00 +0900
1995-02-10 09:00:00 +0900
1996-02-10 09:00:00 +0900
1997-02-09 09:00:00 +0900  # wrong
1998-02-10 09:00:00 +0900
1999-02-10 09:00:00 +0900
2000-02-10 09:00:00 +0900
2001-02-09 09:00:00 +0900  # wrong
2002-02-10 09:00:00 +0900
2003-02-10 09:00:00 +0900
2004-02-10 09:00:00 +0900
2005-02-09 09:00:00 +0900  # wrong
2006-02-10 09:00:00 +0900
2007-02-10 09:00:00 +0900
2008-02-10 09:00:00 +0900
2009-02-09 09:00:00 +0900  # wrong
2010-02-10 09:00:00 +0900

WHAT

I fixed the logic to count leap days.

googlebot · 2019-09-20T10:45:01Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

south37 · 2019-09-20T10:52:19Z

@googlebot I signed it!

googlebot · 2019-09-20T10:52:23Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

TeBoring · 2019-09-23T17:17:15Z

ruby/ext/google/protobuf_c/upb.c

@@ -10124,6 +10124,10 @@ static bool isleap(int year) {
  return (year % 4) == 0 && (year % 100 != 0 || (year % 400) == 0);
 }

+static int isleap_count(int year) {


Add a comment about what 'year' mean and what the returned value mean

OK! I renamed to leap_count and added a comment! 💡
6b4224c

TeBoring · 2019-09-23T17:21:22Z

ruby/ext/google/protobuf_c/upb.c

@@ -10134,7 +10138,7 @@ const unsigned short int __mon_yday[2][13] = {
 int64_t epoch(int year, int yday, int hour, int min, int sec) {
  int64_t years = year - EPOCH_YEAR;

-  int64_t leap_days = years / 4 - years / 100 + years / 400;
+  int64_t leap_days = isleap_count(year - 1) - isleap_count(EPOCH_YEAR - 1);


Why not putting year - 1 into leap_count?

Could you also add a test in

protobuf/conformance/binary_json_conformance_suite.cc

Line 2704 in 7114d73

RunValidJsonTest(

Could you also port your code to https://github.com/protocolbuffers/upb/blob/d3762e96cb8910b8281419faee7bd70b323c753d/upb/json/parser.rl#L1707
and https://github.com/protocolbuffers/protobuf/blob/master/php/ext/google/protobuf/upb.c#L10137

Why not putting year - 1 into leap_count?

static int leap_count(int year) returns the number of leap days between 1 and year.
Therefore, this logic calculated the number of leap days between 1970 and year - 1.

The relation between is_leap and leap_count is shown below.

is_leap(1) => false is_leap(2) => false is_leap(3) => false is_leap(4) => true is_leap(5) => false is_leap(6) => false is_leap(7) => false is_leap(8) => true leap_count(1) => 0 leap_count(2) => 0 leap_count(3) => 0 leap_count(4) => 1 leap_count(5) => 1 leap_count(6) => 1 leap_count(7) => 1 leap_count(8) => 2

Could you also add a test in

OK! I added a new test case!
63837ef

Could you also port your code to https://github.com/protocolbuffers/upb/blob/d3762e96cb8910b8281419faee7bd70b323c753d/upb/json/parser.rl#L1707
and https://github.com/protocolbuffers/protobuf/blob/master/php/ext/google/protobuf/upb.c#L10137

OK! I ported the same code to php!
3574836

I'll make another PR for https://github.com/protocolbuffers/upb .

haberman · 2019-09-23T17:41:03Z

I reworked this code a bit for a new JSON parser I have in progress: https://github.com/haberman/upb/blob/4a2b7d721ad8b210fedda33251d30d3eaa235c73/upb/json_parser.c#L1017-L1038

Could you see if the new code fixes the problem? If so, could we use this new, simpler code instead?

If the new code doesn't fix the problem, that would be good to know too.

haberman · 2019-09-24T18:35:03Z

I created a new PR with the new, simplified code: #6693

south37 · 2019-09-27T15:32:37Z

I reworked this code a bit for a new JSON parser I have in progress: https://github.com/haberman/upb/blob/4a2b7d721ad8b210fedda33251d30d3eaa235c73/upb/json_parser.c#L1017-L1038

Could you see if the new code fixes the problem? If so, could we use this new, simpler code instead?

If the new code doesn't fix the problem, that would be good to know too.

Sorry for the too late reply 🙇

I also tested the haberman:rubyjson2 of #6695 and confirmed that the new code fixed the problem ! 💡
I think it's OK! 😄

vagrant@vagrant:~/haberman/protobuf/ruby$ git branch
  master
* rubyjson2

vagrant@vagrant:~/haberman/protobuf/ruby$ pry
[1] pry(main)> require 'google/protobuf'
=> true
[2] pry(main)> require 'google/protobuf/timestamp_pb'
=> true

[3] pry(main)> [*1900..2010].each { |year| t = Google::Protobuf::Timestamp.decode_json("\"#{year}-02-10T00:00:00.000+00:00\""); print Time.at(t.seconds).to_s + "#{'  # wrong' if Time.at(t.seconds).day != 10 }\n";  }

1900-02-10 00:00:00 +0000
1901-02-10 00:00:00 +0000
1902-02-10 00:00:00 +0000
1903-02-10 00:00:00 +0000
1904-02-10 00:00:00 +0000
1905-02-10 00:00:00 +0000
1906-02-10 00:00:00 +0000
1907-02-10 00:00:00 +0000
1908-02-10 00:00:00 +0000
1909-02-10 00:00:00 +0000
1910-02-10 00:00:00 +0000
1911-02-10 00:00:00 +0000
1912-02-10 00:00:00 +0000
1913-02-10 00:00:00 +0000
1914-02-10 00:00:00 +0000
1915-02-10 00:00:00 +0000
1916-02-10 00:00:00 +0000
1917-02-10 00:00:00 +0000
1918-02-10 00:00:00 +0000
1919-02-10 00:00:00 +0000
1920-02-10 00:00:00 +0000
1921-02-10 00:00:00 +0000
1922-02-10 00:00:00 +0000
1923-02-10 00:00:00 +0000
1924-02-10 00:00:00 +0000
1925-02-10 00:00:00 +0000
1926-02-10 00:00:00 +0000
1927-02-10 00:00:00 +0000
1928-02-10 00:00:00 +0000
1929-02-10 00:00:00 +0000
1930-02-10 00:00:00 +0000
1931-02-10 00:00:00 +0000
1932-02-10 00:00:00 +0000
1933-02-10 00:00:00 +0000
1934-02-10 00:00:00 +0000
1935-02-10 00:00:00 +0000
1936-02-10 00:00:00 +0000
1937-02-10 00:00:00 +0000
1938-02-10 00:00:00 +0000
1939-02-10 00:00:00 +0000
1940-02-10 00:00:00 +0000
1941-02-10 00:00:00 +0000
1942-02-10 00:00:00 +0000
1943-02-10 00:00:00 +0000
1944-02-10 00:00:00 +0000
1945-02-10 00:00:00 +0000
1946-02-10 00:00:00 +0000
1947-02-10 00:00:00 +0000
1948-02-10 00:00:00 +0000
1949-02-10 00:00:00 +0000
1950-02-10 00:00:00 +0000
1951-02-10 00:00:00 +0000
1952-02-10 00:00:00 +0000
1953-02-10 00:00:00 +0000
1954-02-10 00:00:00 +0000
1955-02-10 00:00:00 +0000
1956-02-10 00:00:00 +0000
1957-02-10 00:00:00 +0000
1958-02-10 00:00:00 +0000
1959-02-10 00:00:00 +0000
1960-02-10 00:00:00 +0000
1961-02-10 00:00:00 +0000
1962-02-10 00:00:00 +0000
1963-02-10 00:00:00 +0000
1964-02-10 00:00:00 +0000
1965-02-10 00:00:00 +0000
1966-02-10 00:00:00 +0000
1967-02-10 00:00:00 +0000
1968-02-10 00:00:00 +0000
1969-02-10 00:00:00 +0000
1970-02-10 00:00:00 +0000
1971-02-10 00:00:00 +0000
1972-02-10 00:00:00 +0000
1973-02-10 00:00:00 +0000
1974-02-10 00:00:00 +0000
1975-02-10 00:00:00 +0000
1976-02-10 00:00:00 +0000
1977-02-10 00:00:00 +0000
1978-02-10 00:00:00 +0000
1979-02-10 00:00:00 +0000
1980-02-10 00:00:00 +0000
1981-02-10 00:00:00 +0000
1982-02-10 00:00:00 +0000
1983-02-10 00:00:00 +0000
1984-02-10 00:00:00 +0000
1985-02-10 00:00:00 +0000
1986-02-10 00:00:00 +0000
1987-02-10 00:00:00 +0000
1988-02-10 00:00:00 +0000
1989-02-10 00:00:00 +0000
1990-02-10 00:00:00 +0000
1991-02-10 00:00:00 +0000
1992-02-10 00:00:00 +0000
1993-02-10 00:00:00 +0000
1994-02-10 00:00:00 +0000
1995-02-10 00:00:00 +0000
1996-02-10 00:00:00 +0000
1997-02-10 00:00:00 +0000
1998-02-10 00:00:00 +0000
1999-02-10 00:00:00 +0000
2000-02-10 00:00:00 +0000
2001-02-10 00:00:00 +0000
2002-02-10 00:00:00 +0000
2003-02-10 00:00:00 +0000
2004-02-10 00:00:00 +0000
2005-02-10 00:00:00 +0000
2006-02-10 00:00:00 +0000
2007-02-10 00:00:00 +0000
2008-02-10 00:00:00 +0000
2009-02-10 00:00:00 +0000
2010-02-10 00:00:00 +0000

So, this PR may not be necessary now 🙈

TeBoring · 2019-09-27T17:56:20Z

Still thanks for your help :)

Fixed wrong counting on leap days

7142ebc

googlebot added the cla: no label Sep 20, 2019

googlebot added cla: yes and removed cla: no labels Sep 20, 2019

TeBoring self-assigned this Sep 23, 2019

TeBoring reviewed Sep 23, 2019

View reviewed changes

TeBoring assigned haberman Sep 23, 2019

TeBoring added ruby kokoro:run labels Sep 23, 2019

protobuf-kokoro removed the kokoro:run label Sep 23, 2019

south37 added 3 commits September 27, 2019 22:36

Add explanation to leap_count

6b4224c

Add test case of TimestampWithYearAfterLeapYear

63837ef

Port leap_count to php

3574836

TeBoring closed this Sep 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed wrong counting on leap days #6676

Fixed wrong counting on leap days #6676

south37 commented Sep 20, 2019

googlebot commented Sep 20, 2019

south37 commented Sep 20, 2019

googlebot commented Sep 20, 2019

TeBoring Sep 23, 2019

TeBoring Sep 23, 2019

south37 Sep 27, 2019

TeBoring Sep 23, 2019

TeBoring Sep 23, 2019

TeBoring Sep 23, 2019

south37 Sep 27, 2019

south37 Sep 27, 2019

south37 Sep 27, 2019

haberman commented Sep 23, 2019 •

edited

haberman commented Sep 24, 2019

south37 commented Sep 27, 2019 •

edited

TeBoring commented Sep 27, 2019

Fixed wrong counting on leap days #6676

Fixed wrong counting on leap days #6676

Conversation

south37 commented Sep 20, 2019

REF

WHY

WHAT

googlebot commented Sep 20, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

south37 commented Sep 20, 2019

googlebot commented Sep 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haberman commented Sep 23, 2019 • edited

haberman commented Sep 24, 2019

south37 commented Sep 27, 2019 • edited

TeBoring commented Sep 27, 2019

haberman commented Sep 23, 2019 •

edited

south37 commented Sep 27, 2019 •

edited