Move Usage of Default Timezone From Parser to Evaluator (partiql#448)

noCharger · Sep 24, 2021 · 6301396 · 6301396
1 parent 68e9f61
commit 6301396
Showing 8 changed files with 91 additions and 81 deletions.
diff --git a/lang/resources/org/partiql/type-domains/partiql.ion b/lang/resources/org/partiql/type-domains/partiql.ion
@@ -125,7 +125,7 @@
         // end of sum expr
 
         // Time
-        (product time_value hour::int minute::int second::int nano::int precision::int tz_minutes::(? int))
+        (product time_value hour::int minute::int second::int nano::int precision::int with_time_zone::bool tz_minutes::(? int))
 
         // A "step" within a path expression; that is the components of the expression following the root.
         (sum path_step

diff --git a/lang/src/org/partiql/lang/ast/ExprNodeToStatement.kt b/lang/src/org/partiql/lang/ast/ExprNodeToStatement.kt
@@ -183,6 +183,7 @@ fun ExprNode.toAstExpr(): PartiqlAst.Expr {
                             node.second.toLong(),
                             node.nano.toLong(),
                             node.precision.toLong(),
+                            node.with_time_zone,
                             node.tz_minutes?.toLong()
                         )
                     )

diff --git a/lang/src/org/partiql/lang/ast/StatementToExprNode.kt b/lang/src/org/partiql/lang/ast/StatementToExprNode.kt
@@ -201,6 +201,7 @@ private class StatementTransformer(val ion: IonSystem) {
                     value.second.value.toInt(),
                     value.nano.value.toInt(),
                     value.precision.value.toInt(),
+                    value.withTimeZone.value,
                     value.tzMinutes?.value?.toInt(),
                     metas
                 )

diff --git a/lang/src/org/partiql/lang/ast/ast.kt b/lang/src/org/partiql/lang/ast/ast.kt
@@ -1020,15 +1020,17 @@ sealed class DateTimeType : ExprNode() {
      * @param second represents the second value.
      * @param nano represents the fractional part of the second up to the nanoseconds' precision.
      * @param precision is an optional parameter which, if specified, represents the precision of the fractional second.
-     * @param tz_minutes is the optional time zone in minutes which can be specified with "WITH TIME ZONE".
-     * If [tz_minutes] is null, that means the time zone is undefined.
+     * @param with_time_zone is a boolean to decide whether the time has a time zone. 
+     * True represents "TIME WITH TIME ZONE", while false represents "TIME WITHOUT TIME ZONE". 
+     * @param tz_minutes is the optional time zone in minutes which can be explicitly specified with "WITH TIME ZONE".
      */
     data class Time(
         val hour: Int,
         val minute: Int,
         val second: Int,
         val nano: Int,
         val precision: Int,
+        val with_time_zone: Boolean,
         val tz_minutes: Int? = null,
         override val metas: MetaContainer
     ) : DateTimeType() {

diff --git a/lang/src/org/partiql/lang/ast/passes/AstRewriterBase.kt b/lang/src/org/partiql/lang/ast/passes/AstRewriterBase.kt
@@ -458,6 +458,7 @@ open class AstRewriterBase : AstRewriter {
             node.second,
             node.nano,
             node.precision,
+            node.with_time_zone,
             node.tz_minutes,
             rewriteMetas(node)
         )

diff --git a/lang/src/org/partiql/lang/eval/EvaluatingCompiler.kt b/lang/src/org/partiql/lang/eval/EvaluatingCompiler.kt
@@ -28,6 +28,7 @@ import org.partiql.lang.eval.time.Time
 import org.partiql.lang.eval.visitors.PartiqlAstSanityValidator
 import org.partiql.lang.syntax.SqlParser
 import org.partiql.lang.util.*
+import org.partiql.lang.util.DEFAULT_TIMEZONE_OFFSET
 import java.math.*
 import java.util.*
 import kotlin.collections.*
@@ -1999,9 +2000,19 @@ internal class EvaluatingCompiler(
     }
 
     private fun compileTime(node: DateTimeType.Time) : ThunkEnv {
-        val (hour, minute, second, nano, precision, tz_minutes, metas) = node
+        val (hour, minute, second, nano, precision, with_time_zone, tz_minutes, metas) = node
         return thunkFactory.thunkEnv(metas) {
-            valueFactory.newTime(Time.of(hour, minute, second, nano, precision, tz_minutes))
+            // Add the default time zone if the type "TIME WITH TIME ZONE" does not have an explicitly specified time zone.
+            valueFactory.newTime(
+                Time.of(
+                    hour,
+                    minute,
+                    second,
+                    nano,
+                    precision,
+                    if (with_time_zone && tz_minutes == null) DEFAULT_TIMEZONE_OFFSET.totalMinutes else tz_minutes
+                )
+            )
         }
     }
 

diff --git a/lang/src/org/partiql/lang/syntax/SqlParser.kt b/lang/src/org/partiql/lang/syntax/SqlParser.kt
@@ -34,6 +34,7 @@ import java.time.format.DateTimeFormatter.*
 import java.time.*
 import java.time.format.DateTimeFormatter
 import java.time.format.DateTimeParseException
+import java.time.temporal.Temporal
 
 
 /**
@@ -700,13 +701,19 @@ class SqlParser(private val ion: IonSystem) : Parser {
                 val timeString = token!!.text!!
                 val precision = children[0].token!!.value!!.numberValue().toInt()
                 val time = LocalTime.parse(timeString, DateTimeFormatter.ISO_TIME)
-                DateTimeType.Time(time.hour, time.minute, time.second, time.nano, precision, null, metas)
+                DateTimeType.Time(time.hour, time.minute, time.second, time.nano, precision, false, null, metas)
             }
             TIME_WITH_TIME_ZONE -> {
                 val timeString = token!!.text!!
                 val precision = children[0].token?.value?.numberValue()?.toInt()
-                val time = OffsetTime.parse(timeString)
-                DateTimeType.Time(time.hour, time.minute, time.second, time.nano, precision!!, time.offset.totalSeconds/60, metas)
+                try {
+                    val time = OffsetTime.parse(timeString)
+                    DateTimeType.Time(time.hour, time.minute, time.second, time.nano, precision!!, true, time.offset.totalSeconds/60, metas)
+                } catch (e: DateTimeParseException) {
+                    // In case time zone not explicitly specified
+                    val time = LocalTime.parse(timeString)
+                    DateTimeType.Time(time.hour, time.minute, time.second, time.nano, precision!!, true, null, metas)
+                }
             }
             else -> unsupported("Unsupported syntax for $type", PARSE_UNSUPPORTED_SYNTAX)
         }
@@ -2374,10 +2381,10 @@ class SqlParser(private val ion: IonSystem) : Parser {
 
         var rem = this
 
-        // Parses the time string without the time zone offset.
-        fun tryLocalTimeParsing(time: String?) {
+        // Parses the time string with or without the time zone offset.
+        fun tryTimeParsing(time: String?, formatter: DateTimeFormatter, parse: (String?, DateTimeFormatter) -> Temporal) {
             try {
-                LocalTime.parse(time, DateTimeFormatter.ISO_TIME)
+                parse(time, formatter)
             }
             catch (e: DateTimeParseException) {
                 rem.head.err(e.localizedMessage, PARSE_INVALID_TIME_STRING)
@@ -2389,7 +2396,7 @@ class SqlParser(private val ion: IonSystem) : Parser {
         rem = precision.remaining
 
         // 2. Check for optional "with time zone" tokens and store the boolean
-        val (remainingAfterOptionalTimeZone, isTimeZoneSpecified) = rem.checkForOptionalTimeZone()
+        val (remainingAfterOptionalTimeZone, withTimeZone) = rem.checkForOptionalTimeZone()
         rem = remainingAfterOptionalTimeZone
 
         val timeStringToken = rem.head
@@ -2407,35 +2414,25 @@ class SqlParser(private val ion: IonSystem) : Parser {
             rem.head.err("Invalid format for time string. Expected format is \"TIME [(p)] [WITH TIME ZONE] HH:MM:SS[.ddddd...][+|-HH:MM]\"",
                 PARSE_INVALID_TIME_STRING)
         }
-        var newTimeString = timeString
-        when(isTimeZoneSpecified) {
-            false -> tryLocalTimeParsing(timeString)
-            true -> try {
-                OffsetTime.parse(timeString, DateTimeFormatter.ISO_TIME)
-            } catch (e: DateTimeParseException) {
-                // The exception thrown here is because of the invalid time or time zone offset specified in the timestring.
-                // The valid time zone offsets are in the range of [-18:00 - 18:00]
-                if (timeWithoutTimeZoneRegex.matches(timeString)) {
-                    // Fall back on parsing a string without a time zone offset only if the offset is not specified.
-                    // Add default timezone offset in that case.
-                    tryLocalTimeParsing(timeString)
-                    newTimeString = timeString + DEFAULT_TIMEZONE_OFFSET.getOffsetHHmm()
-                }
-                else {
-                    rem.head.err(e.localizedMessage, PARSE_INVALID_TIME_STRING)
-                }
-            }
+        // For "TIME WITH TIME ZONE", if the time zone is not explicitly specified, we still consider it as valid.
+        // We will add the default time zone to it later in the evaluation phase.
+        if (!withTimeZone || timeWithoutTimeZoneRegex.matches(timeString)) {
+            tryTimeParsing(timeString, ISO_TIME, LocalTime::parse)
+        }
+        else {
+            tryTimeParsing(timeString, ISO_TIME, OffsetTime::parse)
         }
+
         // Extract the precision from the time string representation if the precision is not specified.
         // For e.g., TIME '23:12:12.12300' should have precision of 5.
         // The source span here is just the filler value and does not reflect the actual source location of the precision
         // as it does not exists in case the precision is unspecified.
         val precisionOfValue =  precision.token ?:
-            Token(LITERAL, ion.newInt(getPrecisionFromTimeString(newTimeString)), timeStringToken.span)
+            Token(LITERAL, ion.newInt(getPrecisionFromTimeString(timeString)), timeStringToken.span)
 
         return ParseNode(
-            if (isTimeZoneSpecified) TIME_WITH_TIME_ZONE else TIME,
-            rem.head!!.copy(value = ion.newString(newTimeString)),
+            if (withTimeZone) TIME_WITH_TIME_ZONE else TIME,
+            rem.head!!.copy(value = ion.newString(timeString)),
             listOf(precision.copy(token = precisionOfValue)),
             rem.tail)
     }

diff --git a/lang/test/org/partiql/lang/syntax/SqlParserDateTimeTests.kt b/lang/test/org/partiql/lang/syntax/SqlParserDateTimeTests.kt
@@ -8,13 +8,10 @@ import org.partiql.lang.domains.id
 import java.util.*
 import org.partiql.lang.errors.ErrorCode
 import org.partiql.lang.errors.Property
-import org.partiql.lang.util.DEFAULT_TIMEZONE_OFFSET
 import org.partiql.lang.util.to
 
 class SqlParserDateTimeTests : SqlParserTestBase() {
 
-    private val defaultTimeZoneOffset = (DEFAULT_TIMEZONE_OFFSET.totalSeconds / 60).toLong()
-
     data class DateTimeTestCase(val source: String, val skipTest: Boolean = false, val block: PartiqlAst.Builder.() -> PartiqlAst.PartiqlAstNode)
     private data class Date(val year: Int, val month: Int, val day: Int)
 
@@ -57,140 +54,140 @@ class SqlParserDateTimeTests : SqlParserTestBase() {
             )
         },
         DateTimeTestCase("TIME '02:30:59'") {
-            litTime(timeValue(2, 30, 59, 0, 0, null))
+            litTime(timeValue(2, 30, 59, 0, 0, false, null))
         },
         DateTimeTestCase("TIME (3) '12:59:31'") {
-            litTime(timeValue(12, 59, 31, 0, 3, null))
+            litTime(timeValue(12, 59, 31, 0, 3, false, null))
         },
         DateTimeTestCase("TIME '23:59:59.9999'") {
-            litTime(timeValue(23, 59, 59, 999900000, 4, null))
+            litTime(timeValue(23, 59, 59, 999900000, 4, false, null))
         },
         DateTimeTestCase("TIME (7) '23:59:59.123456789'") {
-            litTime(timeValue(23, 59, 59, 123456789, 7, null))
+            litTime(timeValue(23, 59, 59, 123456789, 7, false, null))
         },
         DateTimeTestCase("TIME (9) '23:59:59.123456789'") {
-            litTime(timeValue(23, 59, 59, 123456789, 9, null))
+            litTime(timeValue(23, 59, 59, 123456789, 9, false, null))
         },
         DateTimeTestCase("TIME (0) '23:59:59.123456789'") {
-            litTime(timeValue(23, 59, 59, 123456789, 0, null))
+            litTime(timeValue(23, 59, 59, 123456789, 0, false, null))
         },
         DateTimeTestCase("TIME '02:30:59-05:30'") {
-            litTime(timeValue(2, 30, 59, 0, 0, null))
+            litTime(timeValue(2, 30, 59, 0, 0, false, null))
         },
         DateTimeTestCase("TIME '02:30:59+05:30'") {
-            litTime(timeValue(2, 30, 59, 0, 0, null))
+            litTime(timeValue(2, 30, 59, 0, 0, false, null))
         },
         DateTimeTestCase("TIME '02:30:59-14:39'") {
-            litTime(timeValue(2, 30, 59, 0, 0, null))
+            litTime(timeValue(2, 30, 59, 0, 0, false, null))
         },
         DateTimeTestCase("TIME '02:30:59+00:00'") {
-            litTime(timeValue(2, 30, 59, 0, 0, null))
+            litTime(timeValue(2, 30, 59, 0, 0, false, null))
         },
         DateTimeTestCase("TIME '02:30:59-00:00'") {
-            litTime(timeValue(2, 30, 59, 0, 0, null))
+            litTime(timeValue(2, 30, 59, 0, 0, false, null))
         },
         DateTimeTestCase("TIME (3) '12:59:31+10:30'") {
-            litTime(timeValue(12, 59, 31, 0, 3, null))
+            litTime(timeValue(12, 59, 31, 0, 3, false, null))
         },
         DateTimeTestCase("TIME (0) '00:00:00+00:00'") {
-            litTime(timeValue(0, 0, 0, 0, 0, null))
+            litTime(timeValue(0, 0, 0, 0, 0, false, null))
         },
         DateTimeTestCase("TIME (0) '00:00:00-00:00'") {
-            litTime(timeValue(0, 0, 0, 0, 0, null))
+            litTime(timeValue(0, 0, 0, 0, 0, false, null))
         },
         DateTimeTestCase("TIME '23:59:59.9999-11:59'") {
-            litTime(timeValue(23, 59, 59, 999900000, 4, null))
+            litTime(timeValue(23, 59, 59, 999900000, 4, false, null))
         },
         DateTimeTestCase("TIME '23:59:59.99990-11:59'") {
-            litTime(timeValue(23, 59, 59, 999900000, 5, null))
+            litTime(timeValue(23, 59, 59, 999900000, 5, false, null))
         },
         DateTimeTestCase("TIME (5) '23:59:59.9999-11:59'") {
-            litTime(timeValue(23, 59, 59, 999900000, 5, null))
+            litTime(timeValue(23, 59, 59, 999900000, 5, false, null))
         },
         DateTimeTestCase("TIME (7) '23:59:59.123456789+01:00'") {
-            litTime(timeValue(23, 59, 59, 123456789, 7, null))
+            litTime(timeValue(23, 59, 59, 123456789, 7, false, null))
         },
         DateTimeTestCase("TIME (9) '23:59:59.123456789-14:50'") {
-            litTime(timeValue(23, 59, 59, 123456789, 9, null))
+            litTime(timeValue(23, 59, 59, 123456789, 9, false, null))
         },
         DateTimeTestCase("TIME (0) '23:59:59.123456789-18:00'") {
-            litTime(timeValue(23, 59, 59, 123456789, 0, null))
+            litTime(timeValue(23, 59, 59, 123456789, 0, false, null))
         },
         DateTimeTestCase("TIME WITH TIME ZONE '02:30:59'") {
-            litTime(timeValue(2, 30, 59, 0, 0, defaultTimeZoneOffset))
+            litTime(timeValue(2, 30, 59, 0, 0, true, null))
         },
         DateTimeTestCase("TIME (3) WITH TIME ZONE '12:59:31'") {
-            litTime(timeValue(12, 59, 31, 0, 3, defaultTimeZoneOffset))
+            litTime(timeValue(12, 59, 31, 0, 3, true, null))
         },
         DateTimeTestCase("TIME WITH TIME ZONE '23:59:59.9999'") {
-            litTime(timeValue(23, 59, 59, 999900000, 4, defaultTimeZoneOffset))
+            litTime(timeValue(23, 59, 59, 999900000, 4, true, null))
         },
         DateTimeTestCase("TIME (7) WITH TIME ZONE '23:59:59.123456789'") {
-            litTime(timeValue(23, 59, 59, 123456789, 7, defaultTimeZoneOffset))
+            litTime(timeValue(23, 59, 59, 123456789, 7, true, null))
         },
         DateTimeTestCase("TIME (9) WITH TIME ZONE '23:59:59.123456789'") {
-            litTime(timeValue(23, 59, 59, 123456789, 9, defaultTimeZoneOffset))
+            litTime(timeValue(23, 59, 59, 123456789, 9, true, null))
         },
         DateTimeTestCase("TIME (0) WITH TIME ZONE '23:59:59.123456789'") {
-            litTime(timeValue(23, 59, 59, 123456789, 0, defaultTimeZoneOffset))
+            litTime(timeValue(23, 59, 59, 123456789, 0, true, null))
         },
         DateTimeTestCase("TIME (0) WITH TIME ZONE '00:00:00+00:00'") {
-            litTime(timeValue(0, 0, 0, 0, 0, 0))
+            litTime(timeValue(0, 0, 0, 0, 0, true, 0))
         },
         DateTimeTestCase("TIME (0) WITH TIME ZONE '00:00:00.0000-00:00'") {
-            litTime(timeValue(0, 0, 0, 0, 0, 0))
+            litTime(timeValue(0, 0, 0, 0, 0, true, 0))
         },
         DateTimeTestCase("TIME WITH TIME ZONE '02:30:59.1234500-05:30'") {
-            litTime(timeValue(2, 30, 59, 123450000, 7, -330))
+            litTime(timeValue(2, 30, 59, 123450000, 7, true, -330))
         },
         DateTimeTestCase("TIME WITH TIME ZONE '02:30:59+05:30'") {
-            litTime(timeValue(2, 30, 59, 0, 0, 330))
+            litTime(timeValue(2, 30, 59, 0, 0, true, 330))
         },
         DateTimeTestCase("TIME WITH TIME ZONE '02:30:59-14:39'") {
-            litTime(timeValue(2, 30, 59, 0, 0, -879))
+            litTime(timeValue(2, 30, 59, 0, 0, true, -879))
         },
         DateTimeTestCase("TIME WITH TIME ZONE '23:59:59.9999-11:59'") {
-            litTime(timeValue(23, 59, 59, 999900000, 4, -719))
+            litTime(timeValue(23, 59, 59, 999900000, 4, true, -719))
         },
         DateTimeTestCase("TIME WITH TIME ZONE '23:59:59.99990-11:59'") {
-            litTime(timeValue(23, 59, 59, 999900000, 5, -719))
+            litTime(timeValue(23, 59, 59, 999900000, 5, true, -719))
         },
         DateTimeTestCase("TIME (5) WITH TIME ZONE '23:59:59.9999-11:59'") {
-            litTime(timeValue(23, 59, 59, 999900000, 5, -719))
+            litTime(timeValue(23, 59, 59, 999900000, 5, true, -719))
         },
         DateTimeTestCase("TIME (7) WITH TIME ZONE '23:59:59.123456789+01:00'") {
-            litTime(timeValue(23, 59, 59, 123456789, 7, 60))
+            litTime(timeValue(23, 59, 59, 123456789, 7, true, 60))
         },
         DateTimeTestCase("TIME (9) WITH TIME ZONE '23:59:59.123456789-14:50'") {
-            litTime(timeValue(23, 59, 59, 123456789, 9, -890))
+            litTime(timeValue(23, 59, 59, 123456789, 9, true, -890))
         },
         DateTimeTestCase("TIME (0) WITH TIME ZONE '23:59:59.123456789-18:00'") {
-            litTime(timeValue(23, 59, 59, 123456789, 0, -1080))
+            litTime(timeValue(23, 59, 59, 123456789, 0, true, -1080))
         },
         // TODO: These tests should pass. Check https://github.com/partiql/partiql-lang-kotlin/issues/395
         DateTimeTestCase("TIME '23:59:59.1234567890'", skipTest = true) {
-            litTime(timeValue(23, 59, 59, 123456789, 9, null))
+            litTime(timeValue(23, 59, 59, 123456789, 9, false, null))
         },
         DateTimeTestCase("TIME '23:59:59.1234567899'", skipTest = true) {
-            litTime(timeValue(23, 59, 59, 123456790, 9, null))
+            litTime(timeValue(23, 59, 59, 123456790, 9, false, null))
         },
         DateTimeTestCase("TIME '23:59:59.1234567890+18:00'", skipTest = true) {
-            litTime(timeValue(23, 59, 59, 123456789, 9, null))
+            litTime(timeValue(23, 59, 59, 123456789, 9, false, null))
         },
         DateTimeTestCase("TIME '23:59:59.1234567899+18:00'", skipTest = true) {
-            litTime(timeValue(23, 59, 59, 123456790, 9, null))
+            litTime(timeValue(23, 59, 59, 123456790, 9, false, null))
         },
         DateTimeTestCase("TIME WITH TIME ZONE '23:59:59.1234567890'", skipTest = true) {
-            litTime(timeValue(23, 59, 59, 123456789, 9, defaultTimeZoneOffset))
+            litTime(timeValue(23, 59, 59, 123456789, 9, true, null))
         },
         DateTimeTestCase("TIME WITH TIME ZONE '23:59:59.1234567899'", skipTest = true) {
-            litTime(timeValue(23, 59, 59, 123456790, 9, defaultTimeZoneOffset))
+            litTime(timeValue(23, 59, 59, 123456790, 9, true, null))
         },
         DateTimeTestCase("TIME WITH TIME ZONE '23:59:59.1234567890+18:00'", skipTest = true) {
-            litTime(timeValue(23, 59, 59, 123456789, 9, 1080))
+            litTime(timeValue(23, 59, 59, 123456789, 9, true, 1080))
         },
         DateTimeTestCase("TIME WITH TIME ZONE '23:59:59.1234567899+18:00'", skipTest = true) {
-            litTime(timeValue(23, 59, 59, 123456790, 9, 1080))
+            litTime(timeValue(23, 59, 59, 123456790, 9, true, 1080))
         }
     )