zabetak commented on code in PR #5421:
URL: https://github.com/apache/hive/pull/5421#discussion_r1750232742
##########
common/src/test/org/apache/hive/common/util/TestDateParser.java:
##########
@@ -45,37 +45,29 @@ void checkInvalidCase(String strValue) {
@Test
public void testValidCases() throws Exception {
- checkValidCase("1945-12-31", Date.valueOf("1945-12-31"));
- checkValidCase("1946-01-01", Date.valueOf("1946-01-01"));
- checkValidCase("2001-11-12", Date.valueOf("2001-11-12"));
- checkValidCase("0004-05-06", Date.valueOf("0004-05-06"));
- checkValidCase("1678-09-10", Date.valueOf("1678-09-10"));
- checkValidCase("9999-10-11", Date.valueOf("9999-10-11"));
+ checkValidCase("1945-12-31", Date.of(1945,12,31));
+ checkValidCase("1946-01-01", Date.of(1946,1,1));
+ checkValidCase("2001-11-12", Date.of(2001,11,12));
+ checkValidCase("0004-05-06", Date.of(4,5,6));
+ checkValidCase("1678-09-10", Date.of(1678,9,10));
+ checkValidCase("9999-10-11", Date.of(9999,10,11));
// Timestamp strings should parse ok
- checkValidCase("2001-11-12 01:02:03", Date.valueOf("2001-11-12"));
+ checkValidCase("2001-11-12 01:02:03", Date.of(2001,11,12));
// Leading spaces
- checkValidCase(" 1946-01-01", Date.valueOf("1946-01-01"));
- checkValidCase(" 2001-11-12 01:02:03", Date.valueOf("2001-11-12"));
+ checkValidCase(" 1946-01-01", Date.of(1946,01,01));
+ checkValidCase(" 2001-11-12 01:02:03", Date.of(2001,11,12));
}
@Test
public void testParseDateFromTimestampWithCommonTimeDelimiter() {
- for (String d : new String[] { "T", " ", "-", ".", "_", "" }) {
+ for (String d : new String[] { "T", " ", "-", ".", "_" }) {
Review Comment:
It seems that with the proposed changes will not be able to extract a date
from the following timestamp `2023-08-0301:02:03` while the leading part
(`2023-08-03`) seems to be a completely valid date.
##########
common/src/test/org/apache/hive/common/util/TestDateParser.java:
##########
@@ -45,19 +45,19 @@ void checkInvalidCase(String strValue) {
@Test
public void testValidCases() throws Exception {
- checkValidCase("1945-12-31", Date.valueOf("1945-12-31"));
- checkValidCase("1946-01-01", Date.valueOf("1946-01-01"));
- checkValidCase("2001-11-12", Date.valueOf("2001-11-12"));
- checkValidCase("0004-05-06", Date.valueOf("0004-05-06"));
- checkValidCase("1678-09-10", Date.valueOf("1678-09-10"));
- checkValidCase("9999-10-11", Date.valueOf("9999-10-11"));
+ checkValidCase("1945-12-31", Date.of(1945,12,31));
+ checkValidCase("1946-01-01", Date.of(1946,1,1));
+ checkValidCase("2001-11-12", Date.of(2001,11,12));
+ checkValidCase("0004-05-06", Date.of(4,5,6));
+ checkValidCase("1678-09-10", Date.of(1678,9,10));
+ checkValidCase("9999-10-11", Date.of(9999,10,11));
Review Comment:
Using `Date.of` instead of `Date.valueOf` definitely makes sense here so we
should get these changes in no matter the outcome of the rest of this PR. If
you isolate these under another PR I will merge it ASAP.
##########
common/src/java/org/apache/hadoop/hive/common/type/Date.java:
##########
@@ -84,7 +84,7 @@ public class Date implements Comparable<Date> {
private static final LocalDate EPOCH = LocalDate.of(1970, 1, 1);
private static final DateTimeFormatter PARSE_FORMATTER =
- new DateTimeFormatterBuilder().appendValue(YEAR, 1, 10,
SignStyle.NORMAL).appendLiteral('-')
+ new DateTimeFormatterBuilder().appendValue(YEAR, 1, 4,
SignStyle.NORMAL).appendLiteral('-')
Review Comment:
The main motivation behind this change is to make dates like `08-08-2023`
invalid (null). Can't we achieve that by setting `minWidth` to 4?
##########
common/src/java/org/apache/hadoop/hive/common/type/Date.java:
##########
@@ -177,13 +177,22 @@ public void setTimeInMillis(long epochMilli) {
* @throws NullPointerException if {@code text} is null
*/
public static Date valueOf(final String text) {
Review Comment:
Is the string '06-07-08' valid before/after the changes?
##########
common/src/test/org/apache/hive/common/util/TestDateParser.java:
##########
@@ -86,5 +78,8 @@ public void testInvalidCases() throws Exception {
checkInvalidCase("0000-00-00");
checkInvalidCase("2001-13-12");
checkInvalidCase("2001-11-31");
+ checkInvalidCase("19999-10-11");
Review Comment:
Hive does not [officially support
dates](https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=82706456#LanguageManualTypes-date)
greater than `9999-12-31`. However, the code accepts dates that exceed this
range since 2018 so not sure if it is a good idea to forbid them now.
##########
common/src/java/org/apache/hadoop/hive/common/type/Date.java:
##########
@@ -84,7 +84,7 @@ public class Date implements Comparable<Date> {
private static final LocalDate EPOCH = LocalDate.of(1970, 1, 1);
private static final DateTimeFormatter PARSE_FORMATTER =
- new DateTimeFormatterBuilder().appendValue(YEAR, 1, 10,
SignStyle.NORMAL).appendLiteral('-')
+ new DateTimeFormatterBuilder().appendValue(YEAR, 1, 4,
SignStyle.NORMAL).appendLiteral('-')
Review Comment:
Do we absolutely need to change/reduce maxWidth?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org
For additional commands, e-mail: gitbox-h...@hive.apache.org
