[Impala-ASF-CR] IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond
Impala Public Jenkins has submitted this change and it was merged. (
http://gerrit.cloudera.org:8080/18718 )
Change subject: IMPALA-11355: Add STRING overloads for
hour/minute/second/millisecond
..
IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond
IMPALA-9531 dropped support for "dateless timestamps",
e.g. cast("12:05:05" as timestamp) now returns NULL.
This led to breaking functions like minute("12:05:05"), as minute()
expects a timestamp, and Impala adds an implicit cast, so what actually
happens is minute(cast("12:05:05" as timestamp)), which returns NULL.
This change adds overloads for similar functions that take STRING
instead of TIMESTAMP parameter. The same functions already take a
STRING parameter in Hive and mySQL.
The changes in the parser mainly restore code removed in IMPALA-9531.
Note that these functions could be potentially optimized by returning
parts of the parse result without converting them to boost time first,
but this is not done here to make the change minimal.
Testing:
- restored related tests in expr-test and added some new ones for
malformed time-of-day strings
- added benchmarks for the new overloads and fixed the ones for the
old functions (they tested NULL)
Change-Id: I6cc1c851ee71ab4fcc58105c7e9931155a483679
Reviewed-on: http://gerrit.cloudera.org:8080/18718
Reviewed-by: Riza Suminto
Tested-by: Impala Public Jenkins
---
M be/src/benchmarks/expr-benchmark.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions-ir.cc
M be/src/exprs/timestamp-functions.h
M be/src/runtime/date-parse-util.cc
M be/src/runtime/datetime-simple-date-format-parser.cc
M be/src/runtime/datetime-simple-date-format-parser.h
M be/src/runtime/timestamp-parse-util.cc
M be/src/runtime/timestamp-parse-util.h
M common/function-registry/impala_functions.py
10 files changed, 202 insertions(+), 78 deletions(-)
Approvals:
Riza Suminto: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/18718
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I6cc1c851ee71ab4fcc58105c7e9931155a483679
Gerrit-Change-Number: 18718
Gerrit-PatchSet: 9
Gerrit-Owner: Csaba Ringhofer
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Riza Suminto
Gerrit-Reviewer: Wenzhe Zhou
[Impala-ASF-CR] IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18718 ) Change subject: IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond .. Patch Set 8: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/18718 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6cc1c851ee71ab4fcc58105c7e9931155a483679 Gerrit-Change-Number: 18718 Gerrit-PatchSet: 8 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Thu, 14 Jul 2022 21:03:42 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/18718 ) Change subject: IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond .. Patch Set 8: Code-Review+2 Carry +1 from Wenzhe. -- To view, visit http://gerrit.cloudera.org:8080/18718 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6cc1c851ee71ab4fcc58105c7e9931155a483679 Gerrit-Change-Number: 18718 Gerrit-PatchSet: 8 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Thu, 14 Jul 2022 17:14:43 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18718 ) Change subject: IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond .. Patch Set 8: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/8312/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/18718 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6cc1c851ee71ab4fcc58105c7e9931155a483679 Gerrit-Change-Number: 18718 Gerrit-PatchSet: 8 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Thu, 14 Jul 2022 16:17:56 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/18718 ) Change subject: IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond .. Patch Set 7: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/18718 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6cc1c851ee71ab4fcc58105c7e9931155a483679 Gerrit-Change-Number: 18718 Gerrit-PatchSet: 7 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Thu, 14 Jul 2022 16:15:21 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/18718 ) Change subject: IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond .. Patch Set 7: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/18718 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6cc1c851ee71ab4fcc58105c7e9931155a483679 Gerrit-Change-Number: 18718 Gerrit-PatchSet: 7 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Thu, 14 Jul 2022 16:09:47 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18718 ) Change subject: IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond .. Patch Set 7: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/10969/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/18718 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6cc1c851ee71ab4fcc58105c7e9931155a483679 Gerrit-Change-Number: 18718 Gerrit-PatchSet: 7 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Thu, 14 Jul 2022 12:07:25 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/18718 ) Change subject: IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond .. Patch Set 7: (2 comments) http://gerrit.cloudera.org:8080/#/c/18718/6/be/src/benchmarks/expr-benchmark.cc File be/src/benchmarks/expr-benchmark.cc: http://gerrit.cloudera.org:8080/#/c/18718/6/be/src/benchmarks/expr-benchmark.cc@940 PS6, Line 940: benchmarks.push_back(); > why comments out? ouch, thanks for spotting this, I commented out other tests to make the run faster http://gerrit.cloudera.org:8080/#/c/18718/6/be/src/exprs/timestamp-functions-ir.cc File be/src/exprs/timestamp-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/18718/6/be/src/exprs/timestamp-functions-ir.cc@300 PS6, Line 300: re > nit: extra spaces Done -- To view, visit http://gerrit.cloudera.org:8080/18718 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6cc1c851ee71ab4fcc58105c7e9931155a483679 Gerrit-Change-Number: 18718 Gerrit-PatchSet: 7 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Thu, 14 Jul 2022 11:51:46 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond
Hello Riza Suminto, Wenzhe Zhou, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18718
to look at the new patch set (#7).
Change subject: IMPALA-11355: Add STRING overloads for
hour/minute/second/millisecond
..
IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond
IMPALA-9531 dropped support for "dateless timestamps",
e.g. cast("12:05:05" as timestamp) now returns NULL.
This led to breaking functions like minute("12:05:05"), as minute()
expects a timestamp, and Impala adds an implicit cast, so what actually
happens is minute(cast("12:05:05" as timestamp)), which returns NULL.
This change adds overloads for similar functions that take STRING
instead of TIMESTAMP parameter. The same functions already take a
STRING parameter in Hive and mySQL.
The changes in the parser mainly restore code removed in IMPALA-9531.
Note that these functions could be potentially optimized by returning
parts of the parse result without converting them to boost time first,
but this is not done here to make the change minimal.
Testing:
- restored related tests in expr-test and added some new ones for
malformed time-of-day strings
- added benchmarks for the new overloads and fixed the ones for the
old functions (they tested NULL)
Change-Id: I6cc1c851ee71ab4fcc58105c7e9931155a483679
---
M be/src/benchmarks/expr-benchmark.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions-ir.cc
M be/src/exprs/timestamp-functions.h
M be/src/runtime/date-parse-util.cc
M be/src/runtime/datetime-simple-date-format-parser.cc
M be/src/runtime/datetime-simple-date-format-parser.h
M be/src/runtime/timestamp-parse-util.cc
M be/src/runtime/timestamp-parse-util.h
M common/function-registry/impala_functions.py
10 files changed, 202 insertions(+), 78 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/18718/7
--
To view, visit http://gerrit.cloudera.org:8080/18718
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6cc1c851ee71ab4fcc58105c7e9931155a483679
Gerrit-Change-Number: 18718
Gerrit-PatchSet: 7
Gerrit-Owner: Csaba Ringhofer
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Riza Suminto
Gerrit-Reviewer: Wenzhe Zhou
[Impala-ASF-CR] IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/18718 ) Change subject: IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond .. Patch Set 6: (2 comments) http://gerrit.cloudera.org:8080/#/c/18718/6/be/src/benchmarks/expr-benchmark.cc File be/src/benchmarks/expr-benchmark.cc: http://gerrit.cloudera.org:8080/#/c/18718/6/be/src/benchmarks/expr-benchmark.cc@940 PS6, Line 940: /*benchmarks.push_back(); why comments out? http://gerrit.cloudera.org:8080/#/c/18718/6/be/src/exprs/timestamp-functions-ir.cc File be/src/exprs/timestamp-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/18718/6/be/src/exprs/timestamp-functions-ir.cc@300 PS6, Line 300: nit: extra spaces -- To view, visit http://gerrit.cloudera.org:8080/18718 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6cc1c851ee71ab4fcc58105c7e9931155a483679 Gerrit-Change-Number: 18718 Gerrit-PatchSet: 6 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Wed, 13 Jul 2022 19:36:46 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/18718 ) Change subject: IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond .. Patch Set 6: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/18718 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6cc1c851ee71ab4fcc58105c7e9931155a483679 Gerrit-Change-Number: 18718 Gerrit-PatchSet: 6 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Wed, 13 Jul 2022 15:08:58 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18718 ) Change subject: IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond .. Patch Set 6: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/10959/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/18718 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6cc1c851ee71ab4fcc58105c7e9931155a483679 Gerrit-Change-Number: 18718 Gerrit-PatchSet: 6 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Wed, 13 Jul 2022 14:24:01 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond
Csaba Ringhofer has posted comments on this change. (
http://gerrit.cloudera.org:8080/18718 )
Change subject: IMPALA-11355: Add STRING overloads for
hour/minute/second/millisecond
..
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18718/5/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
http://gerrit.cloudera.org:8080/#/c/18718/5/be/src/exprs/expr-test.cc@6967
PS5, Line 6967: TestIsNull("minute('09-10-11')", TYPE_INT);
> Should it be "hour('838:59:59')"?
changed it to hour - note that it doesn't matter too much, as all these
functions return NULL if any component of time is invalid
>MySQL's hour function seems to accept string where the hour part is more than
>23.
Checked with Hive and it doesn't accept it. I think that not accepting it is
the safest way.
--
To view, visit http://gerrit.cloudera.org:8080/18718
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6cc1c851ee71ab4fcc58105c7e9931155a483679
Gerrit-Change-Number: 18718
Gerrit-PatchSet: 6
Gerrit-Owner: Csaba Ringhofer
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Riza Suminto
Gerrit-Comment-Date: Wed, 13 Jul 2022 14:06:18 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond
Hello Riza Suminto, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18718
to look at the new patch set (#6).
Change subject: IMPALA-11355: Add STRING overloads for
hour/minute/second/millisecond
..
IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond
IMPALA-9531 dropped support for "dateless timestamps",
e.g. cast("12:05:05" as timestamp) now returns NULL.
This led to breaking functions like minute("12:05:05"), as minute()
expects a timestamp, and Impala adds an implicit cast, so what actually
happens is minute(cast("12:05:05" as timestamp)), which returns NULL.
This change adds overloads for similar functions that take STRING
instead of TIMESTAMP parameter. The same functions already take a
STRING parameter in Hive and mySQL.
The changes in the parser mainly restore code removed in IMPALA-9531.
Note that these functions could be potentially optimized by returning
parts of the parse result without converting them to boost time first,
but this is not done here to make the change minimal.
Testing:
- restored related tests in expr-test and added some new ones for
malformed time-of-day strings
- added benchmarks for the new overloads and fixed the ones for the
old functions (they tested NULL)
Change-Id: I6cc1c851ee71ab4fcc58105c7e9931155a483679
---
M be/src/benchmarks/expr-benchmark.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions-ir.cc
M be/src/exprs/timestamp-functions.h
M be/src/runtime/date-parse-util.cc
M be/src/runtime/datetime-simple-date-format-parser.cc
M be/src/runtime/datetime-simple-date-format-parser.h
M be/src/runtime/timestamp-parse-util.cc
M be/src/runtime/timestamp-parse-util.h
M common/function-registry/impala_functions.py
10 files changed, 204 insertions(+), 80 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/18718/6
--
To view, visit http://gerrit.cloudera.org:8080/18718
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6cc1c851ee71ab4fcc58105c7e9931155a483679
Gerrit-Change-Number: 18718
Gerrit-PatchSet: 6
Gerrit-Owner: Csaba Ringhofer
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Riza Suminto
[Impala-ASF-CR] IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond
Riza Suminto has posted comments on this change. (
http://gerrit.cloudera.org:8080/18718 )
Change subject: IMPALA-11355: Add STRING overloads for
hour/minute/second/millisecond
..
Patch Set 5:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/18718/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18718/2//COMMIT_MSG@24
PS2, Line 24: not
> Done
Thank you.
http://gerrit.cloudera.org:8080/#/c/18718/2//COMMIT_MSG@26
PS2, Line 26: Testing:
> added tests and fixed the old ones - note that my machine seems much slower
Thank you.
http://gerrit.cloudera.org:8080/#/c/18718/2/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
http://gerrit.cloudera.org:8080/#/c/18718/2/be/src/exprs/expr-test.cc@6957
PS2, Line 6957: // These expressions directly extract
hour/minute/second/millis from STRING type
: // to support these functions for timestamp strings without a
date part (IMPALA-11355).
> Done
Thank you.
http://gerrit.cloudera.org:8080/#/c/18718/5/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
http://gerrit.cloudera.org:8080/#/c/18718/5/be/src/exprs/expr-test.cc@6967
PS5, Line 6967: TestIsNull("minute('838:59:59')", TYPE_INT);
Should it be "hour('838:59:59')"?
MySQL's hour function seems to accept string where the hour part is more than
23.
https://dev.mysql.com/doc/refman/8.0/en/date-and-time-functions.html#function_hour
Not sure if its standard though.
--
To view, visit http://gerrit.cloudera.org:8080/18718
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6cc1c851ee71ab4fcc58105c7e9931155a483679
Gerrit-Change-Number: 18718
Gerrit-PatchSet: 5
Gerrit-Owner: Csaba Ringhofer
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Riza Suminto
Gerrit-Comment-Date: Tue, 12 Jul 2022 18:50:44 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18718 ) Change subject: IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond .. Patch Set 5: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/10957/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/18718 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6cc1c851ee71ab4fcc58105c7e9931155a483679 Gerrit-Change-Number: 18718 Gerrit-PatchSet: 5 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Tue, 12 Jul 2022 18:40:01 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18718 ) Change subject: IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond .. Patch Set 4: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/10956/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/18718 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6cc1c851ee71ab4fcc58105c7e9931155a483679 Gerrit-Change-Number: 18718 Gerrit-PatchSet: 4 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Tue, 12 Jul 2022 18:39:29 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18718 ) Change subject: IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/10955/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/18718 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6cc1c851ee71ab4fcc58105c7e9931155a483679 Gerrit-Change-Number: 18718 Gerrit-PatchSet: 3 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Tue, 12 Jul 2022 18:37:28 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond
Hello Riza Suminto, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18718
to look at the new patch set (#5).
Change subject: IMPALA-11355: Add STRING overloads for
hour/minute/second/millisecond
..
IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond
IMPALA-9531 dropped support for "dateless timestamps",
e.g. cast("12:05:05" as timestamp) now returns NULL.
This led to breaking functions like minute("12:05:05"), as minute()
expects a timestamp, and Impala adds an implicit cast, so what actually
happens is minute(cast("12:05:05" as timestamp)), which returns NULL.
This change adds overloads for similar functions that take STRING
instead of TIMESTAMP parameter. The same functions already take a
STRING parameter in Hive and mySQL.
The changes in the parser mainly restore code removed in IMPALA-9531.
Note that these functions could be potentially optimized by returning
parts of the parse result without converting them to boost time first,
but this is not done here to make the change minimal.
Testing:
- restored related tests in expr-test and added some new ones for
malformed time-of-day strings
- added benchmarks for the new overloads and fixed the ones for the
old functions (they tested NULL)
Change-Id: I6cc1c851ee71ab4fcc58105c7e9931155a483679
---
M be/src/benchmarks/expr-benchmark.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions-ir.cc
M be/src/exprs/timestamp-functions.h
M be/src/runtime/date-parse-util.cc
M be/src/runtime/datetime-simple-date-format-parser.cc
M be/src/runtime/datetime-simple-date-format-parser.h
M be/src/runtime/timestamp-parse-util.cc
M be/src/runtime/timestamp-parse-util.h
M common/function-registry/impala_functions.py
10 files changed, 204 insertions(+), 80 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/18718/5
--
To view, visit http://gerrit.cloudera.org:8080/18718
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6cc1c851ee71ab4fcc58105c7e9931155a483679
Gerrit-Change-Number: 18718
Gerrit-PatchSet: 5
Gerrit-Owner: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Riza Suminto
[Impala-ASF-CR] IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond
Hello Riza Suminto, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18718
to look at the new patch set (#4).
Change subject: IMPALA-11355: Add STRING overloads for
hour/minute/second/millisecond
..
IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond
IMPALA-9531 dropped support for "dateless timestamps",
e.g. cast("12:05:05" as timestamp) now returns NULL.
This led to breaking functions like minute("12:05:05"), as minute()
expects a timestamp, and Impala adds an implicit cast, so what actually
happens is minute(cast("12:05:05" as timestamp)), which returns NULL.
This change adds overloads for similar functions that take STRING
instead of TIMESTAMP parameter. The same functions already take a
STRING parameter in Hive and mySQL.
The changes in the parser mainly restore code removed in IMPALA-9531.
Note that these functions could be potentially optimized by returning
parts of the parse result without converting them to boost time first,
but this is not done here to make the change minimal.
Testing:
- restored related tests in expr-test and added some new ones for
malformed time-of-day strings
- added benchmarks for the new overloads and fixed the ones for the
old functions (they tested NULL)
Change-Id: I6cc1c851ee71ab4fcc58105c7e9931155a483679
---
M be/src/benchmarks/expr-benchmark.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions-ir.cc
M be/src/exprs/timestamp-functions.h
M be/src/runtime/date-parse-util.cc
M be/src/runtime/datetime-simple-date-format-parser.cc
M be/src/runtime/datetime-simple-date-format-parser.h
M be/src/runtime/timestamp-parse-util.cc
M be/src/runtime/timestamp-parse-util.h
M common/function-registry/impala_functions.py
10 files changed, 203 insertions(+), 80 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/18718/4
--
To view, visit http://gerrit.cloudera.org:8080/18718
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6cc1c851ee71ab4fcc58105c7e9931155a483679
Gerrit-Change-Number: 18718
Gerrit-PatchSet: 4
Gerrit-Owner: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Riza Suminto
[Impala-ASF-CR] IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond
Csaba Ringhofer has posted comments on this change. (
http://gerrit.cloudera.org:8080/18718 )
Change subject: IMPALA-11355: Add STRING overloads for
hour/minute/second/millisecond
..
Patch Set 5:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/18718/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18718/2//COMMIT_MSG@24
PS2, Line 24: not
> ni: not
Done
http://gerrit.cloudera.org:8080/#/c/18718/2//COMMIT_MSG@26
PS2, Line 26: Testing:
> Do you mind adding this new case in expr-benchmark.cc as well? In Benchmark
added tests and fixed the old ones - note that my machine seems much slower
than the one used for the samples in the files, but the ratios seem ok
http://gerrit.cloudera.org:8080/#/c/18718/2/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
http://gerrit.cloudera.org:8080/#/c/18718/2/be/src/exprs/expr-test.cc@6957
PS2, Line 6957: // These expressions directly extract
hour/minute/second/millis from STRING type
: // to support these functions for timestamp strings without a
date part (IMPALA-11355).
> What is the expected result of "SELECT HOUR("838:59:59");"?
Done
http://gerrit.cloudera.org:8080/#/c/18718/4/be/src/exprs/timestamp-functions-ir.cc
File be/src/exprs/timestamp-functions-ir.cc:
http://gerrit.cloudera.org:8080/#/c/18718/4/be/src/exprs/timestamp-functions-ir.cc@311
PS4, Line 311: IntVal TimestampFunctions::Millisecond(
> line too long (92 > 90)
Done
--
To view, visit http://gerrit.cloudera.org:8080/18718
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6cc1c851ee71ab4fcc58105c7e9931155a483679
Gerrit-Change-Number: 18718
Gerrit-PatchSet: 5
Gerrit-Owner: Csaba Ringhofer
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Riza Suminto
Gerrit-Comment-Date: Tue, 12 Jul 2022 18:23:18 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond
Hello Riza Suminto, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18718
to look at the new patch set (#3).
Change subject: IMPALA-11355: Add STRING overloads for
hour/minute/second/millisecond
..
IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond
IMPALA-9531 dropped support for "dateless timestamps",
e.g. cast("12:05:05" as timestamp) now returns NULL.
This led to breaking functions like minute("12:05:05"), as minute()
expects a timestamp, and Impala adds an implicit cast, so what actually
happens is minute(cast("12:05:05" as timestamp)), which returns NULL.
This change adds overloads for similar functions that take STRING
instead of TIMESTAMP parameter. The same functions already take a
STRING parameter in Hive and mySQL.
The changes in the parser mainly restore code removed in IMPALA-9531.
Note that these functions could be potentially optimized by returning
parts of the parse result without converting them to boost time first,
but this is not done here to make the change minimal.
Testing:
- restored related tests in expr-test and added some new ones for
malformed time-of-day strings
- added benchmarks for the new overloads and fixed the ones for the
old functions (they tested NULL)
Change-Id: I6cc1c851ee71ab4fcc58105c7e9931155a483679
---
M be/src/benchmarks/expr-benchmark.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions-ir.cc
M be/src/exprs/timestamp-functions.h
M be/src/runtime/date-parse-util.cc
M be/src/runtime/datetime-simple-date-format-parser.cc
M be/src/runtime/datetime-simple-date-format-parser.h
M be/src/runtime/timestamp-parse-util.cc
M be/src/runtime/timestamp-parse-util.h
M common/function-registry/impala_functions.py
10 files changed, 202 insertions(+), 80 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/18718/3
--
To view, visit http://gerrit.cloudera.org:8080/18718
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6cc1c851ee71ab4fcc58105c7e9931155a483679
Gerrit-Change-Number: 18718
Gerrit-PatchSet: 3
Gerrit-Owner: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Riza Suminto
[Impala-ASF-CR] IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond
Impala Public Jenkins has posted comments on this change. (
http://gerrit.cloudera.org:8080/18718 )
Change subject: IMPALA-11355: Add STRING overloads for
hour/minute/second/millisecond
..
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18718/4/be/src/exprs/timestamp-functions-ir.cc
File be/src/exprs/timestamp-functions-ir.cc:
http://gerrit.cloudera.org:8080/#/c/18718/4/be/src/exprs/timestamp-functions-ir.cc@311
PS4, Line 311: IntVal TimestampFunctions::Millisecond(FunctionContext* context,
const StringVal& str_val) {
line too long (92 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/18718
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6cc1c851ee71ab4fcc58105c7e9931155a483679
Gerrit-Change-Number: 18718
Gerrit-PatchSet: 4
Gerrit-Owner: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Riza Suminto
Gerrit-Comment-Date: Tue, 12 Jul 2022 18:20:31 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond
Impala Public Jenkins has posted comments on this change. (
http://gerrit.cloudera.org:8080/18718 )
Change subject: IMPALA-11355: Add STRING overloads for
hour/minute/second/millisecond
..
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18718/3/be/src/exprs/timestamp-functions-ir.cc
File be/src/exprs/timestamp-functions-ir.cc:
http://gerrit.cloudera.org:8080/#/c/18718/3/be/src/exprs/timestamp-functions-ir.cc@311
PS3, Line 311: IntVal TimestampFunctions::Millisecond(FunctionContext* context,
const StringVal& str_val) {
line too long (92 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/18718
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6cc1c851ee71ab4fcc58105c7e9931155a483679
Gerrit-Change-Number: 18718
Gerrit-PatchSet: 3
Gerrit-Owner: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Riza Suminto
Gerrit-Comment-Date: Tue, 12 Jul 2022 18:18:40 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond
Riza Suminto has posted comments on this change. (
http://gerrit.cloudera.org:8080/18718 )
Change subject: IMPALA-11355: Add STRING overloads for
hour/minute/second/millisecond
..
Patch Set 2:
(3 comments)
Thanks Csaba for working on this. I have few comments.
http://gerrit.cloudera.org:8080/#/c/18718/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18718/2//COMMIT_MSG@24
PS2, Line 24: note
ni: not
http://gerrit.cloudera.org:8080/#/c/18718/2//COMMIT_MSG@26
PS2, Line 26: Testing:
Do you mind adding this new case in expr-benchmark.cc as well? In
BenchmarkTimestampFunctions.
Maybe also update the benchmark result table as well just for
BenchmarkTimestampFunctions.
http://gerrit.cloudera.org:8080/#/c/18718/2/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
http://gerrit.cloudera.org:8080/#/c/18718/2/be/src/exprs/expr-test.cc@6957
PS2, Line 6957: // These expressions directly extract
hour/minute/second/millis from STRING type
: // to support these functions for timestamp strings without a
date part (IMPALA-11355).
What is the expected result of "SELECT HOUR("838:59:59");"?
Can we also have invalid case where:
- Minutes and seconds is more than 59
- Has "am" or "pm" behind time.
--
To view, visit http://gerrit.cloudera.org:8080/18718
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6cc1c851ee71ab4fcc58105c7e9931155a483679
Gerrit-Change-Number: 18718
Gerrit-PatchSet: 2
Gerrit-Owner: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Riza Suminto
Gerrit-Comment-Date: Mon, 11 Jul 2022 17:40:47 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18718 ) Change subject: IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/10946/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/18718 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6cc1c851ee71ab4fcc58105c7e9931155a483679 Gerrit-Change-Number: 18718 Gerrit-PatchSet: 2 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Mon, 11 Jul 2022 17:40:52 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond
Impala Public Jenkins has posted comments on this change. (
http://gerrit.cloudera.org:8080/18718 )
Change subject: IMPALA-11355: Add STRING overloads for
hour/minute/second/millisecond
..
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18718/2/be/src/exprs/timestamp-functions-ir.cc
File be/src/exprs/timestamp-functions-ir.cc:
http://gerrit.cloudera.org:8080/#/c/18718/2/be/src/exprs/timestamp-functions-ir.cc@311
PS2, Line 311: IntVal TimestampFunctions::Millisecond(FunctionContext* context,
const StringVal& str_val) {
line too long (92 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/18718
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6cc1c851ee71ab4fcc58105c7e9931155a483679
Gerrit-Change-Number: 18718
Gerrit-PatchSet: 2
Gerrit-Owner: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Mon, 11 Jul 2022 17:22:09 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond
Csaba Ringhofer has uploaded a new patch set (#2). (
http://gerrit.cloudera.org:8080/18718 )
Change subject: IMPALA-11355: Add STRING overloads for
hour/minute/second/millisecond
..
IMPALA-11355: Add STRING overloads for hour/minute/second/millisecond
IMPALA-9531 dropped support for "dateless timestamps",
e.g. cast("12:05:05" as timestamp) now returns NULL.
This led to breaking functions like minute("12:05:05"), as minute()
expects a timestamp, and Impala adds an implicit cast, so what actually
happens is minute(cast("12:05:05" as timestamp)), which returns NULL.
This change adds overloads for similar functions that take STRING
instead of TIMESTAMP parameter. The same functions already take a
STRING parameter in Hive and mySQL.
The changes in the parser mainly restore code removed in IMPALA-9531.
Note that these functions could be potentially optimized by returning
parts of the parse result without converting them to boost time first,
but this is note done here to make the change minimal.
Testing:
- restored related tests in expr-test and added some new ones for
malformed time-of-day strings
Change-Id: I6cc1c851ee71ab4fcc58105c7e9931155a483679
---
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions-ir.cc
M be/src/exprs/timestamp-functions.h
M be/src/runtime/date-parse-util.cc
M be/src/runtime/datetime-simple-date-format-parser.cc
M be/src/runtime/datetime-simple-date-format-parser.h
M be/src/runtime/timestamp-parse-util.cc
M be/src/runtime/timestamp-parse-util.h
M common/function-registry/impala_functions.py
9 files changed, 110 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/18718/2
--
To view, visit http://gerrit.cloudera.org:8080/18718
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6cc1c851ee71ab4fcc58105c7e9931155a483679
Gerrit-Change-Number: 18718
Gerrit-PatchSet: 2
Gerrit-Owner: Csaba Ringhofer
