Skip to content

[SPARK-26978][CORE][SQL] Avoid magic time constants - #23878

Closed
MaxGekk wants to merge 16 commits into
apache:masterfrom
MaxGekk:magic-time-constants
Closed

[SPARK-26978][CORE][SQL] Avoid magic time constants#23878
MaxGekk wants to merge 16 commits into
apache:masterfrom
MaxGekk:magic-time-constants

Conversation

@MaxGekk

@MaxGekkMaxGekk commented Feb 23, 2019

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

In the PR, I propose to refactor existing code related to date/time conversions, and replace constants like 1000 and 1000000 by DateTimeUtils constants and transformation functions from java.util.concurrent.TimeUnit._.

How was this patch tested?

The changes are tested by existing test suites.

srowen
srowen requested changes Feb 23, 2019
View reviewed changes

@srowensrowen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good change, just a few comments

Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala Outdated
Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala Outdated
Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/package.scala Outdated
Comment threadsql/core/src/main/scala/org/apache/spark/sql/execution/streaming/GroupStateImpl.scala
Comment threadsql/core/src/test/scala/org/apache/spark/sql/DateFunctionsSuite.scala Outdated
@SparkQA

SparkQA commented Feb 23, 2019

Copy link
Copy Markdown

Test build #102708 has finished for PR 23878 at commit 1ff8839.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA

SparkQA commented Feb 23, 2019

Copy link
Copy Markdown

Test build #102710 has finished for PR 23878 at commit 1b515ba.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

srowen
srowen reviewed Feb 24, 2019
View reviewed changes

@srowensrowen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good; you can remove benchmark

@SparkQA

SparkQA commented Feb 24, 2019

Copy link
Copy Markdown

Test build #102722 has finished for PR 23878 at commit 1886592.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

felixcheung
felixcheung reviewed Feb 24, 2019
View reviewed changes

@felixcheungfelixcheung left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

probably should add [SQL] in the PR title?

@SparkQA

SparkQA commented Feb 25, 2019

Copy link
Copy Markdown

Test build #102725 has finished for PR 23878 at commit 324aadb.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA

SparkQA commented Feb 25, 2019

Copy link
Copy Markdown

Test build #102726 has finished for PR 23878 at commit 43a3d87.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@MaxGekk

MaxGekk commented Feb 25, 2019•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

probably should add [SQL] in the PR title?

The PR touches not only SQL-related files as well as in CORE.

@MaxGekkMaxGekk changed the title [SPARK-26978][CORE] Avoid magic time constants[SPARK-26978][CORE][SQL] Avoid magic time constantsFeb 25, 2019
@MaxGekk

MaxGekk commented Feb 25, 2019

Copy link
Copy Markdown
MemberAuthor

@felixcheung added [SQL] tag too

kiszk
kiszk reviewed Feb 25, 2019
View reviewed changes
Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala
(c, evPrim, evNull) =>
code"""$evPrim =
org.apache.spark.sql.catalyst.util.DateTimeUtils.millisToDays($c / 1000L, $tz);"""
org.apache.spark.sql.catalyst.util.DateTimeUtils.millisToDays(

@kiszkkiszkFeb 25, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have a little bit concern to use a method call in the generated code. This change includes # of Java bytecode to be generated while the original code uses / for long type.
Do we take the readability and maintanance or smaller generated bytecode?

@MaxGekkMaxGekkFeb 25, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

The call toMillis() should be inlined, shouldn't it?

@srowensrowenFeb 25, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree with you @kiszk; this probably only adds a few ops though to invoke a static method vs a division. The issue here is the size of the generated bytecode, so the implementation of toMillis doesn't matter nor whether it's JITted at runtime.

@MaxGekkMaxGekkFeb 25, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Should I revert all changes in code blocks?

@MaxGekkMaxGekkFeb 25, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I reverted it back to division.

kiszk
kiszk reviewed Feb 25, 2019
View reviewed changes
Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala Outdated
@SparkQA

SparkQA commented Feb 26, 2019

Copy link
Copy Markdown

Test build #102765 has finished for PR 23878 at commit a02be99.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@srowen

srowen commented Feb 26, 2019

Copy link
Copy Markdown
Member

Merged to master

@MaxGekkMaxGekk mentioned this pull request Jul 11, 2019
Closed
vinodkc pushed a commit to vinodkc/spark that referenced this pull request Jul 18, 2019
@MaxGekk@vinodkc
[SPARK-26978][SQL][FOLLOWUP] Initialize date-time constants by foldab…
2b9e090
…le expressions
## What changes were proposed in this pull request?
Reverted initialization of date-time constants in `DateTimeUtils` introduced by apache#23878. As a comment in [Delta repo](https://github.com/delta-io/delta) states, the compiler can do additional optimizations if values can be calculated at compile time: https://github.com/delta-io/delta/blob/master/src/main/scala/org/apache/spark/sql/delta/util/DateTimeUtils.scala#L63-L75
## How was this patch tested?
This was tested by existing test suites.
Closesapache#25116 from MaxGekk/datetime-consts-init.
Authored-by: Maxim Gekk <maxim.gekk@databricks.com>
Signed-off-by: herman <herman@databricks.com>
@MaxGekk
MaxGekk deleted the magic-time-constants branch September 18, 2019 15:57
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kiszkkiszkkiszk left review comments
@felixcheungfelixcheungfelixcheung left review comments
@srowensrowensrowen approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@MaxGekk@SparkQA@srowen@kiszk@felixcheung