[Impala-ASF-CR] IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14852 ) Change subject: IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4 .. Patch Set 7: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/14852 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I89a8c1b98742391cb7b331840d216558dbca362b Gerrit-Change-Number: 14852 Gerrit-PatchSet: 7 Gerrit-Owner: Attila Jeges Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Fri, 13 Dec 2019 20:02:15 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14852 ) Change subject: IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4 .. IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4 This patch adds ISO 8601 week-based date format tokens on top of what was introduced in IMPALA-8703, IMPALA-8704 and IMPALA-8705. The ISO 8601 week-based date tokens may be used for both datetime to string and string to datetime conversion. The ISO 8601 week-based date tokens are as follows: - IYYY: 4-digit ISO 8601 week-numbering year. Week-numbering year is the year relating to the ISO 8601 week number (IW), which is the full week (Monday to Sunday) which contains January 4 of the Gregorian year. Behaves similarly to in that for datetime to string conversion, prefix digits for 1, 2, and 3-digit inputs are obtained from current ISO 8601 week-numbering year. - IYY: Last 3 digits of ISO 8601 week-numbering year. Behaves similarly to YYY in that for datetime to string conversion, prefix digit is obtained from current ISO 8601 week-numbering year and can accept 1 or 2-digit input. - IY: Last 2 digits of ISO 8601 week-numbering year. Behaves similarly to YY in that for datetime to string conversion, prefix digits are obtained from current ISO 8601 week-numbering year and can accept 1-digit input. - I:Last digit of ISO 8601 week-numbering year. Behaves similarly to Y in that for datetime to string conversion, prefix digits are obtained from current ISO 8601 week-numbering year. - IW: ISO 8601 week of year (1-53). Begins on the Monday closest to January 1 of the year. For string to datetime conversion, if the input ISO 8601 week does not exist in the input year, an error will be thrown. Note that IW is different from the other week-related tokens WW and W (implemented in IMPALA-8705). With WW and W weeks start with the first day of the year/month. ISO 8601 weeks on the other hand always start with Monday. - ID: ISO 8601 day of week (1-7). 1 means Monday and 7 means Sunday. When doing string to datetime conversion, the ISO 8601 week-based tokens are meant to be used together and not mixed with other ISO SQL date tokens. E.g. '-IW-ID' is an invalid format string. The only exceptions are the day name tokens (DAY and DY) which may be used instead of ID with the rest of the ISO 8601 week-based date tokens. E.g. 'IYYY-IW-DAY' is a valid format string. Change-Id: I89a8c1b98742391cb7b331840d216558dbca362b Reviewed-on: http://gerrit.cloudera.org:8080/14852 Reviewed-by: Impala Public Jenkins Reviewed-by: Gabor Kaszab Tested-by: Impala Public Jenkins --- M be/src/exprs/date-functions-ir.cc M be/src/runtime/date-parse-util.cc M be/src/runtime/date-test.cc M be/src/runtime/date-value.cc M be/src/runtime/date-value.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/timestamp-parse-util.cc M be/src/runtime/timestamp-parse-util.h M tests/query_test/test_cast_with_format.py 13 files changed, 920 insertions(+), 160 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified Gabor Kaszab: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/14852 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I89a8c1b98742391cb7b331840d216558dbca362b Gerrit-Change-Number: 14852 Gerrit-PatchSet: 8 Gerrit-Owner: Attila Jeges Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14852 ) Change subject: IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4 .. Patch Set 7: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/14852 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I89a8c1b98742391cb7b331840d216558dbca362b Gerrit-Change-Number: 14852 Gerrit-PatchSet: 7 Gerrit-Owner: Attila Jeges Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Fri, 13 Dec 2019 15:37:31 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14852 ) Change subject: IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4 .. Patch Set 7: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5339/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/14852 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I89a8c1b98742391cb7b331840d216558dbca362b Gerrit-Change-Number: 14852 Gerrit-PatchSet: 7 Gerrit-Owner: Attila Jeges Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Fri, 13 Dec 2019 15:37:32 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4
Attila Jeges has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/14852 ) Change subject: IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4 .. IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4 This patch adds ISO 8601 week-based date format tokens on top of what was introduced in IMPALA-8703, IMPALA-8704 and IMPALA-8705. The ISO 8601 week-based date tokens may be used for both datetime to string and string to datetime conversion. The ISO 8601 week-based date tokens are as follows: - IYYY: 4-digit ISO 8601 week-numbering year. Week-numbering year is the year relating to the ISO 8601 week number (IW), which is the full week (Monday to Sunday) which contains January 4 of the Gregorian year. Behaves similarly to in that for datetime to string conversion, prefix digits for 1, 2, and 3-digit inputs are obtained from current ISO 8601 week-numbering year. - IYY: Last 3 digits of ISO 8601 week-numbering year. Behaves similarly to YYY in that for datetime to string conversion, prefix digit is obtained from current ISO 8601 week-numbering year and can accept 1 or 2-digit input. - IY: Last 2 digits of ISO 8601 week-numbering year. Behaves similarly to YY in that for datetime to string conversion, prefix digits are obtained from current ISO 8601 week-numbering year and can accept 1-digit input. - I:Last digit of ISO 8601 week-numbering year. Behaves similarly to Y in that for datetime to string conversion, prefix digits are obtained from current ISO 8601 week-numbering year. - IW: ISO 8601 week of year (1-53). Begins on the Monday closest to January 1 of the year. For string to datetime conversion, if the input ISO 8601 week does not exist in the input year, an error will be thrown. Note that IW is different from the other week-related tokens WW and W (implemented in IMPALA-8705). With WW and W weeks start with the first day of the year/month. ISO 8601 weeks on the other hand always start with Monday. - ID: ISO 8601 day of week (1-7). 1 means Monday and 7 means Sunday. When doing string to datetime conversion, the ISO 8601 week-based tokens are meant to be used together and not mixed with other ISO SQL date tokens. E.g. '-IW-ID' is an invalid format string. The only exceptions are the day name tokens (DAY and DY) which may be used instead of ID with the rest of the ISO 8601 week-based date tokens. E.g. 'IYYY-IW-DAY' is a valid format string. Change-Id: I89a8c1b98742391cb7b331840d216558dbca362b --- M be/src/exprs/date-functions-ir.cc M be/src/runtime/date-parse-util.cc M be/src/runtime/date-test.cc M be/src/runtime/date-value.cc M be/src/runtime/date-value.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/timestamp-parse-util.cc M be/src/runtime/timestamp-parse-util.h M tests/query_test/test_cast_with_format.py 13 files changed, 920 insertions(+), 160 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/14852/6 -- To view, visit http://gerrit.cloudera.org:8080/14852 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I89a8c1b98742391cb7b331840d216558dbca362b Gerrit-Change-Number: 14852 Gerrit-PatchSet: 6 Gerrit-Owner: Attila Jeges Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4
Attila Jeges has posted comments on this change. ( http://gerrit.cloudera.org:8080/14852 ) Change subject: IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4 .. Patch Set 6: Code-Review+2 Carry +2 -- To view, visit http://gerrit.cloudera.org:8080/14852 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I89a8c1b98742391cb7b331840d216558dbca362b Gerrit-Change-Number: 14852 Gerrit-PatchSet: 6 Gerrit-Owner: Attila Jeges Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Fri, 13 Dec 2019 15:35:33 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4
Attila Jeges has posted comments on this change. ( http://gerrit.cloudera.org:8080/14852 ) Change subject: IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4 .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/14852/5/tests/query_test/test_cast_with_format.py File tests/query_test/test_cast_with_format.py: http://gerrit.cloudera.org:8080/#/c/14852/5/tests/query_test/test_cast_with_format.py@1541 PS5, Line 1541: nbumbering > nit: numbering Done. Added some extra tests for 3- and 2-digit week numbering years. -- To view, visit http://gerrit.cloudera.org:8080/14852 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I89a8c1b98742391cb7b331840d216558dbca362b Gerrit-Change-Number: 14852 Gerrit-PatchSet: 5 Gerrit-Owner: Attila Jeges Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Fri, 13 Dec 2019 15:34:32 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4
Gabor Kaszab has posted comments on this change. (
http://gerrit.cloudera.org:8080/14852 )
Change subject: IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4
..
Patch Set 5: Code-Review+2
(2 comments)
Thanks for making all these changes! Other than one nit this is good to go.
http://gerrit.cloudera.org:8080/#/c/14852/4/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/14852/4/be/src/runtime/datetime-iso-sql-format-tokenizer.cc@216
PS4, Line 216: bool is_used_day_of_year = IsUsedToken("DDD");
> This is a matter of taste: I don't like looking up the same key twice in a
You convinced me with your reasoning :) The current naming seems better so
let's leave it as it is. Same for the ones below.
http://gerrit.cloudera.org:8080/#/c/14852/5/tests/query_test/test_cast_with_format.py
File tests/query_test/test_cast_with_format.py:
http://gerrit.cloudera.org:8080/#/c/14852/5/tests/query_test/test_cast_with_format.py@1541
PS5, Line 1541: nbumbering
nit: numbering
--
To view, visit http://gerrit.cloudera.org:8080/14852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I89a8c1b98742391cb7b331840d216558dbca362b
Gerrit-Change-Number: 14852
Gerrit-PatchSet: 5
Gerrit-Owner: Attila Jeges
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Gabor Kaszab
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Fri, 13 Dec 2019 15:17:52 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14852 ) Change subject: IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4 .. Patch Set 5: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/5278/ : 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/14852 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I89a8c1b98742391cb7b331840d216558dbca362b Gerrit-Change-Number: 14852 Gerrit-PatchSet: 5 Gerrit-Owner: Attila Jeges Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Fri, 13 Dec 2019 13:42:59 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4
Attila Jeges has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/14852 ) Change subject: IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4 .. IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4 This patch adds ISO 8601 week-based date format tokens on top of what was introduced in IMPALA-8703, IMPALA-8704 and IMPALA-8705. The ISO 8601 week-based date tokens may be used for both datetime to string and string to datetime conversion. The ISO 8601 week-based date tokens are as follows: - IYYY: 4-digit ISO 8601 week-numbering year. Week-numbering year is the year relating to the ISO 8601 week number (IW), which is the full week (Monday to Sunday) which contains January 4 of the Gregorian year. Behaves similarly to in that for datetime to string conversion, prefix digits for 1, 2, and 3-digit inputs are obtained from current ISO 8601 week-numbering year. - IYY: Last 3 digits of ISO 8601 week-numbering year. Behaves similarly to YYY in that for datetime to string conversion, prefix digit is obtained from current ISO 8601 week-numbering year and can accept 1 or 2-digit input. - IY: Last 2 digits of ISO 8601 week-numbering year. Behaves similarly to YY in that for datetime to string conversion, prefix digits are obtained from current ISO 8601 week-numbering year and can accept 1-digit input. - I:Last digit of ISO 8601 week-numbering year. Behaves similarly to Y in that for datetime to string conversion, prefix digits are obtained from current ISO 8601 week-numbering year. - IW: ISO 8601 week of year (1-53). Begins on the Monday closest to January 1 of the year. For string to datetime conversion, if the input ISO 8601 week does not exist in the input year, an error will be thrown. Note that IW is different from the other week-related tokens WW and W (implemented in IMPALA-8705). With WW and W weeks start with the first day of the year/month. ISO 8601 weeks on the other hand always start with Monday. - ID: ISO 8601 day of week (1-7). 1 means Monday and 7 means Sunday. When doing string to datetime conversion, the ISO 8601 week-based tokens are meant to be used together and not mixed with other ISO SQL date tokens. E.g. '-IW-ID' is an invalid format string. The only exceptions are the day name tokens (DAY and DY) which may be used instead of ID with the rest of the ISO 8601 week-based date tokens. E.g. 'IYYY-IW-DAY' is a valid format string. Change-Id: I89a8c1b98742391cb7b331840d216558dbca362b --- M be/src/exprs/date-functions-ir.cc M be/src/runtime/date-parse-util.cc M be/src/runtime/date-test.cc M be/src/runtime/date-value.cc M be/src/runtime/date-value.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/timestamp-parse-util.cc M be/src/runtime/timestamp-parse-util.h M tests/query_test/test_cast_with_format.py 13 files changed, 918 insertions(+), 160 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/14852/5 -- To view, visit http://gerrit.cloudera.org:8080/14852 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I89a8c1b98742391cb7b331840d216558dbca362b Gerrit-Change-Number: 14852 Gerrit-PatchSet: 5 Gerrit-Owner: Attila Jeges Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4
Attila Jeges has posted comments on this change. (
http://gerrit.cloudera.org:8080/14852 )
Change subject: IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4
..
Patch Set 4:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/14852/4/be/src/runtime/date-value.h
File be/src/runtime/date-value.h:
http://gerrit.cloudera.org:8080/#/c/14852/4/be/src/runtime/date-value.h@74
PS4, Line 74: ISO8601
> nit: ISO 8601 here and other occurences
Done
http://gerrit.cloudera.org:8080/#/c/14852/4/be/src/runtime/date-value.h@121
PS4, Line 121: Iso8601WeekNumberingYear
> If i understand correctly this function returns -1 if DateValue object is
Done. Iso8601WeekOfYear() also returns -1 if DateValue instance is invalid.
http://gerrit.cloudera.org:8080/#/c/14852/4/be/src/runtime/date-value.cc
File be/src/runtime/date-value.cc:
http://gerrit.cloudera.org:8080/#/c/14852/4/be/src/runtime/date-value.cc@120
PS4, Line 120: *7
> nit: Not sure, but shouldn't you leave spaces around the operator
Done
http://gerrit.cloudera.org:8080/#/c/14852/4/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/14852/4/be/src/runtime/datetime-iso-sql-format-parser.h@53
PS4, Line 53: bool ExtractYearMonthDay(DateTimeParseResult* result) const;
> nit: Could you add a short comment for this function?
Done
http://gerrit.cloudera.org:8080/#/c/14852/4/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/14852/4/be/src/runtime/datetime-iso-sql-format-parser.cc@207
PS4, Line 207: 0, ,
> Isn't the range is [1..]? As I see 0001-01-01 is Monday that is also th
Year can be 0 when the input doesn't have all four digits and has to be
prefixed.
E.g.:
select cast('000-01-01' as timestamp FORMAT 'IYYY-IW-ID');
or
select cast('0-01-01' as timestamp FORMAT 'I-IW-ID');
In L87 we also accept 0 for YEAR token.
http://gerrit.cloudera.org:8080/#/c/14852/4/be/src/runtime/datetime-iso-sql-format-parser.cc@212
PS4, Line 212: PrefixYearFromCurrentYear(token_len,
dt_ctx.current_time,
: _week_based_date_values.year);
This looks like a bug though. The docs for IYYY states:
"Behaves similarly to in that for datetime to string conversion, prefix
digits for 1, 2, and 3-digit inputs are obtained from current week-numbering
year."
We have to use the current ISO 8601 week-numbering year instead of the current
year to prefix 'iso8601_week_based_date_values.year'. I'' fix this in the next
patch-set.
http://gerrit.cloudera.org:8080/#/c/14852/4/be/src/runtime/datetime-iso-sql-format-parser.cc@403
PS4, Line 403: CreateFromIso8601WeekBasedDateVals
> Shouldn't you check IsSet() before calling CreateFromIso..()? I feel that i
If IsSet() is false, this function shouldn't be called at all, so I think a
DCHECK would be better.
Done.
http://gerrit.cloudera.org:8080/#/c/14852/4/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/14852/4/be/src/runtime/datetime-iso-sql-format-tokenizer.cc@216
PS4, Line 216: bool is_used_day_of_year = IsUsedToken("DDD");
> is_used_day_of_year name is longer than IsUsedToken("DDD"). Additionally, t
This is a matter of taste: I don't like looking up the same key twice in a map.
No matter how fast the lookup algorithm is, it is still going to be slower than
reading a variable.
I could use a shorter name, like 'found_day_of_year' or 'used_day_of_year' or
'used_ddd'. What do you think?
http://gerrit.cloudera.org:8080/#/c/14852/4/be/src/runtime/datetime-iso-sql-format-tokenizer.cc@217
PS4, Line 217: is_used_day_of_month
> Same as above.
See L216.
http://gerrit.cloudera.org:8080/#/c/14852/4/be/src/runtime/datetime-iso-sql-format-tokenizer.cc@261
PS4, Line 261: bool is_used_iso8601_week_of_year = IsUsedToken("IW");
> Again, the variable name is longer than the O(1) function call. Is there a
See L216.
http://gerrit.cloudera.org:8080/#/c/14852/4/tests/query_test/test_cast_with_format.py
File tests/query_test/test_cast_with_format.py:
http://gerrit.cloudera.org:8080/#/c/14852/4/tests/query_test/test_cast_with_format.py@840
PS4, Line 840: String to datetime
> nit: String to datetime:
Done
http://gerrit.cloudera.org:8080/#/c/14852/4/tests/query_test/test_cast_with_format.py@927
PS4, Line 927: in a datetime to string path.
> nit: This is not needed with the prefix you added to this comment.
Done
http://gerrit.cloudera.org:8080/#/c/14852/4/tests/query_test/test_cast_with_format.py@1513
PS4, Line 1513: # Format 4, 3, 2, 1-digit week numbering year.
> I might miss it but I don't see e2e tests for the case where the input prov
Added them to the select in L1524.
--
To view, visit http://gerrit.cloudera.org:8080/14852
To unsubscribe, visit
[Impala-ASF-CR] IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4
Gabor Kaszab has posted comments on this change. (
http://gerrit.cloudera.org:8080/14852 )
Change subject: IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4
..
Patch Set 4:
(16 comments)
The latest patch is in good shape in general, I only have some minor comments.
http://gerrit.cloudera.org:8080/#/c/14852/4/be/src/runtime/date-value.h
File be/src/runtime/date-value.h:
http://gerrit.cloudera.org:8080/#/c/14852/4/be/src/runtime/date-value.h@74
PS4, Line 74: ISO8601
nit: ISO 8601 here and other occurences
http://gerrit.cloudera.org:8080/#/c/14852/4/be/src/runtime/date-value.h@121
PS4, Line 121: Iso8601WeekNumberingYear
If i understand correctly this function returns -1 if DateValue object is
invalid. If this is true, could you please document that.
http://gerrit.cloudera.org:8080/#/c/14852/2/be/src/runtime/date-value.cc
File be/src/runtime/date-value.cc:
http://gerrit.cloudera.org:8080/#/c/14852/2/be/src/runtime/date-value.cc@110
PS2, Line 110:
> Renamed it to 'GetFirstIso8601MondayOfYear'
Thanks!
http://gerrit.cloudera.org:8080/#/c/14852/2/be/src/runtime/date-value.cc@110
PS2, Line 110:
> I don't want to include cctz/civil_time.h in date-value.h because that woul
Thanks for the explanation! Let's leave them here then.
http://gerrit.cloudera.org:8080/#/c/14852/2/be/src/runtime/date-value.cc@116
PS2, Line 116: && 1 <= day_of_week && day_of_week <= 7) {
: const cctz::civil_day first_monday =
GetFirstIso8601MondayOfYear(year);
: const cctz::civil_day last_sunday =
GetLastIso8601SundayOfYear(year);
:
> I think, it is easier to read it like this.
Thanks for explaining!
http://gerrit.cloudera.org:8080/#/c/14852/2/be/src/runtime/date-value.cc@129
PS2, Line 129: rser::ParseSi
> Renamed it to 'GetLastIso8601SundayOfYear'.
Thx!
http://gerrit.cloudera.org:8080/#/c/14852/4/be/src/runtime/date-value.cc
File be/src/runtime/date-value.cc:
http://gerrit.cloudera.org:8080/#/c/14852/4/be/src/runtime/date-value.cc@120
PS4, Line 120: *7
nit: Not sure, but shouldn't you leave spaces around the operator
http://gerrit.cloudera.org:8080/#/c/14852/4/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/14852/4/be/src/runtime/datetime-iso-sql-format-parser.h@53
PS4, Line 53: bool ExtractYearMonthDay(DateTimeParseResult* result) const;
nit: Could you add a short comment for this function?
http://gerrit.cloudera.org:8080/#/c/14852/4/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/14852/4/be/src/runtime/datetime-iso-sql-format-parser.cc@207
PS4, Line 207: 0, ,
Isn't the range is [1..]? As I see 0001-01-01 is Monday that is also the
beginning of the first week both for WW and IW so I don't see any input where
year should be accepted here. Do I miss something?
http://gerrit.cloudera.org:8080/#/c/14852/4/be/src/runtime/datetime-iso-sql-format-parser.cc@403
PS4, Line 403: CreateFromIso8601WeekBasedDateVals
Shouldn't you check IsSet() before calling CreateFromIso..()? I feel that it's
better doing here instead of relying to the user to call IsSet() before calling
this function.
Hmm, not sure. a DCHECK maybe is enough. What do you think?
http://gerrit.cloudera.org:8080/#/c/14852/4/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/14852/4/be/src/runtime/datetime-iso-sql-format-tokenizer.cc@216
PS4, Line 216: bool is_used_day_of_year = IsUsedToken("DDD");
is_used_day_of_year name is longer than IsUsedToken("DDD"). Additionally, the
complexity is O(1) for IsUsedToken so we won't win much. Is there a reason to
introduce a variable?
http://gerrit.cloudera.org:8080/#/c/14852/4/be/src/runtime/datetime-iso-sql-format-tokenizer.cc@217
PS4, Line 217: is_used_day_of_month
Same as above.
http://gerrit.cloudera.org:8080/#/c/14852/4/be/src/runtime/datetime-iso-sql-format-tokenizer.cc@261
PS4, Line 261: bool is_used_iso8601_week_of_year = IsUsedToken("IW");
Again, the variable name is longer than the O(1) function call. Is there a
reason to do this?
http://gerrit.cloudera.org:8080/#/c/14852/4/tests/query_test/test_cast_with_format.py
File tests/query_test/test_cast_with_format.py:
http://gerrit.cloudera.org:8080/#/c/14852/4/tests/query_test/test_cast_with_format.py@840
PS4, Line 840: String to datetime
nit: String to datetime:
http://gerrit.cloudera.org:8080/#/c/14852/4/tests/query_test/test_cast_with_format.py@927
PS4, Line 927: in a datetime to string path.
nit: This is not needed with the prefix you added to this comment.
http://gerrit.cloudera.org:8080/#/c/14852/4/tests/query_test/test_cast_with_format.py@1513
PS4, Line 1513: # Format 4, 3, 2, 1-digit week numbering year.
I might miss it
[Impala-ASF-CR] IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14852 ) Change subject: IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4 .. Patch Set 4: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/5239/ : 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/14852 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I89a8c1b98742391cb7b331840d216558dbca362b Gerrit-Change-Number: 14852 Gerrit-PatchSet: 4 Gerrit-Owner: Attila Jeges Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 10 Dec 2019 16:20:04 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4
Attila Jeges has posted comments on this change. ( http://gerrit.cloudera.org:8080/14852 ) Change subject: IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4 .. Patch Set 4: > Uploaded patch set 4. This renames DateValue::GetIso8601WeekNumberingYear() back to DateValue::Iso8601WeekNumberingYear() -- To view, visit http://gerrit.cloudera.org:8080/14852 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I89a8c1b98742391cb7b331840d216558dbca362b Gerrit-Change-Number: 14852 Gerrit-PatchSet: 4 Gerrit-Owner: Attila Jeges Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 10 Dec 2019 15:51:06 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4
Attila Jeges has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/14852 ) Change subject: IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4 .. IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4 This patch adds ISO 8601 week-based date format tokens on top of what was introduced in IMPALA-8703, IMPALA-8704 and IMPALA-8705. The ISO 8601 week-based date tokens may be used for both datetime to string and string to datetime conversion. The ISO 8601 week-based date tokens are as follows: - IYYY: 4-digit ISO 8601 week-numbering year. Week-numbering year is the year relating to the ISO 8601 week number (IW), which is the full week (Monday to Sunday) which contains January 4 of the Gregorian year. Behaves similarly to in that for datetime to string conversion, prefix digits for 1, 2, and 3-digit inputs are obtained from current week-numbering year. - IYY: Last 3 digits of ISO 8601 week-numbering year. Behaves similarly to YYY in that for datetime to string conversion, prefix digit is obtained from current week-numbering year and can accept 1 or 2-digit input. - IY: Last 2 digits of ISO 8601 week-numbering year. Behaves similarly to YY in that for datetime to string conversion, prefix digits are obtained from current week-numbering year and can accept 1-digit input. - I:Last digit of ISO 8601 week-numbering year. Behaves similarly to Y in that for datetime to string conversion, prefix digits are obtained from current week-numbering year. - IW: ISO 8601 week of year (1-53). Begins on the Monday closest to January 1 of the year. For string to datetime conversion, if the input ISO 8601 week does not exist in the input year, an error will be thrown. Note that IW is different from the other week-related tokens WW and W (implemented in IMPALA-8705). With WW and W weeks start with the first day of the year/month. ISO 8601 weeks on the other hand always start with Monday. - ID: ISO 8601 day of week (1-7). 1 means Monday and 7 means Sunday. When doing string to datetime conversion, the ISO 8601 week-based tokens are meant to be used together and not mixed with other ISO SQL date tokens. E.g. '-IW-ID' is an invalid format string. The only exceptions are the day name tokens (DAY and DY) which may be used instead of ID with the rest of the ISO 8601 week-based date tokens. E.g. 'IYYY-IW-DAY' is a valid format string. Change-Id: I89a8c1b98742391cb7b331840d216558dbca362b --- M be/src/exprs/date-functions-ir.cc M be/src/runtime/date-parse-util.cc M be/src/runtime/date-test.cc M be/src/runtime/date-value.cc M be/src/runtime/date-value.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/timestamp-parse-util.cc M be/src/runtime/timestamp-parse-util.h M tests/query_test/test_cast_with_format.py 13 files changed, 861 insertions(+), 151 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/14852/4 -- To view, visit http://gerrit.cloudera.org:8080/14852 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I89a8c1b98742391cb7b331840d216558dbca362b Gerrit-Change-Number: 14852 Gerrit-PatchSet: 4 Gerrit-Owner: Attila Jeges Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4
Attila Jeges has posted comments on this change. ( http://gerrit.cloudera.org:8080/14852 ) Change subject: IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4 .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/14852/3/be/src/runtime/timestamp-parse-util.cc File be/src/runtime/timestamp-parse-util.cc: http://gerrit.cloudera.org:8080/#/c/14852/3/be/src/runtime/timestamp-parse-util.cc@342 PS3, Line 342: GetIso8601WeekNumberingYear Hm, I renamed this as well accidentally. I will upload patch-set 4 to fix this. -- To view, visit http://gerrit.cloudera.org:8080/14852 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I89a8c1b98742391cb7b331840d216558dbca362b Gerrit-Change-Number: 14852 Gerrit-PatchSet: 3 Gerrit-Owner: Attila Jeges Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 10 Dec 2019 15:24:53 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14852 ) Change subject: IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4 .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/5238/ : 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/14852 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I89a8c1b98742391cb7b331840d216558dbca362b Gerrit-Change-Number: 14852 Gerrit-PatchSet: 3 Gerrit-Owner: Attila Jeges Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 10 Dec 2019 15:19:09 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4
Attila Jeges has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/14852 ) Change subject: IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4 .. IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4 This patch adds ISO 8601 week-based date format tokens on top of what was introduced in IMPALA-8703, IMPALA-8704 and IMPALA-8705. The ISO 8601 week-based date tokens may be used for both datetime to string and string to datetime conversion. The ISO 8601 week-based date tokens are as follows: - IYYY: 4-digit ISO 8601 week-numbering year. Week-numbering year is the year relating to the ISO 8601 week number (IW), which is the full week (Monday to Sunday) which contains January 4 of the Gregorian year. Behaves similarly to in that for datetime to string conversion, prefix digits for 1, 2, and 3-digit inputs are obtained from current week-numbering year. - IYY: Last 3 digits of ISO 8601 week-numbering year. Behaves similarly to YYY in that for datetime to string conversion, prefix digit is obtained from current week-numbering year and can accept 1 or 2-digit input. - IY: Last 2 digits of ISO 8601 week-numbering year. Behaves similarly to YY in that for datetime to string conversion, prefix digits are obtained from current week-numbering year and can accept 1-digit input. - I:Last digit of ISO 8601 week-numbering year. Behaves similarly to Y in that for datetime to string conversion, prefix digits are obtained from current week-numbering year. - IW: ISO 8601 week of year (1-53). Begins on the Monday closest to January 1 of the year. For string to datetime conversion, if the input ISO 8601 week does not exist in the input year, an error will be thrown. Note that IW is different from the other week-related tokens WW and W (implemented in IMPALA-8705). With WW and W weeks start with the first day of the year/month. ISO 8601 weeks on the other hand always start with Monday. - ID: ISO 8601 day of week (1-7). 1 means Monday and 7 means Sunday. When doing string to datetime conversion, the ISO 8601 week-based tokens are meant to be used together and not mixed with other ISO SQL date tokens. E.g. '-IW-ID' is an invalid format string. The only exceptions are the day name tokens (DAY and DY) which may be used instead of ID with the rest of the ISO 8601 week-based date tokens. E.g. 'IYYY-IW-DAY' is a valid format string. Change-Id: I89a8c1b98742391cb7b331840d216558dbca362b --- M be/src/exprs/date-functions-ir.cc M be/src/runtime/date-parse-util.cc M be/src/runtime/date-test.cc M be/src/runtime/date-value.cc M be/src/runtime/date-value.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/timestamp-parse-util.cc M be/src/runtime/timestamp-parse-util.h M tests/query_test/test_cast_with_format.py 13 files changed, 861 insertions(+), 151 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/14852/3 -- To view, visit http://gerrit.cloudera.org:8080/14852 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I89a8c1b98742391cb7b331840d216558dbca362b Gerrit-Change-Number: 14852 Gerrit-PatchSet: 3 Gerrit-Owner: Attila Jeges Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4
Gabor Kaszab has posted comments on this change. (
http://gerrit.cloudera.org:8080/14852 )
Change subject: IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4
..
Patch Set 2:
(33 comments)
Thanks for taking care of this, Attila! I did a round on the patch except the
tests.
http://gerrit.cloudera.org:8080/#/c/14852/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14852/2//COMMIT_MSG@9
PS2, Line 9: week-based
I'd say something like "ISO 8601 week-numbering" tokens or such here and below.
http://gerrit.cloudera.org:8080/#/c/14852/2//COMMIT_MSG@10
PS2, Line 10: IMPALA-8705
It would also be worth mentioning that IMPALA-8705 also has week-related tokens
but there the week starts from the 1st of the year/month and these new tokens
start from the first Monday instead.
http://gerrit.cloudera.org:8080/#/c/14852/2//COMMIT_MSG@40
PS2, Line 40: 1-52 or 1-53
I think 1-53 is enough here. E.g. For day of year we also say it's 1-366 and
not 1-365 or 1-366
http://gerrit.cloudera.org:8080/#/c/14852/2//COMMIT_MSG@49
PS2, Line 49: When doing string to datetime conversion, the week-based tokens
: are meant to be used together and not mixed with other ISO SQL
: date tokens.
Could you also add an example here?
http://gerrit.cloudera.org:8080/#/c/14852/2/be/src/runtime/date-value.h
File be/src/runtime/date-value.h:
http://gerrit.cloudera.org:8080/#/c/14852/2/be/src/runtime/date-value.h@115
PS2, Line 115: /// 'week_numbering_year', 'week_of_year' and 'week_day'.
This line is probably not needed as you mention all of these in the following
rows.
http://gerrit.cloudera.org:8080/#/c/14852/2/be/src/runtime/date-value.cc
File be/src/runtime/date-value.cc:
http://gerrit.cloudera.org:8080/#/c/14852/2/be/src/runtime/date-value.cc@103
PS2, Line 103: first week of 'year'
Please clarify which week-numbering this statement is based on. Same for
GetLastSunday().
http://gerrit.cloudera.org:8080/#/c/14852/2/be/src/runtime/date-value.cc@110
PS2, Line 110: GetFirstMonday
GetFirstMondayOfYear() would be more verbose in my opinion.
http://gerrit.cloudera.org:8080/#/c/14852/2/be/src/runtime/date-value.cc@110
PS2, Line 110: GetFirstMonday
Is there a reason you didn't put this and GetLastSunday() into DateValue class?
http://gerrit.cloudera.org:8080/#/c/14852/2/be/src/runtime/date-value.cc@112
PS2, Line 112: cctz::civil_day first_monday
unused variable
http://gerrit.cloudera.org:8080/#/c/14852/2/be/src/runtime/date-value.cc@115
PS2, Line 115: cctz::next_weekday(jan1, cctz::weekday::monday) - 7
Would it work to say prev_weekday(jan1, cctz::weekday::monday)?
http://gerrit.cloudera.org:8080/#/c/14852/2/be/src/runtime/date-value.cc@116
PS2, Line 116: else {
: // Get the next Monday.
: return cctz::next_weekday(jan1, cctz::weekday::monday);
: }
No need for the else branch just simply return as you do in L118.
http://gerrit.cloudera.org:8080/#/c/14852/2/be/src/runtime/date-value.cc@129
PS2, Line 129: GetLastSunday
GetLastSundayOfYear() would be more verbose in my opinion.
http://gerrit.cloudera.org:8080/#/c/14852/2/be/src/runtime/date-value.cc@133
PS2, Line 133: cctz::prev_weekday(dec31, cctz::weekday::sunday) + 7
next_weekday(.. sunday)?
http://gerrit.cloudera.org:8080/#/c/14852/2/be/src/runtime/date-value.cc@134
PS2, Line 134: else {
: // Get the previous Sunday.
: return cctz::prev_weekday(dec31, cctz::weekday::sunday);
: }
Again, no need for the else branch.
http://gerrit.cloudera.org:8080/#/c/14852/2/be/src/runtime/date-value.cc@293
PS2, Line 293: DCHECK(today < first_monday);
: DCHECK(today.year() > MIN_YEAR && today.year() <= MAX_YEAR);
: return today.year() - 1;
no need to put this into an else branch
http://gerrit.cloudera.org:8080/#/c/14852/2/be/src/runtime/date-value.cc@299
PS2, Line 299: week_numbering_year
simply 'year' would be enough if the function name shows that the inputs are
ISO8601 week numbering format.
http://gerrit.cloudera.org:8080/#/c/14852/2/be/src/runtime/date-value.cc@299
PS2, Line 299: CreateFromIso8601WeekBasedDateVals
nit: Do you think moving this function closer to the constructor makes sense?
http://gerrit.cloudera.org:8080/#/c/14852/2/be/src/runtime/date-value.cc@299
PS2, Line 299: CreateFromIso8601WeekBasedDateVals
Additionally, I'm wondering if renaming this to a more constructor-like would
help. like DateValueFromIso8601WeekNumbering().
http://gerrit.cloudera.org:8080/#/c/14852/2/be/src/runtime/date-value.cc@300
PS2, Line 300: week_day
nit: if the second param is 'week_of_year' this might be 'day_of_week'
http://gerrit.cloudera.org:8080/#/c/14852/2/be/src/runtime/datetime-iso-sql-format-parser.cc
File be/src/runtime/datetime-iso-sql-format-parser.cc:
[Impala-ASF-CR] IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4
Attila Jeges has removed Tim Armstrong from this change. ( http://gerrit.cloudera.org:8080/14852 ) Change subject: IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4 .. Removed reviewer Tim Armstrong. -- To view, visit http://gerrit.cloudera.org:8080/14852 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: I89a8c1b98742391cb7b331840d216558dbca362b Gerrit-Change-Number: 14852 Gerrit-PatchSet: 2 Gerrit-Owner: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14852 ) Change subject: IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4 .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/5212/ : 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/14852 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I89a8c1b98742391cb7b331840d216558dbca362b Gerrit-Change-Number: 14852 Gerrit-PatchSet: 2 Gerrit-Owner: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 05 Dec 2019 17:18:50 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14852 ) Change subject: IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4 .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/5211/ : 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/14852 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I89a8c1b98742391cb7b331840d216558dbca362b Gerrit-Change-Number: 14852 Gerrit-PatchSet: 1 Gerrit-Owner: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 05 Dec 2019 17:15:27 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4
Attila Jeges has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/14852 ) Change subject: IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4 .. IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4 This patch adds week-based date format tokens on top of what was introduced in IMPALA-8703, IMPALA-8704 and IMPALA-8705. Week-based date tokens may be used both for datetime to string and string to datetime conversion. The week-based date tokens are as follows: - IYYY: 4-digit ISO 8601 week-numbering year. Week-numbering year is the year relating to the ISO 8601 week number (IW), which is the full week (Monday to Sunday) which contains January 4 of the Gregorian year. Behaves similarly to in that for datetime to string conversion, prefix digits for 1, 2, and 3-digit inputs are obtained from current week-numbering year. - IYY: Last 3 digits of ISO 8601 week-numbering year. Behaves similarly to YYY in that for datetime to string conversion, prefix digit is obtained from current week-numbering year and can accept 1 or 2-digit input. - IY: Last 2 digits of ISO 8601 week-numbering year. Behaves similarly to YY in that for datetime to string conversion, prefix digits are obtained from current week-numbering year and can accept 1-digit input. - I:Last digit of ISO 8601 week-numbering year. Behaves similarly to Y in that for datetime to string conversion, prefix digits are obtained from current week-numbering year. - IW: ISO 8601 week of year (1-52 or 1-53). Begins on the Monday closest to January 1 of the year. For string to datetime conversion, if the input week does not exist in the input year, an error will be thrown. - ID: ISO 8601 day of week (1-7). 1 means Monday and 7 means Sunday. When doing string to datetime conversion, the week-based tokens are meant to be used together and not mixed with other ISO SQL date tokens. The only exceptions are the day name tokens (DAY and Dy) which may be used instead of ID with the rest of the week-based date tokens. Change-Id: I89a8c1b98742391cb7b331840d216558dbca362b --- M be/src/exprs/date-functions-ir.cc M be/src/runtime/date-parse-util.cc M be/src/runtime/date-test.cc M be/src/runtime/date-value.cc M be/src/runtime/date-value.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/timestamp-parse-util.cc M be/src/runtime/timestamp-parse-util.h M tests/query_test/test_cast_with_format.py 13 files changed, 847 insertions(+), 136 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/14852/2 -- To view, visit http://gerrit.cloudera.org:8080/14852 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I89a8c1b98742391cb7b331840d216558dbca362b Gerrit-Change-Number: 14852 Gerrit-PatchSet: 2 Gerrit-Owner: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14852 ) Change subject: IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4 .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/14852/1/tests/query_test/test_cast_with_format.py File tests/query_test/test_cast_with_format.py: http://gerrit.cloudera.org:8080/#/c/14852/1/tests/query_test/test_cast_with_format.py@1438 PS1, Line 1438: d flake8: E303 too many blank lines (2) http://gerrit.cloudera.org:8080/#/c/14852/1/tests/query_test/test_cast_with_format.py@1548 PS1, Line 1548: \ flake8: E502 the backslash is redundant between brackets -- To view, visit http://gerrit.cloudera.org:8080/14852 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I89a8c1b98742391cb7b331840d216558dbca362b Gerrit-Change-Number: 14852 Gerrit-PatchSet: 1 Gerrit-Owner: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 05 Dec 2019 16:46:42 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4
Attila Jeges has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14852 Change subject: IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4 .. IMPALA-8706: ISO:SQL:2016 datetime patterns - Milestone 4 This patch adds week-based date format tokens on top of what was introduced in IMPALA-8703, IMPALA-8704 and IMPALA-8705. Week-based date tokens may be used both for datetime to string and string to datetime conversion. The week-based date tokens are as follows: - IYYY: 4-digit ISO 8601 week-numbering year. Week-numbering year is the year relating to the ISO 8601 week number (IW), which is the full week (Monday to Sunday) which contains January 4 of the Gregorian year. Behaves similarly to in that for datetime to string conversion, prefix digits for 1, 2, and 3-digit inputs are obtained from current week-numbering year. - IYY: Last 3 digits of ISO 8601 week-numbering year. Behaves similarly to YYY in that for datetime to string conversion, prefix digit is obtained from current week-numbering year and can accept 1 or 2-digit input. - IY: Last 2 digits of ISO 8601 week-numbering year. Behaves similarly to YY in that for datetime to string conversion, prefix digits are obtained from current week-numbering year and can accept 1-digit input. - I:Last digit of ISO 8601 week-numbering year. Behaves similarly to Y in that for datetime to string conversion, prefix digits are obtained from current week-numbering year. - IW: ISO 8601 week of year (1-52 or 1-53). Begins on the Monday closest to January 1 of the year. For string to datetime conversion, if the input week does not exist in the input year, an error will be thrown. - ID: ISO 8601 day of week (1-7). 1 means Monday and 7 means Sunday. When doing string to datetime conversion, the week-based tokens are meant to be used together and not mixed with other ISO SQL date tokens. The only exceptions are the day name tokens (DAY and Dy) which may be used instead of ID with the rest of the week-based date tokens. Change-Id: I89a8c1b98742391cb7b331840d216558dbca362b --- M be/src/exprs/date-functions-ir.cc M be/src/runtime/date-parse-util.cc M be/src/runtime/date-test.cc M be/src/runtime/date-value.cc M be/src/runtime/date-value.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/timestamp-parse-util.cc M be/src/runtime/timestamp-parse-util.h M tests/query_test/test_cast_with_format.py 13 files changed, 849 insertions(+), 136 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/14852/1 -- To view, visit http://gerrit.cloudera.org:8080/14852 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I89a8c1b98742391cb7b331840d216558dbca362b Gerrit-Change-Number: 14852 Gerrit-PatchSet: 1 Gerrit-Owner: Attila Jeges
