You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Attila Jeges (Code Review)" <ge...@cloudera.org> on 2017/03/31 13:20:52 UTC
[Impala-ASF-CR] IMPALA-3381: Support AM/PM marker in date and time format strings
Attila Jeges has uploaded a new change for review.
http://gerrit.cloudera.org:8080/6523
Change subject: IMPALA-3381: Support AM/PM marker in date and time format strings
......................................................................
IMPALA-3381: Support AM/PM marker in date and time format strings
This change adds AM/PM marker to format strings used in 'to_timestamp'
'unix_timestamp' and 'from_unixtime' functions.
It uses 'a' for the AM/PM marker following the Hive impelentation (
which follows Java 'SimpleDateFormat' patterns). Similarly to Hive,
the 'a' pattern letter can be repeated any number of times in the
format string without affecting the corresponding presentation.
For example:
> select from_unixtime(
> unix_timestamp('2017-03-31 11:19:23 PM', 'yyyy-MM-dd HH:mm:ss a'),
> 'yyyy-MM-dd HH:mm:ss aaa');
2017-03-31 11:19:23 PM
Change-Id: I99794a3e152f1712c6c469bb266d23a81d19ca34
---
M be/src/exprs/expr-test.cc
M be/src/runtime/timestamp-parse-util.cc
M be/src/runtime/timestamp-parse-util.h
3 files changed, 137 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/23/6523/1
--
To view, visit http://gerrit.cloudera.org:8080/6523
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I99794a3e152f1712c6c469bb266d23a81d19ca34
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Attila Jeges <at...@cloudera.com>
[Impala-ASF-CR] IMPALA-3381: Support AM/PM marker in date and time format strings
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-3381: Support AM/PM marker in date and time format strings
......................................................................
Patch Set 2:
What did we decide here? Can we move this forward or should we abandon the change?
--
To view, visit http://gerrit.cloudera.org:8080/6523
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I99794a3e152f1712c6c469bb266d23a81d19ca34
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3381: Support AM/PM marker in date and time format strings
Posted by "Greg Rahn (Code Review)" <ge...@cloudera.org>.
Greg Rahn has posted comments on this change.
Change subject: IMPALA-3381: Support AM/PM marker in date and time format strings
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6523/1/be/src/runtime/timestamp-parse-util.cc
File be/src/runtime/timestamp-parse-util.cc:
Line 173: case 'a': tok_type = AM_PM_MARKER; dt_ctx->has_am_pm_marker = true; break;
> I decided to use "a" for the AM/PM marker because this is what Hive does.
The mask can not hard-code any value -- it is how to interpret the date/time string. If the input string contains no AM/PM string then the mask should not attempt to add it and it needs to be read as 24h time. For example if the value is '2000-01-01 05:00' one should not be able to use a mask to make this be interpreted as '2000-01-01 05:00 PM'.
--
To view, visit http://gerrit.cloudera.org:8080/6523
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I99794a3e152f1712c6c469bb266d23a81d19ca34
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3381: Support AM/PM marker in date and time format strings
Posted by "Attila Jeges (Code Review)" <ge...@cloudera.org>.
Attila Jeges has abandoned this change.
Change subject: IMPALA-3381: Support AM/PM marker in date and time format strings
......................................................................
Abandoned
--
To view, visit http://gerrit.cloudera.org:8080/6523
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: I99794a3e152f1712c6c469bb266d23a81d19ca34
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
[Impala-ASF-CR] IMPALA-3381: Support AM/PM marker in date and time format strings
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-3381: Support AM/PM marker in date and time format strings
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/6523/1/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
Line 5641: // AM/PM marker can be repeated and placed anywhere in the format string
Do we have a check that prevents having multiple separate am/pm markers?
http://gerrit.cloudera.org:8080/#/c/6523/1/be/src/runtime/timestamp-parse-util.cc
File be/src/runtime/timestamp-parse-util.cc:
Line 173: case 'a': tok_type = AM_PM_MARKER; dt_ctx->has_am_pm_marker = true; break;
Have you considered supporting 'am' and 'pm' as tokens, too, like Greg suggested in the JIRA?
Line 201: if (tok_len != 2) {
You could remove this if statement (and add 0 below).
PS1, Line 467: strncmp
You can use strncasecmp() and simplify the code.
--
To view, visit http://gerrit.cloudera.org:8080/6523
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I99794a3e152f1712c6c469bb266d23a81d19ca34
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3381: Support AM/PM marker in date and time format strings
Posted by "Attila Jeges (Code Review)" <ge...@cloudera.org>.
Attila Jeges has uploaded a new patch set (#2).
Change subject: IMPALA-3381: Support AM/PM marker in date and time format strings
......................................................................
IMPALA-3381: Support AM/PM marker in date and time format strings
This change adds AM/PM marker to format strings used in 'to_timestamp'
'unix_timestamp' and 'from_unixtime' functions.
It uses 'a' for the AM/PM marker following the Hive impelentation (
which follows Java 'SimpleDateFormat' patterns). Similarly to Hive,
the 'a' pattern letter can be repeated any number of times in the
format string without affecting the corresponding presentation.
For example:
> select from_unixtime(
> unix_timestamp('2017-03-31 11:19:23 PM', 'yyyy-MM-dd HH:mm:ss a'),
> 'yyyy-MM-dd HH:mm:ss aaa');
2017-03-31 11:19:23 PM
Change-Id: I99794a3e152f1712c6c469bb266d23a81d19ca34
---
M be/src/exprs/expr-test.cc
M be/src/runtime/timestamp-parse-util.cc
M be/src/runtime/timestamp-parse-util.h
3 files changed, 133 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/23/6523/2
--
To view, visit http://gerrit.cloudera.org:8080/6523
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I99794a3e152f1712c6c469bb266d23a81d19ca34
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-3381: Support AM/PM marker in date and time format strings
Posted by "Attila Jeges (Code Review)" <ge...@cloudera.org>.
Attila Jeges has posted comments on this change.
Change subject: IMPALA-3381: Support AM/PM marker in date and time format strings
......................................................................
Patch Set 2:
> What did we decide here? Can we move this forward or should we
> abandon the change?
Impala's implementation of pattern letters follows SimpleDateFormat but it also deviates from SimpleDateFormat in some aspects, which causes some discrepancies. No decision has been made about resolving these discrepancies. Since this is a low-priority issue, I'm abandoning it for now.
--
To view, visit http://gerrit.cloudera.org:8080/6523
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I99794a3e152f1712c6c469bb266d23a81d19ca34
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3381: Support AM/PM marker in date and time format strings
Posted by "Attila Jeges (Code Review)" <ge...@cloudera.org>.
Attila Jeges has posted comments on this change.
Change subject: IMPALA-3381: Support AM/PM marker in date and time format strings
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/6523/1/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
Line 5641: // AM/PM marker can be repeated and placed anywhere in the format string
> Do we have a check that prevents having multiple separate am/pm markers?
No, we don't. Other pattern letters can have multiple separate occurrences as well.
http://gerrit.cloudera.org:8080/#/c/6523/1/be/src/runtime/timestamp-parse-util.cc
File be/src/runtime/timestamp-parse-util.cc:
Line 173: case 'a': tok_type = AM_PM_MARKER; dt_ctx->has_am_pm_marker = true; break;
> Have you considered supporting 'am' and 'pm' as tokens, too, like Greg sugg
I decided to use "a" for the AM/PM marker because this is what Hive does.
Supporting both "a" and "am" at the same time is probably not a good idea as it would create ambiguity in format strings ("am" could mean "am/pm marker" OR "am/pm marker" + "minute in hour").
Line 201: if (tok_len != 2) {
> You could remove this if statement (and add 0 below).
Done
PS1, Line 467: strncmp
> You can use strncasecmp() and simplify the code.
Done
--
To view, visit http://gerrit.cloudera.org:8080/6523
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I99794a3e152f1712c6c469bb266d23a81d19ca34
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3381: Support AM/PM marker in date and time format strings
Posted by "Attila Jeges (Code Review)" <ge...@cloudera.org>.
Attila Jeges has uploaded a new patch set (#2).
Change subject: IMPALA-3381: Support AM/PM marker in date and time format strings
......................................................................
IMPALA-3381: Support AM/PM marker in date and time format strings
This change adds AM/PM marker to format strings used in 'to_timestamp'
'unix_timestamp' and 'from_unixtime' functions.
It uses 'a' for the AM/PM marker following the Hive impelentation (
which follows Java 'SimpleDateFormat' patterns). Similarly to Hive,
the 'a' pattern letter can be repeated any number of times in the
format string without affecting the corresponding presentation.
For example:
> select from_unixtime(
> unix_timestamp('2017-03-31 11:19:23 PM', 'yyyy-MM-dd HH:mm:ss a'),
> 'yyyy-MM-dd HH:mm:ss aaa');
2017-03-31 11:19:23 PM
Change-Id: I99794a3e152f1712c6c469bb266d23a81d19ca34
---
M be/src/exprs/expr-test.cc
M be/src/runtime/timestamp-parse-util.cc
M be/src/runtime/timestamp-parse-util.h
3 files changed, 133 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/23/6523/2
--
To view, visit http://gerrit.cloudera.org:8080/6523
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I99794a3e152f1712c6c469bb266d23a81d19ca34
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>