[SPARK-46623][CORE][MLLIB][SQL] Replace SimpleDateFormat with DateTimeFormatter #44616

beliefer · 2024-01-07T13:07:28Z

What changes were proposed in this pull request?

This PR propose to replace SimpleDateFormat with DateTimeFormatter.

Why are the changes needed?

According to the javadoc of SimpleDateFormat, it recommended to use DateTimeFormatter too.

In addition, DateTimeFormatter have better performance than SimpleDateFormat too.

Note: SimpleDateFormat and DateTimeFormatter are not completely compatible, for example, the formats supported by parse are not exactly the same.

Does this PR introduce any user-facing change?

'No'.

How was this patch tested?

GA and manual test.

Was this patch authored or co-authored using generative AI tooling?

'No'.

HyukjinKwon · 2024-01-08T00:03:33Z

cc @MaxGekk BTW, we would have to replace FastDateFormat too if possible.

beliefer · 2024-01-08T13:45:24Z

ping @dongjoon-hyun @srowen @LuciferYang

beliefer · 2024-01-10T03:50:40Z

cc @mridulm

mridulm

+CC @srowen as well

mridulm · 2024-01-16T02:18:44Z

core/src/main/scala/org/apache/spark/deploy/master/Master.scala

@@ -58,7 +59,10 @@ private[deploy] class Master(
  private val appIdPattern = conf.get(APP_ID_PATTERN)

  // For application IDs
-  private def createDateFormat = new SimpleDateFormat("yyyyMMddHHmmss", Locale.US)
+  private lazy val dateTimeFormatter =


nit: Does not need to be lazy and can be moved to object Master, same in other places as well.

mridulm · 2024-01-16T02:23:05Z

core/src/main/scala/org/apache/spark/rdd/NewHadoopRDD.scala

+  @transient
+  private val dateTimeFormatter =
+    DateTimeFormatter
+      .ofPattern("yyyyMMddHHmmss", Locale.US)
+      .withZone(ZoneId.systemDefault())
+
+  private val jobTrackerId: String = dateTimeFormatter.format(new Date().toInstant)


We dont need this field

Suggested change

@transient

private val dateTimeFormatter =

DateTimeFormatter

.ofPattern("yyyyMMddHHmmss", Locale.US)

.withZone(ZoneId.systemDefault())

private val jobTrackerId: String = dateTimeFormatter.format(new Date().toInstant)

private val jobTrackerId: String = {

val dateTimeFormatter =

DateTimeFormatter

.ofPattern("yyyyMMddHHmmss", Locale.US)

.withZone(ZoneId.systemDefault())

dateTimeFormatter.format(new Date().toInstant)

}

mridulm

Looks good to me. +CC @LuciferYang as well.

LuciferYang · 2024-01-17T05:04:29Z

core/src/main/scala/org/apache/spark/deploy/worker/Worker.scala

@@ -821,7 +820,7 @@ private[deploy] class Worker(
  }

  private def generateWorkerId(): String = {
-    workerIdPattern.format(createDateFormat.format(new Date), host, port)
+    workerIdPattern.format(Worker.DATE_TIME_FORMATTER.format(new Date().toInstant), host, port)


Is it possible to directly use Instant.now() instead of new Date().toInstant?

LuciferYang · 2024-01-17T05:06:39Z

core/src/main/scala/org/apache/spark/rdd/NewHadoopRDD.scala

+      DateTimeFormatter
+        .ofPattern("yyyyMMddHHmmss", Locale.US)
+        .withZone(ZoneId.systemDefault())
+    dateTimeFormatter.format(new Date().toInstant)


LuciferYang · 2024-01-17T05:06:50Z

mllib/src/main/scala/org/apache/spark/mllib/pmml/export/PMMLModelExport.scala

@@ -34,7 +40,7 @@ private[mllib] trait PMMLModelExport {
    val version = getClass.getPackage.getImplementationVersion
    val app = new Application("Apache Spark MLlib").setVersion(version)
    val timestamp = new Timestamp()
-      .addContent(new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss", Locale.US).format(new Date()))
+      .addContent(DATE_TIME_FORMATTER.format(new Date().toInstant))


LuciferYang · 2024-01-17T05:08:15Z

...n/scala/org/apache/spark/sql/execution/streaming/continuous/ContinuousTextSocketSource.scala

-                Timestamp.valueOf(
-                  TextSocketReader.DATE_FORMAT.format(Calendar.getInstance().getTime()))
+                Timestamp.valueOf(TextSocketReader.DATE_TIME_FORMATTER.format(
+                  Calendar.getInstance().getTime().toInstant))


Is it possible to directly use Instant.now() instead of new alendar.getInstance().getTime().toInstant?

LuciferYang · 2024-01-17T05:10:38Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/execution/HiveTempPath.scala

@@ -41,6 +42,11 @@ class HiveTempPath(session: SparkSession, val hadoopConf: Configuration, path: P

  lazy val externalTempPath: Path = getExternalTmpPath(path)

+  private lazy val dateTimeFormatter =


This definition can also be moved to object HiveTempPath

beliefer · 2024-01-17T11:04:01Z

The GA failure is unrelated.

mridulm · 2024-01-18T08:24:44Z

Merged to master.
Thanks for fixing this @beliefer !
Thanks for the review @LuciferYang :-)

mridulm · 2024-01-18T08:27:04Z

I had to manually resolve the jira @beliefer - can you please check if I assigned it to you ? There were two id's with same name.

beliefer · 2024-01-18T11:40:52Z

@mridulm @LuciferYang @HyukjinKwon Thank you!

beliefer · 2024-01-18T12:47:38Z

I had to manually resolve the jira @beliefer - can you please check if I assigned it to you ? There were two id's with same name.

I checked. It's OK. Thank you!

github-actions bot added the CORE label Jan 7, 2024

beliefer force-pushed the replace-sdf-with-dtf branch from c27cdbe to 9f1c744 Compare January 8, 2024 05:49

github-actions bot added SQL MLLIB labels Jan 8, 2024

beliefer changed the title ~~[WIP][CORE] Replace SimpleDateFormat with DateTimeFormatter~~ [WIP][CORE][MLLIB][SQL] Replace SimpleDateFormat with DateTimeFormatter Jan 8, 2024

beliefer force-pushed the replace-sdf-with-dtf branch from 9f1c744 to 348b333 Compare January 8, 2024 10:00

github-actions bot added the STRUCTURED STREAMING label Jan 8, 2024

beliefer changed the title ~~[WIP][CORE][MLLIB][SQL] Replace SimpleDateFormat with DateTimeFormatter~~ [SPARK-46623][CORE][MLLIB][SQL] Replace SimpleDateFormat with DateTimeFormatter Jan 8, 2024

beliefer requested a review from dongjoon-hyun January 8, 2024 13:35

mridulm reviewed Jan 16, 2024

View reviewed changes

[WIP][CORE][MLLIB][SQL] Replace SimpleDateFormat with DateTimeFormatter

f771604

beliefer force-pushed the replace-sdf-with-dtf branch from 348b333 to f771604 Compare January 16, 2024 08:19

mridulm reviewed Jan 16, 2024

View reviewed changes

LuciferYang reviewed Jan 17, 2024

View reviewed changes

U

b00495d

LuciferYang approved these changes Jan 17, 2024

View reviewed changes

mridulm closed this in bede614 Jan 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-46623][CORE][MLLIB][SQL] Replace SimpleDateFormat with DateTimeFormatter #44616

[SPARK-46623][CORE][MLLIB][SQL] Replace SimpleDateFormat with DateTimeFormatter #44616

beliefer commented Jan 7, 2024

HyukjinKwon commented Jan 8, 2024

beliefer commented Jan 8, 2024

beliefer commented Jan 10, 2024

mridulm left a comment

mridulm Jan 16, 2024

LuciferYang Jan 16, 2024

mridulm Jan 16, 2024

mridulm left a comment

LuciferYang Jan 17, 2024

LuciferYang Jan 17, 2024

LuciferYang Jan 17, 2024

LuciferYang Jan 17, 2024

LuciferYang Jan 17, 2024

beliefer commented Jan 17, 2024

mridulm commented Jan 18, 2024

mridulm commented Jan 18, 2024

beliefer commented Jan 18, 2024

beliefer commented Jan 18, 2024

		@@ -41,6 +42,11 @@ class HiveTempPath(session: SparkSession, val hadoopConf: Configuration, path: P

		lazy val externalTempPath: Path = getExternalTmpPath(path)

		private lazy val dateTimeFormatter =

[SPARK-46623][CORE][MLLIB][SQL] Replace SimpleDateFormat with DateTimeFormatter #44616

[SPARK-46623][CORE][MLLIB][SQL] Replace SimpleDateFormat with DateTimeFormatter #44616

Conversation

beliefer commented Jan 7, 2024

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

HyukjinKwon commented Jan 8, 2024

beliefer commented Jan 8, 2024

beliefer commented Jan 10, 2024

mridulm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mridulm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beliefer commented Jan 17, 2024

mridulm commented Jan 18, 2024

mridulm commented Jan 18, 2024

beliefer commented Jan 18, 2024

beliefer commented Jan 18, 2024