Change JVM time zone in tests to better test corner cases

[findepi]

This changes default JVM time zone in tests from Asia/Katmandu to America/Bahia_Banderas. The latter zone has more 'nice' features, see #10078 for context and explanation.

Note: this doesn't fix #10078 fully. We should change default session zone as well (TestingSession defaults to UTC).

[findepi]

America/Bahia_Banderas is so good at exploiting corner cases that Hive doesn't work with it:

Caused by: com.facebook.presto.hive.$internal.org.joda.time.IllegalInstantException: Illegal instant due to time zone offset transition (daylight savings time 'gap'): 1970-01-01T00:00:00.000 (America/Bahia_Banderas)
	at com.facebook.presto.hive.$internal.org.joda.time.chrono.ZonedChronology.localToUTC(ZonedChronology.java:143)
	at com.facebook.presto.hive.$internal.org.joda.time.chrono.ZonedChronology.getDateTimeMillis(ZonedChronology.java:118)
	at com.facebook.presto.hive.$internal.org.joda.time.chrono.AssembledChronology.getDateTimeMillis(AssembledChronology.java:133)
	at com.facebook.presto.hive.$internal.org.joda.time.base.BaseDateTime.<init>(BaseDateTime.java:258)
	at com.facebook.presto.hive.$internal.org.joda.time.base.BaseDateTime.<init>(BaseDateTime.java:199)
	at com.facebook.presto.hive.$internal.org.joda.time.DateTime.<init>(DateTime.java:476)
	at org.apache.hive.common.util.TimestampParser.<clinit>(TimestampParser.java:49)

(i reported this as https://issues.apache.org/jira/browse/HIVE-18925 ...)

@electrum @dain @haozhun @losipiuk is it something we could work around?

[findepi]

prestodb/presto-hive-apache#31 copies TimestampParser into our code base and fixes it.

[findepi]

So far this change found:

• bugs
  • Fix current_date when session zone had gap at 1970-01-01 00:00:00 #10206
  • https://issues.apache.org/jira/browse/HIVE-18925
• incorrect code in tests
  • Fix epoch day to java.sql.Date conversion #10187

[findepi]

Travis says this change is cool.

@dain @electrum @haozhun @losipiuk , what do you think?

[losipiuk]

I am fine with this one. It does not solve all the issues. But it makes our test coverage slightly less forgiving. This is good.

[haozhun]

When I do grep -Irn 'Kathmandu' .. I see a few occurrences that wasn't changed. (I removed a few from the list below that obviously doesn't matter.) Should they be changed?

./presto-teradata-functions/src/test/java/com/facebook/presto/teradata/functions/TestTeradataDateFunctions.java:39:    private static final TimeZoneKey TIME_ZONE_KEY = getTimeZoneKey("Asia/Kathmandu");
./presto-tests/src/test/java/com/facebook/presto/tests/AbstractTestEngineOnlyQueries.java:46:        Session kathmandu = Session.builder(getSession()).setTimeZoneKey(TimeZoneKey.getTimeZoneKey("Asia/Kathmandu")).build();
./presto-product-tests/README.md:397:    - VM options: `-ea -Xmx2G -Dconfig=etc/config.properties -Dlog.levels-file=etc/log.properties -DHADOOP_USER_NAME=hive -Duser.timezone=Asia/Kathmandu`
./presto-product-tests/conf/presto/etc/jvm.config:17:-Duser.timezone=Asia/Kathmandu
./presto-product-tests/conf/docker/files/run-tempto.sh:19:  -Duser.timezone=Asia/Kathmandu \
./presto-product-tests/src/main/java/com/facebook/presto/tests/TemptoProductTestRunner.java:22:    public static final DateTimeZone PRODUCT_TESTS_TIME_ZONE = DateTimeZone.forID("Asia/Kathmandu");
./presto-hive-hadoop2/bin/run_hive_tests.sh:28:  -Dhive.hadoop2.timeZone=Asia/Kathmandu \
./presto-main/src/test/java/com/facebook/presto/operator/scalar/TestDateTimeFunctionsBase.java:81:    protected static final TimeZoneKey TIME_ZONE_KEY = getTimeZoneKey("Asia/Kathmandu");
./presto-main/src/test/java/com/facebook/presto/operator/scalar/TestDateTimeFunctionsBase.java:681:        assertFunction("format_datetime(" + TIMESTAMP_LITERAL + ", 'YYYY/MM/dd HH:mm ZZZZ')", VARCHAR, "2001/08/22 03:04 Asia/Kathmandu");

[findepi]

@haozhun good catch. I am hesitant as to what places should be changed. I think we should change product tests, but I am not entirely sure. I added this as a TODO item to #10078. Anyway, this could be done in a separate PR, right?

[haozhun]

I didn't look at this carefully. But I feel like all the ones I listed should be changed. If I were you, I would read each instance to get a basic understanding of their intended use. I'll then change them (most likely all), and go ahead if all tests pass after the change.

I don't think time will give us more understanding as to what needs to be changed. And I think a mixed state will cause confusions.

[haozhun]

Looks good assuming nothing significant comes up when you address my concern about the occurrences of Kathmandu that I listed.

[findepi]

@haozhun i see your point. Let's defer this after #10193 is merged.

[findepi]

Ready for re-review.

[haozhun]

What is the intent behind "Upgrade Presto hive-apache to 1.2.2-2"? A description in the body of the commit will help.

[haozhun]

All but "Upgrade Presto hive-apache to 1.2.2-2" and "Simplify SqlTimestamp construction in tests" looks good.

For "Simplify SqlTimestamp construction in tests", I don't quite like the state after your commit.

I would like to see it simplified, whereas the commit adds a new one without removing the existing ones.

Here is my proposal. (I would prefer java.time in tests. But I don't mind if you go with joda.) "Existing" in the notes below represents the current state of this PR.

/**
 * Legacy-aware SqlTimestamp factory
 */
sqlTimestampOf(year, month, day, hour, minute, second, milli, session) {
  // notes: existing 1st
  return sqlTimestampOf(ZonedDateTime.of(..., ZoneId.of(session.getTimeZoneKey().getId())), session.isLegacyTimestamp);
}

/**
 * Legacy-aware SqlTimestamp factory
 */
sqlTimestampOf(zonedDateTime, legacyTimestamp) {
  // notes: new
  // Here, it's legacyTimestamp instead of session because session already contains the zone, which makes it very confusing to read.
  if (legacyTimestamp) {
    return sqlTimestampOfLegacy(zonedDateTime);
  } else {
    return sqlTimestampOf(zonedDateTime.toLocalDateTime());
  }
}

sqlTimestampOf(localDateTime) {
  // notes: existing 4th
  // notes: leave as is
}

sqlTimestampOfLegacy(zonedDateTime) {
  // notes: new
  return new SqlTimestamp(zonedDateTime.toInstant().toEpochMilli(), TimeZoneKey.getTimeZoneKey(zonedDateTime.getZone().getId()));
}

These will be removed:

// existing 2nd, 3rd
sqlTimestampOf(year, month, day, hour, minute, second, millis, baseZone, timestampZone, session/connectorSession) {
  // notes: Remove.
  // notes: When baseZone is equal to timestampZone, removal should be trivial because all callsites has timestampZone == session.getTimeZoneKey
  // notes: When not, use sqlTimestampOf(zonedDateTime, legacyTimestamp) instead:
  // notes: sqlTimestampOf(ZonedDateTime.of(..., ZoneId.of(baseZone.getID())), session)
}

// existing 5th/6th
sqlTimestampOf(zonedDateTime, session/connectorSession) {
  // notes: Remove. Use sqlTimestampOf(zonedDateTime, legacyTimestamp) instead.
}

// existing 7th
sqlTimestampOf(millis, session) {
  // notes: Remove. Use sqlTimestampOf(zonedDateTime, legacyTimestamp) instead.
  // notes: I acknowledge that the following replacement is a bit long. The reason I want to get rid of this one is that the semantics of millis is quite unclear.
  // notes: There are 5 usages total.
  // notes: Change them to `sqlTimestampOf(ZonedDateTime.ofInstant(Instant.ofEpochMilli(epochMilli), ZoneId.of(SESSION.getTimeZoneKey().getId())), ZoneId.of(SESSION.isLegacyTimestamp()))`.
}

[findepi]

@haozhun thanks for your review!

What is the intent behind "Upgrade Presto hive-apache to 1.2.2-2"? A description in the body of the commit will help.

Added this to the cmt msg:

This includes a change that prevents failure when JVM zone had change
on 1970-01-01.

For "Simplify SqlTimestamp construction in tests", I don't quite like the state after your commit.
I would like to see it simplified, whereas the commit adds a new one without removing the existing ones.

All simplifiable caller sites is what got simplified in this commit.

I improved the test code more, but didn't implement your proposal yet. At least, now there are no more overloads than it used to be. I assume this removes the last obstacle for this PR and this cleanup just sits on the TODO list a little longer.