Skip to content

Revert "[SPARK-33995][SQL] Expose make_interval as a Scala function"#33143

Closed
MaxGekk wants to merge 1 commit intoapache:masterfrom
MaxGekk:revert-make_interval
Closed

Revert "[SPARK-33995][SQL] Expose make_interval as a Scala function"#33143
MaxGekk wants to merge 1 commit intoapache:masterfrom
MaxGekk:revert-make_interval

Conversation

@MaxGekk
Copy link
Member

@MaxGekk MaxGekk commented Jun 29, 2021

What changes were proposed in this pull request?

This reverts commit e6753c9.

Why are the changes needed?

The make_interval function aims to construct values of the legacy interval type CalendarIntervalType which will be substituted by ANSI interval types (see SPARK-27790). Since the function has not been released yet, it would be better to don't expose it via public API at all.

Does this PR introduce any user-facing change?

Should not since the make_interval function has not been released yet.

How was this patch tested?

By existing test suites, and GA/jenkins builds.

@github-actions github-actions bot added the SQL label Jun 29, 2021
@SparkQA
Copy link

SparkQA commented Jun 29, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44901/

@dongjoon-hyun
Copy link
Member

cc @MrPowers and @cloud-fan

@SparkQA
Copy link

SparkQA commented Jun 29, 2021

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44901/

@HyukjinKwon
Copy link
Member

I think it's fine though since we didn't deprecate CalendarIntervalType. That specific type has been there from 1.5.0.

@SparkQA
Copy link

SparkQA commented Jun 30, 2021

Test build #140402 has finished for PR 33143 at commit 33b5b9a.

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

@AmplabJenkins
Copy link

Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/140402/

@MrPowers
Copy link
Contributor

👍

Thanks for creating something better @MaxGekk and thanks for the ping @dongjoon-hyun.

* @group datetime_funcs
* @since 3.2.0
*/
def make_interval(
Copy link
Contributor

Choose a reason for hiding this comment

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

do we have make_ym_interval and make_dt_interval now?

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't remove until we decide to deprecate CalendarIntervalType?

Copy link
Member

Choose a reason for hiding this comment

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

Is it better to remove it before a release? @since 3.2.0

Copy link
Member

Choose a reason for hiding this comment

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

Will we deprecate and remove CalendarIntervalType?

Copy link
Member Author

@MaxGekk MaxGekk Jun 30, 2021

Choose a reason for hiding this comment

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

do we have make_ym_interval and make_dt_interval now?

@cloud-fan As SQL functions, see #32645 and #32601 . The functions haven't been exposed to Scala/Python/R APIs yet.

Will we deprecate and remove CalendarIntervalType?

@HyukjinKwon Yes, we will I hope. The question is only when.

Copy link
Member

Choose a reason for hiding this comment

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

okie then i am good

@MaxGekk
Copy link
Member Author

MaxGekk commented Jun 30, 2021

Thanks all for reviews. Merging to master.

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.

8 participants