[GitHub] [calcite] thomasrebele commented on a diff in pull request #3394: [CALCITE-5914] Cache compiled regular expressions in SQL function runtime

["thomasrebele (via GitHub)" <gi...@apache.org>]

thomasrebele commented on code in PR #3394:
URL: https://github.com/apache/calcite/pull/3394#discussion_r1305491467


##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##########
@@ -1195,6 +1220,10 @@ private static <E> boolean allSame(List<E> list) {
     return true;
   }
 
+  private static boolean isStatic(Method method) {
+    return (method.getModifiers() & Modifier.STATIC) != 0;
+  }

Review Comment:
   A call to `java.lang.reflect.Modifier.isStatic(int)` could avoid the bit-operations. The function could be defined as `isStatic(java.lang.reflect.Member)` and put it into a helper to make it available to other classes as well (e.g., AggregateFunctionImpl).



##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -3160,97 +3293,176 @@ public static String timeWithLocalTimeZoneToString(int v, TimeZone timeZone) {
         .toString();
   }
 
-  private static String internalFormatDatetime(String fmtString, java.util.Date date) {
-    StringBuilder sb = new StringBuilder();
-    List<FormatElement> elements = FormatModels.BIG_QUERY.parse(fmtString);
-    elements.forEach(ele -> ele.format(sb, date));
-    return sb.toString();
-  }
+  /** State for {@code FORMAT_DATE}, {@code FORMAT_TIMESTAMP},
+   * {@code FORMAT_DATETIME}, {@code FORMAT_TIME}, {@code TO_CHAR} functions. */
+  @Deterministic
+  public static class DateFormatFunction {
+    /** Work space for various functions. Clear it before you use it. */
+    final StringBuilder sb = new StringBuilder();
+
+    /** Cache key. */
+    private static class Key extends MapEntry<FormatModel, String> {
+      Key(FormatModel formatModel, String format) {
+        super(formatModel, format);
+      }
+    }
 
-  public static String formatTimestamp(DataContext ctx, String fmtString, long timestamp) {
-    return internalFormatDatetime(fmtString, internalToTimestamp(timestamp));
-  }
+    private final LoadingCache<Key, List<FormatElement>> formatCache =
+        CacheBuilder.newBuilder()
+            .maximumSize(FUNCTION_LEVEL_CACHE_MAX_SIZE.value())
+            .build(CacheLoader.from(key -> key.t.parseNoCache(key.u)));
 
-  public static String toChar(long timestamp, String pattern) {
-    List<FormatElement> elements = FormatModels.POSTGRESQL.parse(pattern);
-    StringBuilder sb = new StringBuilder();
-    elements.forEach(ele ->  ele.format(sb, internalToTimestamp(timestamp)));
-    return sb.toString().trim();
-  }
+    /** Given a format string and a format model, calls an action with the
+     * list of elements obtained by parsing that format string. */
+    protected final void withElements(FormatModel formatModel, String format,
+        Consumer<List<FormatElement>> consumer) {
+      List<FormatElement> elements =
+          formatCache.getUnchecked(new Key(formatModel, format));
+      consumer.accept(elements);
+    }
 
-  public static String formatDate(DataContext ctx, String fmtString, int date) {
-    return internalFormatDatetime(fmtString, internalToDate(date));
-  }
+    private String internalFormatDatetime(String fmtString,
+        java.util.Date date) {
+      sb.setLength(0);
+      withElements(FormatModels.BIG_QUERY, fmtString, elements ->
+          elements.forEach(element -> element.format(sb, date)));
+      return sb.toString();
+    }
 
-  public static String formatTime(DataContext ctx, String fmtString, int time) {
-    return internalFormatDatetime(fmtString, internalToTime(time));
-  }
+    public String formatTimestamp(DataContext ctx, String fmtString,
+        long timestamp) {
+      return internalFormatDatetime(fmtString, internalToTimestamp(timestamp));
+    }
 
-  private static String parseDatetimePattern(String fmtString) {
-    StringBuilder sb = new StringBuilder();
-    List<FormatElement> elements = FormatModels.BIG_QUERY.parse(fmtString);
-    elements.forEach(ele -> ele.toPattern(sb));
-    return sb.toString();
-  }
+    public String toChar(long timestamp, String pattern) {
+      final Timestamp sqlTimestamp = internalToTimestamp(timestamp);
+      sb.setLength(0);
+      withElements(FormatModels.POSTGRESQL, pattern, elements ->
+          elements.forEach(element -> element.format(sb, sqlTimestamp)));
+      return sb.toString().trim();
+    }
 
-  private static long internalParseDatetime(String fmtString, String datetime) {
-    return internalParseDatetime(fmtString, datetime,
-        DateTimeUtils.DEFAULT_ZONE);
-  }
-
-  private static long internalParseDatetime(String fmt, String datetime,
-      TimeZone tz) {
-    final String javaFmt = parseDatetimePattern(fmt);
-    // TODO: make Locale configurable. ENGLISH set for weekday parsing (e.g.
-    // Thursday, Friday).
-    final DateFormat parser = new SimpleDateFormat(javaFmt, Locale.ENGLISH);
-    final ParsePosition pos = new ParsePosition(0);
-    parser.setLenient(false);
-    parser.setCalendar(Calendar.getInstance(tz, Locale.ROOT));
-    Date parsed = parser.parse(datetime, pos);
-    // Throw if either the parse was unsuccessful, or the format string did not
-    // contain enough elements to parse the datetime string completely.
-    if (pos.getErrorIndex() >= 0 || pos.getIndex() != datetime.length()) {
-      SQLException e =
-          new SQLException(
-              String.format(Locale.ROOT,
-                  "Invalid format: '%s' for datetime string: '%s'.", fmt,
-                  datetime));
-      throw Util.toUnchecked(e);
+    public String formatDate(DataContext ctx, String fmtString, int date) {
+      return internalFormatDatetime(fmtString, internalToDate(date));
     }
-    // Suppress the Errorprone warning "[JavaUtilDate] Date has a bad API that
-    // leads to bugs; prefer java.time.Instant or LocalDate" because we know
-    // what we're doing.
-    @SuppressWarnings("JavaUtilDate")
-    final long millisSinceEpoch = parsed.getTime();
-    return millisSinceEpoch;
-  }
 
-  public static int parseDate(String fmtString, String date) {
-    final long millisSinceEpoch = internalParseDatetime(fmtString, date);
-    return toInt(new java.sql.Date(millisSinceEpoch));
+    public String formatTime(DataContext ctx, String fmtString, int time) {
+      return internalFormatDatetime(fmtString, internalToTime(time));
+    }
   }
 
-  public static long parseDatetime(String fmtString, String datetime) {
-    final long millisSinceEpoch = internalParseDatetime(fmtString, datetime);
-    return toLong(new java.sql.Timestamp(millisSinceEpoch));
-  }
+  /** State for {@code PARSE_DATE}, {@code PARSE_TIMESTAMP},
+   * {@code PARSE_DATETIME}, {@code PARSE_TIME} functions. */
+  @Deterministic
+  public static class DateParseFunction {
+    /** Use a {@link DateFormatFunction} for its cache of parsed
+     * format strings. */
+    final DateFormatFunction f = new DateFormatFunction();
+
+    private final LoadingCache<Key, DateFormat> cache =
+        CacheBuilder.newBuilder()
+            .maximumSize(FUNCTION_LEVEL_CACHE_MAX_SIZE.value())
+            .build(CacheLoader.from(key -> key.toDateFormat(f)));
+
+    private <T> T withParser(String fmt, String timeZone,
+        Function<DateFormat, T> action) {
+      final DateFormat dateFormat = cache.getUnchecked(new Key(fmt, timeZone));

Review Comment:
   I'm just wondering whether this method can be called by several threads, as DateFormat is not thread-safe.



-- 
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: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org