Use DateTimeFormatter instead of SimpleDateFormat to resolve thread s…

…afety issue in the JDBC client (#14723) Use DateTimeFormatter instead of SimpleDateFormat to resolve thread safety issue in the JDBC client
apache · Dec 30, 2024 · 3de0265 · 3de0265
1 parent 5e5cea9
commit 3de0265
Show file tree

Hide file tree

Showing 2 changed files with 111 additions and 28 deletions.
diff --git a/...-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/utils/DateTimeUtils.java b/...-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/utils/DateTimeUtils.java
@@ -21,8 +21,12 @@
 import java.sql.Date;
 import java.sql.Time;
 import java.sql.Timestamp;
-import java.text.ParseException;
-import java.text.SimpleDateFormat;
+import java.time.Instant;
+import java.time.LocalDate;
+import java.time.LocalDateTime;
+import java.time.LocalTime;
+import java.time.ZoneId;
+import java.time.format.DateTimeFormatter;
 import java.util.Calendar;
 
 
@@ -32,48 +36,50 @@ private DateTimeUtils() {
 
   private static final String TIMESTAMP_FORMAT_STR = "yyyy-MM-dd HH:mm:ss";
   private static final String DATE_FORMAT_STR = "yyyy-MM-dd";
-  private static final SimpleDateFormat DATE_FORMAT = new SimpleDateFormat(DATE_FORMAT_STR);
-  private static final SimpleDateFormat TIMESTAMP_FORMAT = new SimpleDateFormat(TIMESTAMP_FORMAT_STR);
+  private static final DateTimeFormatter DATE_FORMATTER = DateTimeFormatter.ofPattern(DATE_FORMAT_STR);
+  private static final DateTimeFormatter TIMESTAMP_FORMATTER = DateTimeFormatter.ofPattern(TIMESTAMP_FORMAT_STR);
 
-  public static Date getDateFromString(String value, Calendar cal)
-      throws ParseException {
-    DATE_FORMAT.setTimeZone(cal.getTimeZone());
-    java.util.Date date = DATE_FORMAT.parse(value);
-    Date sqlDate = new Date(date.getTime());
-    return sqlDate;
+  public static Date getDateFromString(String value, Calendar cal) {
+    // Parse the input string to a LocalDate
+    LocalDate localDate = LocalDate.parse(value, DATE_FORMATTER);
+
+    // Convert LocalDate to a java.sql.Date, using the Calendar's time zone
+    ZoneId zoneId = cal.getTimeZone().toZoneId();
+    return new Date(localDate.atStartOfDay(zoneId).toInstant().toEpochMilli());
   }
 
-  public static Time getTimeFromString(String value, Calendar cal)
-      throws ParseException {
-    TIMESTAMP_FORMAT.setTimeZone(cal.getTimeZone());
-    java.util.Date date = TIMESTAMP_FORMAT.parse(value);
-    Time sqlTime = new Time(date.getTime());
-    return sqlTime;
+  public static Time getTimeFromString(String value, Calendar cal) {
+    // Parse the input string to a LocalTime
+    LocalTime localTime = LocalTime.parse(value, TIMESTAMP_FORMATTER);
+
+    // Convert LocalTime to java.sql.Time, considering the Calendar's time zone
+    ZoneId zoneId = cal.getTimeZone().toZoneId();
+    return new Time(localTime.atDate(LocalDate.ofEpochDay(0)).atZone(zoneId).toInstant().toEpochMilli());
   }
 
-  public static Timestamp getTimestampFromString(String value, Calendar cal)
-      throws ParseException {
-    TIMESTAMP_FORMAT.setTimeZone(cal.getTimeZone());
-    java.util.Date date = TIMESTAMP_FORMAT.parse(value);
-    Timestamp sqlTime = new Timestamp(date.getTime());
-    return sqlTime;
+  public static Timestamp getTimestampFromString(String value, Calendar cal) {
+    // Parse the input string to a LocalDateTime
+    LocalDateTime localDateTime = LocalDateTime.parse(value, TIMESTAMP_FORMATTER);
+
+    // Convert LocalDateTime to java.sql.Timestamp, considering the Calendar's time zone
+    ZoneId zoneId = cal.getTimeZone().toZoneId();
+    return new Timestamp(localDateTime.atZone(zoneId).toInstant().toEpochMilli());
   }
 
   public static Timestamp getTimestampFromLong(Long value) {
-    Timestamp sqlTime = new Timestamp(value);
-    return sqlTime;
+    return new Timestamp(value);
   }
 
   public static String dateToString(Date date) {
-    return DATE_FORMAT.format(date.getTime());
+    return date.toLocalDate().format(DATE_FORMATTER);
   }
 
   public static String timeToString(Time time) {
-    return TIMESTAMP_FORMAT.format(time.getTime());
+    return TIMESTAMP_FORMATTER.format(Instant.ofEpochMilli(time.getTime()).atZone(ZoneId.systemDefault()));
   }
 
   public static String timeStampToString(Timestamp timestamp) {
-    return TIMESTAMP_FORMAT.format(timestamp.getTime());
+    return TIMESTAMP_FORMATTER.format(Instant.ofEpochMilli(timestamp.getTime()).atZone(ZoneId.systemDefault()));
   }
 
   public static long timeStampToLong(Timestamp timestamp) {

diff --git a/...t-clients/pinot-jdbc-client/src/test/java/org/apache/pinot/client/PinotResultSetTest.java b/...t-clients/pinot-jdbc-client/src/test/java/org/apache/pinot/client/PinotResultSetTest.java
@@ -26,6 +26,10 @@
 import java.util.Collections;
 import java.util.Date;
 import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicReference;
 import org.apache.commons.io.IOUtils;
 import org.apache.pinot.client.utils.DateTimeUtils;
 import org.apache.pinot.spi.utils.JsonUtils;
@@ -139,7 +143,7 @@ public void testFetchDates()
 
   @Test
   public void testFetchBigDecimals()
-    throws Exception {
+      throws Exception {
     ResultSetGroup resultSetGroup = getResultSet(TEST_RESULT_SET_RESOURCE);
     ResultSet resultSet = resultSetGroup.getResultSet(0);
     PinotResultSet pinotResultSet = new PinotResultSet(resultSet);
@@ -207,6 +211,79 @@ public void testGetCalculatedScale() {
     Assert.assertEquals(calculatedResult, 3);
   }
 
+  @Test
+  public void testDateFromStringConcurrent()
+      throws Throwable {
+    ExecutorService executorService = Executors.newFixedThreadPool(10);
+    AtomicReference<Throwable> throwable = new AtomicReference<>();
+    for (int i = 0; i < 10; i++) {
+      executorService.submit(() -> {
+        try {
+          Assert.assertEquals(DateTimeUtils.getDateFromString("2020-01-01", Calendar.getInstance()).toString(),
+              "2020-01-01");
+        } catch (Throwable t) {
+          throwable.set(t);
+        }
+      });
+    }
+
+    executorService.shutdown();
+    executorService.awaitTermination(1000, TimeUnit.MILLISECONDS);
+
+    if (throwable.get() != null) {
+      throw throwable.get();
+    }
+  }
+
+  @Test
+  public void testTimeFromStringConcurrent()
+      throws Throwable {
+    ExecutorService executorService = Executors.newFixedThreadPool(10);
+    AtomicReference<Throwable> throwable = new AtomicReference<>();
+    for (int i = 0; i < 10; i++) {
+      executorService.submit(() -> {
+        try {
+          Assert.assertEquals(DateTimeUtils.getTimeFromString("2020-01-01 12:00:00", Calendar.getInstance()).toString(),
+              "12:00:00");
+        } catch (Throwable t) {
+          throwable.set(t);
+        }
+      });
+    }
+
+    executorService.shutdown();
+    executorService.awaitTermination(1000, TimeUnit.MILLISECONDS);
+
+    if (throwable.get() != null) {
+      throw throwable.get();
+    }
+  }
+
+  @Test
+  public void testTimestampFromStringConcurrent()
+      throws Throwable {
+    ExecutorService executorService = Executors.newFixedThreadPool(10);
+    AtomicReference<Throwable> throwable = new AtomicReference<>();
+    for (int i = 0; i < 10; i++) {
+      executorService.submit(() -> {
+        try {
+          Assert.assertEquals(
+              DateTimeUtils.getTimestampFromString("2020-01-01 12:00:00", Calendar.getInstance()).toString(),
+              "2020-01-01 12:00:00.0");
+        } catch (Throwable t) {
+          throwable.set(t);
+        }
+      });
+    }
+
+    executorService.shutdown();
+    executorService.awaitTermination(1000, TimeUnit.MILLISECONDS);
+
+    if (throwable.get() != null) {
+      throw throwable.get();
+    }
+  }
+
   private ResultSetGroup getResultSet(String resourceName) {
     _dummyJsonTransport._resource = resourceName;
     Connection connection = ConnectionFactory.fromHostList(Collections.singletonList("dummy"), _dummyJsonTransport);