You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tim Armstrong (Code Review)" <ge...@cloudera.org> on 2018/04/09 19:09:59 UTC

[Impala-ASF-CR] IMPALA-5607: part 1: change date part to return bigint 

Tim Armstrong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9957


Change subject: IMPALA-5607: part 1: change date_part to return bigint
......................................................................

IMPALA-5607: part 1: change date_part to return bigint

This is the compatibility-breaking part of Jinchul Kim's change
to add additional units. To support nanoseconds we need to
widen the output type of these functions. We also change
the meaning of "milliseconds" to include the seconds component.

Cherry-picks: not for 2.x

Change-Id: I42d83712d9bb3a4900bec38a9c009dcf2a1fe019
---
M be/src/exprs/expr-test.cc
M be/src/exprs/udf-builtins-ir.cc
M be/src/exprs/udf-builtins.h
M common/function-registry/impala_functions.py
4 files changed, 91 insertions(+), 83 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I42d83712d9bb3a4900bec38a9c009dcf2a1fe019
Gerrit-Change-Number: 9957
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5607: part 1: breaking extract/date part changes

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9957 )

Change subject: IMPALA-5607: part 1: breaking extract/date_part changes
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9957/2/be/src/exprs/udf-builtins-ir.cc
File be/src/exprs/udf-builtins-ir.cc:

http://gerrit.cloudera.org:8080/#/c/9957/2/be/src/exprs/udf-builtins-ir.cc@117
PS2, Line 117: static int64_t ExtractMillisecond(const time_duration& time) {
             :   // Fractional seconds are nanoseconds because Boost is configured
             :   // to use nanoseconds precision
             :   return time.fractional_seconds() / (NANOS_PER_MICRO * MICROS_PER_MILLI)
             :        + time.seconds() * MILLIS_PER_SEC;
             : }
> How about util/time.h ? It seems to be a generic place for time related uti
util/time.h hasn't been polluted with boost::datetime stuff yet :) . It feels like we should avoid that.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I42d83712d9bb3a4900bec38a9c009dcf2a1fe019
Gerrit-Change-Number: 9957
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 09 Apr 2018 22:14:28 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5607: part 1: breaking extract/date part changes

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9957 )

Change subject: IMPALA-5607: part 1: breaking extract/date_part changes
......................................................................


Patch Set 3:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I42d83712d9bb3a4900bec38a9c009dcf2a1fe019
Gerrit-Change-Number: 9957
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Apr 2018 00:06:02 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5607: part 1: breaking extract/date part changes

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9957 )

Change subject: IMPALA-5607: part 1: breaking extract/date_part changes
......................................................................


Patch Set 3: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I42d83712d9bb3a4900bec38a9c009dcf2a1fe019
Gerrit-Change-Number: 9957
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Apr 2018 04:00:36 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5607: part 1: breaking extract/date part changes

Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/9957 )

Change subject: IMPALA-5607: part 1: breaking extract/date_part changes
......................................................................


Patch Set 2: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I42d83712d9bb3a4900bec38a9c009dcf2a1fe019
Gerrit-Change-Number: 9957
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Mon, 09 Apr 2018 21:44:29 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5607: part 1: breaking extract/date part changes

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9957 )

Change subject: IMPALA-5607: part 1: breaking extract/date_part changes
......................................................................


Patch Set 3: Code-Review+2

Missed updating result types in some query tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I42d83712d9bb3a4900bec38a9c009dcf2a1fe019
Gerrit-Change-Number: 9957
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Apr 2018 00:05:58 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5607: part 1: breaking extract/date part changes

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9957 )

Change subject: IMPALA-5607: part 1: breaking extract/date_part changes
......................................................................


Patch Set 2: Code-Review+1

(1 comment)

This looks reasonable to me, I just have one minor comment.

http://gerrit.cloudera.org:8080/#/c/9957/2/be/src/exprs/udf-builtins-ir.cc
File be/src/exprs/udf-builtins-ir.cc:

http://gerrit.cloudera.org:8080/#/c/9957/2/be/src/exprs/udf-builtins-ir.cc@117
PS2, Line 117: static int64_t ExtractMillisecond(const time_duration& time) {
             :   // Fractional seconds are nanoseconds because Boost is configured
             :   // to use nanoseconds precision
             :   return time.fractional_seconds() / (NANOS_PER_MICRO * MICROS_PER_MILLI)
             :        + time.seconds() * MILLIS_PER_SEC;
             : }
Is there an appropriate util file that this could live in?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I42d83712d9bb3a4900bec38a9c009dcf2a1fe019
Gerrit-Change-Number: 9957
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Mon, 09 Apr 2018 21:37:44 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5607: part 1: breaking extract/date part changes

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9957 )

Change subject: IMPALA-5607: part 1: breaking extract/date_part changes
......................................................................


Patch Set 2:

I started a dry run here an hour or two ago
: https://jenkins.impala.io/job/gerrit-verify-dryrun/2262/.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I42d83712d9bb3a4900bec38a9c009dcf2a1fe019
Gerrit-Change-Number: 9957
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 09 Apr 2018 21:50:55 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5607: part 1: breaking extract/date part changes

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Philip Zeyliger, Sailesh Mukil, 

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

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

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

Change subject: IMPALA-5607: part 1: breaking extract/date_part changes
......................................................................

IMPALA-5607: part 1: breaking extract/date_part changes

This is the compatibility-breaking part of Jinchul Kim's change
to add additional units. To support nanoseconds we need to
widen the output type of these functions. We also change
the meaning of "milliseconds" to include the seconds component.

Cherry-picks: not for 2.x

Change-Id: I42d83712d9bb3a4900bec38a9c009dcf2a1fe019
---
M be/src/exprs/expr-test.cc
M be/src/exprs/udf-builtins-ir.cc
M be/src/exprs/udf-builtins.h
M common/function-registry/impala_functions.py
M testdata/workloads/functional-query/queries/QueryTest/aggregation.test
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
6 files changed, 94 insertions(+), 86 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I42d83712d9bb3a4900bec38a9c009dcf2a1fe019
Gerrit-Change-Number: 9957
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5607: part 1: breaking extract/date part changes

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9957 )

Change subject: IMPALA-5607: part 1: breaking extract/date_part changes
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9957/2/be/src/exprs/udf-builtins-ir.cc
File be/src/exprs/udf-builtins-ir.cc:

http://gerrit.cloudera.org:8080/#/c/9957/2/be/src/exprs/udf-builtins-ir.cc@117
PS2, Line 117: static int64_t ExtractMillisecond(const time_duration& time) {
             :   // Fractional seconds are nanoseconds because Boost is configured
             :   // to use nanoseconds precision
             :   return time.fractional_seconds() / (NANOS_PER_MICRO * MICROS_PER_MILLI)
             :        + time.seconds() * MILLIS_PER_SEC;
             : }
> Is there an appropriate util file that this could live in?
I don't see an obvious place. be/src/runtime/timestamp-value.h is the closest but this isn't operating directly on a TimestampValue.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I42d83712d9bb3a4900bec38a9c009dcf2a1fe019
Gerrit-Change-Number: 9957
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 09 Apr 2018 21:49:48 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5607: part 1: breaking extract/date part changes

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9957 )

Change subject: IMPALA-5607: part 1: breaking extract/date_part changes
......................................................................


Patch Set 2: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9957/2/be/src/exprs/udf-builtins-ir.cc
File be/src/exprs/udf-builtins-ir.cc:

http://gerrit.cloudera.org:8080/#/c/9957/2/be/src/exprs/udf-builtins-ir.cc@117
PS2, Line 117: static int64_t ExtractMillisecond(const time_duration& time) {
             :   // Fractional seconds are nanoseconds because Boost is configured
             :   // to use nanoseconds precision
             :   return time.fractional_seconds() / (NANOS_PER_MICRO * MICROS_PER_MILLI)
             :        + time.seconds() * MILLIS_PER_SEC;
             : }
> util/time.h hasn't been polluted with boost::datetime stuff yet :) . It fee
Ok, sounds good to me.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I42d83712d9bb3a4900bec38a9c009dcf2a1fe019
Gerrit-Change-Number: 9957
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 09 Apr 2018 22:19:53 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5607: part 1: breaking extract/date part changes

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9957 )

Change subject: IMPALA-5607: part 1: breaking extract/date_part changes
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9957/2/be/src/exprs/udf-builtins-ir.cc
File be/src/exprs/udf-builtins-ir.cc:

http://gerrit.cloudera.org:8080/#/c/9957/2/be/src/exprs/udf-builtins-ir.cc@117
PS2, Line 117: static int64_t ExtractMillisecond(const time_duration& time) {
             :   // Fractional seconds are nanoseconds because Boost is configured
             :   // to use nanoseconds precision
             :   return time.fractional_seconds() / (NANOS_PER_MICRO * MICROS_PER_MILLI)
             :        + time.seconds() * MILLIS_PER_SEC;
             : }
> I don't see an obvious place. be/src/runtime/timestamp-value.h is the close
How about util/time.h ? It seems to be a generic place for time related util functions.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I42d83712d9bb3a4900bec38a9c009dcf2a1fe019
Gerrit-Change-Number: 9957
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 09 Apr 2018 22:00:57 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5607: part 1: breaking extract/date part changes

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9957 )

Change subject: IMPALA-5607: part 1: breaking extract/date_part changes
......................................................................

IMPALA-5607: part 1: breaking extract/date_part changes

This is the compatibility-breaking part of Jinchul Kim's change
to add additional units. To support nanoseconds we need to
widen the output type of these functions. We also change
the meaning of "milliseconds" to include the seconds component.

Cherry-picks: not for 2.x

Change-Id: I42d83712d9bb3a4900bec38a9c009dcf2a1fe019
Reviewed-on: http://gerrit.cloudera.org:8080/9957
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/exprs/expr-test.cc
M be/src/exprs/udf-builtins-ir.cc
M be/src/exprs/udf-builtins.h
M common/function-registry/impala_functions.py
M testdata/workloads/functional-query/queries/QueryTest/aggregation.test
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
6 files changed, 94 insertions(+), 86 deletions(-)

Approvals:
  Tim Armstrong: Looks good to me, approved
  Impala Public Jenkins: Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I42d83712d9bb3a4900bec38a9c009dcf2a1fe019
Gerrit-Change-Number: 9957
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5607: part 1: breaking extract/date part changes

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/9957 )

Change subject: IMPALA-5607: part 1: breaking extract/date_part changes
......................................................................

IMPALA-5607: part 1: breaking extract/date_part changes

This is the compatibility-breaking part of Jinchul Kim's change
to add additional units. To support nanoseconds we need to
widen the output type of these functions. We also change
the meaning of "milliseconds" to include the seconds component.

Cherry-picks: not for 2.x

Change-Id: I42d83712d9bb3a4900bec38a9c009dcf2a1fe019
---
M be/src/exprs/expr-test.cc
M be/src/exprs/udf-builtins-ir.cc
M be/src/exprs/udf-builtins.h
M common/function-registry/impala_functions.py
4 files changed, 91 insertions(+), 83 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I42d83712d9bb3a4900bec38a9c009dcf2a1fe019
Gerrit-Change-Number: 9957
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>