phoenix-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "James Taylor (JIRA)" <>
Subject [jira] [Commented] (PHOENIX-1142) Improve CsvBulkLoadTool to parse different Date formats
Date Mon, 02 Feb 2015 21:51:35 GMT


James Taylor commented on PHOENIX-1142:

Let's let [~gabriel.reid] review too. Here's some additional concerns:
- The SimpleDateFormat is not thread safe for parsing, so you can't store it in a local like
this in DateUtil (but maybe I'm missing something?):
+    private static DateTimeParser defaultDateTimeParser;
- There's a facility to set the date timezone and date format on a config property which might
occur at connection time. This code seems to disable that?
diff --git phoenix-core/src/main/java/org/apache/phoenix/parse/ phoenix-core/src/main/java/org/apache/phoenix/parse/
index 46bca63..ab1d76f 100644
--- phoenix-core/src/main/java/org/apache/phoenix/parse/
+++ phoenix-core/src/main/java/org/apache/phoenix/parse/
@@ -38,10 +38,10 @@ public class ToDateParseNode extends FunctionParseNode {
     public FunctionExpression create(List<Expression> children, StatementContext context)
throws SQLException {
-        Format dateParser;
+        DateUtil.DateTimeParser dateParser;
         String dateFormat = (String) ((LiteralExpression) children.get(1)).getValue();
         String timeZoneId = (String) ((LiteralExpression) children.get(2)).getValue();
-        TimeZone parserTimeZone = context.getDateFormatTimeZone();
+        TimeZone parserTimeZone = null;
         if (dateFormat == null) {
             dateFormat = context.getDateFormat();
- It appears that the default date parsing is being set as a static variable (JVM-wide), while
ConnectionQueryServicesImpl is a per cluster object. This doesn't seem to be correct. I think
we should consider where the DateTimeParser should live/be scoped to:
@@ -249,6 +238,8 @@ public class ConnectionQueryServicesImpl extends DelegateQueryServices
                 .expireAfterWrite(halfStatsUpdateFreq, TimeUnit.MILLISECONDS)
+        // Initialize DateUtil DefaultDatetimeParser
+        DateUtil.init(this.config);
- In general, we use the QueryServices.getProps() method to access property values rather
than go through Configuration. The reason is that we've seen this impact performance because
Configuration does locking while ReadOnlyProps does not. The locking is not necessary, because
the Configuration is not changing in-place.
- What will the impact be of changing the DATE_TIME_PARSER (now ISO_DATE_TIME_PARSER) on CSV
bulk loading? We append the ' ' literal because we use a YYYY-MM-DD HH:MM:SS.zzz format (which
isn't ISO standard, I believe, as ISO wants a 'T' instead).
+    private static final DateTimeFormatter ISO_DATE_TIME_PARSER = new DateTimeFormatterBuilder()
             .appendOptional(new DateTimeFormatterBuilder()
-                    .appendLiteral(' ')
+                    .appendLiteral(' ').toParser())
+            .appendOptional(new DateTimeFormatterBuilder()
+            .withZoneUTC()

> Improve CsvBulkLoadTool to parse different Date formats
> -------------------------------------------------------
>                 Key: PHOENIX-1142
>                 URL:
>             Project: Phoenix
>          Issue Type: Improvement
>            Reporter: David Kjerrumgaard
>            Assignee: Jeffrey Zhong
>         Attachments: PHOENIX-1142.patch
> Currently, the CsvBulkLoadTool uses a single 'default' format to parse dates from the
source CSV file. This can sometimes cause issues when the date fields don't match this default
> Would it be possible to add a method for specifying / overriding the 'default' date format?

This message was sent by Atlassian JIRA

View raw message