[Impala-ASF-CR] IMPALA-5599: Fix for mis-use of TimestampValue

2017-09-22 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8084 )

Change subject: IMPALA-5599: Fix for mis-use of TimestampValue
..

IMPALA-5599: Fix for mis-use of TimestampValue

The TimestampValue class is being used for non-database purposes
in many places, such as in log messages.

This change proposes to introduce APIs to convert Unix timetamps
into the corresponding date-time strings. We provide a series of
functions for different input time units, and also give the user
control over the precision of the output date-time string. APIs
are provided to format in UTC and local time zones. The new APIs
can be used to replace (or instead of) TimestampValue::ToString()
in those places where Unix timestamps are being converted to
strings for printing.

The current commit implements the APIs and replaces calls to
TimestampValue::ToString() in be/src/service.

A new unit test, time-test, has been added to the back-end tests.

Other uses of TimestampValue in be/src/service, such as to track
start and end times of queries, etc., will be analyzed and changed
as appropriate in a follow-up commit.

Change-Id: I9b0ae06f6d94968c87a199625aa3332b26988142
Reviewed-on: http://gerrit.cloudera.org:8080/8084
Reviewed-by: Dan Hecht 
Tested-by: Impala Public Jenkins
---
M be/src/service/impala-server.cc
M be/src/util/CMakeLists.txt
A be/src/util/time-test.cc
M be/src/util/time.cc
M be/src/util/time.h
5 files changed, 274 insertions(+), 13 deletions(-)

Approvals:
  Dan Hecht: Looks good to me, approved
  Impala Public Jenkins: Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I9b0ae06f6d94968c87a199625aa3332b26988142
Gerrit-Change-Number: 8084
Gerrit-PatchSet: 8
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Zoram Thanga

[Impala-ASF-CR] IMPALA-5599: Fix for mis-use of TimestampValue

2017-09-22 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8084 )

Change subject: IMPALA-5599: Fix for mis-use of TimestampValue
..


Patch Set 7: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b0ae06f6d94968c87a199625aa3332b26988142
Gerrit-Change-Number: 8084
Gerrit-PatchSet: 7
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Sat, 23 Sep 2017 02:46:00 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5599: Fix for mis-use of TimestampValue

2017-09-22 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8084 )

Change subject: IMPALA-5599: Fix for mis-use of TimestampValue
..


Patch Set 7:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1261/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b0ae06f6d94968c87a199625aa3332b26988142
Gerrit-Change-Number: 8084
Gerrit-PatchSet: 7
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 22 Sep 2017 22:43:59 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5599: Fix for mis-use of TimestampValue

2017-09-22 Thread Dan Hecht (Code Review) 
Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8084 )

Change subject: IMPALA-5599: Fix for mis-use of TimestampValue
..


Patch Set 7: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b0ae06f6d94968c87a199625aa3332b26988142
Gerrit-Change-Number: 8084
Gerrit-PatchSet: 7
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 22 Sep 2017 22:43:46 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5599: Fix for mis-use of TimestampValue

2017-09-22 Thread Zoram Thanga (Code Review) 
Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8084 )

Change subject: IMPALA-5599: Fix for mis-use of TimestampValue
..


Patch Set 6:

(1 comment)

Changed to C++ static_cast.

http://gerrit.cloudera.org:8080/#/c/8084/6/be/src/util/time-test.cc
File be/src/util/time-test.cc:

http://gerrit.cloudera.org:8080/#/c/8084/6/be/src/util/time-test.cc@85
PS6, Line 85: (time_t *)
> nit: per our style guide we use static_cast<> rather than C-style cast. sor
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b0ae06f6d94968c87a199625aa3332b26988142
Gerrit-Change-Number: 8084
Gerrit-PatchSet: 6
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 22 Sep 2017 21:33:20 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5599: Fix for mis-use of TimestampValue

2017-09-22 Thread Zoram Thanga (Code Review) 
Hello Matthew Jacobs, Dan Hecht,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8084

to look at the new patch set (#7).

Change subject: IMPALA-5599: Fix for mis-use of TimestampValue
..

IMPALA-5599: Fix for mis-use of TimestampValue

The TimestampValue class is being used for non-database purposes
in many places, such as in log messages.

This change proposes to introduce APIs to convert Unix timetamps
into the corresponding date-time strings. We provide a series of
functions for different input time units, and also give the user
control over the precision of the output date-time string. APIs
are provided to format in UTC and local time zones. The new APIs
can be used to replace (or instead of) TimestampValue::ToString()
in those places where Unix timestamps are being converted to
strings for printing.

The current commit implements the APIs and replaces calls to
TimestampValue::ToString() in be/src/service.

A new unit test, time-test, has been added to the back-end tests.

Other uses of TimestampValue in be/src/service, such as to track
start and end times of queries, etc., will be analyzed and changed
as appropriate in a follow-up commit.

Change-Id: I9b0ae06f6d94968c87a199625aa3332b26988142
---
M be/src/service/impala-server.cc
M be/src/util/CMakeLists.txt
A be/src/util/time-test.cc
M be/src/util/time.cc
M be/src/util/time.h
5 files changed, 274 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/84/8084/7
--
To view, visit http://gerrit.cloudera.org:8080/8084
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9b0ae06f6d94968c87a199625aa3332b26988142
Gerrit-Change-Number: 8084
Gerrit-PatchSet: 7
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Zoram Thanga

[Impala-ASF-CR] IMPALA-5599: Fix for mis-use of TimestampValue

2017-09-22 Thread Dan Hecht (Code Review) 
Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8084 )

Change subject: IMPALA-5599: Fix for mis-use of TimestampValue
..


Patch Set 6: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8084/6/be/src/util/time-test.cc
File be/src/util/time-test.cc:

http://gerrit.cloudera.org:8080/#/c/8084/6/be/src/util/time-test.cc@85
PS6, Line 85: (time_t *)
nit: per our style guide we use static_cast<> rather than C-style cast. sorry i 
missed that earlier.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b0ae06f6d94968c87a199625aa3332b26988142
Gerrit-Change-Number: 8084
Gerrit-PatchSet: 6
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 22 Sep 2017 21:14:53 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5599: Fix for mis-use of TimestampValue

2017-09-22 Thread Zoram Thanga (Code Review) 
Hello Matthew Jacobs, Dan Hecht,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8084

to look at the new patch set (#6).

Change subject: IMPALA-5599: Fix for mis-use of TimestampValue
..

IMPALA-5599: Fix for mis-use of TimestampValue

The TimestampValue class is being used for non-database purposes
in many places, such as in log messages.

This change proposes to introduce APIs to convert Unix timetamps
into the corresponding date-time strings. We provide a series of
functions for different input time units, and also give the user
control over the precision of the output date-time string. APIs
are provided to format in UTC and local time zones. The new APIs
can be used to replace (or instead of) TimestampValue::ToString()
in those places where Unix timestamps are being converted to
strings for printing.

The current commit implements the APIs and replaces calls to
TimestampValue::ToString() in be/src/service.

A new unit test, time-test, has been added to the back-end tests.

Other uses of TimestampValue in be/src/service, such as to track
start and end times of queries, etc., will be analyzed and changed
as appropriate in a follow-up commit.

Change-Id: I9b0ae06f6d94968c87a199625aa3332b26988142
---
M be/src/service/impala-server.cc
M be/src/util/CMakeLists.txt
A be/src/util/time-test.cc
M be/src/util/time.cc
M be/src/util/time.h
5 files changed, 274 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/84/8084/6
--
To view, visit http://gerrit.cloudera.org:8080/8084
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9b0ae06f6d94968c87a199625aa3332b26988142
Gerrit-Change-Number: 8084
Gerrit-PatchSet: 6
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Zoram Thanga

[Impala-ASF-CR] IMPALA-5599: Fix for mis-use of TimestampValue

2017-09-22 Thread Zoram Thanga (Code Review) 
Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8084 )

Change subject: IMPALA-5599: Fix for mis-use of TimestampValue
..


Patch Set 5:

(1 comment)

Fixed the test code as suggested. Please review latest patch.

http://gerrit.cloudera.org:8080/#/c/8084/5/be/src/util/time-test.cc
File be/src/util/time-test.cc:

http://gerrit.cloudera.org:8080/#/c/8084/5/be/src/util/time-test.cc@90
PS5, Line 90: EXPECT_EQ(string(now_buf), ToStringFromUnix(now_s));
> What I meant was:
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b0ae06f6d94968c87a199625aa3332b26988142
Gerrit-Change-Number: 8084
Gerrit-PatchSet: 5
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 22 Sep 2017 19:13:38 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5599: Fix for mis-use of TimestampValue

2017-09-22 Thread Dan Hecht (Code Review) 
Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8084 )

Change subject: IMPALA-5599: Fix for mis-use of TimestampValue
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8084/5/be/src/util/time-test.cc
File be/src/util/time-test.cc:

http://gerrit.cloudera.org:8080/#/c/8084/5/be/src/util/time-test.cc@90
PS5, Line 90: EXPECT_EQ(string(now_buf), ToStringFromUnix(now_s));
What I meant was:

EXPECT_EQ(string(now_buf), ToStringFromUnix(now_s)) << "now_s=" << now_s;



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b0ae06f6d94968c87a199625aa3332b26988142
Gerrit-Change-Number: 8084
Gerrit-PatchSet: 5
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 22 Sep 2017 16:31:17 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5599: Fix for mis-use of TimestampValue

2017-09-22 Thread Zoram Thanga (Code Review) 
Hello Matthew Jacobs, Dan Hecht,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8084

to look at the new patch set (#5).

Change subject: IMPALA-5599: Fix for mis-use of TimestampValue
..

IMPALA-5599: Fix for mis-use of TimestampValue

The TimestampValue class is being used for non-database purposes
in many places, such as in log messages.

This change proposes to introduce APIs to convert Unix timetamps
into the corresponding date-time strings. We provide a series of
functions for different input time units, and also give the user
control over the precision of the output date-time string. APIs
are provided to format in UTC and local time zones. The new APIs
can be used to replace (or instead of) TimestampValue::ToString()
in those places where Unix timestamps are being converted to
strings for printing.

The current commit implements the APIs and replaces calls to
TimestampValue::ToString() in be/src/service.

A new unit test, time-test, has been added to the back-end tests.

Other uses of TimestampValue in be/src/service, such as to track
start and end times of queries, etc., will be analyzed and changed
as appropriate in a follow-up commit.

Change-Id: I9b0ae06f6d94968c87a199625aa3332b26988142
---
M be/src/service/impala-server.cc
M be/src/util/CMakeLists.txt
A be/src/util/time-test.cc
M be/src/util/time.cc
M be/src/util/time.h
5 files changed, 284 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/84/8084/5
--
To view, visit http://gerrit.cloudera.org:8080/8084
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9b0ae06f6d94968c87a199625aa3332b26988142
Gerrit-Change-Number: 8084
Gerrit-PatchSet: 5
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Zoram Thanga

[Impala-ASF-CR] IMPALA-5599: Fix for mis-use of TimestampValue

2017-09-22 Thread Zoram Thanga (Code Review) 
Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8084 )

Change subject: IMPALA-5599: Fix for mis-use of TimestampValue
..


Patch Set 4:

(1 comment)

Made the last suggested change.

http://gerrit.cloudera.org:8080/#/c/8084/4/be/src/util/time-test.cc
File be/src/util/time-test.cc:

http://gerrit.cloudera.org:8080/#/c/8084/4/be/src/util/time-test.cc@85
PS4, Line 85: );
> for debugging, it might be helpful to log the 'now_s' raw value in case of
Done. Have added two extra checks for this purpose. The extra checks are on 
"now_s:date-time" strings from our API and strftime().



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b0ae06f6d94968c87a199625aa3332b26988142
Gerrit-Change-Number: 8084
Gerrit-PatchSet: 4
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 22 Sep 2017 06:25:08 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5599: Fix for mis-use of TimestampValue

2017-09-21 Thread Dan Hecht (Code Review) 
Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8084 )

Change subject: IMPALA-5599: Fix for mis-use of TimestampValue
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8084/4/be/src/util/time-test.cc
File be/src/util/time-test.cc:

http://gerrit.cloudera.org:8080/#/c/8084/4/be/src/util/time-test.cc@85
PS4, Line 85: );
for debugging, it might be helpful to log the 'now_s' raw value in case of 
failure, which you can do use << on the EXPECT_EQ()



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b0ae06f6d94968c87a199625aa3332b26988142
Gerrit-Change-Number: 8084
Gerrit-PatchSet: 4
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 21 Sep 2017 23:26:02 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5599: Fix for mis-use of TimestampValue

2017-09-21 Thread Zoram Thanga (Code Review) 
Hello Matthew Jacobs, Dan Hecht,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8084

to look at the new patch set (#4).

Change subject: IMPALA-5599: Fix for mis-use of TimestampValue
..

IMPALA-5599: Fix for mis-use of TimestampValue

The TimestampValue class is being used for non-database purposes
in many places, such as in log messages.

This change proposes to introduce APIs to convert Unix timetamps
into the corresponding date-time strings. We provide a series of
functions for different input time units, and also give the user
control over the precision of the output date-time string. APIs
are provided to convert to UTC and local time zones. The new APIs
can be used to replace (or instead of) TimestampValue::ToString()
in those places where Unix timestamps are being converted to
strings for printing.

The current commit implements the APIs and replaces calls to
TimestampValue::ToString() in be/src/service.

A new unit test, time-test, has been added to the back-end tests.

Other uses of TimestampValue in be/src/service, such as to track
start and end times of queries, etc., will be analyzed and changed
as required in a follow-up commit.

Change-Id: I9b0ae06f6d94968c87a199625aa3332b26988142
---
M be/src/service/impala-server.cc
M be/src/util/CMakeLists.txt
A be/src/util/time-test.cc
M be/src/util/time.cc
M be/src/util/time.h
5 files changed, 272 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/84/8084/4
--
To view, visit http://gerrit.cloudera.org:8080/8084
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9b0ae06f6d94968c87a199625aa3332b26988142
Gerrit-Change-Number: 8084
Gerrit-PatchSet: 4
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Zoram Thanga

[Impala-ASF-CR] IMPALA-5599: Fix for mis-use of TimestampValue

2017-09-19 Thread Dan Hecht (Code Review) 
Dan Hecht has posted comments on this change.

Change subject: IMPALA-5599: Fix for mis-use of TimestampValue
..


Patch Set 3:

(5 comments)

> (15 comments)
 > 
 > > (15 comments)
 > >
 > > Do you plan to take care of the other cases noted in the jira?
 > > Okay to do it in a follow on commit but wondering the plan.
 > >
 > Other cases, such as use of TimestampValue as a class member (see
 > ImpalaServer, for instance where it is used to track start and end
 > times of queries), need more analysis. The plan is to look at these
 > other cases in a separate commit.
 > 

Sounds good. Let's be sure to not resolve the jira until all the cases it lists 
are handled (or file a new jira to handle those).

 > > A unit test would be good for testing this kind of code.  You can
 > > take a look at the various *-test.cc files be/src/*/ for
 > examples.
 > 
 > Adding a unit test. Will refresh the patch again once I have it. In
 > the meantime, please let me know if I have addressed all of your
 > comments on the implementation.

Just a few more minor comments. I'll take another look after the unit test is 
added.

http://gerrit.cloudera.org:8080/#/c/8084/3/be/src/util/time.cc
File be/src/util/time.cc:

PS3, Line 33: time zone specified by the boolean argument 'utc'
since 'utc' doesn't tell you the other time zone, how about:

... in UTC if 'utc' is true, or the local time zone if 'utc' is false.

or similar.


PS3, Line 75: // Convert time point 't' into date-time string at precision 'p' 
in the local
: // time zone.
: static string ToString(const chrono::system_clock::time_point& t, 
TimePrecision p)
: {
:   stringstream ss;
:   ss << TimepointToString(t, false);
:   ss << FormatSubSecond(t, p);
:   return ss.str();
: }
: 
: // Convert time point 't' into date-time string at precision 'p' 
in the UTC
: // time zone.
: static string ToUtcString(const chrono::system_clock::time_point& 
t, TimePrecision p)
: {
:   stringstream ss;
:   ss << TimepointToString(t, true);
:   ss << FormatSubSecond(t, p);
:   return ss.str();
: }
up to you, but you could combine these into a single ToString() function that 
takes the bool utc, since these functions are the same otherwise.


http://gerrit.cloudera.org:8080/#/c/8084/3/be/src/util/time.h
File be/src/util/time.h:

PS3, Line 79: ,
maybe add: 's'
just to clarify which input.


PS3, Line 80: strin
string


PS3, Line 81: form
format


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9b0ae06f6d94968c87a199625aa3332b26988142
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5599: Fix for mis-use of TimestampValue

2017-09-19 Thread Zoram Thanga (Code Review) 
Zoram Thanga has uploaded a new patch set (#3).

Change subject: IMPALA-5599: Fix for mis-use of TimestampValue
..

IMPALA-5599: Fix for mis-use of TimestampValue

The TimestampValue class is being used for non-database purposes
in many places, such as in log messages.

This change proposes to introduce APIs to convert Unix timetamps
into the corresponding date-time strings. We provide a series of
functions for different input time units, and also give the user
control over the precision of the output date-time string. APIs
are provided to convert to UTC and local time zones. The new APIs
can be used to replace (or instead of) TimestampValue::ToString()
in those places where Unix timestamps are being converted to
strings for printing.

The current commit implements the APIs and replaces calls to
TimestampValue::ToString() in be/src/service.

Change-Id: I9b0ae06f6d94968c87a199625aa3332b26988142
---
M be/src/service/impala-server.cc
M be/src/util/time.cc
M be/src/util/time.h
3 files changed, 165 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/84/8084/3
-- 
To view, visit http://gerrit.cloudera.org:8080/8084
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9b0ae06f6d94968c87a199625aa3332b26988142
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Zoram Thanga

[Impala-ASF-CR] IMPALA-5599: Fix for mis-use of TimestampValue

2017-09-19 Thread Zoram Thanga (Code Review) 
Zoram Thanga has posted comments on this change.

Change subject: IMPALA-5599: Fix for mis-use of TimestampValue
..


Patch Set 2:

(15 comments)

> (15 comments)
 > 
 > Do you plan to take care of the other cases noted in the jira? 
 > Okay to do it in a follow on commit but wondering the plan.
 >
Other cases, such as use of TimestampValue as a class member (see ImpalaServer, 
for instance where it is used to track start and end times of queries), need 
more analysis. The plan is to look at these other cases in a separate commit.
 
 > A unit test would be good for testing this kind of code.  You can
 > take a look at the various *-test.cc files be/src/*/ for examples.

Adding a unit test. Will refresh the patch again once I have it. In the 
meantime, please let me know if I have addressed all of your comments on the 
implementation.

http://gerrit.cloudera.org:8080/#/c/8084/2/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

PS2, Line 921: UnixMillis()
> for consistency in these values, how about setting this to 'now_us / MICROS
Done


PS2, Line 1146:  Precision::Second
> seems okay to print milliseconds here
Done


PS2, Line 1845: , Precision::Second
> here too
Done


PS2, Line 1924:  Precision::Second
> and here
Done


http://gerrit.cloudera.org:8080/#/c/8084/2/be/src/util/time.cc
File be/src/util/time.cc:

Line 32: static std::string TimepointToString(const 
chrono::system_clock::time_point& t,
> for static things not defined in header, it's helpful to include a short fu
Done


PS2, Line 46:   std::string s(buf);
:   return s;
> return string(buf);
Done


Line 53: 
> nit: consider removing blank lines here and line 68 to make more code fit o
Done


Line 65: // 1-second precision or unknown unit. Return empty string.
> nice to document (and prove) that invariant with a dcheck:
Done. But I had to change TimePrecision from an enum class to a regular enum, 
because DCHECK requires the argument to have an "<<" operator.


PS2, Line 89:   chrono::system_clock::time_point 
t(chrono::duration_cast(
: chrono::seconds(s)));
:   return t;
> could consider combing these like suggested at line 46-47 (but given how lo
Done


PS2, Line 95: system_clock
> the docs for chrono::system_clock::time_point claim that the epoch is unspe
It's correct that the standard does not specify the epoch for 
chrono::system_clock. The Windows and Linux implementations, however, are based 
on the Unix epoch. This is a good read 
http://www.modernescpp.com/index.php/the-three-clocks.


PS2, Line 101: chrono::microseconds
> given that the "to" type is the same as the "from" type, why is the duratio
Removed the cast by combining class instantiation and return statements.


http://gerrit.cloudera.org:8080/#/c/8084/2/be/src/util/time.h
File be/src/util/time.h:

PS2, Line 72: Precision
> "precision" seems kind of generic. How about calling that TimePrecision?
Done


PS2, Line 79: Unix timestamp
> I think we usually refer to this as "Unix time", and timestamp often invoke
Done


PS2, Line 81: p
> nit: in header comments to distinguish variables, we usually use single quo
Done


PS2, Line 85: Precision p=Precision::Second
> normally we should avoid default arguments, but in this case it does seem r
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9b0ae06f6d94968c87a199625aa3332b26988142
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5599: Fix for mis-use of TimestampValue

2017-09-18 Thread Dan Hecht (Code Review) 
Dan Hecht has posted comments on this change.

Change subject: IMPALA-5599: Fix for mis-use of TimestampValue
..


Patch Set 2:

(15 comments)

Do you plan to take care of the other cases noted in the jira?  Okay to do it 
in a follow on commit but wondering the plan.

A unit test would be good for testing this kind of code.  You can take a look 
at the various *-test.cc files be/src/*/ for examples.

http://gerrit.cloudera.org:8080/#/c/8084/2/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

PS2, Line 921: UnixMillis()
for consistency in these values, how about setting this to 'now_us / 
MICROS_PER_MILLI'?


PS2, Line 1146:  Precision::Second
seems okay to print milliseconds here


PS2, Line 1845: , Precision::Second
here too


PS2, Line 1924:  Precision::Second
and here


http://gerrit.cloudera.org:8080/#/c/8084/2/be/src/util/time.cc
File be/src/util/time.cc:

Line 32: static std::string TimepointToString(const 
chrono::system_clock::time_point& t,
for static things not defined in header, it's helpful to include a short 
function comment for them at the function definition.


PS2, Line 46:   std::string s(buf);
:   return s;
return string(buf);

(and you don't the namespace in the cc files. you can also take a look at 
names.h and see how it's included to pull in the "common" names to .cc files).


Line 53: 
nit: consider removing blank lines here and line 68 to make more code fit on 
screen, since it doesn't seem like these blanks help readability.


Line 65: // 1-second precision or unknown unit. Return empty string.
nice to document (and prove) that invariant with a dcheck:
DCHECK_EQ(p, Precision::Second);


PS2, Line 89:   chrono::system_clock::time_point 
t(chrono::duration_cast(
: chrono::seconds(s)));
:   return t;
could consider combing these like suggested at line 46-47 (but given how long 
the type name is, use your judgement).


PS2, Line 95: system_clock
the docs for chrono::system_clock::time_point claim that the epoch is 
unspecified, so we can we be sure this works everywhere? it does seem likely to 
always use unix epoch, but technically it doesn't have to.


PS2, Line 101: chrono::microseconds
given that the "to" type is the same as the "from" type, why is the 
duration_cast needed?


http://gerrit.cloudera.org:8080/#/c/8084/2/be/src/util/time.h
File be/src/util/time.h:

PS2, Line 72: Precision
"precision" seems kind of generic. How about calling that TimePrecision?


PS2, Line 79: Unix timestamp
I think we usually refer to this as "Unix time", and timestamp often invokes 
the thought of our timestamp type, so how about we say:
... the input Unix time, specified in seconds since the Unix epoch, to a ...


PS2, Line 81: p
nit: in header comments to distinguish variables, we usually use single quotes: 
'p'


PS2, Line 85: Precision p=Precision::Second
normally we should avoid default arguments, but in this case it does seem 
reasonable to assume the output precision should be the same as the input, so 
I'm okay with that. but please put a space around the = just like in other 
parts of the code.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9b0ae06f6d94968c87a199625aa3332b26988142
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: Yes