You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Kim Jin Chul (Code Review)" <ge...@cloudera.org> on 2017/10/19 12:04:42 UTC
[Impala-ASF-CR] IMPALA-5607: Add additional units to EXTRACT, DATE PART, TRUNC
Kim Jin Chul has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8311
Change subject: IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC
......................................................................
IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC
Return type of EXTRACT/DATE_PART is changed from INT to BIGINT
because INT data type cannot cover NANOSECOND's value.
* Add the following fields to EXTRACT and DATE_PART:
WEEK
DOW
DOY
SECOND/SECONDS
MICROSECOND/MICROSECONDS
NANOSECOND/NANOSECONDS
* Add the following field to TRUNC:
SS
* Testing:
Extend unit tests in expr-test
Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
---
M be/src/exprs/expr-test.cc
M be/src/exprs/udf-builtins-ir.cc
M be/src/exprs/udf-builtins.cc
M be/src/exprs/udf-builtins.h
M common/function-registry/impala_functions.py
M common/thrift/Exprs.thrift
6 files changed, 221 insertions(+), 84 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/8311/2
--
To view, visit http://gerrit.cloudera.org:8080/8311
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
Gerrit-Change-Number: 8311
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
[Impala-ASF-CR] IMPALA-5607: Add additional units to EXTRACT, DATE PART, TRUNC
Posted by "Kim Jin Chul (Code Review)" <ge...@cloudera.org>.
Kim Jin Chul has abandoned this change. ( http://gerrit.cloudera.org:8080/8311 )
Change subject: IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC
......................................................................
Abandoned
I guess the reviewers have a bunch of review requests and they may have the hassle to distinguish suspended changes from pending review requests. Impala 3.0 is also far from now.
These are the reason why I would like to abandon this change.
I appreciate all your help and see you before 3.0!
--
To view, visit http://gerrit.cloudera.org:8080/8311
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
Gerrit-Change-Number: 8311
Gerrit-PatchSet: 6
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
[Impala-ASF-CR] IMPALA-5607: Add additional units to EXTRACT, DATE PART, TRUNC
Posted by "Kim Jin Chul (Code Review)" <ge...@cloudera.org>.
Kim Jin Chul has posted comments on this change. ( http://gerrit.cloudera.org:8080/8311 )
Change subject: IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/8311/2/be/src/exprs/udf-builtins-ir.cc
File be/src/exprs/udf-builtins-ir.cc:
http://gerrit.cloudera.org:8080/#/c/8311/2/be/src/exprs/udf-builtins-ir.cc@120
PS2, Line 120: return time.fractional_seconds() + time.seconds() * 1000 * 1000 * 1000;
> Use NANOS_PER_SEC
Done
http://gerrit.cloudera.org:8080/#/c/8311/2/be/src/exprs/udf-builtins-ir.cc@125
PS2, Line 125: return ExtractNanosecond(time) / 1000;
> Instead of dividing the result of another function, these should be impleme
Done
http://gerrit.cloudera.org:8080/#/c/8311/2/be/src/exprs/udf-builtins-ir.cc@130
PS2, Line 130: return ExtractMicrosecond(time) / 1000;
> same comment applies
Done
--
To view, visit http://gerrit.cloudera.org:8080/8311
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
Gerrit-Change-Number: 8311
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Oct 2017 12:03:38 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5607: Add additional units to EXTRACT, DATE PART, TRUNC
Posted by "Kim Jin Chul (Code Review)" <ge...@cloudera.org>.
Hello Greg Rahn, Zach Amsden,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8311
to look at the new patch set (#3).
Change subject: IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC
......................................................................
IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC
Return type of EXTRACT/DATE_PART is changed from INT to BIGINT
because INT data type cannot cover NANOSECOND's value.
* Add the following fields to EXTRACT and DATE_PART:
WEEK
DOW
DOY
SECOND/SECONDS
MICROSECOND/MICROSECONDS
NANOSECOND/NANOSECONDS
* Add the following field to TRUNC:
SS
* Testing:
Extend unit tests in expr-test
Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
---
M be/src/exprs/expr-test.cc
M be/src/exprs/udf-builtins-ir.cc
M be/src/exprs/udf-builtins.cc
M be/src/exprs/udf-builtins.h
M common/function-registry/impala_functions.py
M common/thrift/Exprs.thrift
6 files changed, 210 insertions(+), 84 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/8311/3
--
To view, visit http://gerrit.cloudera.org:8080/8311
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
Gerrit-Change-Number: 8311
Gerrit-PatchSet: 3
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
[Impala-ASF-CR] IMPALA-5607: Add additional units to EXTRACT, DATE PART, TRUNC
Posted by "Kim Jin Chul (Code Review)" <ge...@cloudera.org>.
Kim Jin Chul has posted comments on this change. ( http://gerrit.cloudera.org:8080/8311 )
Change subject: IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8311/4/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
http://gerrit.cloudera.org:8080/#/c/8311/4/be/src/exprs/expr-test.cc@6037
PS4, Line 6037: TYPE_BIGINT, 28123);
> This is a compatibility breaking change; we need to document the issue.
Should we document incompatibility change on this change? or do we document it on a different change if your release date is not upcoming?
http://gerrit.cloudera.org:8080/#/c/8311/4/be/src/exprs/udf-builtins-ir.cc
File be/src/exprs/udf-builtins-ir.cc:
http://gerrit.cloudera.org:8080/#/c/8311/4/be/src/exprs/udf-builtins-ir.cc@134
PS4, Line 134: return time.fractional_seconds() / NANOS_PER_MICRO / MICROS_PER_MILLI + time.seconds() * MILLIS_PER_SEC;
> Too confusing - please use / (NANOS_PER_MICRO * MICROS_PER_MILLI)
Done
--
To view, visit http://gerrit.cloudera.org:8080/8311
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
Gerrit-Change-Number: 8311
Gerrit-PatchSet: 4
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Oct 2017 05:58:23 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5607: Add additional units to EXTRACT, DATE PART, TRUNC
Posted by "Kim Jin Chul (Code Review)" <ge...@cloudera.org>.
Hello Greg Rahn, Zach Amsden,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8311
to look at the new patch set (#5).
Change subject: IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC
......................................................................
IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC
Return type of EXTRACT/DATE_PART is changed from INT to BIGINT
because INT data type cannot cover NANOSECOND's value.
* Add the following fields to EXTRACT and DATE_PART:
WEEK
DOW
DOY
SECOND/SECONDS
MICROSECOND/MICROSECONDS
NANOSECOND/NANOSECONDS
* Add the following field to TRUNC:
SS
* Testing:
Extend unit tests in expr-test
Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
---
M be/src/exprs/expr-test.cc
M be/src/exprs/udf-builtins-ir.cc
M be/src/exprs/udf-builtins.cc
M be/src/exprs/udf-builtins.h
M common/function-registry/impala_functions.py
M common/thrift/Exprs.thrift
6 files changed, 210 insertions(+), 84 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/8311/5
--
To view, visit http://gerrit.cloudera.org:8080/8311
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
Gerrit-Change-Number: 8311
Gerrit-PatchSet: 5
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
[Impala-ASF-CR] IMPALA-5607: Add additional units to EXTRACT, DATE PART, TRUNC
Posted by "Kim Jin Chul (Code Review)" <ge...@cloudera.org>.
Kim Jin Chul has posted comments on this change. ( http://gerrit.cloudera.org:8080/8311 )
Change subject: IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC
......................................................................
Patch Set 6:
Jim, okay, let's hold this change.
--
To view, visit http://gerrit.cloudera.org:8080/8311
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
Gerrit-Change-Number: 8311
Gerrit-PatchSet: 6
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 Nov 2017 04:13:09 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5607: Add additional units to EXTRACT, DATE PART, TRUNC
Posted by "Kim Jin Chul (Code Review)" <ge...@cloudera.org>.
Hello Greg Rahn, Jim Apple, Zach Amsden,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8311
to look at the new patch set (#6).
Change subject: IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC
......................................................................
IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC
Return type of EXTRACT/DATE_PART is changed from INT to BIGINT
because INT data type cannot cover NANOSECOND's value.
* Add the following fields to EXTRACT and DATE_PART:
WEEK
DOW
DOY
SECOND/SECONDS
MICROSECOND/MICROSECONDS
NANOSECOND/NANOSECONDS
* Add the following field to TRUNC:
SS
* Testing:
Extend unit tests in expr-test
Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
---
M be/src/exprs/expr-test.cc
M be/src/exprs/udf-builtins-ir.cc
M be/src/exprs/udf-builtins.cc
M be/src/exprs/udf-builtins.h
M common/function-registry/impala_functions.py
M common/thrift/Exprs.thrift
6 files changed, 211 insertions(+), 84 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/8311/6
--
To view, visit http://gerrit.cloudera.org:8080/8311
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
Gerrit-Change-Number: 8311
Gerrit-PatchSet: 6
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
[Impala-ASF-CR] IMPALA-5607: Add additional units to EXTRACT, DATE PART, TRUNC
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change. ( http://gerrit.cloudera.org:8080/8311 )
Change subject: IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC
......................................................................
Patch Set 6:
Based on the conversation on dev@ (https://lists.apache.org/thread.html/bf1cbd4644a18489f99ee708291b348cef9a379a26d5ebe6d043d3f2@%3Cdev.impala.apache.org%3E), I suggest we wait on this until 3.0.
--
To view, visit http://gerrit.cloudera.org:8080/8311
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
Gerrit-Change-Number: 8311
Gerrit-PatchSet: 6
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Wed, 15 Nov 2017 15:58:22 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5607: Add additional units to EXTRACT, DATE PART, TRUNC
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has posted comments on this change. ( http://gerrit.cloudera.org:8080/8311 )
Change subject: IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC
......................................................................
Patch Set 4:
(3 comments)
Let's figure out how to address MILLISECONDS in the JIRA. We also should figure out what to do about rounding vs. truncation when converting to less precise units.
http://gerrit.cloudera.org:8080/#/c/8311/4/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
http://gerrit.cloudera.org:8080/#/c/8311/4/be/src/exprs/expr-test.cc@6037
PS4, Line 6037: TYPE_BIGINT, 28123);
This is a compatibility breaking change; we need to document the issue.
http://gerrit.cloudera.org:8080/#/c/8311/4/be/src/exprs/udf-builtins-ir.cc
File be/src/exprs/udf-builtins-ir.cc:
http://gerrit.cloudera.org:8080/#/c/8311/4/be/src/exprs/udf-builtins-ir.cc@a193
PS4, Line 193:
This breaks compatibility; let's discuss what the most appropriate resolution is.
http://gerrit.cloudera.org:8080/#/c/8311/4/be/src/exprs/udf-builtins-ir.cc@134
PS4, Line 134: return time.fractional_seconds() / NANOS_PER_MICRO / MICROS_PER_MILLI + time.seconds() * MILLIS_PER_SEC;
Too confusing - please use / (NANOS_PER_MICRO * MICROS_PER_MILLI)
--
To view, visit http://gerrit.cloudera.org:8080/8311
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
Gerrit-Change-Number: 8311
Gerrit-PatchSet: 4
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Oct 2017 00:19:57 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5607: Add additional units to EXTRACT, DATE PART, TRUNC
Posted by "Kim Jin Chul (Code Review)" <ge...@cloudera.org>.
Hello Greg Rahn, Zach Amsden,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8311
to look at the new patch set (#4).
Change subject: IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC
......................................................................
IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC
Return type of EXTRACT/DATE_PART is changed from INT to BIGINT
because INT data type cannot cover NANOSECOND's value.
* Add the following fields to EXTRACT and DATE_PART:
WEEK
DOW
DOY
SECOND/SECONDS
MICROSECOND/MICROSECONDS
NANOSECOND/NANOSECONDS
* Add the following field to TRUNC:
SS
* Testing:
Extend unit tests in expr-test
Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
---
M be/src/exprs/expr-test.cc
M be/src/exprs/udf-builtins-ir.cc
M be/src/exprs/udf-builtins.cc
M be/src/exprs/udf-builtins.h
M common/function-registry/impala_functions.py
M common/thrift/Exprs.thrift
6 files changed, 210 insertions(+), 84 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/8311/4
--
To view, visit http://gerrit.cloudera.org:8080/8311
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
Gerrit-Change-Number: 8311
Gerrit-PatchSet: 4
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
[Impala-ASF-CR] IMPALA-5607: Add additional units to EXTRACT, DATE PART, TRUNC
Posted by "Kim Jin Chul (Code Review)" <ge...@cloudera.org>.
Kim Jin Chul has posted comments on this change. ( http://gerrit.cloudera.org:8080/8311 )
Change subject: IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/8311/2/be/src/exprs/udf-builtins-ir.cc
File be/src/exprs/udf-builtins-ir.cc:
http://gerrit.cloudera.org:8080/#/c/8311/2/be/src/exprs/udf-builtins-ir.cc@120
PS2, Line 120: return time.fractional_seconds() + time.seconds() * 1000 * 1000 * 1000;
The seconds field, including fractional parts, multiplied by 1 000 000 000; note that this includes full seconds
11:22:33.123456789
=> 33123456789
http://gerrit.cloudera.org:8080/#/c/8311/2/be/src/exprs/udf-builtins-ir.cc@154
PS2, Line 154: BigIntVal
Return type should be changed from INT to BIGINT because INT data type's range cannot cover NANOSECOND.
http://gerrit.cloudera.org:8080/#/c/8311/2/be/src/exprs/udf-builtins-ir.cc@194
PS2, Line 194: case TExtractField::NANOSECONDS:
MILLISECONDS, MICROSECONDS and NANOSECONDS look like useless, but ExtractFromExpr::EXTRACT_FIELDS should require them.
The available field type check in fe needs the aliases. The check code is in
src/main/java/org/apache/impala/analysis/ExtractFromExpr.java
--
To view, visit http://gerrit.cloudera.org:8080/8311
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
Gerrit-Change-Number: 8311
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Comment-Date: Thu, 19 Oct 2017 12:18:55 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5607: Add additional units to EXTRACT, DATE PART, TRUNC
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has posted comments on this change. ( http://gerrit.cloudera.org:8080/8311 )
Change subject: IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC
......................................................................
Patch Set 2:
Looks like we want NANOSECONDS after all, so the BIGINT change is needed.
--
To view, visit http://gerrit.cloudera.org:8080/8311
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
Gerrit-Change-Number: 8311
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Oct 2017 23:24:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5607: Add additional units to EXTRACT, DATE PART, TRUNC
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change. ( http://gerrit.cloudera.org:8080/8311 )
Change subject: IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC
......................................................................
Patch Set 6:
> Jim, okay, let's hold this change.
To get an open code review off of your reviewers' docket, you can click "Abandon". YOu can "unAbandon" later - this review won't get deleted, because Abandon doesn't delete.
This action is optional on your part. Reviewers can always take themselves off of a review if they want to keep a tidy docket.
Thanks for sending this - hopefully we can land it in 3.0, with your help!
--
To view, visit http://gerrit.cloudera.org:8080/8311
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
Gerrit-Change-Number: 8311
Gerrit-PatchSet: 6
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 Nov 2017 17:38:29 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5607: Add additional units to EXTRACT, DATE PART, TRUNC
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change. ( http://gerrit.cloudera.org:8080/8311 )
Change subject: IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8311/4/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
http://gerrit.cloudera.org:8080/#/c/8311/4/be/src/exprs/expr-test.cc@6037
PS4, Line 6037: TYPE_BIGINT, 28123);
> This is a compatibility breaking change; we need to document the issue.
I would suggest discussing this on dev@, since some other compat-breaking changes have been pushed to 3.0 and some other compat-breaking changes have been hidden behind a feature flag.
--
To view, visit http://gerrit.cloudera.org:8080/8311
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
Gerrit-Change-Number: 8311
Gerrit-PatchSet: 4
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Oct 2017 14:41:36 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5607: Add additional units to EXTRACT, DATE PART, TRUNC
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has posted comments on this change. ( http://gerrit.cloudera.org:8080/8311 )
Change subject: IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC
......................................................................
Patch Set 2:
(6 comments)
Overall this looks good - and thank you for the tests. It is worth debating whether NANOSECONDs should be supported at all - let's do that in the JIRA.
http://gerrit.cloudera.org:8080/#/c/8311/2/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
http://gerrit.cloudera.org:8080/#/c/8311/2/be/src/exprs/expr-test.cc@5959
PS2, Line 5959: TestStringValue(
I don't think the additional test cases add any value unless you add greater precision, i.e.
"cast(trunc(cast('2012-09-10 07:02:03.1234' as timestamp), 'SS') as string)"
http://gerrit.cloudera.org:8080/#/c/8311/2/be/src/exprs/udf-builtins-ir.cc
File be/src/exprs/udf-builtins-ir.cc:
http://gerrit.cloudera.org:8080/#/c/8311/2/be/src/exprs/udf-builtins-ir.cc@120
PS2, Line 120: return time.fractional_seconds() + time.seconds() * 1000 * 1000 * 1000;
> The seconds field, including fractional parts, multiplied by 1 000 000 000;
Use NANOS_PER_SEC
http://gerrit.cloudera.org:8080/#/c/8311/2/be/src/exprs/udf-builtins-ir.cc@125
PS2, Line 125: return ExtractNanosecond(time) / 1000;
Instead of dividing the result of another function, these should be implement directly, i.e.
time.fractional_seconds() / NANOS_PER_MICRO + time.seconds() * MICROS_PER_SEC
http://gerrit.cloudera.org:8080/#/c/8311/2/be/src/exprs/udf-builtins-ir.cc@130
PS2, Line 130: return ExtractMicrosecond(time) / 1000;
same comment applies
http://gerrit.cloudera.org:8080/#/c/8311/2/be/src/exprs/udf-builtins-ir.cc@154
PS2, Line 154: BigIntVal
> Return type should be changed from INT to BIGINT because INT data type's ra
We only need to change this if we actually implement nanosecond units for EXTRACT; no other referenced system in the JIRA currently supports that, so it is worth debating whether we should or not.
http://gerrit.cloudera.org:8080/#/c/8311/2/common/thrift/Exprs.thrift
File common/thrift/Exprs.thrift:
http://gerrit.cloudera.org:8080/#/c/8311/2/common/thrift/Exprs.thrift@92
PS2, Line 92: MILLISECONDS,
We shouldn't have redundant field definitions here; instead we should be lenient with parsing and convert MILISECONDS to MILISECOND.
--
To view, visit http://gerrit.cloudera.org:8080/8311
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
Gerrit-Change-Number: 8311
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Oct 2017 23:13:01 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5607: Add additional units to EXTRACT, DATE PART, TRUNC
Posted by "Kim Jin Chul (Code Review)" <ge...@cloudera.org>.
Kim Jin Chul has posted comments on this change. ( http://gerrit.cloudera.org:8080/8311 )
Change subject: IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8311/2/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
http://gerrit.cloudera.org:8080/#/c/8311/2/be/src/exprs/expr-test.cc@5959
PS2, Line 5959: "cast(trunc(cast('2012-09-10 01:02:03' as timestamp), 'DAY') as string)",
> I don't think the additional test cases add any value unless you add greate
Done
http://gerrit.cloudera.org:8080/#/c/8311/2/common/thrift/Exprs.thrift
File common/thrift/Exprs.thrift:
http://gerrit.cloudera.org:8080/#/c/8311/2/common/thrift/Exprs.thrift@92
PS2, Line 92: MILLISECONDS,
> We shouldn't have redundant field definitions here; instead we should be le
I think we need them even though they look like redundant. See my comment at line 194: https://gerrit.cloudera.org/#/c/8311/2/be/src/exprs/udf-builtins-ir.cc
--
To view, visit http://gerrit.cloudera.org:8080/8311
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
Gerrit-Change-Number: 8311
Gerrit-PatchSet: 3
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Oct 2017 09:43:49 +0000
Gerrit-HasComments: Yes