Skip to content
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.

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

[SPARK-22040] Add current_date function with timezone id #19261

Closed
wants to merge 1 commit into from

Conversation

mgaido91
Copy link
Contributor

What changes were proposed in this pull request?

Add current_date function with timezone id.

How was this patch tested?

Manual test on spark-shell and pyspark.

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

* @group datetime_funcs
* @since 2.3.0
*/
def current_date(timeZone: String): Column = withExpr { CurrentDate(Option(timeZone)) }
Copy link
Contributor

Choose a reason for hiding this comment

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

s/Option/Some

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What if someone pass a null as value? Option is safer in this case...

@@ -2508,6 +2508,14 @@ object functions {
def current_date(): Column = withExpr { CurrentDate() }

/**
* Returns the current date in the given timezone as a date column.
Copy link
Contributor

Choose a reason for hiding this comment

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

@return?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is consistent with all the other functions in this file.

@@ -793,12 +793,12 @@ def ntile(n):
# ---------------------- Date/Timestamp functions ------------------------------

@since(1.5)
def current_date():
def current_date(timeZone=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

Would the change beg a different @since? It's no longer true that it existed since(1.5), is it? Just asking...no idea how it really should be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly I have no idea too.. Do you know who can we ask to? Thanks.

@gatorsmile
Copy link
Member

gatorsmile commented Sep 17, 2017

Any other database allows users to do it?

@jaceklaskowski
Copy link
Contributor

@gatorsmile Dunno, but the logical operator does.

@rxin
Copy link
Contributor

rxin commented Sep 18, 2017

What does this even mean?

@gatorsmile
Copy link
Member

I think we should not do it, because no DB vendor does it.

@jaceklaskowski
Copy link
Contributor

@rxin @gatorsmile Let me ask you a very similar question then, why does CurrentDate operator has the optional timezone parameter? What's the purpose? Wouldn't that answer your questions?

I don't mind not having the change, but am curious what is the reason for the "mismatch"?

@gatorsmile
Copy link
Member

When we introduce the external APIs, we have to be very careful.

Regarding the usage of timezone, you can check the codes. It is for internal usage.

@jaceklaskowski
Copy link
Contributor

jaceklaskowski commented Sep 26, 2017

OK I feel convinced that you feel convinced Spark SQL should not offer this as part of the public API. Thanks for being with me for so long and patient to explain the things. Thanks.

@mgaido91 What do you think about closing this PR and the corresponding JIRA issue (to record your commitment on it)? Thanks.

@mgaido91 mgaido91 closed this Sep 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants