From 3de02653dfdbc9b4638dece14735ab27560c26ca Mon Sep 17 00:00:00 2001 From: Yash Mayya Date: Mon, 30 Dec 2024 14:02:46 +0700 Subject: [PATCH] Use DateTimeFormatter instead of SimpleDateFormat to resolve thread safety issue in the JDBC client (#14723) Use DateTimeFormatter instead of SimpleDateFormat to resolve thread safety issue in the JDBC client --- .../pinot/client/utils/DateTimeUtils.java | 60 +++++++------- .../pinot/client/PinotResultSetTest.java | 79 ++++++++++++++++++- 2 files changed, 111 insertions(+), 28 deletions(-) diff --git a/pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/utils/DateTimeUtils.java b/pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/utils/DateTimeUtils.java index 3ca537b518fe..87c53f831a5e 100644 --- a/pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/utils/DateTimeUtils.java +++ b/pinot-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/pinot-clients/pinot-jdbc-client/src/test/java/org/apache/pinot/client/PinotResultSetTest.java b/pinot-clients/pinot-jdbc-client/src/test/java/org/apache/pinot/client/PinotResultSetTest.java index 255d14d47087..c62a9b9e5465 100644 --- a/pinot-clients/pinot-jdbc-client/src/test/java/org/apache/pinot/client/PinotResultSetTest.java +++ b/pinot-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 = 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 = 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 = 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);