Skip to content

Comments

[SPARK-30547][SQL] Add unstable annotation to the CalendarInterval class#27258

Closed
yaooqinn wants to merge 3 commits intoapache:masterfrom
yaooqinn:SPARK-30547
Closed

[SPARK-30547][SQL] Add unstable annotation to the CalendarInterval class#27258
yaooqinn wants to merge 3 commits intoapache:masterfrom
yaooqinn:SPARK-30547

Conversation

@yaooqinn
Copy link
Member

What changes were proposed in this pull request?

CalendarInterval is maintained as a private class but might be used in a public way by users
e.g.

scala> spark.udf.register("getIntervalMonth", (_:org.apache.spark.unsafe.types.CalendarInterval).months)

scala> sql("select interval 2 month 1 day a").selectExpr("getIntervalMonth(a)").show
+-------------------+
|getIntervalMonth(a)|
+-------------------+
|                  2|
+-------------------+

And it exists since 1.5.0, now we go to the 3.x era,may be it's time to make it public

Why are the changes needed?

make the interval more future-proofing

Does this PR introduce any user-facing change?

doc change

How was this patch tested?

add ut.

@yaooqinn yaooqinn requested a review from cloud-fan January 17, 2020 11:19
@SparkQA
Copy link

SparkQA commented Jan 17, 2020

Test build #116922 has finished for PR 27258 at commit 1a90651.

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


/**
* The internal representation of interval type.
* The data type representing calendar intervals. The calendar interval is stored internally in
Copy link
Contributor

Choose a reason for hiding this comment

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

data type? how about The class representing ...

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

@SparkQA
Copy link

SparkQA commented Jan 17, 2020

Test build #116941 has finished for PR 27258 at commit 1cf83ec.

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

@SparkQA
Copy link

SparkQA commented Jan 19, 2020

Test build #116997 has finished for PR 27258 at commit d9b85ff.

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

@yaooqinn yaooqinn requested a review from cloud-fan January 19, 2020 10:51
@cloud-fan cloud-fan closed this in 4806cc5 Jan 20, 2020
@cloud-fan
Copy link
Contributor

thanks, merging to master!

* they are two separated fields from microseconds. One month may be equal to 28, 29, 30 or 31 days
* and one day may be equal to 23, 24 or 25 hours (daylight saving).
*
* @since 1.5.0
Copy link
Member

Choose a reason for hiding this comment

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

Just a question: @cloud-fan .
Although this is technically correct, if this is going to public in 3.00, @since 3.0.0 is better?

Copy link
Member

Choose a reason for hiding this comment

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

If this is 1.5.0, people will get confused and may try to use it 2.4.x.

Copy link
Member

Choose a reason for hiding this comment

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

cc @gatorsmile , too.

Copy link
Contributor

Choose a reason for hiding this comment

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

this is a good point. Do we have a clear rule about how to decide the since version. To me 3.0 is better here, as we've changed this class a lot in 3.0.

Copy link
Member Author

Choose a reason for hiding this comment

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

checked the java doc about since https://www.oracle.com/technetwork/java/javase/documentation/index-137868.html#@since

When a class (or interface) is introduced, specify one since tag in its class description and no since tags in the members. Add a since tag only to members added in a later version than the class. ...

If a member changes from protected to public in a later release, the since tag would not change, even though it is now usable by any caller, not just subclassers.

1.5.0 should be fine I guess

Copy link
Contributor

Choose a reason for hiding this comment

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

... the since tag would not change ...

This is different from what we are facing here. We are adding a since tag. not changing an existing one.

Copy link
Member Author

Choose a reason for hiding this comment

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

raise a followup to fix this #27299, thanks @dongjoon-hyun @cloud-fan

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