Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201702131706.v1DH691P003804@ip-10-146-233-104.ec2.internal>
Date: Mon, 13 Feb 2017 17:06:09 +0000
From: "Jim Apple (Code Review)" <gerrit@cloudera.org>
To: Taras Bobrovytsky <tbobrovytsky@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Matthew Jacobs <mj@cloudera.com>, Alex Behm <alex.behm@cloudera.com>,
        Jim Apple <jbapple-impala@apache.org>
Reply-To: jbapple-impala@apache.org
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-4546=3A_Fix_Moscow_timezone_conversion_after_2014=0A?=
In-Reply-To: <gerrit.1486760606000.Id6e3f2c9f6ba29749a26bc1087e664637bc02528@gerrit.cloudera.org>
References: <gerrit.1486760606000.Id6e3f2c9f6ba29749a26bc1087e664637bc02528@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.7
archived-at: Mon, 13 Feb 2017 17:06:19 -0000

Jim Apple has posted comments on this change.

Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................


Patch Set 1:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/5969/1/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:

Line 3588:   // IMPALA-4209: Moscow time change in 2011.
Moscow could use a test on its own, rather than adding so many lines to TimestampFunctions.


PS1, Line 3590: cast(from_utc_timestamp('2010-10-30 22:59:00', "
              :       "'Europe/Moscow') as string)
This could be turned into a template of calling TestStringValue with "cast(to_utc_timestamp('" and "'Europe/Moscow') as string)" appearing just once in this file to make reading what each  test is supposed to do easier.


Line 3596:   TestIsNull("cast(to_utc_timestamp('2010-10-31 02:00:00', "
Each one of these lines could use a short comment like "spring forward into DST, so 2am to 3am did not exist" or "TZs with ST/DST transitions were discontiguous on this region"


http://gerrit.cloudera.org:8080/#/c/5969/1/be/src/exprs/timestamp-functions.cc
File be/src/exprs/timestamp-functions.cc:

PS1, Line 194: January 19, 1992 
What about in 1991?


http://gerrit.cloudera.org:8080/#/c/5969/1/be/src/exprs/timezone_db.h
File be/src/exprs/timezone_db.h:

PS1, Line 41: "tz"
'tz', because the parameter tz is a std::string and so "tz" is a valid value of 'tz'.


Line 45:   /// Moscow timezone UTC+3 with DST, for use before 27 March 2011.
Between 27 March 2011 and 26 October 2014, which timezone was Moscow in?


PS1, Line 48:  October 26 2014
"26 October 2014" for parallelism with above comment.


-- 
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbapple-impala@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-HasComments: Yes