[SPARK-29362][SQL] Move timestamp expressions and tests to separate files

[MaxGekk]

What changes were proposed in this pull request?

In the PR, I propose refactoring of date and timestamp expressions by:

• Moving all timestamp expressions from datetimeExpressions.scala to timestampExpressions.scala
• Renaming datetimeExpressions.scala to dateExpressions.scala
• Moving all timestamp expressions tests from DateExpressionsSuite.scala to TimestampExpressionsSuite.scala.
• Moving all timestamp functions tests to from DateFunctionsSuite.scala to TimestampFunctionsSuite.scala.

Why are the changes needed?

The datetimeExpressions.scala file has been becoming large. Its size is more than 2000 lines, at the moment. Also it contains 2 kind of expressions - date expressions and timestamp expressions. To make easier navigation and maintainability of date-time expressions, it would be nice to separate the expressions. The same reason is applicable for timestamp expressions and functions.

Does this PR introduce any user-facing change?

No

How was this patch tested?

By existing tests from the test suite DateExpressionsSuite and TimestampExpressionSuite, and by new test suites TimestampExpressionsSuite and TimestampFunctionsSuite.

[SparkQA]

Test build #111801 has finished for PR 26031 at commit cd713e7.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
• class TimestampFunctionsSuite extends QueryTest with SharedSparkSession

[MaxGekk]

@dongjoon-hyun @srowen @cloud-fan @HyukjinKwon Could you take a look at the PR, please.

[srowen]

I'm kind of neutral on it. Yeah it's a big file but there are lots of date/time functions, and they're related.
I usually wonder if this will cause a few merge conflict problems, and how much it's worth in comparison.

[MaxGekk]

I am just worry IntelliJ IDEA heats my laptop so much when I change this file that I can lost it at some moment. ;-)

[dongjoon-hyun]

I also agree with @srowen .

cc @gatorsmile since there was a similar discussion on the other big SQL files. IIRC, it ended up with a negative conclusion. In any cases, this is a case-by-case decision.

[MaxGekk]

Yeah it's a big file but there are lots of date/time functions, and they're related.

Large file is only one reason. Another one is date expressions and timestamp expressions in datetimeExpressions.scala are not related. They don't depend on each other. This is the only one place where we mixed 2 types, probably because of historical reasons.

... this will cause a few merge conflict problems ...

It seems this is main concern, right? As far as I understand, if a private Spark fork has additional date or timestamp expressions, this will cause some conflicts. ... but such conflicts could be easily resolved by keeping an expression in the same place (date expression) or just move it to timestampExpressions.scala. Is it a real problem? Especially taking into account that such private expressions are placed to a separate package like com.databricks....

[dongjoon-hyun]

@MaxGekk . Since you were not there, I'll give you more context. In addition to them, the last blocker concern of the last discussion was not about resolving conflicts. It's about weakening the traceability due to the loss of the commit history on that file. At Spark 2.0, we did many refactoring like this PR, and this removes the line-by-line commit history and increased the chance of bugs. That was the reason why the PMCs are reluctant to do that again. And, basically, I've heard that comment from @gatorsmile .

[gatorsmile]

Normally, we will not do such a file split. It will make us [especially for new contributors] very hard to track the change history. For example, this function, we can get the change history in IntelliJ

[gatorsmile]

I would agree with @dongjoon-hyun . I have to say -1 for such a PR, like what I did in the other similar PRs. Reading our change history is the most efficient way to learn our code base. I did it almost everyday.

[MaxGekk]

Before closing this PR, let's look at moved expression CurrentTimestamp from datetimeExpression.scala to timestampExpressions.scala.


IntelliJ IDEA can show the blame history as well:

[srowen]

Does it preserve git history for both files? I'd imagine at most one of them can be a rename, so the other appears all new.

[MaxGekk]

Does it preserve git history for both files?

@srowen Git (and IntelliJ IDEA) is able to track history for both files. I see original history (and blame annotations) in the renamed file (datetimeExpressions.scala -> dateExpressions.scala) as is. In the new file timestampExpressions.scala, I am as the last author of whole file but you can see the git log and blames as I showed above. When you select Annotate Previous Revision, IDEA opens parent revision on the original file datetimeExpressions.scala. Somehow it recognizes the original places where the code moved from. The same for history new file timestampExpressions.scala. Its history does not start from scratch (see the second screenshot above).

[MaxGekk]

Here is a related answer https://stackoverflow.com/a/40466759 . The command below show blame annotation of copied code:

 git blame -C ./timestampExpressions.scala

[srowen]

Interesting, so you can get git to notice that the file contents were copied. It isn't the default? so I assume IJ wouldn't automatically do this, but it's available. I'd still be overall neutral. Does it really cause problems in IJ? didn't seem too hard to deal with when I opened it, but maybe complex ops with git take a while?

[MaxGekk]

Interesting, so you can get git to notice that the file contents were copied. It isn't the default?

Yes, you just need to pass additional options to git -C (https://git-scm.com/docs/git-blame#Documentation/git-blame.txt--Cltnumgt) or -M(https://git-scm.com/docs/git-blame#Documentation/git-blame.txt--Mltnumgt) otherwise it will show who copied the code.

but maybe complex ops with git take a while?

Less than 2 seconds:

 $ time git blame -C ./timestampExpressions.scala > /dev/null
git blame -C ./timestampExpressions.scala > /dev/null  1.21s user 0.09s system 99% cpu 1.312 total

[dongjoon-hyun]

That's good. Let me check that too, @MaxGekk .

[dongjoon-hyun]

@MaxGekk . GitHub doesn't support that by default. Please see you new file in GitHub. Everything is under one commit.

• https://github.com/apache/spark/blame/cd713e733b582591dd9805bcc7d372231b3283d5/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/TimestampExpressionsSuite.scala

In this case, I'm still -1.

[srowen]

Yes, and I was also wondering about IJ. I get that you can do this on the command line, which is great. I was also asking what the IJ perf problem is.

[MaxGekk]

I am closing this PR.

[dongjoon-hyun]

Thank you for the decision, @MaxGekk .

[cloud-fan]

This is a tradeoff that we need to evaluate. We should not blindly reject any cross-file code movement simply because it makes commit tracking harder. Users can still find the code movement commit and continue to look at the commits of the old file. It's still doable but harder.

For this particular case, I think datetimeExpressions.scala is OK to hold the date and timestamp expressions. It's better to split into 2 files as it has grown too big, we should do it if we are going to add many more datetime expressions later.

[HyukjinKwon]

We should not blindly reject any cross-file code movement simply because it makes commit tracking harder

Yea, I think so too.