Skip to content

[SPARK-31443][SQL] Fix perf regression of toJavaDate #28212

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
MaxGekk wants to merge 7 commits into from
Closed

[SPARK-31443][SQL] Fix perf regression of toJavaDate #28212

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
12f88c8
Rewrite toJavaDate
MaxGekk Apr 14, 2020
92253cf
Benchmark collecting dates
MaxGekk Apr 14, 2020
6295260
Re-gen DateTimeBenchmark results on JDK 8
MaxGekk Apr 14, 2020
1ff47ca
Re-gen DateTimeBenchmark results on JDK 11
MaxGekk Apr 14, 2020
f5c97d8
Merge remote-tracking branch 'origin/master' into optimize-toJavaDate
MaxGekk Apr 14, 2020
58156bf
Re-gen DateTimeBenchmark results on JDK 8
MaxGekk Apr 14, 2020
614ec2a
Re-gen DateTimeBenchmark results on JDK 11
MaxGekk Apr 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ import java.util.concurrent.TimeUnit._

import scala.util.control.NonFatal

import sun.util.calendar.ZoneInfo

import org.apache.spark.sql.catalyst.util.DateTimeConstants._
import org.apache.spark.sql.catalyst.util.RebaseDateTime._
import org.apache.spark.sql.types.Decimal
Expand Down Expand Up @@ -121,8 +123,13 @@ object DateTimeUtils {
* @return A `java.sql.Date` from number of days since epoch.
*/
def toJavaDate(daysSinceEpoch: SQLDate): Date = {
val localDate = LocalDate.ofEpochDay(daysSinceEpoch)
new Date(localDate.getYear - 1900, localDate.getMonthValue - 1, localDate.getDayOfMonth)
val rebasedDays = rebaseGregorianToJulianDays(daysSinceEpoch)
val localMillis = Math.multiplyExact(rebasedDays, MILLIS_PER_DAY)
val timeZoneOffset = TimeZone.getDefault match {
case zoneInfo: ZoneInfo => zoneInfo.getOffsetsByWall(localMillis, null)
case timeZone: TimeZone => timeZone.getOffset(localMillis - timeZone.getRawOffset)
}
Comment on lines +128 to +131
Copy link
Member Author

@MaxGekk MaxGekk Apr 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code is adopted from https://github.com/AdoptOpenJDK/openjdk-jdk8u/blob/aa318070b27849f1fe00d14684b2a40f7b29bf79/jdk/src/share/classes/java/util/GregorianCalendar.java#L2795-L2801

Copy link
Contributor

@cloud-fan cloud-fan Apr 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we apply this change for spark 2.4 as well?

Copy link
Member Author

@MaxGekk MaxGekk Apr 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we can do that in 2.4, sure but see #28216 (comment)

Copy link
Member

@kiszk kiszk Apr 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to https://spark.apache.org/versioning-policy.html,, a maintenance release would not include performance improvement patch.

For 3.0, it looks good due to the small size of this change.

Copy link
Member

@HyukjinKwon HyukjinKwon Apr 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's best to follow the guide.

Copy link
Contributor

@cloud-fan cloud-fan Apr 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was asking for the timezone offset change, which seems wrong in 2.4: https://github.com/apache/spark/blob/branch-2.4/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala#L1083-L1118

Copy link
Contributor

@cloud-fan cloud-fan Apr 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't tell that the 2 lines here behave the same as that long method in 2.4. If they are the same and it's just an improvement, then we shouldn't backport. If 2.4 is wrong and then we should fix it as it's a correctness issue.

new Date(localMillis - timeZoneOffset)
}

/**
Expand Down
Loading