[Impala-ASF-CR] IMPALA-6136: Part 1: Query duration should not be normally negative.

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8430 )

Change subject: IMPALA-6136: Part 1: Query duration should not be normally 
negative.
..


Patch Set 3:

> Please update the comment as suggested by Phil.

Done


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib6f971ebd5f0f00f3e38df0495692ffe9d52ef90
Gerrit-Change-Number: 8430
Gerrit-PatchSet: 3
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Wed, 01 Nov 2017 21:24:10 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6136: Part 1: Query duration should not be normally negative.

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8430 )

Change subject: IMPALA-6136: Part 1: Query duration should not be normally 
negative.
..


Patch Set 3:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8430/2/be/src/service/impala-http-handler.cc@303
PS2, Line 303:   // record.end_time_us might still be zero if the query is not 
yet done
> nit: Could you update the following comment in client-request-state.h and i
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib6f971ebd5f0f00f3e38df0495692ffe9d52ef90
Gerrit-Change-Number: 8430
Gerrit-PatchSet: 3
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Wed, 01 Nov 2017 21:23:03 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6136: Part 1: Query duration should not be normally negative.

[Zoram Thanga (Code Review)]

Hello Michael Ho, Philip Zeyliger, Dan Hecht,

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

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

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

Change subject: IMPALA-6136: Part 1: Query duration should not be normally 
negative.
..

IMPALA-6136: Part 1: Query duration should not be normally negative.

The second commit for IMPALA-5599, 1640aa97, introduced a regression
where calculating the duration of an in-progress query can result
in negative values. This can happen for two reasons:

1. The value of ClientRequestState::end_time_us_ is not initialized
   in the constructor, and may have some random value until
   ClientRequestState::Done() is called.
2. If ClientRequestState::end_time_us_ happens to have a positive
   value less than ClientRequestState::start_time_us_, the query
   duration will be negative. Here, since the query is still in
   flight, we need to use the local current time as the end time.

The fix has been manually verified by executing long-running queries
and checking the query profiles to ensure the durations are not some
random junks. A new test case will be added to check for sane time
values in a follow-on commit.

Since we are using Unix time (system clock) for time stamps, it is
still possible for end_time_us_ to be less than start_time_us_ if
the system clock gets reset while the query is executing. If there
are clients that assume that a query duration is never negative,
we really should switch to a monotonic clock to time queries.

Change-Id: Ib6f971ebd5f0f00f3e38df0495692ffe9d52ef90
---
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.h
4 files changed, 13 insertions(+), 2 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib6f971ebd5f0f00f3e38df0495692ffe9d52ef90
Gerrit-Change-Number: 8430
Gerrit-PatchSet: 3
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-5599: Clean up references to TimestampValue in be/src.

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8305 )

Change subject: IMPALA-5599: Clean up references to TimestampValue in be/src.
..


Patch Set 11:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8305/11/be/src/service/client-request-state.h
File be/src/service/client-request-state.h:

http://gerrit.cloudera.org:8080/#/c/8305/11/be/src/service/client-request-state.h@342
PS11, Line 342: start_time_us_, end_time_us_;
> These two may need to be initialized.

https://gerrit.cloudera.org/#/c/8430/


http://gerrit.cloudera.org:8080/#/c/8305/11/be/src/service/impala-http-handler.cc
File be/src/service/impala-http-handler.cc:

http://gerrit.cloudera.org:8080/#/c/8305/11/be/src/service/impala-http-handler.cc@303
PS11, Line 303: record.end_time_us - record.start_time_us;
> This may be broken if end_time_us is not set yet. The old code will use the
Fixing via https://gerrit.cloudera.org/#/c/8430/


http://gerrit.cloudera.org:8080/#/c/8305/11/be/src/service/impala-server.h
File be/src/service/impala-server.h:

http://gerrit.cloudera.org:8080/#/c/8305/11/be/src/service/impala-server.h@622
PS11, Line 622: /// Start and end time of the query, in Unix microseconds.
  : int64_t start_time_us, end_time_us;
> Can these be const ? They are always initialized in the constructor, right
The default constructor does not handle this well if we make them constants. 
We'll have to make all/most of the other fields constant as well if we are 
going to fix this.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I642a1d713597826bb7c15cd2ecb6638cb813a02c
Gerrit-Change-Number: 8305
Gerrit-PatchSet: 11
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Wed, 01 Nov 2017 20:21:22 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6136: Part 1: Query duration should not be normally negative.

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8430 )

Change subject: IMPALA-6136: Part 1: Query duration should not be normally 
negative.
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8430/1/be/src/service/impala-http-handler.cc
File be/src/service/impala-http-handler.cc:

http://gerrit.cloudera.org:8080/#/c/8430/1/be/src/service/impala-http-handler.cc@306
PS1, Line 306:  record.end_time_us
> record.end_time_us > 0
Done


http://gerrit.cloudera.org:8080/#/c/8430/1/be/src/service/impala-http-handler.cc@306
PS1, Line 306: ending_time_us
> end_time_us
Changed as suggested.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib6f971ebd5f0f00f3e38df0495692ffe9d52ef90
Gerrit-Change-Number: 8430
Gerrit-PatchSet: 1
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Wed, 01 Nov 2017 06:28:24 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6136: Part 1: Query duration should not be normally negative.

[Zoram Thanga (Code Review)]

Hello Michael Ho, Dan Hecht,

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

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

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

Change subject: IMPALA-6136: Part 1: Query duration should not be normally 
negative.
..

IMPALA-6136: Part 1: Query duration should not be normally negative.

The second commit for IMPALA-5599, 1640aa97, introduced a regression
where calculating the duration of an in-progress query can result
in negative values. This can happen for two reasons:

1. The value of ClientRequestState::end_time_us_ is not initialized
   in the constructor, and may have some random value until
   ClientRequestState::Done() is called.
2. If ClientRequestState::end_time_us_ happens to have a positive
   value less than ClientRequestState::start_time_us_, the query
   duration will be negative. Here, since the query is still in
   flight, we need to use the local current time as the end time.

The fix has been manually verified by executing long-running queries
and checking the query profiles to ensure the durations are not some
random junks. A new test case will be added to check for sane time
values in a follow-on commit.

Since we are using Unix time (system clock) for time stamps, it is
still possible for end_time_us_ to be less than start_time_us_ if
the system clock gets reset while the query is executing. If there
are clients that assume that a query duration is never negative,
we really should switch to a monotonic clock to time queries.

Change-Id: Ib6f971ebd5f0f00f3e38df0495692ffe9d52ef90
---
M be/src/service/client-request-state.cc
M be/src/service/impala-http-handler.cc
2 files changed, 7 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/30/8430/2
--
To view, visit http://gerrit.cloudera.org:8080/8430
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib6f971ebd5f0f00f3e38df0495692ffe9d52ef90
Gerrit-Change-Number: 8430
Gerrit-PatchSet: 2
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6136: Part 1: Query duration should not be normally negative.

[Zoram Thanga (Code Review)]

Zoram Thanga has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8430


Change subject: IMPALA-6136: Part 1: Query duration should not be normally 
negative.
..

IMPALA-6136: Part 1: Query duration should not be normally negative.

The second commit for IMPALA-5599, 1640aa97, introduced a regression
where calculating the duration of an in-progress query can result
in negative values. This can happen for two reasons:

1. The value of ClientRequestState::end_time_us_ is not initialized
   in the constructor, and may have some random value until
   ClientRequestState::Done() is called.
2. If ClientRequestState::end_time_us_ happens to have a positive
   value less than ClientRequestState::start_time_us_, the query
   duration will be negative. Here, since the query is still in
   flight, we need to use the local current time as the end time.

The fix has been manually verified by executing long-running queries
and checking the query profiles to ensure the durations are not some
random junks. A new test case will be added to check for sane time
values in a follow-on commit.

Since we are using Unix time (system clock) for time stamps, it is
still possible for end_time_us_ to be less than start_time_us_ if
the system clock gets reset while the query is executing. If there
are clients that assume that a query duration is never negative,
we really should switch to a monotonic clock to time queries.

Change-Id: Ib6f971ebd5f0f00f3e38df0495692ffe9d52ef90
---
M be/src/service/client-request-state.cc
M be/src/service/impala-http-handler.cc
2 files changed, 7 insertions(+), 2 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/30/8430/1
--
To view, visit http://gerrit.cloudera.org:8080/8430
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib6f971ebd5f0f00f3e38df0495692ffe9d52ef90
Gerrit-Change-Number: 8430
Gerrit-PatchSet: 1
Gerrit-Owner: Zoram Thanga 

[Impala-ASF-CR] IMPALA-5599: Clean up references to TimestampValue in be/src.

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8305 )

Change subject: IMPALA-5599: Clean up references to TimestampValue in be/src.
..


Patch Set 10:

Fixed the clang tidy warnings. 
https://jenkins.impala.io/job/gerrit-verify-dryrun-external/21/console


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I642a1d713597826bb7c15cd2ecb6638cb813a02c
Gerrit-Change-Number: 8305
Gerrit-PatchSet: 10
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Tue, 24 Oct 2017 16:50:30 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5599: Clean up references to TimestampValue in be/src.

[Zoram Thanga (Code Review)]

Hello Michael Ho, Dan Hecht, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-5599: Clean up references to TimestampValue in be/src.
..

IMPALA-5599: Clean up references to TimestampValue in be/src.

This is a follow-on commit to d53f43b4. In this patch we replace
all inappropriate usage of TimestampValue in be/src with simpler
data types for Unix timestamps, and where conversions to strings
are needed, we use the interfaces introduced by the previous
commit.

TimestampValue::{LocalTime(), UtcTime()} have been retired.

BE, FE, EE, JDBC, and Cluster Tests were run to check for
regressions. All the tests passed. Also manually inspected query
profiles to check that datestamps and time intervals are sane.

Change-Id: I642a1d713597826bb7c15cd2ecb6638cb813a02c
---
M be/src/exprs/expr-test.cc
M be/src/runtime/timestamp-value.h
M be/src/runtime/timestamp-value.inline.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/statestore/statestore-subscriber.cc
M be/src/util/common-metrics.cc
M be/src/util/time.h
11 files changed, 59 insertions(+), 67 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/8305/10
--
To view, visit http://gerrit.cloudera.org:8080/8305
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I642a1d713597826bb7c15cd2ecb6638cb813a02c
Gerrit-Change-Number: 8305
Gerrit-PatchSet: 10
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-5599: Clean up references to TimestampValue in be/src.

[Zoram Thanga (Code Review)]

Hello Michael Ho, Dan Hecht,

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

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

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

Change subject: IMPALA-5599: Clean up references to TimestampValue in be/src.
..

IMPALA-5599: Clean up references to TimestampValue in be/src.

This is a follow-on commit to d53f43b4. In this patch we replace
all inappropriate usage of TimestampValue in be/src with simpler
data types for Unix timestamps, and where conversions to strings
are needed, we use the interfaces introduced by the previous
commit.

TimestampValue::{LocalTime(), UtcTime()} have been retired.

BE, FE, EE, JDBC, and Cluster Tests were run to check for
regressions. All the tests passed. Also manually inspected query
profiles to check that datestamps and time intervals are sane.

Change-Id: I642a1d713597826bb7c15cd2ecb6638cb813a02c
---
M be/src/exprs/expr-test.cc
M be/src/runtime/timestamp-value.h
M be/src/runtime/timestamp-value.inline.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/statestore/statestore-subscriber.cc
M be/src/util/common-metrics.cc
M be/src/util/time.h
11 files changed, 59 insertions(+), 67 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/8305/9
--
To view, visit http://gerrit.cloudera.org:8080/8305
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I642a1d713597826bb7c15cd2ecb6638cb813a02c
Gerrit-Change-Number: 8305
Gerrit-PatchSet: 9
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-5599: Clean up references to TimestampValue in be/src.

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8305 )

Change subject: IMPALA-5599: Clean up references to TimestampValue in be/src.
..


Patch Set 8:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8305/8/be/src/util/time.h@106
PS8, Line 106: /// Convenience function to return the date-time string of the 
current time
 : /// derived from UnixMicros().
> how about we mention that this is in the local time zone.

Changed the wording to say that the date-time string conversion is in the local 
time zone.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I642a1d713597826bb7c15cd2ecb6638cb813a02c
Gerrit-Change-Number: 8305
Gerrit-PatchSet: 8
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Mon, 23 Oct 2017 19:15:16 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5599: Clean up references to TimestampValue in be/src.

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8305 )

Change subject: IMPALA-5599: Clean up references to TimestampValue in be/src.
..


Patch Set 8:

(2 comments)

Please check the latest patch #8

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

http://gerrit.cloudera.org:8080/#/c/8305/7/be/src/util/time.h@106
PS7, Line 106: /// Convenience function to return the date-time string of the 
current time
> Convenience function to return the date-time string of the current time der
Done


http://gerrit.cloudera.org:8080/#/c/8305/7/be/src/util/time.h@107
PS7, Line 107: ixMicros().
> How about CurrentTimeString() ?
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I642a1d713597826bb7c15cd2ecb6638cb813a02c
Gerrit-Change-Number: 8305
Gerrit-PatchSet: 8
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Mon, 23 Oct 2017 17:59:07 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5599: Clean up references to TimestampValue in be/src.

[Zoram Thanga (Code Review)]

Hello Michael Ho, Dan Hecht,

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

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

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

Change subject: IMPALA-5599: Clean up references to TimestampValue in be/src.
..

IMPALA-5599: Clean up references to TimestampValue in be/src.

This is a follow-on commit to d53f43b4. In this patch we replace
all inappropriate usage of TimestampValue in be/src with simpler
data types for Unix timestamps, and where conversions to strings
are needed, we use the interfaces introduced by the previous
commit.

TimestampValue::{LocalTime(), UtcTime()} have been retired.

BE, FE, EE, JDBC, and Cluster Tests were run to check for
regressions. All the tests passed. Also manually inspected query
profiles to check that datestamps and time intervals are sane.

Change-Id: I642a1d713597826bb7c15cd2ecb6638cb813a02c
---
M be/src/exprs/expr-test.cc
M be/src/runtime/timestamp-value.h
M be/src/runtime/timestamp-value.inline.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/statestore/statestore-subscriber.cc
M be/src/util/common-metrics.cc
M be/src/util/time.h
11 files changed, 59 insertions(+), 67 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/8305/8
--
To view, visit http://gerrit.cloudera.org:8080/8305
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I642a1d713597826bb7c15cd2ecb6638cb813a02c
Gerrit-Change-Number: 8305
Gerrit-PatchSet: 8
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-5599: Clean up references to TimestampValue in be/src.

[Zoram Thanga (Code Review)]

Hello Michael Ho, Dan Hecht,

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

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

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

Change subject: IMPALA-5599: Clean up references to TimestampValue in be/src.
..

IMPALA-5599: Clean up references to TimestampValue in be/src.

This is a follow-on commit to d53f43b4. In this patch we replace
all inappropriate usage of TimestampValue in be/src with simpler
data types for Unix timestamps, and where conversions to strings
are needed, we use the interfaces introduced by the previous
commit.

TimestampValue::{LocalTime(), UtcTime()} have been retired.

BE, FE, EE, JDBC, and Cluster Tests were run to check for
regressions. All the tests passed. Also manually inspected query
profiles to check that datestamps and time intervals are sane.

Change-Id: I642a1d713597826bb7c15cd2ecb6638cb813a02c
---
M be/src/exprs/expr-test.cc
M be/src/runtime/timestamp-value.h
M be/src/runtime/timestamp-value.inline.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/statestore/statestore-subscriber.cc
M be/src/util/common-metrics.cc
M be/src/util/time.h
11 files changed, 58 insertions(+), 67 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I642a1d713597826bb7c15cd2ecb6638cb813a02c
Gerrit-Change-Number: 8305
Gerrit-PatchSet: 7
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-5599: Clean up references to TimestampValue in be/src.

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8305 )

Change subject: IMPALA-5599: Clean up references to TimestampValue in be/src.
..


Patch Set 5:

(3 comments)

> (3 comments)

Uploading a new patch.

http://gerrit.cloudera.org:8080/#/c/8305/5/be/src/runtime/timestamp-value.h
File be/src/runtime/timestamp-value.h:

http://gerrit.cloudera.org:8080/#/c/8305/5/be/src/runtime/timestamp-value.h@107
PS5, Line 107:   static TimestampValue FromUnixTimeMicros(int64_t 
unix_time_micros);
> nit: blank line between function declaration.
Fixed


http://gerrit.cloudera.org:8080/#/c/8305/5/be/src/runtime/timestamp-value.inline.h
File be/src/runtime/timestamp-value.inline.h:

http://gerrit.cloudera.org:8080/#/c/8305/5/be/src/runtime/timestamp-value.inline.h@45
PS5, Line 45: }
> nit: blank line between functions.
Fixed


http://gerrit.cloudera.org:8080/#/c/8305/5/be/src/util/common-metrics.cc
File be/src/util/common-metrics.cc:

http://gerrit.cloudera.org:8080/#/c/8305/5/be/src/util/common-metrics.cc@36
PS5, Line 36: ToStringFromUnixMicros(UnixMicros())
> This seems to be useful in general and used in multiple places in this patc
Came up with NowMicrosToString(). Please suggest a better name; there are only 
two callers at this time, but hopefully there will be more in the future.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I642a1d713597826bb7c15cd2ecb6638cb813a02c
Gerrit-Change-Number: 8305
Gerrit-PatchSet: 5
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 20 Oct 2017 23:26:00 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5599: Clean up references to TimestampValue in be/src.

[Zoram Thanga (Code Review)]

Hello Michael Ho, Dan Hecht,

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

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

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

Change subject: IMPALA-5599: Clean up references to TimestampValue in be/src.
..

IMPALA-5599: Clean up references to TimestampValue in be/src.

This is a follow-on commit to d53f43b4. In this patch we replace
all inappropriate usage of TimestampValue in be/src with simpler
data types for Unix timestamps, and where conversions to strings
are needed, we use the interfaces introduced by the previous
commit.

TimestampValue::{LocalTime(), UtcTime()} have been retired.

BE, FE, EE, JDBC, and Cluster Tests were run to check for
regressions. All the tests passed. Also manually inspected query
profiles to check that datestamps and time intervals are sane.

Change-Id: I642a1d713597826bb7c15cd2ecb6638cb813a02c
---
M be/src/exprs/expr-test.cc
M be/src/runtime/timestamp-value.h
M be/src/runtime/timestamp-value.inline.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/statestore/statestore-subscriber.cc
M be/src/util/common-metrics.cc
10 files changed, 52 insertions(+), 67 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I642a1d713597826bb7c15cd2ecb6638cb813a02c
Gerrit-Change-Number: 8305
Gerrit-PatchSet: 6
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-5599: Clean up references to TimestampValue in be/src.

[Zoram Thanga (Code Review)]

Hello Michael Ho, Dan Hecht,

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

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

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

Change subject: IMPALA-5599: Clean up references to TimestampValue in be/src.
..

IMPALA-5599: Clean up references to TimestampValue in be/src.

This is a follow-on commit to d53f43b4. In this patch we replace
all inappropriate usage of TimestampValue in be/src with simpler
data types for Unix timestamps, and where conversions to strings
are needed, we use the interfaces introduced by the previous
commit.

TimestampValue::{LocalTime(), UtcTime()} have been retired.

BE, FE, EE, JDBC, and Cluster Tests were run to check for
regressions. All the tests passed. Also manually inspected query
profiles to check that datestamps and time intervals are sane.

Change-Id: I642a1d713597826bb7c15cd2ecb6638cb813a02c
---
M be/src/exprs/expr-test.cc
M be/src/runtime/timestamp-value.h
M be/src/runtime/timestamp-value.inline.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/statestore/statestore-subscriber.cc
M be/src/util/common-metrics.cc
10 files changed, 52 insertions(+), 67 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I642a1d713597826bb7c15cd2ecb6638cb813a02c
Gerrit-Change-Number: 8305
Gerrit-PatchSet: 5
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-5599: Clean up references to TimestampValue in be/src.

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8305 )

Change subject: IMPALA-5599: Clean up references to TimestampValue in be/src.
..


Patch Set 4:

> What about common-metrics.cc?
 >
 > And then, I think we should move TimestampValue::{LocalTime(),
 > UtcTime()} to expr-test.cc so that it's not even part of the
 > TimestampValue interface.

Done. The TimestampValue interfaces mentioned above have been retired.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I642a1d713597826bb7c15cd2ecb6638cb813a02c
Gerrit-Change-Number: 8305
Gerrit-PatchSet: 4
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 19 Oct 2017 23:29:25 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5599: Clean up references to TimestampValue in be/src.

[Zoram Thanga (Code Review)]

Hello Michael Ho, Dan Hecht,

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

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

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

Change subject: IMPALA-5599: Clean up references to TimestampValue in be/src.
..

IMPALA-5599: Clean up references to TimestampValue in be/src.

This is a follow-on commit to d53f43b4. In this patch we replace all
inappropriate usage of TimestampValue in be/src/service and
be/src/statetore with simpler data types for Unix timestamps, and
where conversions to strings are needed, we use the interfaces
introduced by the previous commit.

BE, FE, EE, JDBC, and Cluster Tests were run to check for
regressions. All the tests passed. Also manually inspected query
profiles to check that datestamps and time intervals are sane.

Change-Id: I642a1d713597826bb7c15cd2ecb6638cb813a02c
---
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/statestore/statestore-subscriber.cc
6 files changed, 29 insertions(+), 42 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I642a1d713597826bb7c15cd2ecb6638cb813a02c
Gerrit-Change-Number: 8305
Gerrit-PatchSet: 4
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-5599: Clean up references to TimestampValue in be/src.

[Zoram Thanga (Code Review)]

Hello Michael Ho, Dan Hecht,

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

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

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

Change subject: IMPALA-5599: Clean up references to TimestampValue in be/src.
..

IMPALA-5599: Clean up references to TimestampValue in be/src.

This is a follow-on commit to d53f43b4. In this patch we replace all
inappropriate usage of TimestampValue in be/src/service and
be/src/statetore with simpler data types for Unix timestamps, and
where conversions to strings are needed, we use the interfaces
introduced by the previous commit.

BE, FE, EE, JDBC, and Cluster Tests were run to check for
regressions. All the tests passed.

Change-Id: I642a1d713597826bb7c15cd2ecb6638cb813a02c
---
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/statestore/statestore-subscriber.cc
6 files changed, 29 insertions(+), 42 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I642a1d713597826bb7c15cd2ecb6638cb813a02c
Gerrit-Change-Number: 8305
Gerrit-PatchSet: 3
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-5599: Clean up references to TimestampValue in be/src.

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8305 )

Change subject: IMPALA-5599: Clean up references to TimestampValue in be/src.
..


Patch Set 2:

> (1 comment)
 >
 > Code change looks good.
 >
 > How did you test it? Please add a Testing section to commit message
 > indicating that. Please at least manually inspect each affected
 > field by e.g. looking at the profile.

Thanks for the suggestion to look at the profiles. Here's an example, and it 
looks fine to me:

Query (id=394caf0e5f233559:8d02422c):
  DEBUG MODE WARNING: Query profile created while running a DEBUG build of 
Impala. Use RELEASE builds to measure query performance.
  Summary:
Session ID: 864fbccb3e7bee09:7fa78fb245fa3688
Session Type: BEESWAX
Start Time: 2017-10-18 20:21:59.972644
End Time: 2017-10-18 20:22:00.097281
Query Type: N/A
Query State: EXCEPTION
Query Status: AnalysisException: Could not resolve table reference: 
'tpch_avro.lineitem'

Impala Version: impalad version 2.11.0-SNAPSHOT DEBUG (build 
979c3dfd6fc40dbfb413cf45a9560f81495d1f5a)
User: zoram
Connected User: zoram
Delegated User:
Network Address: 
Default Db: default
Sql Statement: select l_orderkey from tpch_avro.lineitem having 
count(l_orderkey) > 1
Coordinator: impala-1:22000
Query Timeline: 124.796ms
   - Query submitted: 45.687us (45.687us)
   - Unregister query: 124.627ms (124.582ms)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I642a1d713597826bb7c15cd2ecb6638cb813a02c
Gerrit-Change-Number: 8305
Gerrit-PatchSet: 2
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 19 Oct 2017 03:32:49 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5599: Clean up references to TimestampValue in be/src.

[Zoram Thanga (Code Review)]

Hello Michael Ho, Dan Hecht,

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

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

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

Change subject: IMPALA-5599: Clean up references to TimestampValue in be/src.
..

IMPALA-5599: Clean up references to TimestampValue in be/src.

This is a follow-on commit to d53f43b4. In this patch we replace all
inappropriate usage of TimestampValue in be/src/service and
be/src/statetore with simpler data types for Unix timestamps, and
where conversions to strings are needed, we use the interfaces
introduced by the previous commit.

Change-Id: I642a1d713597826bb7c15cd2ecb6638cb813a02c
---
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/statestore/statestore-subscriber.cc
6 files changed, 29 insertions(+), 42 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/8305/2
--
To view, visit http://gerrit.cloudera.org:8080/8305
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I642a1d713597826bb7c15cd2ecb6638cb813a02c
Gerrit-Change-Number: 8305
Gerrit-PatchSet: 2
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-5599: Clean up references to TimestampValue in be/src/service.

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8305 )

Change subject: IMPALA-5599: Clean up references to TimestampValue in 
be/src/service.
..


Patch Set 1:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/client-request-state.h
File be/src/service/client-request-state.h:

http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/client-request-state.h@341
PS1, Line 341: time
> indicate the clock. e.g. monotonic vs unix time.
Done. This uses Unix time.


http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:

http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/client-request-state.cc@a91
PS1, Line 91:
> What do you think about going a step further and replacing all callers of T
Changed the caller in be/src/statetore. The other calls in be/src/exprs are in 
test code, and seem legit


http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/impala-http-handler.cc
File be/src/service/impala-http-handler.cc:

http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/impala-http-handler.cc@303
PS1, Line 303:   int64_t duration_us = record.end_time_us - 
record.start_time_us;
> That's true but that also seems to be a pre-existing bug. I agree that we s
Please see my response to DanH's comment...


http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/impala-http-handler.cc@303
PS1, Line 303:   int64_t duration_us = record.end_time_us - 
record.start_time_us;
> that can be negative if settimeofday() occurred in the mean time. What shou
The existing code does not handle this either. Looking at 
PrettyPrinter::Print(), it will basically print a negative elapsed time in 
nanoseconds. The only way to reliably handle intervening settimeofday(), or 
something like that would be to use a monotonic clock, perhaps 
MonotonicStopWatch.

Are you suggesting we go down that route?


http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/impala-server.h
File be/src/service/impala-server.h:

http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/impala-server.h@622
PS1, Line 622: /// Start and end time of the query
> same comment about clock
Done


http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/impala-server.h@640
PS1, Line 640: Unix milliseconds
> like that
Done


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

http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/impala-server.cc@1009
PS1, Line 1009:   int64_t duration_ms = duration_us / MICROS_PER_MILLI;
> same question (though old code could go negative).
Please see my earlier response.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I642a1d713597826bb7c15cd2ecb6638cb813a02c
Gerrit-Change-Number: 8305
Gerrit-PatchSet: 1
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Wed, 18 Oct 2017 22:24:48 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5599: Clean up references to TimestampValue in be/src/service.

[Zoram Thanga (Code Review)]

Zoram Thanga has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8305


Change subject: IMPALA-5599: Clean up references to TimestampValue in 
be/src/service.
..

IMPALA-5599: Clean up references to TimestampValue in be/src/service.

This is a follow-on commit to d53f43b4. In this patch we replace all
inappropriate usage of TimestampValue in be/src/service with simpler
data types for Unix timestamps, and where conversions to strings are
needed, we use the interfaces introduced by the previous commit.

Change-Id: I642a1d713597826bb7c15cd2ecb6638cb813a02c
---
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
5 files changed, 26 insertions(+), 39 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/8305/1
--
To view, visit http://gerrit.cloudera.org:8080/8305
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I642a1d713597826bb7c15cd2ecb6638cb813a02c
Gerrit-Change-Number: 8305
Gerrit-PatchSet: 1
Gerrit-Owner: Zoram Thanga 

[Impala-ASF-CR] IMPALA-4682: Remove Preconditions check from analyzeAggregation().

[Zoram Thanga (Code Review)]

Hello Michael Ho, Dimitris Tsirogiannis, anujphadke, Alex Behm,

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

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

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

Change subject: IMPALA-4682: Remove Preconditions check from 
analyzeAggregation().
..

IMPALA-4682: Remove Preconditions check from analyzeAggregation().

When one runs a query like

'select * from t order by count(a)'

we are incorrectly throwing an IllegalStateException, with a
Preconditions check which asserts that the column is not "*". Now,
this query is invalid, and we are correctly handling it if "*" is
replaced by a specific column:

'select a from t order by count(b)'

which produces the error message

"ERROR: AnalysisException: select list expression not produced by
aggregation output (missing from GROUP BY clause?): a"

This patch fixes the handling of "*" in this context, by removing
the Preconditions check, so that the error becomes

"ERROR: AnalysisException: select list expression not produced by
aggregation output (missing from GROUP BY clause?): *"

Note that the second changed line is required because
selectListItem.Expr_ is null when SelectListItem.isStar_ is true.

A new FE unit test has been added for this use case.

Change-Id: I57c20aeed401275d45913fedfd61c206c38641b7
---
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
2 files changed, 4 insertions(+), 2 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I57c20aeed401275d45913fedfd61c206c38641b7
Gerrit-Change-Number: 8143
Gerrit-PatchSet: 4
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Reviewer: anujphadke 

[Impala-ASF-CR] IMPALA-4682 Fix IllegalStateException thrown by aggregate function.

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8143 )

Change subject: IMPALA-4682 Fix IllegalStateException thrown by aggregate 
function.
..


Patch Set 3:

(2 comments)

Please see the latest patch set.

http://gerrit.cloudera.org:8080/#/c/8143/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8143/3//COMMIT_MSG@7
PS3, Line 7: IMPALA-4682 Fix IllegalStateException thrown by aggregate function.
> Our format is
Done


http://gerrit.cloudera.org:8080/#/c/8143/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java:

http://gerrit.cloudera.org:8080/#/c/8143/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@1943
PS3, Line 1943:
> remove newline
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I57c20aeed401275d45913fedfd61c206c38641b7
Gerrit-Change-Number: 8143
Gerrit-PatchSet: 3
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Mon, 09 Oct 2017 22:24:19 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4682 Fix IllegalStateException thrown by aggregate function.

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8143 )

Change subject: IMPALA-4682 Fix IllegalStateException thrown by aggregate 
function.
..


Patch Set 2:

(1 comment)

Condensed new test cases to just one, and moved it under TestAggregates().

http://gerrit.cloudera.org:8080/#/c/8143/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java:

http://gerrit.cloudera.org:8080/#/c/8143/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@893
PS2, Line 893: AnalysisError("select l_partkey from tpch_avro.lineitem 
order by count(l_orderkey)",
> Test seems to be misplaced and/or duplicated. This test is TestStar() yet t
Thanks for the suggestion. I can now see that the other test cases I was trying 
to add are already covered by existing test cases in TestAggregates(). I am 
retaining only the 'select *...' test case.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I57c20aeed401275d45913fedfd61c206c38641b7
Gerrit-Change-Number: 8143
Gerrit-PatchSet: 2
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Thu, 05 Oct 2017 18:31:58 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4682 Fix IllegalStateException thrown by aggregate function.

[Zoram Thanga (Code Review)]

Hello Michael Ho, Dimitris Tsirogiannis, anujphadke, Alex Behm,

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

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

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

Change subject: IMPALA-4682 Fix IllegalStateException thrown by aggregate 
function.
..

IMPALA-4682 Fix IllegalStateException thrown by aggregate function.

When one runs a query like

'select * from t order by count(a)'

we are incorrectly throwing an IllegalStateException. This is an
invalid query, and we are correctly handling it if "*" is replaced
by a specific column:

'select a from t order by count(b)'

which produces the error message

"ERROR: AnalysisException: select list expression not produced by
aggregation output (missing from GROUP BY clause?): a"

This patch fixes the handling of '*' in this context, so that the
error becomes

"ERROR: AnalysisException: select list expression not produced by
aggregation output (missing from GROUP BY clause?): *"

Note that the second changed line is required because
selectListItem.Expr_ is null when SelectListItem.isStar_ is true.

A new FE unit test has been added for this use case.

Change-Id: I57c20aeed401275d45913fedfd61c206c38641b7
---
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
2 files changed, 5 insertions(+), 2 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I57c20aeed401275d45913fedfd61c206c38641b7
Gerrit-Change-Number: 8143
Gerrit-PatchSet: 3
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Reviewer: anujphadke 

[Impala-ASF-CR] IMPALA-4682 Fix IllegalStateException issue

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8143 )

Change subject: IMPALA-4682 Fix IllegalStateException issue
..


Patch Set 1:

(2 comments)

> Zoram, let's try to move this forward. We don't want too many open
 > CRs.

Done. Have added new test cases to exercise the scenario reported in the JIRA.

http://gerrit.cloudera.org:8080/#/c/8143/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8143/1//COMMIT_MSG@7
PS1, Line 7: IMPALA-4682 Fix IllegalStateException issue
> Can you state the issue in short?
Done


http://gerrit.cloudera.org:8080/#/c/8143/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
File fe/src/main/java/org/apache/impala/analysis/SelectStmt.java:

http://gerrit.cloudera.org:8080/#/c/8143/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@731
PS1, Line 731: + selectListItem.toSql());
> Can  we add a test  for this?
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I57c20aeed401275d45913fedfd61c206c38641b7
Gerrit-Change-Number: 8143
Gerrit-PatchSet: 1
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Wed, 04 Oct 2017 23:10:54 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4682 Fix IllegalStateException thrown by aggregate function.

[Zoram Thanga (Code Review)]

Hello Michael Ho, Dimitris Tsirogiannis, anujphadke, Alex Behm,

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

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

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

Change subject: IMPALA-4682 Fix IllegalStateException thrown by aggregate 
function.
..

IMPALA-4682 Fix IllegalStateException thrown by aggregate function.

When one runs a query like

'select * from t order by count(a)'

we are incorrectly throwing an IllegalStateException. This is an
invalid query, and we are correctly handling it if "*" is replaced
by a specific column:

'select a from t order by count(b)'

which produces the error message

"ERROR: AnalysisException: select list expression not produced by
aggregation output (missing from GROUP BY clause?): a"

This patch fixes the handling of '*' in this context, so that the
error becomes

"ERROR: AnalysisException: select list expression not produced by
aggregation output (missing from GROUP BY clause?): *"

Note that the second changed line is required because
selectListItem.Expr_ is null when SelectListItem.isStar_ is true.

New FE unit tests are added for this use case.

Change-Id: I57c20aeed401275d45913fedfd61c206c38641b7
---
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
2 files changed, 29 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/8143/2
--
To view, visit http://gerrit.cloudera.org:8080/8143
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I57c20aeed401275d45913fedfd61c206c38641b7
Gerrit-Change-Number: 8143
Gerrit-PatchSet: 2
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Reviewer: anujphadke 

[Impala-ASF-CR] IMPALA-4682 Fix IllegalStateException issue

[Zoram Thanga (Code Review)]

Zoram Thanga has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8143


Change subject: IMPALA-4682 Fix IllegalStateException issue
..

IMPALA-4682 Fix IllegalStateException issue

When one runs a query like

'select * from t order by count(a)'

we are throwing an IllegalStateException. The desired behavior here
is to handle the problem as an AnalysisExeption with a more
meaningful error message.

This patch fixes the handling of '*' in this context, so that the
error becomes

"ERROR: AnalysisException: select list expression not produced by
aggregation output (missing from GROUP BY clause?): *"

Note that the second changed line is required because
selectListItem.Expr_ is null when SelectListItem.isStar_ is true.

Change-Id: I57c20aeed401275d45913fedfd61c206c38641b7
---
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
1 file changed, 1 insertion(+), 2 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/8143/1
--
To view, visit http://gerrit.cloudera.org:8080/8143
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I57c20aeed401275d45913fedfd61c206c38641b7
Gerrit-Change-Number: 8143
Gerrit-PatchSet: 1
Gerrit-Owner: Zoram Thanga 

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

[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

[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

[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

[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

[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

[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

[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

[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

[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