[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14714 ) Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 .. IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 This patch adds additional datetime format tokens on top of Milestone 1 (IMPALA-8703) and Milestone 2 (IMPALA-8704). The tokens introduced: - Full month name (MONTH, Month, month): In a string to datetime conversion this token can parse textual month name into a datetime type. In a datetime to string conversion this token gives the textual representation of a month. - Short month name (MON, Mon, mon): Similar to the full month name token but this works for 3-character month names like 'JAN'. - Full day name (DAY, Day, day): In a datetime to string conversion this token gives the textual representation of a day like 'Tuesday.' Not suppported in a string to datetime conversion. - Short day name (DY, Dy, dy): Similar to full day name token but this works for 3-character day names like 'TUE'. Not suppported in a string to datetime conversion. - Day of week (D): In a datetime to string conversion this gives a number in [1-7] where 1 represents Sunday. Not supported in a string to datetime conversion. - Quarter of year (Q): In a datetime to string conversion this gives a number in [1-4] representing a quarter of the year. Not supported in a string to datetime conversion. - Week of year (WW): In a datetime to string conversion this gives a number in [1-53] to represent the week of year where the first week starts from 1st of January. Not supported in a string to datetime conversion. - Week of month (W): In a datetime to string conversion this gives a number in [1-5] to represent the week of month where the first week starts from the first day of the month. Not supported in a string to datetime conversion. Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f Reviewed-on: http://gerrit.cloudera.org:8080/14714 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M be/src/benchmarks/convert-timestamp-benchmark.cc M be/src/benchmarks/parse-timestamp-benchmark.cc M be/src/common/init.cc M be/src/exprs/date-functions-ir.cc M be/src/exprs/timestamp-functions-ir.cc M be/src/exprs/timestamp-functions.cc M be/src/exprs/timestamp-functions.h M be/src/runtime/date-parse-util.cc M be/src/runtime/date-parse-util.h M be/src/runtime/datetime-iso-sql-format-parser.cc M be/src/runtime/datetime-iso-sql-format-parser.h M be/src/runtime/datetime-iso-sql-format-tokenizer.cc M be/src/runtime/datetime-parser-common.cc M be/src/runtime/datetime-parser-common.h 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 tests/query_test/test_cast_with_format.py 18 files changed, 1,060 insertions(+), 111 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/14714 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f Gerrit-Change-Number: 14714 Gerrit-PatchSet: 12 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14714 ) Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 .. Patch Set 11: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/14714 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f Gerrit-Change-Number: 14714 Gerrit-PatchSet: 11 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 05 Dec 2019 14:19:40 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14714 ) Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 .. Patch Set 11: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/14714 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f Gerrit-Change-Number: 14714 Gerrit-PatchSet: 11 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 05 Dec 2019 09:55:33 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14714 ) Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 .. Patch Set 11: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5313/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/14714 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f Gerrit-Change-Number: 14714 Gerrit-PatchSet: 11 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 05 Dec 2019 09:55:34 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Attila Jeges has posted comments on this change. ( http://gerrit.cloudera.org:8080/14714 ) Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 .. Patch Set 10: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/14714 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f Gerrit-Change-Number: 14714 Gerrit-PatchSet: 10 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 05 Dec 2019 08:20:35 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14714 ) Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 .. Patch Set 10: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/5196/ : 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/14714 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f Gerrit-Change-Number: 14714 Gerrit-PatchSet: 10 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 04 Dec 2019 17:03:54 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Gabor Kaszab has posted comments on this change. (
http://gerrit.cloudera.org:8080/14714 )
Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
..
Patch Set 9:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/14714/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14714/7//COMMIT_MSG@22
PS7, Line 22: - Short day name (DY, Dy, dy): Similar to full day name token but
: this works for 3-character day names like 'TUE'.
> Add
Done
http://gerrit.cloudera.org:8080/#/c/14714/7/be/src/runtime/datetime-iso-sql-format-parser.h
File be/src/runtime/datetime-iso-sql-format-parser.h:
http://gerrit.cloudera.org:8080/#/c/14714/7/be/src/runtime/datetime-iso-sql-format-parser.h@45
PS7, Line 45: functions
> function
Done
http://gerrit.cloudera.org:8080/#/c/14714/7/be/src/runtime/datetime-iso-sql-format-parser.h@47
PS7, Line 47: Return
> nit: Returns
Done
http://gerrit.cloudera.org:8080/#/c/14714/7/be/src/runtime/datetime-iso-sql-format-tokenizer.cc
File be/src/runtime/datetime-iso-sql-format-tokenizer.cc:
http://gerrit.cloudera.org:8080/#/c/14714/7/be/src/runtime/datetime-iso-sql-format-tokenizer.cc@207
PS7, Line 207: IsUsedToken("MM") || IsUsedToken("MONTH") ||
:IsUsedToken("MON")
> provided_month_tokens == 1
Done
http://gerrit.cloudera.org:8080/#/c/14714/7/be/src/runtime/datetime-parser-common.h
File be/src/runtime/datetime-parser-common.h:
http://gerrit.cloudera.org:8080/#/c/14714/7/be/src/runtime/datetime-parser-common.h@279
PS7, Line 279: number
> nit: ordinal number
Done
http://gerrit.cloudera.org:8080/#/c/14714/7/be/src/runtime/datetime-parser-common.cc
File be/src/runtime/datetime-parser-common.cc:
http://gerrit.cloudera.org:8080/#/c/14714/7/be/src/runtime/datetime-parser-common.cc@81
PS7, Line 81: token
> nit: tokens
Done
--
To view, visit http://gerrit.cloudera.org:8080/14714
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f
Gerrit-Change-Number: 14714
Gerrit-PatchSet: 9
Gerrit-Owner: Gabor Kaszab
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Gabor Kaszab
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Wed, 04 Dec 2019 16:34:44 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Hello Attila Jeges, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14714 to look at the new patch set (#10). Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 .. IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 This patch adds additional datetime format tokens on top of Milestone 1 (IMPALA-8703) and Milestone 2 (IMPALA-8704). The tokens introduced: - Full month name (MONTH, Month, month): In a string to datetime conversion this token can parse textual month name into a datetime type. In a datetime to string conversion this token gives the textual representation of a month. - Short month name (MON, Mon, mon): Similar to the full month name token but this works for 3-character month names like 'JAN'. - Full day name (DAY, Day, day): In a datetime to string conversion this token gives the textual representation of a day like 'Tuesday.' Not suppported in a string to datetime conversion. - Short day name (DY, Dy, dy): Similar to full day name token but this works for 3-character day names like 'TUE'. Not suppported in a string to datetime conversion. - Day of week (D): In a datetime to string conversion this gives a number in [1-7] where 1 represents Sunday. Not supported in a string to datetime conversion. - Quarter of year (Q): In a datetime to string conversion this gives a number in [1-4] representing a quarter of the year. Not supported in a string to datetime conversion. - Week of year (WW): In a datetime to string conversion this gives a number in [1-53] to represent the week of year where the first week starts from 1st of January. Not supported in a string to datetime conversion. - Week of month (W): In a datetime to string conversion this gives a number in [1-5] to represent the week of month where the first week starts from the first day of the month. Not supported in a string to datetime conversion. Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f --- M be/src/benchmarks/convert-timestamp-benchmark.cc M be/src/benchmarks/parse-timestamp-benchmark.cc M be/src/common/init.cc M be/src/exprs/date-functions-ir.cc M be/src/exprs/timestamp-functions-ir.cc M be/src/exprs/timestamp-functions.cc M be/src/exprs/timestamp-functions.h M be/src/runtime/date-parse-util.cc M be/src/runtime/date-parse-util.h M be/src/runtime/datetime-iso-sql-format-parser.cc M be/src/runtime/datetime-iso-sql-format-parser.h M be/src/runtime/datetime-iso-sql-format-tokenizer.cc M be/src/runtime/datetime-parser-common.cc M be/src/runtime/datetime-parser-common.h 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 tests/query_test/test_cast_with_format.py 18 files changed, 1,060 insertions(+), 111 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/14714/10 -- To view, visit http://gerrit.cloudera.org:8080/14714 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f Gerrit-Change-Number: 14714 Gerrit-PatchSet: 10 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Attila Jeges has posted comments on this change. (
http://gerrit.cloudera.org:8080/14714 )
Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
..
Patch Set 9:
(6 comments)
Some nit comments:
http://gerrit.cloudera.org:8080/#/c/14714/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14714/7//COMMIT_MSG@22
PS7, Line 22: - Short day name (DY, Dy, dy): Similar to full day name token but
: this works for 3-character day names like 'TUE'.
Add
"Not suppported in a string to datetime conversion."
http://gerrit.cloudera.org:8080/#/c/14714/7/be/src/runtime/datetime-iso-sql-format-parser.h
File be/src/runtime/datetime-iso-sql-format-parser.h:
http://gerrit.cloudera.org:8080/#/c/14714/7/be/src/runtime/datetime-iso-sql-format-parser.h@45
PS7, Line 45: functions
function
http://gerrit.cloudera.org:8080/#/c/14714/7/be/src/runtime/datetime-iso-sql-format-parser.h@47
PS7, Line 47: Return
nit: Returns
http://gerrit.cloudera.org:8080/#/c/14714/7/be/src/runtime/datetime-iso-sql-format-tokenizer.cc
File be/src/runtime/datetime-iso-sql-format-tokenizer.cc:
http://gerrit.cloudera.org:8080/#/c/14714/7/be/src/runtime/datetime-iso-sql-format-tokenizer.cc@207
PS7, Line 207: IsUsedToken("MM") || IsUsedToken("MONTH") ||
:IsUsedToken("MON")
provided_month_tokens == 1
http://gerrit.cloudera.org:8080/#/c/14714/7/be/src/runtime/datetime-parser-common.h
File be/src/runtime/datetime-parser-common.h:
http://gerrit.cloudera.org:8080/#/c/14714/7/be/src/runtime/datetime-parser-common.h@279
PS7, Line 279: number
nit: ordinal number
http://gerrit.cloudera.org:8080/#/c/14714/7/be/src/runtime/datetime-parser-common.cc
File be/src/runtime/datetime-parser-common.cc:
http://gerrit.cloudera.org:8080/#/c/14714/7/be/src/runtime/datetime-parser-common.cc@81
PS7, Line 81: token
nit: tokens
--
To view, visit http://gerrit.cloudera.org:8080/14714
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f
Gerrit-Change-Number: 14714
Gerrit-PatchSet: 9
Gerrit-Owner: Gabor Kaszab
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Gabor Kaszab
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Wed, 04 Dec 2019 14:26:48 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14714 ) Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 .. Patch Set 9: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/5195/ : 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/14714 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f Gerrit-Change-Number: 14714 Gerrit-PatchSet: 9 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 04 Dec 2019 11:33:58 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14714 ) Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 .. Patch Set 8: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/5194/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/14714 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f Gerrit-Change-Number: 14714 Gerrit-PatchSet: 8 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 04 Dec 2019 11:30:38 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Hello Attila Jeges, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14714 to look at the new patch set (#9). Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 .. IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 This patch adds additional datetime format tokens on top of Milestone 1 (IMPALA-8703) and Milestone 2 (IMPALA-8704). The tokens introduced: - Full month name (MONTH, Month, month): In a string to datetime conversion this token can parse textual month name into a datetime type. In a datetime to string conversion this token gives the textual representation of a month. - Short month name (MON, Mon, mon): Similar to the full month name token but this works for 3-character month names like 'JAN'. - Full day name (DAY, Day, day): In a datetime to string conversion this token gives the textual representation of a day like 'Tuesday.' Not suppported in a string to datetime conversion. - Short day name (DY, Dy, dy): Similar to full day name token but this works for 3-character day names like 'TUE'. - Day of week (D): In a datetime to string conversion this gives a number in [1-7] where 1 represents Sunday. Not supported in a string to datetime conversion. - Quarter of year (Q): In a datetime to string conversion this gives a number in [1-4] representing a quarter of the year. Not supported in a string to datetime conversion. - Week of year (WW): In a datetime to string conversion this gives a number in [1-53] to represent the week of year where the first week starts from 1st of January. Not supported in a string to datetime conversion. - Week of month (W): In a datetime to string conversion this gives a number in [1-5] to represent the week of month where the first week starts from the first day of the month. Not supported in a string to datetime conversion. Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f --- M be/src/benchmarks/convert-timestamp-benchmark.cc M be/src/benchmarks/parse-timestamp-benchmark.cc M be/src/common/init.cc M be/src/exprs/date-functions-ir.cc M be/src/exprs/timestamp-functions-ir.cc M be/src/exprs/timestamp-functions.cc M be/src/exprs/timestamp-functions.h M be/src/runtime/date-parse-util.cc M be/src/runtime/date-parse-util.h M be/src/runtime/datetime-iso-sql-format-parser.cc M be/src/runtime/datetime-iso-sql-format-parser.h M be/src/runtime/datetime-iso-sql-format-tokenizer.cc M be/src/runtime/datetime-parser-common.cc M be/src/runtime/datetime-parser-common.h 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 tests/query_test/test_cast_with_format.py 18 files changed, 1,055 insertions(+), 105 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/14714/9 -- To view, visit http://gerrit.cloudera.org:8080/14714 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f Gerrit-Change-Number: 14714 Gerrit-PatchSet: 9 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/14714 ) Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 .. Patch Set 8: PS8 is a rebase with master -- To view, visit http://gerrit.cloudera.org:8080/14714 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f Gerrit-Change-Number: 14714 Gerrit-PatchSet: 8 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 04 Dec 2019 11:01:41 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14714 ) Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 .. Patch Set 8: (6 comments) http://gerrit.cloudera.org:8080/#/c/14714/8/tests/query_test/test_cast_with_format.py File tests/query_test/test_cast_with_format.py: http://gerrit.cloudera.org:8080/#/c/14714/8/tests/query_test/test_cast_with_format.py@1565 PS8, Line 1565: >>> IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 flake8: E305 expected 2 blank lines after class or function definition, found 0 http://gerrit.cloudera.org:8080/#/c/14714/8/tests/query_test/test_cast_with_format.py@1565 PS8, Line 1565: > flake8: E999 SyntaxError: invalid syntax http://gerrit.cloudera.org:8080/#/c/14714/8/tests/query_test/test_cast_with_format.py@1565 PS8, Line 1565: > flake8: E225 missing whitespace around operator http://gerrit.cloudera.org:8080/#/c/14714/8/tests/query_test/test_cast_with_format.py@1565 PS8, Line 1565: - flake8: E226 missing whitespace around arithmetic operator http://gerrit.cloudera.org:8080/#/c/14714/8/tests/query_test/test_cast_with_format.py@1565 PS8, Line 1565: : flake8: E231 missing whitespace after ':' http://gerrit.cloudera.org:8080/#/c/14714/8/tests/query_test/test_cast_with_format.py@1565 PS8, Line 1565: : flake8: E231 missing whitespace after ':' -- To view, visit http://gerrit.cloudera.org:8080/14714 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f Gerrit-Change-Number: 14714 Gerrit-PatchSet: 8 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 04 Dec 2019 11:01:48 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14714 ) Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 .. Patch Set 7: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/14714 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f Gerrit-Change-Number: 14714 Gerrit-PatchSet: 7 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 03 Dec 2019 13:57:14 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14714 ) Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 .. Patch Set 7: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5308/ DRY_RUN=true -- To view, visit http://gerrit.cloudera.org:8080/14714 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f Gerrit-Change-Number: 14714 Gerrit-PatchSet: 7 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 03 Dec 2019 09:44:23 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14714 ) Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 .. Patch Set 7: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/5171/ : 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/14714 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f Gerrit-Change-Number: 14714 Gerrit-PatchSet: 7 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 28 Nov 2019 15:31:24 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Gabor Kaszab has posted comments on this change. (
http://gerrit.cloudera.org:8080/14714 )
Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
..
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h
File be/src/runtime/datetime-parser-common.h:
http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h@70
PS5, Line 70: {"sep", {"tember", 9}},
: {"oct", {"ober", 10}},
: {"nov", {"ember",
> What I meant was that it is a bit confusing that the comment says "Mapping
I see. I re-wrote the comment for this map.
--
To view, visit http://gerrit.cloudera.org:8080/14714
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f
Gerrit-Change-Number: 14714
Gerrit-PatchSet: 7
Gerrit-Owner: Gabor Kaszab
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Gabor Kaszab
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Thu, 28 Nov 2019 14:48:17 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Hello Attila Jeges, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14714 to look at the new patch set (#7). Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 .. IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 This patch adds additional datetime format tokens on top of Milestone 1 (IMPALA-8703) and Milestone 2 (IMPALA-8704). The tokens introduced: - Full month name (MONTH, Month, month): In a string to datetime conversion this token can parse textual month name into a datetime type. In a datetime to string conversion this token gives the textual representation of a month. - Short month name (MON, Mon, mon): Similar to the full month name token but this works for 3-character month names like 'JAN'. - Full day name (DAY, Day, day): In a datetime to string conversion this token gives the textual representation of a day like 'Tuesday.' Not suppported in a string to datetime conversion. - Short day name (DY, Dy, dy): Similar to full day name token but this works for 3-character day names like 'TUE'. - Day of week (D): In a datetime to string conversion this gives a number in [1-7] where 1 represents Sunday. Not supported in a string to datetime conversion. - Quarter of year (Q): In a datetime to string conversion this gives a number in [1-4] representing a quarter of the year. Not supported in a string to datetime conversion. - Week of year (WW): In a datetime to string conversion this gives a number in [1-53] to represent the week of year where the first week starts from 1st of January. Not supported in a string to datetime conversion. - Week of month (W): In a datetime to string conversion this gives a number in [1-5] to represent the week of month where the first week starts from the first day of the month. Not supported in a string to datetime conversion. Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f --- M be/src/benchmarks/convert-timestamp-benchmark.cc M be/src/benchmarks/parse-timestamp-benchmark.cc M be/src/common/init.cc M be/src/exprs/date-functions-ir.cc M be/src/exprs/timestamp-functions-ir.cc M be/src/exprs/timestamp-functions.cc M be/src/exprs/timestamp-functions.h M be/src/runtime/date-parse-util.cc M be/src/runtime/date-parse-util.h M be/src/runtime/datetime-iso-sql-format-parser.cc M be/src/runtime/datetime-iso-sql-format-parser.h M be/src/runtime/datetime-iso-sql-format-tokenizer.cc M be/src/runtime/datetime-parser-common.cc M be/src/runtime/datetime-parser-common.h 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 tests/query_test/test_cast_with_format.py 18 files changed, 1,055 insertions(+), 105 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/14714/7 -- To view, visit http://gerrit.cloudera.org:8080/14714 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f Gerrit-Change-Number: 14714 Gerrit-PatchSet: 7 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Attila Jeges has posted comments on this change. (
http://gerrit.cloudera.org:8080/14714 )
Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
..
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-iso-sql-format-parser.cc
File be/src/runtime/datetime-iso-sql-format-parser.cc:
http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-iso-sql-format-parser.cc@297
PS5, Line 297: if (tok.type == MONTH_NAME && fx_provided && !tok.fm_modifier) {
: if (input_len < MAX_MONTH_NAME_LENGTH) return nullptr;
: return input_str + MAX_MONTH_NAME_LENGTH;
: }
> If there is no following separator then the month name token will have the
Thanks!
http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h
File be/src/runtime/datetime-parser-common.h:
http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h@70
PS5, Line 70: {"dec", {"ember", 12}}
: };
:
> Being a bit more specific here would help.
What I meant was that it is a bit confusing that the comment says "Mapping
between textual month name and the number of month" while the name of the map
is 'MONTH_PREFIX_TO_SUFFIX'.
--
To view, visit http://gerrit.cloudera.org:8080/14714
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f
Gerrit-Change-Number: 14714
Gerrit-PatchSet: 6
Gerrit-Owner: Gabor Kaszab
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Gabor Kaszab
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Thu, 28 Nov 2019 14:31:39 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14714 ) Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 .. Patch Set 6: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/5169/ : 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/14714 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f Gerrit-Change-Number: 14714 Gerrit-PatchSet: 6 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 28 Nov 2019 12:05:37 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Gabor Kaszab has posted comments on this change. (
http://gerrit.cloudera.org:8080/14714 )
Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
..
Patch Set 6:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-iso-sql-format-parser.cc
File be/src/runtime/datetime-iso-sql-format-parser.cc:
http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-iso-sql-format-parser.cc@297
PS5, Line 297: if (tok.type == MONTH_NAME && fx_provided && !tok.fm_modifier) {
: if (input_len < MAX_MONTH_NAME_LENGTH) return nullptr;
: return input_str + MAX_MONTH_NAME_LENGTH;
: }
> What happens if tok.type == MONTH_NAME && (!fx_provided || tok.fm_modifier)
If there is no following separator then the month name token will have the
length of MAX_MONTH_NAME_LENGTH and later on it will be adjusted in
ParseMonthNameToken(). See L233-236 in datetime-parser-common.cc
I added a comment to the header about this.
http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h
File be/src/runtime/datetime-parser-common.h:
http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h@43
PS5, Line 43: const int FRACTIONAL_SECOND_MAX_LENGTH = 9;
> Is this still used?
Removed
http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h@57
PS5, Line 57: ir nit: ordinal number
Done
http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h@63
PS5, Line 63:
> nit: ordinal number
Done
http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h@59
PS5, Line 59: {"jan", {"uary", 1}},
: {"feb", {"ruary", 2}},
: {"mar", {"ch", 3}},
: {"apr", {"il", 4}},
: {"may", {"", 5}},
: {"jun", {"e", 6}},
: {"jul", {"y", 7}},
: {"aug", {"ust", 8}},
: {"sep", {"tember", 9}},
: {"oct", {"ober
These are no longer needed since the introduction of MONTH_PREFIX_TO_SUFFIX.
Dropped
http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h@70
PS5, Line 70: {"dec", {"ember", 12}}
: };
:
> Fix comment.
Being a bit more specific here would help.
I don't think that this comment is broken. This is a mapping between textual
month name and the ordinal number of month. So this part is true. This is used
for string to datetime conversion so this part is true as well.
I could mention in the comment that the key part of the map is the 3-letter
prefix of the month names and this is mapped to the rest of the month name and
the number of month but anyone who takes a look at this part of the code would
see this immediately without reading the comment so I didn't see the point.
http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h@88
PS5, Line 88:
> closing ' is missing.
Done
http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h@295
PS5, Line 295:
: /// Gets a year and the number of days passed since 1st of
January t
> Please fix the comment.
Done
http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h@308
PS5, Line 308: casing of the
> MONTH_RANGES and LEAP_YEAR_MONTH_RANGES
Instead I removed the end of the comment.
http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h@328
PS5, Line 328: Wee
> nit: 1
Done
http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.cc
File be/src/runtime/datetime-parser-common.cc:
http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.cc@193
PS5, Line 193: int* month) {
> This parametr is not really needed. It is used only in a DCHECK.
Done
http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.cc@235
PS5, Line 235:
> Maybe this would be more straightforward:
Agree. Done
--
To view, visit http://gerrit.cloudera.org:8080/14714
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f
Gerrit-Change-Number: 14714
Gerrit-PatchSet: 6
Gerrit-Owner: Gabor Kaszab
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Gabor Kaszab
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Thu, 28 Nov 2019 11:21:44 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Hello Attila Jeges, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14714 to look at the new patch set (#6). Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 .. IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 This patch adds additional datetime format tokens on top of Milestone 1 (IMPALA-8703) and Milestone 2 (IMPALA-8704). The tokens introduced: - Full month name (MONTH, Month, month): In a string to datetime conversion this token can parse textual month name into a datetime type. In a datetime to string conversion this token gives the textual representation of a month. - Short month name (MON, Mon, mon): Similar to the full month name token but this works for 3-character month names like 'JAN'. - Full day name (DAY, Day, day): In a datetime to string conversion this token gives the textual representation of a day like 'Tuesday.' Not suppported in a string to datetime conversion. - Short day name (DY, Dy, dy): Similar to full day name token but this works for 3-character day names like 'TUE'. - Day of week (D): In a datetime to string conversion this gives a number in [1-7] where 1 represents Sunday. Not supported in a string to datetime conversion. - Quarter of year (Q): In a datetime to string conversion this gives a number in [1-4] representing a quarter of the year. Not supported in a string to datetime conversion. - Week of year (WW): In a datetime to string conversion this gives a number in [1-53] to represent the week of year where the first week starts from 1st of January. Not supported in a string to datetime conversion. - Week of month (W): In a datetime to string conversion this gives a number in [1-5] to represent the week of month where the first week starts from the first day of the month. Not supported in a string to datetime conversion. Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f --- M be/src/benchmarks/convert-timestamp-benchmark.cc M be/src/benchmarks/parse-timestamp-benchmark.cc M be/src/common/init.cc M be/src/exprs/date-functions-ir.cc M be/src/exprs/timestamp-functions-ir.cc M be/src/exprs/timestamp-functions.cc M be/src/exprs/timestamp-functions.h M be/src/runtime/date-parse-util.cc M be/src/runtime/date-parse-util.h M be/src/runtime/datetime-iso-sql-format-parser.cc M be/src/runtime/datetime-iso-sql-format-parser.h M be/src/runtime/datetime-iso-sql-format-tokenizer.cc M be/src/runtime/datetime-parser-common.cc M be/src/runtime/datetime-parser-common.h 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 tests/query_test/test_cast_with_format.py 18 files changed, 1,052 insertions(+), 105 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/14714/6 -- To view, visit http://gerrit.cloudera.org:8080/14714 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f Gerrit-Change-Number: 14714 Gerrit-PatchSet: 6 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Attila Jeges has posted comments on this change. ( http://gerrit.cloudera.org:8080/14714 ) Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.cc File be/src/runtime/datetime-parser-common.cc: http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.cc@235 PS5, Line 235: *token_end -= buff.length() - prefix.length() - expected_suffix.length(); Maybe this would be more straightforward: *token_end = token_start + prefix.length() + expected_suffix.length(); -- To view, visit http://gerrit.cloudera.org:8080/14714 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f Gerrit-Change-Number: 14714 Gerrit-PatchSet: 5 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 28 Nov 2019 07:09:03 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Attila Jeges has posted comments on this change. (
http://gerrit.cloudera.org:8080/14714 )
Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
..
Patch Set 5:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-iso-sql-format-parser.cc
File be/src/runtime/datetime-iso-sql-format-parser.cc:
http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-iso-sql-format-parser.cc@297
PS5, Line 297: if (tok.type == MONTH_NAME && fx_provided && !tok.fm_modifier) {
: if (input_len < MAX_MONTH_NAME_LENGTH) return nullptr;
: return input_str + MAX_MONTH_NAME_LENGTH;
: }
What happens if tok.type == MONTH_NAME && (!fx_provided || tok.fm_modifier)?
I think in that case, FindEndOfToken() doesn't return the proper end of the
token, since month names are not necessarily followed by a separator.
http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h
File be/src/runtime/datetime-parser-common.h:
http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h@43
PS5, Line 43: const int MONTH_LENGTHS[12] = {31, 28, 31, 30, 31, 30, 31, 31,
30, 31, 30, 31};
Is this still used?
http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h@57
PS5, Line 57: number
nit: ordinal number
http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h@63
PS5, Line 63: number
nit: ordinal number
http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h@70
PS5, Line 70: /// Mapping between textual month name and the number of month.
Used for string to
: /// datetime conversions.
: const std::unordered_maphttp://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h@88
PS5, Line 88: 'FEB
closing ' is missing.
http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h@295
PS5, Line 295: /// If the month part of the input is not followed by a
separator then the end of the
: /// month part is found by probing the input with different
lengths.
Please fix the comment.
http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h@308
PS5, Line 308: MONTH_LENGTHS
MONTH_RANGES and LEAP_YEAR_MONTH_RANGES
http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h@328
PS5, Line 328: one
nit: 1
http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.cc
File be/src/runtime/datetime-parser-common.cc:
http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.cc@193
PS5, Line 193: const char* input_end
This parametr is not really needed. It is used only in a DCHECK.
--
To view, visit http://gerrit.cloudera.org:8080/14714
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f
Gerrit-Change-Number: 14714
Gerrit-PatchSet: 5
Gerrit-Owner: Gabor Kaszab
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Gabor Kaszab
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Wed, 27 Nov 2019 16:11:48 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14714 ) Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 .. Patch Set 5: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/5158/ : 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/14714 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f Gerrit-Change-Number: 14714 Gerrit-PatchSet: 5 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 27 Nov 2019 12:45:03 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Gabor Kaszab has posted comments on this change. (
http://gerrit.cloudera.org:8080/14714 )
Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
..
Patch Set 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/14714/4/be/src/runtime/datetime-parser-common.cc
File be/src/runtime/datetime-parser-common.cc:
http://gerrit.cloudera.org:8080/#/c/14714/4/be/src/runtime/datetime-parser-common.cc@212
PS4, Line 212: if (fx_modifier && !tok.fm_modifier) {
: DCHECK(buff.length() == MAX_MONTH_NAME_LENGTH);
: trim_right_if(buff, is_any_of(" "));
: }
> This block is probably not necessary. The parsing below should succeed with
If I remove this block then L233-234 would fail for "padded" month names (like
"May ").
Additionaly L233-234 is needed not to accept "2010-JANUARY-10" with
"FX-MONTH-DD" where the month name is not padded to 9 length.
http://gerrit.cloudera.org:8080/#/c/14714/4/be/src/runtime/datetime-parser-common.cc@225
PS4, Line 225: const char* actual_suffix = buff.c_str() +
SHORT_MONTH_NAME_LENGTH;
: if (strncmp(actual_suffix, expected_suffix.c_str(),
expected_suffix.length()) != 0) {
: return false;
: }
> It would be faster to use a const char* pointer and strncmp() for comparis
Done
http://gerrit.cloudera.org:8080/#/c/14714/4/be/src/runtime/datetime-parser-common.cc@232
PS4, Line 232: // separator then the end of the month token has
> No need to create a new string instance. Only the length of 'month_name' is
Done
--
To view, visit http://gerrit.cloudera.org:8080/14714
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f
Gerrit-Change-Number: 14714
Gerrit-PatchSet: 5
Gerrit-Owner: Gabor Kaszab
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Gabor Kaszab
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Wed, 27 Nov 2019 12:02:31 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Hello Attila Jeges, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14714 to look at the new patch set (#5). Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 .. IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 This patch adds additional datetime format tokens on top of Milestone 1 (IMPALA-8703) and Milestone 2 (IMPALA-8704). The tokens introduced: - Full month name (MONTH, Month, month): In a string to datetime conversion this token can parse textual month name into a datetime type. In a datetime to string conversion this token gives the textual representation of a month. - Short month name (MON, Mon, mon): Similar to the full month name token but this works for 3-character month names like 'JAN'. - Full day name (DAY, Day, day): In a datetime to string conversion this token gives the textual representation of a day like 'Tuesday.' Not suppported in a string to datetime conversion. - Short day name (DY, Dy, dy): Similar to full day name token but this works for 3-character day names like 'TUE'. - Day of week (D): In a datetime to string conversion this gives a number in [1-7] where 1 represents Sunday. Not supported in a string to datetime conversion. - Quarter of year (Q): In a datetime to string conversion this gives a number in [1-4] representing a quarter of the year. Not supported in a string to datetime conversion. - Week of year (WW): In a datetime to string conversion this gives a number in [1-53] to represent the week of year where the first week starts from 1st of January. Not supported in a string to datetime conversion. - Week of month (W): In a datetime to string conversion this gives a number in [1-5] to represent the week of month where the first week starts from the first day of the month. Not supported in a string to datetime conversion. Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f --- M be/src/benchmarks/convert-timestamp-benchmark.cc M be/src/benchmarks/parse-timestamp-benchmark.cc M be/src/common/init.cc M be/src/exprs/date-functions-ir.cc M be/src/exprs/timestamp-functions-ir.cc M be/src/exprs/timestamp-functions.cc M be/src/exprs/timestamp-functions.h M be/src/runtime/date-parse-util.cc M be/src/runtime/date-parse-util.h M be/src/runtime/datetime-iso-sql-format-parser.cc M be/src/runtime/datetime-iso-sql-format-parser.h M be/src/runtime/datetime-iso-sql-format-tokenizer.cc M be/src/runtime/datetime-parser-common.cc M be/src/runtime/datetime-parser-common.h 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 tests/query_test/test_cast_with_format.py 18 files changed, 1,063 insertions(+), 101 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/14714/5 -- To view, visit http://gerrit.cloudera.org:8080/14714 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f Gerrit-Change-Number: 14714 Gerrit-PatchSet: 5 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Attila Jeges has posted comments on this change. (
http://gerrit.cloudera.org:8080/14714 )
Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
..
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/14714/4/be/src/runtime/datetime-parser-common.cc
File be/src/runtime/datetime-parser-common.cc:
http://gerrit.cloudera.org:8080/#/c/14714/4/be/src/runtime/datetime-parser-common.cc@212
PS4, Line 212: if (fx_modifier && !tok.fm_modifier) {
: DCHECK(buff.length() == MAX_MONTH_NAME_LENGTH);
: trim_right_if(buff, is_any_of(" "));
: }
This block is probably not necessary. The parsing below should succeed without
trimming 'buff'.
http://gerrit.cloudera.org:8080/#/c/14714/4/be/src/runtime/datetime-parser-common.cc@225
PS4, Line 225: const string& actual_suffix =
: buff.substr(SHORT_MONTH_NAME_LENGTH,
expected_suffix.length());
: if (actual_suffix != expected_suffix) return false;
: *month = it->second.second;
It would be faster to use a const char* pointer and strncmp() for comparison
instead of creating an 'actual_suffix' string instance.
http://gerrit.cloudera.org:8080/#/c/14714/4/be/src/runtime/datetime-parser-common.cc@232
PS4, Line 232: const string month_name = prefix + actual_suffix;
No need to create a new string instance. Only the length of 'month_name' is
used in L233-236.
--
To view, visit http://gerrit.cloudera.org:8080/14714
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f
Gerrit-Change-Number: 14714
Gerrit-PatchSet: 4
Gerrit-Owner: Gabor Kaszab
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Gabor Kaszab
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Wed, 27 Nov 2019 10:25:19 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14714 ) Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 .. Patch Set 4: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/5145/ : 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/14714 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f Gerrit-Change-Number: 14714 Gerrit-PatchSet: 4 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 26 Nov 2019 09:47:38 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Hello Attila Jeges, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14714 to look at the new patch set (#4). Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 .. IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 This patch adds additional datetime format tokens on top of Milestone 1 (IMPALA-8703) and Milestone 2 (IMPALA-8704). The tokens introduced: - Full month name (MONTH, Month, month): In a string to datetime conversion this token can parse textual month name into a datetime type. In a datetime to string conversion this token gives the textual representation of a month. - Short month name (MON, Mon, mon): Similar to the full month name token but this works for 3-character month names like 'JAN'. - Full day name (DAY, Day, day): In a datetime to string conversion this token gives the textual representation of a day like 'Tuesday.' Not suppported in a string to datetime conversion. - Short day name (DY, Dy, dy): Similar to full day name token but this works for 3-character day names like 'TUE'. - Day of week (D): In a datetime to string conversion this gives a number in [1-7] where 1 represents Sunday. Not supported in a string to datetime conversion. - Quarter of year (Q): In a datetime to string conversion this gives a number in [1-4] representing a quarter of the year. Not supported in a string to datetime conversion. - Week of year (WW): In a datetime to string conversion this gives a number in [1-53] to represent the week of year where the first week starts from 1st of January. Not supported in a string to datetime conversion. - Week of month (W): In a datetime to string conversion this gives a number in [1-5] to represent the week of month where the first week starts from the first day of the month. Not supported in a string to datetime conversion. Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f --- M be/src/benchmarks/convert-timestamp-benchmark.cc M be/src/benchmarks/parse-timestamp-benchmark.cc M be/src/common/init.cc M be/src/exprs/date-functions-ir.cc M be/src/exprs/timestamp-functions-ir.cc M be/src/exprs/timestamp-functions.cc M be/src/exprs/timestamp-functions.h M be/src/runtime/date-parse-util.cc M be/src/runtime/date-parse-util.h M be/src/runtime/datetime-iso-sql-format-parser.cc M be/src/runtime/datetime-iso-sql-format-parser.h M be/src/runtime/datetime-iso-sql-format-tokenizer.cc M be/src/runtime/datetime-parser-common.cc M be/src/runtime/datetime-parser-common.h 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 tests/query_test/test_cast_with_format.py 18 files changed, 1,063 insertions(+), 101 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/14714/4 -- To view, visit http://gerrit.cloudera.org:8080/14714 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f Gerrit-Change-Number: 14714 Gerrit-PatchSet: 4 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Gabor Kaszab has posted comments on this change. (
http://gerrit.cloudera.org:8080/14714 )
Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
..
Patch Set 4:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/date-parse-util.cc
File be/src/runtime/date-parse-util.cc:
http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/date-parse-util.cc@160
PS3, Line 160: case MONTH_NAME_SHORT: {
: result.append(FormatMonthName(month, tok));
: break;
: }
> Instead of "DDD" I meant "MMM" above of course :)
Nice catch. I haven't run a full test suite on the latest patch.
Done.
http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-iso-sql-format-tokenizer.cc
File be/src/runtime/datetime-iso-sql-format-tokenizer.cc:
http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-iso-sql-format-tokenizer.cc@200
PS3, Line 200: t provided_month_tokens = IsUsedToken("MM") +
IsUsedToken("MONTH") +
: IsUsedToken("MON");
> Probably it would be simpler to do this check with a counter, similarly to
Done
http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-iso-sql-format-tokenizer.cc@207
PS3, Line 207: sUsedToken("MM") || IsUsedToken("MONTH
> You probably have to add:
Done
http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-parser-common.h
File be/src/runtime/datetime-parser-common.h:
http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-parser-common.h@22
PS3, Line 22: #include
: #include
: #include
> nit: order includes alphabetically.
Done
http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-parser-common.h@70
PS3, Line 70: /// Mapping between textual month name and
> I think this constant and the others below should be int instead.
Done
http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-parser-common.h@75
PS3, Line 75:
> Closing apostrophe is missing.
Done
http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-parser-common.h@273
PS3, Line 273:
> MONTH_NAME_SHORT
Done
http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-parser-common.h@315
PS3, Line 315: r, int days_
> nit: month or day name
Done
http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-parser-common.cc
File be/src/runtime/datetime-parser-common.cc:
http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-parser-common.cc@199
PS3, Line 199: token_len = *
> (token_len < SHORT_MONTH_NAME_LENGTH)
Done
http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-parser-common.cc@223
PS3, Line 223: const string& expected_suffix = it->second.first;
: if (buff.length() - SHORT_MONTH_NAME_LENGTH <
expected_suffix.length()) return false;
: const string& actual_suffix =
: buff.substr(SHORT_MONTH_NAME_LENGTH,
expected_suffix.length());
: if (actual_suffix != expected_suffix) return false;
: *month = it->second.second;
:
: // If the end of the month token wasn't identified because it
wasn't followed by a
: // separator then the end of the month token has to be
adjusted.
: const string month_name = prefix + actual_suffix;
: if (month_name.length() < buff.length()) {
:
> I think, we could do this faster by taking advantage of the fact that check
Done
http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-parser-common.cc@329
PS3, Line 329: DCHECK(tok.type == DAY_NAME || tok.type == DAY_NAME_SHORT ||
tok.type == MONTH_NAME ||
> Add: && tok.len >= SHORT_MONTH_NAME_LENGTH condition.
Done
http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/timestamp-parse-util.cc
File be/src/runtime/timestamp-parse-util.cc:
http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/timestamp-parse-util.cc@255
PS3, Line 255: case DAY_OF_WEEK: {
> Please add a comment explaining that 1 means Sunday, 2 means Monday, and so
Done
--
To view, visit http://gerrit.cloudera.org:8080/14714
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f
Gerrit-Change-Number: 14714
Gerrit-PatchSet: 4
Gerrit-Owner: Gabor Kaszab
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Gabor Kaszab
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Tue, 26 Nov 2019 09:03:39 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Attila Jeges has posted comments on this change. (
http://gerrit.cloudera.org:8080/14714 )
Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
..
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/date-parse-util.cc
File be/src/runtime/date-parse-util.cc:
http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/date-parse-util.cc@160
PS3, Line 160: case MONTH_NAME_SHORT: {
: result.append(FormatMonthName(month, tok));
: break;
: }
> Using MONTH_NAME_SHORT both for "MMM" (simple format) and "MON" (ISO SQL fo
Instead of "DDD" I meant "MMM" above of course :)
--
To view, visit http://gerrit.cloudera.org:8080/14714
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f
Gerrit-Change-Number: 14714
Gerrit-PatchSet: 3
Gerrit-Owner: Gabor Kaszab
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Gabor Kaszab
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Fri, 22 Nov 2019 13:11:15 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Attila Jeges has posted comments on this change. (
http://gerrit.cloudera.org:8080/14714 )
Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
..
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/date-parse-util.cc
File be/src/runtime/date-parse-util.cc:
http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/date-parse-util.cc@160
PS3, Line 160: case MONTH_NAME_SHORT: {
: result.append(FormatMonthName(month, tok));
: break;
: }
Using MONTH_NAME_SHORT both for "MMM" (simple format) and "MON" (ISO SQL
format) tokens introduces a backward compatibility bug here:
"DDD" should always be formatted as a capitalized string (e.g. "Jan"), but
after this change it will be formatted as a full-uppercase string ("JAN")
because "DDD" is full-uppercase.
This breaks some BE-tests too (in date-test and timestamp-test).
--
To view, visit http://gerrit.cloudera.org:8080/14714
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f
Gerrit-Change-Number: 14714
Gerrit-PatchSet: 3
Gerrit-Owner: Gabor Kaszab
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Gabor Kaszab
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Fri, 22 Nov 2019 13:06:38 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Attila Jeges has posted comments on this change. ( http://gerrit.cloudera.org:8080/14714 ) Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 .. Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-parser-common.h File be/src/runtime/datetime-parser-common.h: http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-parser-common.h@70 PS3, Line 70: const unsigned SHORT_MONTH_NAME_LENGTH = 3; I think this constant and the others below should be int instead. These constants are usually compared against ints (e.g. tok.len) or used in int expressions. Mixing unsigned int with int is causing unnecessary warnings and should be avoided. http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-parser-common.cc File be/src/runtime/datetime-parser-common.cc: http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-parser-common.cc@199 PS3, Line 199: token_len < 3 (token_len < SHORT_MONTH_NAME_LENGTH) -- To view, visit http://gerrit.cloudera.org:8080/14714 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f Gerrit-Change-Number: 14714 Gerrit-PatchSet: 3 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 20 Nov 2019 16:24:49 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Attila Jeges has posted comments on this change. ( http://gerrit.cloudera.org:8080/14714 ) Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-parser-common.h File be/src/runtime/datetime-parser-common.h: http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-parser-common.h@273 PS3, Line 273: SHORT_MONTH_NAME MONTH_NAME_SHORT -- To view, visit http://gerrit.cloudera.org:8080/14714 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f Gerrit-Change-Number: 14714 Gerrit-PatchSet: 3 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 20 Nov 2019 16:01:31 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Attila Jeges has posted comments on this change. (
http://gerrit.cloudera.org:8080/14714 )
Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
..
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-iso-sql-format-tokenizer.cc
File be/src/runtime/datetime-iso-sql-format-tokenizer.cc:
http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-iso-sql-format-tokenizer.cc@207
PS3, Line 207: IsUsedToken("DD") || IsUsedToken("MM")
You probably have to add:
|| IsUsedToken("MONTH") || IsUsedToken("MON")
+ some tests for these scenarios
--
To view, visit http://gerrit.cloudera.org:8080/14714
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f
Gerrit-Change-Number: 14714
Gerrit-PatchSet: 3
Gerrit-Owner: Gabor Kaszab
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Gabor Kaszab
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Wed, 20 Nov 2019 14:03:54 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Attila Jeges has posted comments on this change. (
http://gerrit.cloudera.org:8080/14714 )
Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
..
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-parser-common.cc
File be/src/runtime/datetime-parser-common.cc:
http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-parser-common.cc@210
PS3, Line 210: DCHECK(tok.type == MONTH_NAME);
: bool is_token_padded = fx_modifier && !tok.fm_modifier;
: bool following_separator_found = token_start + token_len !=
input_end &&
: IsoSqlFormatTokenizer::IsSeparator(*(token_start +
token_len));
: if (is_token_padded || following_separator_found) {
: DCHECK(token_len == MAX_MONTH_NAME_LENGTH ||
!is_token_padded);
: if (is_token_padded) trim_right_if(buff,
boost::algorithm::is_any_of(" "));
: const auto iter = MONTH_NAME_TO_INDEX.find(buff);
: if (UNLIKELY(iter == MONTH_NAME_TO_INDEX.end())) return
false;
: *month = iter->second;
: return true;
: }
If you implement the optimization suggested below, this block probably wouldn't
be needed at all.
http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-parser-common.cc@329
PS3, Line 329: DCHECK(tok.len >= SHORT_DAY_NAME_LENGTH);
Add: && tok.len >= SHORT_MONTH_NAME_LENGTH condition.
http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/timestamp-parse-util.cc
File be/src/runtime/timestamp-parse-util.cc:
http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/timestamp-parse-util.cc@255
PS3, Line 255: case DAY_OF_WEEK: {
Please add a comment explaining that 1 means Sunday, 2 means Monday, and so on.
--
To view, visit http://gerrit.cloudera.org:8080/14714
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f
Gerrit-Change-Number: 14714
Gerrit-PatchSet: 3
Gerrit-Owner: Gabor Kaszab
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Gabor Kaszab
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Wed, 20 Nov 2019 09:44:56 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Attila Jeges has posted comments on this change. (
http://gerrit.cloudera.org:8080/14714 )
Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
..
Patch Set 3:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-iso-sql-format-tokenizer.cc
File be/src/runtime/datetime-iso-sql-format-tokenizer.cc:
http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-iso-sql-format-tokenizer.cc@200
PS3, Line 200: (IsUsedToken("MM") && (IsUsedToken("MONTH") ||
IsUsedToken("MON"))) ||
: (IsUsedToken("MONTH") && IsUsedToken("MON"))
Probably it would be simpler to do this check with a counter, similarly to
L215-219.
http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-parser-common.h
File be/src/runtime/datetime-parser-common.h:
http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-parser-common.h@22
PS3, Line 22: #include
: #include
: #include
nit: order includes alphabetically.
http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-parser-common.h@75
PS3, Line 75: TUE
Closing apostrophe is missing.
http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-parser-common.h@315
PS3, Line 315: month or day
nit: month or day name
http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-parser-common.cc
File be/src/runtime/datetime-parser-common.cc:
http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-parser-common.cc@223
PS3, Line 223: // If it's ambiguous where the end of the month token is in
the input then try to find
: // the end of the month token by probing the input string.
: int search_len = token_len;
: while (search_len >= 3) {
: const auto iter = MONTH_NAME_TO_INDEX.find(buff.substr(0,
search_len));
: if (iter != MONTH_NAME_TO_INDEX.end()) {
: *month = iter->second;
: *token_end = token_start + search_len;
: return true;
: }
: --search_len;
: }
I think, we could do this faster by taking advantage of the fact that checking
the first 3 letters of the month is enough to decide what month to look for.
const unordered_map month_prefix_to_suffix = {
{"jan", "uary"},
{"feb", "ruary"},
..
{"dec", "ember"}
};
if (token_len >= 3) {
const auto& prefix = buff.substr(0, 3);
const auto it = month_prefix_to_suffix.find(prefix);
if (it != month_prefix_to_suffix.end()) {
const auto& suffix = it->second;
//
// Test that 'suffix' matches the rest of 'buff'.
// ..
}
}
return false;
What do you think?
--
To view, visit http://gerrit.cloudera.org:8080/14714
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f
Gerrit-Change-Number: 14714
Gerrit-PatchSet: 3
Gerrit-Owner: Gabor Kaszab
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Gabor Kaszab
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Tue, 19 Nov 2019 16:02:57 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14714 ) Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/5064/ : 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/14714 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f Gerrit-Change-Number: 14714 Gerrit-PatchSet: 3 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 19 Nov 2019 12:12:28 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14714 ) Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/5063/ : 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/14714 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f Gerrit-Change-Number: 14714 Gerrit-PatchSet: 2 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 19 Nov 2019 09:51:52 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Attila Jeges has posted comments on this change. (
http://gerrit.cloudera.org:8080/14714 )
Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
..
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/14714/1/be/src/runtime/datetime-parser-common.cc
File be/src/runtime/datetime-parser-common.cc:
http://gerrit.cloudera.org:8080/#/c/14714/1/be/src/runtime/datetime-parser-common.cc@270
PS1, Line 270: TimestampFunctions::TextCase text_case =
TimestampFunctions::CAMELCASE;
: if (ascii_islower(*tok.val)) {
: text_case = TimestampFunctions::LOWERCASE;
: } else if (strncmp(tok.val, "MONTH", 5) == 0 ||
strncmp(tok.val, "MON", 3) == 0) {
: text_case = TimestampFunctions::UPPERCASE;
: }
This logic is a bit weird:
impala> select cast(DATE '2011-01-01' as string format
'-mONTH-MOnth-MONth-MONTh');
2011-january -January -JANUARY -JANUARY
Maybe these alternatively spelled format tokens shouldn't even be accepted.
http://gerrit.cloudera.org:8080/#/c/14714/1/be/src/runtime/datetime-parser-common.cc@290
PS1, Line 290: if (ascii_islower(*tok.val)) {
: text_case = TimestampFunctions::LOWERCASE;
: } else if (strncmp(tok.val, "DAY", 3) == 0 ||
strncmp(tok.val, "DY", 2) == 0) {
: text_case = TimestampFunctions::UPPERCASE;
: }
Same as L270
http://gerrit.cloudera.org:8080/#/c/14714/1/tests/query_test/test_cast_with_format.py
File tests/query_test/test_cast_with_format.py:
http://gerrit.cloudera.org:8080/#/c/14714/1/tests/query_test/test_cast_with_format.py@399
PS1, Line 399:# Test different lowercase vs uppercase scenarios with the
datetime to string path
: # when FM is provided.
: result = self.execute_query("select cast(date'2010-10-18' as
string FORMAT "
: "'FMMONTH FMMonth FMmonth')")
: assert result.data == ["OCTOBER October october"]
This is a bit weird that we have to add FM modifier to the MONTH token to avoid
padding in the resulting string. Hive works this way too?
http://gerrit.cloudera.org:8080/#/c/14714/1/tests/query_test/test_cast_with_format.py@834
PS1, Line 834: result = self.execute_query("select cast(date'2019-01-01' as
string "
: "format 'FX FMDAY DD')")
: assert result.data == ["2019 TUESDAY 01"]
Same as L403
--
To view, visit http://gerrit.cloudera.org:8080/14714
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f
Gerrit-Change-Number: 14714
Gerrit-PatchSet: 1
Gerrit-Owner: Gabor Kaszab
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Mon, 18 Nov 2019 13:35:06 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Attila Jeges has posted comments on this change. ( http://gerrit.cloudera.org:8080/14714 ) Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/14714/1/be/src/runtime/datetime-parser-common.cc File be/src/runtime/datetime-parser-common.cc: http://gerrit.cloudera.org:8080/#/c/14714/1/be/src/runtime/datetime-parser-common.cc@197 PS1, Line 197: const auto* search_map = _NAME_TO_INDEX; : if (type == MONTH_NAME_SHORT) search_map = _MONTH_NAME_TO_INDEX; > nit: Using the ternary operator and a constant reference instead of a point Ok, I messed this up :) const auto& search_map = (type == MONTH_NAME) ? MONTH_NAME_TO_INDEX : SHORT_MONTH_NAME_TO_INDEX; -- To view, visit http://gerrit.cloudera.org:8080/14714 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f Gerrit-Change-Number: 14714 Gerrit-PatchSet: 1 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Fri, 15 Nov 2019 14:55:57 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Attila Jeges has posted comments on this change. (
http://gerrit.cloudera.org:8080/14714 )
Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
..
Patch Set 1:
(17 comments)
http://gerrit.cloudera.org:8080/#/c/14714/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14714/1//COMMIT_MSG@12
PS1, Line 12: The tokens introduced:
: - Full month name: In a string to datetime conversion this token
can
: parse textual month name into a datetime type. In a datetime to
: string conversion this token gives the textual representation
of a
: month.
: - Short month name: Similar to the full month name token but this
: works for 3-character month names like 'JAN'.
: - Full day name: In a datetime to string conversion this token
gives
: the textual representation of a day like 'Tuesday.' Not
suppported
: in a string to datetime conversion.
: - Short day name: Similar to full day name token but this works
for
: 3-character day names like 'TUE'.
: - Day of week: In a datetime to string conversion this gives a
number
: in [1-7] where 1 represents Sunday. Not supported in a string to
: datetime conversion.
: - Quarter of year: In a datetime to string conversion this gives a
: number in [1-4] representing a quarter of the year. Not
supported
: in a string to datetime conversion.
: - Week of year: In a datetime to string conversion this gives a
: number in [1-53] to represent the week of year where the first
week
: starts from 1st of January. Not supported in a string to
datetime
: conversion.
: - Week of month: In a datetime to string conversion this gives a
: number in [1-5] to represent the week of month where the first
week
: starts from the first day of the month. Not supported in a
string
: to datetime conversion.
Please add the actual format tokens to the commit message as well.
http://gerrit.cloudera.org:8080/#/c/14714/1/be/src/exprs/timestamp-functions.h
File be/src/exprs/timestamp-functions.h:
http://gerrit.cloudera.org:8080/#/c/14714/1/be/src/exprs/timestamp-functions.h@71
PS1, Line 71: CAMELCASE
CAMELCASE is a bit misleading as strings like "eBay", "iPhone" are also called
camel case (https://en.wikipedia.org/wiki/Camel_case).
CAPITALIZED or something similar would be a better name.
http://gerrit.cloudera.org:8080/#/c/14714/1/be/src/exprs/timestamp-functions.cc
File be/src/exprs/timestamp-functions.cc:
http://gerrit.cloudera.org:8080/#/c/14714/1/be/src/exprs/timestamp-functions.cc@59
PS1, Line 59: const string TimestampFunctions::DAY_ARRAY[7] = {"Sun", "Mon",
"Tue", "Wed", "Thu",
: "Fri", "Sat"};
Is this still necessary? We can just use SHORT_DAY_NAMES[CAMELCASE] instead.
http://gerrit.cloudera.org:8080/#/c/14714/1/be/src/runtime/date-parse-util.cc
File be/src/runtime/date-parse-util.cc:
http://gerrit.cloudera.org:8080/#/c/14714/1/be/src/runtime/date-parse-util.cc@212
PS1, Line 212: int DateParser::GetDayOfWeek(const DateValue& date) {
DCHECK(date.IsValid());
http://gerrit.cloudera.org:8080/#/c/14714/1/be/src/runtime/datetime-iso-sql-format-tokenizer.cc
File be/src/runtime/datetime-iso-sql-format-tokenizer.cc:
http://gerrit.cloudera.org:8080/#/c/14714/1/be/src/runtime/datetime-iso-sql-format-tokenizer.cc@77
PS1, Line 77: {"FF3", 3}
Does "FF3" have to be listed here? The length of the token is already 3.
http://gerrit.cloudera.org:8080/#/c/14714/1/be/src/runtime/datetime-iso-sql-format-tokenizer.cc@77
PS1, Line 77: {"HH", 2},
Does "HH" have to be listed here? The length of the token is already 2.
http://gerrit.cloudera.org:8080/#/c/14714/1/be/src/runtime/datetime-parser-common.h
File be/src/runtime/datetime-parser-common.h:
http://gerrit.cloudera.org:8080/#/c/14714/1/be/src/runtime/datetime-parser-common.h@69
PS1, Line 69: MAX_SHORT_MONTH_NAME_LENGTH
Probably this should be just called 'SHORT_MONTH_NAME_LENGTH' as all short
month names are of the same length.
http://gerrit.cloudera.org:8080/#/c/14714/1/be/src/runtime/datetime-parser-common.h@74
PS1, Line 74: // Length of short day names like 'JAN', 'FEB, etc.
Comment should be about short day names and not about short month names.
http://gerrit.cloudera.org:8080/#/c/14714/1/be/src/runtime/datetime-parser-common.h@75
PS1, Line 75: MAX_SHORT_DAY_NAME_LENGTH
Same as L74: should be called 'SHORT_DAY_NAME_LENGTH'.
http://gerrit.cloudera.org:8080/#/c/14714/1/be/src/runtime/datetime-parser-common.h@296
PS1, Line 296: ased on
: /// how the month token is given in 'tok'
nit: Based on the casing of the month format token in 'tok'.
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14714 ) Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/5025/ : 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/14714 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f Gerrit-Change-Number: 14714 Gerrit-PatchSet: 1 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Fri, 15 Nov 2019 09:43:46 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3
Gabor Kaszab has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14714 Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 .. IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 This patch adds additional datetime format tokens on top of Milestone 1 (IMPALA-8703) and Milestone 2 (IMPALA-8704). The tokens introduced: - Full month name: In a string to datetime conversion this token can parse textual month name into a datetime type. In a datetime to string conversion this token gives the textual representation of a month. - Short month name: Similar to the full month name token but this works for 3-character month names like 'JAN'. - Full day name: In a datetime to string conversion this token gives the textual representation of a day like 'Tuesday.' Not suppported in a string to datetime conversion. - Short day name: Similar to full day name token but this works for 3-character day names like 'TUE'. - Day of week: In a datetime to string conversion this gives a number in [1-7] where 1 represents Sunday. Not supported in a string to datetime conversion. - Quarter of year: In a datetime to string conversion this gives a number in [1-4] representing a quarter of the year. Not supported in a string to datetime conversion. - Week of year: In a datetime to string conversion this gives a number in [1-53] to represent the week of year where the first week starts from 1st of January. Not supported in a string to datetime conversion. - Week of month: In a datetime to string conversion this gives a number in [1-5] to represent the week of month where the first week starts from the first day of the month. Not supported in a string to datetime conversion. Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f --- M be/src/benchmarks/convert-timestamp-benchmark.cc M be/src/benchmarks/parse-timestamp-benchmark.cc M be/src/common/init.cc M be/src/exprs/date-functions-ir.cc M be/src/exprs/timestamp-functions-ir.cc M be/src/exprs/timestamp-functions.cc M be/src/exprs/timestamp-functions.h M be/src/runtime/date-parse-util.cc M be/src/runtime/date-parse-util.h M be/src/runtime/datetime-iso-sql-format-parser.cc M be/src/runtime/datetime-iso-sql-format-tokenizer.cc M be/src/runtime/datetime-parser-common.cc M be/src/runtime/datetime-parser-common.h 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 tests/query_test/test_cast_with_format.py 17 files changed, 930 insertions(+), 74 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/14714/1 -- To view, visit http://gerrit.cloudera.org:8080/14714 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f Gerrit-Change-Number: 14714 Gerrit-PatchSet: 1 Gerrit-Owner: Gabor Kaszab
