[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
[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
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 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
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
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
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 (#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
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
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
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 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
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
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
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
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
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
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(&BenchmarkLiterals); 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
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
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(&BenchmarkLiterals); > 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
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
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
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
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 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: 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
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