Fix second UDF not respecting sub-minute time zone offsets

Author: kevinwilfong

presto-function-server/src/test/java/com/facebook/presto/tests/TestRestRemoteFunctions.java

@@ -75,9 +75,9 @@ public void testRemoteFunctions()
                         .getMaterializedRows().get(0).getField(0).toString(),
                 "1230");
         assertEquals(
-                computeActual(session, "select rest.default.second(CAST('2001-01-02 03:04:05' as timestamp))")
+                computeActual(session, "select rest.default.day(interval '2' day)")
                         .getMaterializedRows().get(0).getField(0).toString(),
-                "5");
+                "2");
         assertEquals(
                 computeActual(session, "select rest.default.length(CAST('AB' AS VARBINARY))")
                         .getMaterializedRows().get(0).getField(0).toString(),

presto-main-base/src/main/java/com/facebook/presto/operator/scalar/DateTimeFunctions.java

@@ -851,41 +851,43 @@ public static long millisecondFromInterval(@SqlType(StandardTypes.INTERVAL_DAY_T
     @Description("second of the minute of the given timestamp")
     @ScalarFunction("second")
     @SqlType(StandardTypes.BIGINT)
-    public static long secondFromTimestamp(@SqlType(StandardTypes.TIMESTAMP) long timestamp)
+    public static long secondFromTimestamp(SqlFunctionProperties properties, @SqlType(StandardTypes.TIMESTAMP) long timestamp)
     {
-        // No need to check isLegacyTimestamp:
-        // * Under legacy semantics, the session zone matters. But a zone always has offset of whole minutes.
-        // * Under new semantics, timestamp is agnostic to the session zone.
-        return SECOND_OF_MINUTE.get(timestamp);
+        if (properties.isLegacyTimestamp()) {
+            return getChronology(properties.getTimeZoneKey()).secondOfMinute().get(timestamp);
+        }
+        else {
+            return SECOND_OF_MINUTE.get(timestamp);
+        }
     }
 
     @Description("second of the minute of the given timestamp")
     @ScalarFunction("second")
     @SqlType(StandardTypes.BIGINT)
     public static long secondFromTimestampWithTimeZone(@SqlType(StandardTypes.TIMESTAMP_WITH_TIME_ZONE) long timestampWithTimeZone)
     {
-        // No need to check the associated zone here. A zone always has offset of whole minutes.
-        return SECOND_OF_MINUTE.get(unpackMillisUtc(timestampWithTimeZone));
+        return unpackChronology(timestampWithTimeZone).secondOfMinute().get(unpackMillisUtc(timestampWithTimeZone));
     }
 
     @Description("second of the minute of the given time")
     @ScalarFunction("second")
     @SqlType(StandardTypes.BIGINT)
-    public static long secondFromTime(@SqlType(StandardTypes.TIME) long time)
+    public static long secondFromTime(SqlFunctionProperties properties, @SqlType(StandardTypes.TIME) long time)
     {
-        // No need to check isLegacyTimestamp:
-        // * Under legacy semantics, the session zone matters. But a zone always has offset of whole minutes.
-        // * Under new semantics, time is agnostic to the session zone.
-        return SECOND_OF_MINUTE.get(time);
+        if (properties.isLegacyTimestamp()) {
+            return getChronology(properties.getTimeZoneKey()).secondOfMinute().get(time);
+        }
+        else {
+            return SECOND_OF_MINUTE.get(time);
+        }
     }
 
     @Description("second of the minute of the given time")
     @ScalarFunction("second")
     @SqlType(StandardTypes.BIGINT)
-    public static long secondFromTimeWithTimeZone(@SqlType(StandardTypes.TIME_WITH_TIME_ZONE) long time)
+    public static long secondFromTimeWithTimeZone(@SqlType(StandardTypes.TIME_WITH_TIME_ZONE) long timeWithTimeZone)
     {
-        // No need to check the associated zone here. A zone always has offset of whole minutes.
-        return SECOND_OF_MINUTE.get(unpackMillisUtc(time));
+        return unpackChronology(timeWithTimeZone).secondOfMinute().get(unpackMillisUtc(timeWithTimeZone));
     }
 
     @Description("second of the minute of the given interval")

presto-main-base/src/test/java/com/facebook/presto/operator/scalar/TestDateTimeFunctionsBase.java

@@ -382,6 +382,19 @@ public void testPartFunctions()
         assertFunction("hour(" + INTERVAL_LITERAL + ")", BIGINT, DAY_TO_SECOND_INTERVAL.getSeconds() / 3600 % 24);
     }
 
+    @Test
+    public void testPartFunctionsWithSecondsOffset()
+    {
+        // Prior to 1920 Asia/Kathmandu's offset was 5:41:16
+        DateTime dateTime = new DateTime(1910, 1, 22, 3, 4, 5, 678, KATHMANDU_ZONE);
+        String dateTimeLiteral = "TIMESTAMP '1910-01-22 03:04:05.678 Asia/Kathmandu'";
+
+        assertFunction("millisecond(" + dateTimeLiteral + ")", BIGINT, (long) dateTime.getMillisOfSecond());
+        assertFunction("second(" + dateTimeLiteral + ")", BIGINT, (long) dateTime.getSecondOfMinute());
+        assertFunction("minute(" + dateTimeLiteral + ")", BIGINT, (long) dateTime.getMinuteOfHour());
+        assertFunction("hour(" + dateTimeLiteral + ")", BIGINT, (long) dateTime.getHourOfDay());
+    }
+
     @Test
     public void testYearOfWeek()
     {

presto-main-base/src/test/java/com/facebook/presto/sql/gen/TestExpressionCompiler.java

@@ -1536,7 +1536,7 @@ private static long callExtractFunction(ConnectorSession session, long value, Fi
             case MINUTE:
                 return DateTimeFunctions.minuteFromTimestamp(session.getSqlFunctionProperties(), value);
             case SECOND:
-                return DateTimeFunctions.secondFromTimestamp(value);
+                return DateTimeFunctions.secondFromTimestamp(session.getSqlFunctionProperties(), value);
             case TIMEZONE_MINUTE:
                 return DateTimeFunctions.timeZoneMinuteFromTimestampWithTimeZone(packDateTimeWithZone(value, session.getSqlFunctionProperties().getTimeZoneKey()));
             case TIMEZONE_HOUR: