Jackie-Jiang commented on a change in pull request #8118:
URL: https://github.com/apache/pinot/pull/8118#discussion_r799918947
##########
File path:
pinot-spi/src/main/java/org/apache/pinot/spi/utils/TimestampUtils.java
##########
@@ -19,19 +19,27 @@
package org.apache.pinot.spi.utils;
import java.sql.Timestamp;
+import java.text.ParseException;
+import java.text.SimpleDateFormat;
public class TimestampUtils {
+
+ private static final String[] SDF_FORMATS = {
Review comment:
We can pre-compile these formats to `SimpleDateFormat` to avoid
compiling them for each method call
##########
File path:
pinot-spi/src/main/java/org/apache/pinot/spi/utils/TimestampUtils.java
##########
@@ -60,8 +67,30 @@ public static long toMillisSinceEpoch(String
timestampString) {
try {
return Long.parseLong(timestampString);
} catch (Exception e1) {
- throw new IllegalArgumentException(String.format("Invalid timestamp:
'%s'", timestampString));
+ try {
+ return dateTimeToTimestamp(timestampString).getTime();
+ } catch (Exception e2) {
+ throw new IllegalArgumentException(String.format("Invalid timestamp:
'%s'", timestampString));
+ }
+ }
+ }
+ }
+
+ /**
+ * Infers a date time format from a valid {@link Timestamp} string.
+ */
+ private static Timestamp dateTimeToTimestamp(String timestampString) {
+ Timestamp result = null;
+ for (String parse : SDF_FORMATS) {
+ SimpleDateFormat sdf = new SimpleDateFormat(parse);
+ try {
+ result = Timestamp.from(sdf.parse(timestampString).toInstant());
+ } catch (ParseException e) {
}
}
+ if (result == null) {
+ throw new IllegalArgumentException(String.format("Date format not
recognized: '%s'", timestampString));
+ }
+ return result;
}
-}
+}
Review comment:
(minor) revert the empty line removal
##########
File path:
pinot-common/src/test/java/org/apache/pinot/common/utils/PinotDataTypeTest.java
##########
@@ -358,4 +362,4 @@ public String toString() {
return _value;
}
}
-}
+}
Review comment:
(minor) revert the empty line removal
##########
File path:
pinot-spi/src/main/java/org/apache/pinot/spi/utils/TimestampUtils.java
##########
@@ -19,19 +19,27 @@
package org.apache.pinot.spi.utils;
import java.sql.Timestamp;
+import java.text.ParseException;
+import java.text.SimpleDateFormat;
public class TimestampUtils {
+
+ private static final String[] SDF_FORMATS = {
+ "yyyy-MM-dd'T'HH:mm:ss'Z'", "yyyy-MM-dd'T'HH:mm:ssZ",
+ "yyyy-MM-dd'T'HH:mm:ss", "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'",
+ "yyyy-MM-dd'T'HH:mm:ss.SSSZ", "yyyy-MM-dd HH:mm:ss",
Review comment:
No need to add `yyyy-MM-dd HH:mm:ss` as it is the java timestamp format
##########
File path:
pinot-spi/src/main/java/org/apache/pinot/spi/utils/TimestampUtils.java
##########
@@ -19,19 +19,27 @@
package org.apache.pinot.spi.utils;
import java.sql.Timestamp;
+import java.text.ParseException;
+import java.text.SimpleDateFormat;
public class TimestampUtils {
+
+ private static final String[] SDF_FORMATS = {
+ "yyyy-MM-dd'T'HH:mm:ss'Z'", "yyyy-MM-dd'T'HH:mm:ssZ",
+ "yyyy-MM-dd'T'HH:mm:ss", "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'",
+ "yyyy-MM-dd'T'HH:mm:ss.SSSZ", "yyyy-MM-dd HH:mm:ss",
Review comment:
`yyyy-MM-dd'T'HH:mm:ss.SSS` is missing
##########
File path:
pinot-spi/src/main/java/org/apache/pinot/spi/utils/TimestampUtils.java
##########
@@ -19,19 +19,27 @@
package org.apache.pinot.spi.utils;
import java.sql.Timestamp;
+import java.text.ParseException;
+import java.text.SimpleDateFormat;
public class TimestampUtils {
+
+ private static final String[] SDF_FORMATS = {
+ "yyyy-MM-dd'T'HH:mm:ss'Z'", "yyyy-MM-dd'T'HH:mm:ssZ",
+ "yyyy-MM-dd'T'HH:mm:ss", "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'",
+ "yyyy-MM-dd'T'HH:mm:ss.SSSZ", "yyyy-MM-dd HH:mm:ss",
+ "MM/dd/yyyy HH:mm:ss", "MM/dd/yyyy'T'HH:mm:ss.SSS'Z'",
Review comment:
Not sure if these are commonly used timestamp format
##########
File path:
pinot-spi/src/main/java/org/apache/pinot/spi/utils/TimestampUtils.java
##########
@@ -19,19 +19,27 @@
package org.apache.pinot.spi.utils;
import java.sql.Timestamp;
+import java.text.ParseException;
+import java.text.SimpleDateFormat;
public class TimestampUtils {
+
+ private static final String[] SDF_FORMATS = {
+ "yyyy-MM-dd'T'HH:mm:ss'Z'", "yyyy-MM-dd'T'HH:mm:ssZ",
+ "yyyy-MM-dd'T'HH:mm:ss", "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'",
+ "yyyy-MM-dd'T'HH:mm:ss.SSSZ", "yyyy-MM-dd HH:mm:ss",
+ "MM/dd/yyyy HH:mm:ss", "MM/dd/yyyy'T'HH:mm:ss.SSS'Z'",
Review comment:
`yyyy-MM-dd` seems a commonly used `date` format
##########
File path:
pinot-spi/src/main/java/org/apache/pinot/spi/utils/TimestampUtils.java
##########
@@ -19,19 +19,27 @@
package org.apache.pinot.spi.utils;
import java.sql.Timestamp;
+import java.text.ParseException;
+import java.text.SimpleDateFormat;
public class TimestampUtils {
+
+ private static final String[] SDF_FORMATS = {
+ "yyyy-MM-dd'T'HH:mm:ss'Z'", "yyyy-MM-dd'T'HH:mm:ssZ",
+ "yyyy-MM-dd'T'HH:mm:ss", "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'",
+ "yyyy-MM-dd'T'HH:mm:ss.SSSZ", "yyyy-MM-dd HH:mm:ss",
+ "MM/dd/yyyy HH:mm:ss", "MM/dd/yyyy'T'HH:mm:ss.SSS'Z'",
+ "MM/dd/yyyy'T'HH:mm:ss.SSSZ", "MM/dd/yyyy'T'HH:mm:ss.SSS",
+ "MM/dd/yyyy'T'HH:mm:ssZ", "MM/dd/yyyy'T'HH:mm:ss",
+ "yyyy:MM:dd HH:mm:ss", "yyyyMMdd", "MM/dd/yyyy"
Review comment:
`yyyyMMdd` cannot be supported because it will be parsed as `long`
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
