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-24151][SQL] Case insensitive resolution of CURRENT_DATE and CURRENT_TIMESTAMP #22440

Closed
wants to merge 2 commits into from

Conversation

mgaido91
Copy link
Contributor

What changes were proposed in this pull request?

SPARK-22333 introduced a regression in the resolution of CURRENT_DATE and CURRENT_TIMESTAMP. Before that ticket, these 2 functions were resolved in a case insensitive way. After, this depends on the value of spark.sql.caseSensitive.

The PR restores the previous behavior and makes their resolution case insensitive anyhow. The PR takes over #21217, therefore it closes #21217 and credit for this patch should be given to @jamesthomp.

How was this patch tested?

added UT

@mgaido91
Copy link
Contributor Author

@@ -1879,6 +1879,7 @@ working with timestamps in `pandas_udf`s to get the best performance, see

## Upgrading From Spark SQL 2.3 to 2.4

- In version 2.3 and earlier, if `spark.sql.caseSensitive` is set to true, then the `CURRENT_DATE` and `CURRENT_TIMESTAMP` functions incorrectly became case-sensitive and would resolve to columns (unless typed in lower case). In Spark 2.4 this has been fixed and the functions are no longer case-sensitive.
Copy link
Member

Choose a reason for hiding this comment

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

Shall we give the specified version range like https://github.com/apache/spark/pull/21217/files#r203205223?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated specifying the versions. As this is included in the migration guide to 2.4 and I don't think this commit will be backported to 2.3, I think it is fine to state 2.3, but it is good to mention that also 2.2.1 is affected. Thanks.

@HyukjinKwon
Copy link
Member

ok to test

@SparkQA
Copy link

SparkQA commented Sep 17, 2018

Test build #96130 has finished for PR 22440 at commit 49595c0.

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

@dongjoon-hyun
Copy link
Member

Thank you, @mgaido91 .

@SparkQA
Copy link

SparkQA commented Sep 17, 2018

Test build #96136 has finished for PR 22440 at commit 49595c0.

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

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Sep 17, 2018

@mgaido91 . I know that your test code is bigger than the remaining original code in Analyzer.scala.

However, we changes our way of credits into an explicit manner. If you preserve some of @jamesthomp 's commits, he will be the original author and you will be a co-author as you requested. I prefer this way.

Otherwise, could you add an empty commit with @jamesthomp 's name and email at least? In that case, he will be co-author automatically.

@SparkQA
Copy link

SparkQA commented Sep 17, 2018

Test build #96129 has finished for PR 22440 at commit e207e60.

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

@mgaido91
Copy link
Contributor Author

I see @dongjoon-hyun. Sure, sorry I wasn't aware of how this is working. Shall I create a new PR based on @jamesthomp's commits or is it enough to update this adding his commits?

Sorry for the trouble and thanks for guiding me on this.

@dongjoon-hyun
Copy link
Member

For now, I think it's enough to add an empty commit for him in this PR. Then, I will try to moderate during setting the main author.

Copy link
Member

@gatorsmile gatorsmile left a comment

Choose a reason for hiding this comment

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

LGTM to me. Thanks for taking it over.

@gatorsmile
Copy link
Member

Thanks! Merged to master/2.4

asfgit pushed a commit that referenced this pull request Sep 18, 2018
…RRENT_TIMESTAMP

## What changes were proposed in this pull request?

SPARK-22333 introduced a regression in the resolution of `CURRENT_DATE` and `CURRENT_TIMESTAMP`. Before that ticket, these 2 functions were resolved in a case insensitive way. After, this depends on the value of `spark.sql.caseSensitive`.

The PR restores the previous behavior and makes their resolution case insensitive anyhow. The PR takes over #21217, therefore it closes #21217 and credit for this patch should be given to jamesthomp.

## How was this patch tested?

added UT

Closes #22440 from mgaido91/SPARK-24151.

Lead-authored-by: James Thompson <jamesthomp@users.noreply.github.com>
Co-authored-by: Marco Gaido <marcogaido91@gmail.com>
Signed-off-by: gatorsmile <gatorsmile@gmail.com>
(cherry picked from commit ba838fe)
Signed-off-by: gatorsmile <gatorsmile@gmail.com>
@asfgit asfgit closed this in ba838fe Sep 18, 2018
@dongjoon-hyun
Copy link
Member

Thank you for merging, @gatorsmile ! I like it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants