[SPARK-9136][SQL] fix several bugs in DateTimeUtils.stringToTimestamp

[cloud-fan]

a follow up of #7353

1. we should use Calendar.HOUR_OF_DAY instead of Calendar.HOUR(this is for AM, PM).
2. we should call c.set(Calendar.MILLISECOND, 0) after Calendar.getInstance

I'm not sure why the tests didn't fail in jenkins, but I ran latest spark master branch locally and DateTimeUtilsSuite failed.

[cloud-fan]

cc @tarekauel @davies

[davies]

LGTM

[tarekbecker]

I don't know why you have changed this. I divided by 1000 first in order to remove the call of c.set(Calendar.MILLISECOND, 0) in Line 323. But I am fine with this, I guess it's a personal preference.

[cloud-fan]

divide by 1000 is not right when c.getTimeInMillis is negative.
I'm really confused why jenkins didn't report test error for this case...

[tarekbecker]

Sorry I didn't get this. Why is divide by 1000 a problem, if the date is before 1.1.1970? It's a long value and the last three digits are the millis. In order to cut them off, I divide by 1000. This has no impact on the sign or anything else, does it?

[cloud-fan]

Let's say now is a second before 1970-1-1:

val c = Calendar.getInstance(TimeZone.getTimeZone("GMT"))
c.set(1969, 11, 31, 23, 59, 59)
c.getTimeInMillis // something bigger than -1000, maybe -700
c.getTimeInMillis / 1000 * 1000000 == 0
c.set(Calendar.MILLISECOND, 0)
c.getTimeInMillis // exactly -1000
c.getTimeInMillis * 1000 == -1000000

In the test case we were testing "1969-12-31 16:00:00", so if the time zone in jenkins test environment is less than GMT-8, c.getTimeInMillis will be positive and this bug will be hidden.

[tarekbecker]

Got it! Thanks for the explanation.

[tarekbecker]

LGTM

1. yes that was a bug. Thanks for fixing it.
2. see my comment.

[SparkQA]

Test build #1096 has finished for PR 7473 at commit 66cdaf2.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • logDebug("isMulticlass = " + metadata.isMulticlass)
  • * (i.e., if isMulticlass && isSpaceSufficientForAllCategoricalSplits),
  • logDebug("isMulticlass = " + metadata.isMulticlass)
  • abstract class UnsafeProjection extends Projection
  • case class FromUnsafeProjection(fields: Seq[DataType]) extends Projection
  • abstract class BaseProjection extends Projection
  • class SpecificProjection extends $
  • class SpecificProjection extends $

[SparkQA]

Test build #37644 has finished for PR 7473 at commit 66cdaf2.

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