You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Matthew Jacobs (Code Review)" <ge...@cloudera.org> on 2017/03/29 18:24:12 UTC

[Impala-ASF-CR] IMPALA-5137: pt1, Refactor TimestampValue constructors 

Matthew Jacobs has uploaded a new change for review.

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

Change subject: IMPALA-5137: pt1, Refactor TimestampValue constructors
......................................................................

IMPALA-5137: pt1, Refactor TimestampValue constructors

In preparation for supporting Kudu TIMESTAMPs, some cleanup
of the TimestampValue code will be helpful first.

This change just refactors some of the TimestampValue
constructors which do more than simple construction, e.g.
parsing or converting, to static factory methods.

Change-Id: Id25e19f7984e5ebf9073d9c569faf69cec142fa1
---
M be/src/benchmarks/parse-timestamp-benchmark.cc
M be/src/exec/data-source-scan-node.cc
M be/src/exec/text-converter.inline.h
M be/src/exprs/aggregate-functions-ir.cc
M be/src/exprs/cast-functions-ir.cc
M be/src/exprs/decimal-operators-ir.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/expr-value.h
M be/src/exprs/literal.cc
M be/src/exprs/timestamp-functions-ir.cc
M be/src/runtime/raw-value-test.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/timestamp-test.cc
M be/src/runtime/timestamp-value.cc
M be/src/runtime/timestamp-value.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/util/dict-test.cc
18 files changed, 229 insertions(+), 129 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Id25e19f7984e5ebf9073d9c569faf69cec142fa1
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>

[Impala-ASF-CR] IMPALA-5137: pt1, Refactor TimestampValue constructors

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5137: pt1, Refactor TimestampValue constructors
......................................................................


Patch Set 2: Verified-1

Build failed: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/425/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id25e19f7984e5ebf9073d9c569faf69cec142fa1
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5137: pt1, Refactor TimestampValue constructors

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.

Change subject: IMPALA-5137: pt1, Refactor TimestampValue constructors
......................................................................


Patch Set 1:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/6510/1/be/src/exec/data-source-scan-node.cc
File be/src/exec/data-source-scan-node.cc:

PS1, Line 284: FromUnixTimeNanos
does it make sense that this conversion is subjected to use local tz flag? in any case, okay to leave this change as purely non-functional and we can deal with this later.


http://gerrit.cloudera.org:8080/#/c/6510/1/be/src/exprs/aggregate-functions-ir.cc
File be/src/exprs/aggregate-functions-ir.cc:

PS1, Line 354: TimestampValue
maybe use "const TimetampValue& tv = ..." here (and elsewhere), though maybe unnecessary if the compiler is able to optimize away the copy anyway.


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

PS1, Line 81: lexical_cast<string>(t)
how about getting rid of this implicit code as well by defining a ToString() (or maybe rename DebugString() to ToString())?


http://gerrit.cloudera.org:8080/#/c/6510/1/be/src/runtime/timestamp-test.cc
File be/src/runtime/timestamp-test.cc:

PS1, Line 619: 
heh


PS1, Line 632: coverted
converted


PS1, Line 641:   EXPECT_TRUE(leap_tv1.ToUnixTime(&leap_time_t));
             :   EXPECT_EQ(915148800, leap_time_t);
             :   // Converted to the Unix time representation
             :   EXPECT_EQ("1999-01-01 00:00:00", leap_tv1.DebugString());
maybe do these cases on leap_ptime2 as well, just to be sure everything is really the same.


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

PS1, Line 91: Conversion to local time will be done if
Return the corresponding timestamp in the local timezone if ....


PS1, Line 92: .
Otherwise, return the corresponding timestamp in UTC.


PS1, Line 97: 'FromUnixTime'
For functions, we usually write: FromUnixTime()


PS1, Line 105: unix_time
'unix_time'


PS1, Line 287: operator
do we need this other than that lexical_cast I commented on?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id25e19f7984e5ebf9073d9c569faf69cec142fa1
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5137: pt1, Refactor TimestampValue constructors

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has uploaded a new patch set (#2).

Change subject: IMPALA-5137: pt1, Refactor TimestampValue constructors
......................................................................

IMPALA-5137: pt1, Refactor TimestampValue constructors

In preparation for supporting Kudu TIMESTAMPs, some cleanup
of the TimestampValue code will be helpful first.

This change just refactors some of the TimestampValue
constructors which do more than simple construction, e.g.
parsing or converting, to static factory methods.

Change-Id: Id25e19f7984e5ebf9073d9c569faf69cec142fa1
---
M be/src/benchmarks/parse-timestamp-benchmark.cc
M be/src/exec/data-source-scan-node.cc
M be/src/exec/text-converter.inline.h
M be/src/exprs/aggregate-functions-ir.cc
M be/src/exprs/cast-functions-ir.cc
M be/src/exprs/decimal-operators-ir.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/expr-value.h
M be/src/exprs/literal.cc
M be/src/exprs/timestamp-functions-ir.cc
M be/src/exprs/timestamp-functions.cc
M be/src/runtime/raw-value-test.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/timestamp-test.cc
M be/src/runtime/timestamp-value.cc
M be/src/runtime/timestamp-value.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/query-exec-state.cc
M be/src/statestore/statestore-subscriber.cc
M be/src/util/dict-test.cc
21 files changed, 254 insertions(+), 150 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id25e19f7984e5ebf9073d9c569faf69cec142fa1
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>

[Impala-ASF-CR] IMPALA-5137: pt1, Refactor TimestampValue constructors

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5137: pt1, Refactor TimestampValue constructors
......................................................................


Patch Set 8:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/530/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id25e19f7984e5ebf9073d9c569faf69cec142fa1
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5137: pt1, Refactor TimestampValue constructors

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5137: pt1, Refactor TimestampValue constructors
......................................................................


Patch Set 9:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/532/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id25e19f7984e5ebf9073d9c569faf69cec142fa1
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5137: pt1, Refactor TimestampValue constructors

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has abandoned this change.

Change subject: IMPALA-5137: pt1, Refactor TimestampValue constructors
......................................................................


Abandoned

merging with https://gerrit.cloudera.org/#/c/6526/

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: Id25e19f7984e5ebf9073d9c569faf69cec142fa1
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>

[Impala-ASF-CR] IMPALA-5137: pt1, Refactor TimestampValue constructors

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5137: pt1, Refactor TimestampValue constructors
......................................................................


Patch Set 9: Verified-1

Build failed: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/532/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id25e19f7984e5ebf9073d9c569faf69cec142fa1
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5137: pt1, Refactor TimestampValue constructors

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5137: pt1, Refactor TimestampValue constructors
......................................................................


Patch Set 2:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/425/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id25e19f7984e5ebf9073d9c569faf69cec142fa1
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5137: pt1, Refactor TimestampValue constructors

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5137: pt1, Refactor TimestampValue constructors
......................................................................


Patch Set 8: Verified-1

Build failed: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/530/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id25e19f7984e5ebf9073d9c569faf69cec142fa1
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5137: pt1, Refactor TimestampValue constructors

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5137: pt1, Refactor TimestampValue constructors
......................................................................


Patch Set 8: Code-Review+2

rebase, fixed a merge conflict

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id25e19f7984e5ebf9073d9c569faf69cec142fa1
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5137: pt1, Refactor TimestampValue constructors

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5137: pt1, Refactor TimestampValue constructors
......................................................................


Patch Set 9: Code-Review+2

Had to change a new instance of TimestampValue::DebugString -> ToString that came in after rebasing.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id25e19f7984e5ebf9073d9c569faf69cec142fa1
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5137: pt1, Refactor TimestampValue constructors

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5137: pt1, Refactor TimestampValue constructors
......................................................................


Patch Set 7:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/528/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id25e19f7984e5ebf9073d9c569faf69cec142fa1
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5137: pt1, Refactor TimestampValue constructors

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.

Change subject: IMPALA-5137: pt1, Refactor TimestampValue constructors
......................................................................


Patch Set 2: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6510/2/be/src/exprs/timestamp-functions-ir.cc
File be/src/exprs/timestamp-functions-ir.cc:

Line 81:       TimestampValue::FromUnixTime(intp.val).ToString());
not your change and don't have to address it, but it looks like this has weird behavior when the input unix time is out of range of a TimestampValue. Looks like the result is an empty string, whereas StringValFromTimestamp() used below gives null. Filed IIMPALA-5146 for that.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id25e19f7984e5ebf9073d9c569faf69cec142fa1
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5137: pt1, Refactor TimestampValue constructors

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins, Dan Hecht,

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

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

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

Change subject: IMPALA-5137: pt1, Refactor TimestampValue constructors
......................................................................

IMPALA-5137: pt1, Refactor TimestampValue constructors

In preparation for supporting Kudu TIMESTAMPs, some cleanup
of the TimestampValue code will be helpful first.

This change just refactors some of the TimestampValue
constructors which do more than simple construction, e.g.
parsing or converting, to static factory methods.

Change-Id: Id25e19f7984e5ebf9073d9c569faf69cec142fa1
---
M be/src/benchmarks/parse-timestamp-benchmark.cc
M be/src/exec/data-source-scan-node.cc
M be/src/exec/parquet-column-readers.cc
M be/src/exec/text-converter.inline.h
M be/src/exprs/aggregate-functions-ir.cc
M be/src/exprs/cast-functions-ir.cc
M be/src/exprs/decimal-operators-ir.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/expr-value.h
M be/src/exprs/literal.cc
M be/src/exprs/timestamp-functions-ir.cc
M be/src/exprs/timestamp-functions.cc
M be/src/runtime/raw-value-test.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/timestamp-test.cc
M be/src/runtime/timestamp-value.cc
M be/src/runtime/timestamp-value.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/query-exec-state.cc
M be/src/statestore/statestore-subscriber.cc
M be/src/util/dict-test.cc
22 files changed, 255 insertions(+), 151 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id25e19f7984e5ebf9073d9c569faf69cec142fa1
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>

[Impala-ASF-CR] IMPALA-5137: pt1, Refactor TimestampValue constructors

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5137: pt1, Refactor TimestampValue constructors
......................................................................


Patch Set 1:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/6510/1/be/src/exec/data-source-scan-node.cc
File be/src/exec/data-source-scan-node.cc:

PS1, Line 284: FromUnixTimeNanos
> does it make sense that this conversion is subjected to use local tz flag? 
Hm I'm not sure, though if so I'll file a separate JIRA so it can be tracked externally as this would be a breaking change (which probably affects nearly nobody).


http://gerrit.cloudera.org:8080/#/c/6510/1/be/src/exprs/aggregate-functions-ir.cc
File be/src/exprs/aggregate-functions-ir.cc:

PS1, Line 354: TimestampValue
> maybe use "const TimetampValue& tv = ..." here (and elsewhere), though mayb
Done


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

PS1, Line 81: lexical_cast<string>(t)
> how about getting rid of this implicit code as well by defining a ToString(
yeah this is weird, agreed


http://gerrit.cloudera.org:8080/#/c/6510/1/be/src/runtime/timestamp-test.cc
File be/src/runtime/timestamp-test.cc:

PS1, Line 632: coverted
> converted
Done


PS1, Line 641:   EXPECT_TRUE(leap_tv1.ToUnixTime(&leap_time_t));
             :   EXPECT_EQ(915148800, leap_time_t);
             :   // Converted to the Unix time representation
             :   EXPECT_EQ("1999-01-01 00:00:00", leap_tv1.DebugString());
> maybe do these cases on leap_ptime2 as well, just to be sure everything is 
Done


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

PS1, Line 91: Conversion to local time will be done if
> Return the corresponding timestamp in the local timezone if ....
Done


PS1, Line 92: .
> Otherwise, return the corresponding timestamp in UTC.
Done


PS1, Line 97: 'FromUnixTime'
> For functions, we usually write: FromUnixTime()
Done


PS1, Line 105: unix_time
> 'unix_time'
Done


PS1, Line 287: operator
> do we need this other than that lexical_cast I commented on?
I think we could work around it, but this does get used in a few places where we branch on the type and directly use this operator, e.g. literal.cc and raw-value.cc


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id25e19f7984e5ebf9073d9c569faf69cec142fa1
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5137: pt1, Refactor TimestampValue constructors

Posted by "Attila Jeges (Code Review)" <ge...@cloudera.org>.
Attila Jeges has posted comments on this change.

Change subject: IMPALA-5137: pt1, Refactor TimestampValue constructors
......................................................................


Patch Set 11:

(2 comments)

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

PS11, Line 282:   
nit: indentation is 4 for line continuation (here and below).


http://gerrit.cloudera.org:8080/#/c/6510/11/be/src/exprs/timestamp-functions-ir.cc
File be/src/exprs/timestamp-functions-ir.cc:

PS11, Line 93: TimestampValue t 
You might want to use "const TimestampValue& t =" here (and below), just like you did in aggregate-functions-ir.cc


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id25e19f7984e5ebf9073d9c569faf69cec142fa1
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5137: pt1, Refactor TimestampValue constructors

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5137: pt1, Refactor TimestampValue constructors
......................................................................


Patch Set 7: Code-Review+2

sorry for all the rebasing noise, I'm holding off on committing this until the TIMESTAMP patch gets reviewed in case this needs to change:

https://gerrit.cloudera.org/#/c/6526/5

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id25e19f7984e5ebf9073d9c569faf69cec142fa1
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: No