Skip to content

Conversation

@MaxGekk
Copy link
Member

@MaxGekk MaxGekk commented Nov 2, 2019

What changes were proposed in this pull request?

I propose 2 new methods for CalendarInterval:

  • extractAsPeriod() returns the date part of an interval as an instance of java.time.Period
  • extractAsDuration() returns the time part of an interval as an instance of java.time.Duration

For example:

scala> import org.apache.spark.unsafe.types.CalendarInterval
scala> import java.time._
scala> val i = spark.sql("select interval 1 year 3 months 4 days 10 hours 30 seconds").collect()(0).getAs[CalendarInterval](0)
scala> LocalDate.of(2019, 11, 1).plus(i.period())
res8: java.time.LocalDate = 2021-02-05
scala> ZonedDateTime.parse("2019-11-01T12:13:14Z").plus(i.extractAsPeriod()).plus(i.extractAsDuration())
res9: java.time.ZonedDateTime = 2021-02-05T22:13:44Z

Why are the changes needed?

Taking into account that CalendarInterval has been already partially exposed to users via the collect operation, and probably it will be fully exposed in the future, it could be convenient for users to get the date and time parts of intervals as java classes:

  • to avoid unnecessary dependency from Spark's classes in user code
  • to easily use external libraries that accept standard Java classes.

Does this PR introduce any user-facing change?

No

How was this patch tested?

By new test in CalendarIntervalSuite.

@MaxGekk
Copy link
Member Author

MaxGekk commented Nov 2, 2019

@cloud-fan @hvanhovell Please, take a look at this. If it makes sense for you, we could discuss alternative names for methods like getDatePart()/getTimePart() or extractDatePart/extractTimePart(), and etc.

@SparkQA
Copy link

SparkQA commented Nov 2, 2019

Test build #113129 has finished for PR 26368 at commit cfb8b5a.

  • This patch fails SparkR unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@MaxGekk
Copy link
Member Author

MaxGekk commented Nov 3, 2019

jenkins, retest this, please

@SparkQA
Copy link

SparkQA commented Nov 3, 2019

Test build #113156 has finished for PR 26368 at commit cfb8b5a.

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

@cloud-fan
Copy link
Contributor

how about extractAsPeriod and extractAsDuration? We need to make it clear that CalendarInterval is a mix of Period and Duration.

…va-period-duration

# Conflicts:
#	common/unsafe/src/main/java/org/apache/spark/unsafe/types/CalendarInterval.java
@SparkQA
Copy link

SparkQA commented Nov 4, 2019

Test build #113199 has finished for PR 26368 at commit 2f9f17a.

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

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM. Thank you, @MaxGekk and @cloud-fan .
Merged to master.

@MaxGekk MaxGekk deleted the interval-java-period-duration branch June 5, 2020 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants