[GitHub] [spark] MaxGekk commented on a change in pull request #26285: [SPARK-29623][SQL] do not allow multiple unit TO unit statements in interval literal syntax

[GitBox <gi...@apache.org>]

MaxGekk commented on a change in pull request #26285: [SPARK-29623][SQL] do not allow multiple unit TO unit statements in interval literal syntax
URL: https://github.com/apache/spark/pull/26285#discussion_r340144603
 
 

 ##########
 File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
 ##########
 @@ -1929,72 +1916,98 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging
     }
   }
 
-  /**
-   * Create a [[CalendarInterval]] literal expression. An interval expression can contain multiple
-   * unit value pairs, for instance: interval 2 months 2 days.
-   */
   override def visitInterval(ctx: IntervalContext): Literal = withOrigin(ctx) {
-    val intervals = ctx.intervalField.asScala.map(visitIntervalField)
-    validate(intervals.nonEmpty, "at least one time unit should be given for interval literal", ctx)
-    Literal(intervals.reduce(_.add(_)))
+    if (ctx.errorCapturingMultiUnitsInterval != null) {
+      val innerCtx = ctx.errorCapturingMultiUnitsInterval
+      if (innerCtx.unitToUnitInterval != null) {
+        throw new ParseException(
+          "Can only have a single unit TO unit statement in the interval literal syntax",
+          innerCtx.unitToUnitInterval)
+      }
+      Literal(visitMultiUnitsInterval(innerCtx.multiUnitsInterval))
+    } else {
+      assert(ctx.errorCapturingUnitToUnitInterval != null)
+      val innerCtx = ctx.errorCapturingUnitToUnitInterval
+      if (innerCtx.error1 != null || innerCtx.error2 != null) {
+        val errorCtx = if (innerCtx.error1 != null) innerCtx.error1 else innerCtx.error2
+        throw new ParseException(
+          "Can only have a single unit TO unit statement in the interval literal syntax",
+          errorCtx)
+      }
+      Literal(visitUnitToUnitInterval(innerCtx.body))
+    }
   }
 
   /**
-   * Create a [[CalendarInterval]] for a unit value pair. Two unit configuration types are
-   * supported:
-   * - Single unit.
-   * - From-To unit ('YEAR TO MONTH', 'DAY TO HOUR', 'DAY TO MINUTE', 'DAY TO SECOND',
-   * 'HOUR TO MINUTE', 'HOUR TO SECOND' and 'MINUTE TO SECOND' are supported).
+   * Creates a [[CalendarInterval]] literal expression with multiple units, e.g. 1 YEAR 2 DAYS.
    */
-  override def visitIntervalField(ctx: IntervalFieldContext): CalendarInterval = withOrigin(ctx) {
-    import ctx._
-    val s = getIntervalValue(value)
-    try {
-      val unitText = unit.getText.toLowerCase(Locale.ROOT)
-      val interval = (unitText, Option(to).map(_.getText.toLowerCase(Locale.ROOT))) match {
-        case (u, None) =>
-          CalendarInterval.fromUnitStrings(Array(normalizeInternalUnit(u)), Array(s))
-        case ("year", Some("month")) =>
-          CalendarInterval.fromYearMonthString(s)
-        case ("day", Some("hour")) =>
-          CalendarInterval.fromDayTimeString(s, "day", "hour")
-        case ("day", Some("minute")) =>
-          CalendarInterval.fromDayTimeString(s, "day", "minute")
-        case ("day", Some("second")) =>
-          CalendarInterval.fromDayTimeString(s, "day", "second")
-        case ("hour", Some("minute")) =>
-          CalendarInterval.fromDayTimeString(s, "hour", "minute")
-        case ("hour", Some("second")) =>
-          CalendarInterval.fromDayTimeString(s, "hour", "second")
-        case ("minute", Some("second")) =>
-          CalendarInterval.fromDayTimeString(s, "minute", "second")
-        case (from, Some(t)) =>
-          throw new ParseException(s"Intervals FROM $from TO $t are not supported.", ctx)
+  override def visitMultiUnitsInterval(ctx: MultiUnitsIntervalContext): CalendarInterval = {
+    withOrigin(ctx) {
+      val units = ctx.intervalUnit().asScala.map { unit =>
+        val u = unit.getText.toLowerCase(Locale.ROOT)
+        // Handle plural forms, e.g: yearS/monthS/weekS/dayS/hourS/minuteS/hourS/...
+        if (u.endsWith("s")) u.substring(0, u.length - 1) else u
+      }.toArray
+
+      val values = ctx.intervalValue().asScala.map { value =>
+        if (value.STRING() != null) {
+          string(value.STRING())
+        } else {
+          value.getText
+        }
+      }.toArray
+
+      try {
+        CalendarInterval.fromUnitStrings(units, values)
+      } catch {
+        case i: IllegalArgumentException =>
+          val e = new ParseException(i.getMessage, ctx)
+          e.setStackTrace(i.getStackTrace)
+          throw e
       }
-      validate(interval != null, "No interval can be constructed", ctx)
-      interval
-    } catch {
-      // Handle Exceptions thrown by CalendarInterval
-      case e: IllegalArgumentException =>
-        val pe = new ParseException(e.getMessage, ctx)
-        pe.setStackTrace(e.getStackTrace)
-        throw pe
     }
   }
 
-  private def getIntervalValue(value: IntervalValueContext): String = {
-    if (value.STRING() != null) {
-      string(value.STRING())
-    } else {
-      value.getText
+  /**
+   * Creates a [[CalendarInterval]] literal expression with the unit TO unit syntax,
+   * e.g. '2-1' YEAR TO MONTH.
+   */
+  override def visitUnitToUnitInterval(ctx: UnitToUnitIntervalContext): CalendarInterval = {
+    withOrigin(ctx) {
+      val value = string(ctx.STRING())
+      try {
+        val from = ctx.from.getText.toLowerCase(Locale.ROOT)
+        val to = ctx.to.getText.toLowerCase(Locale.ROOT)
+        val interval = (from, to) match {
+          case ("year", "month") =>
+            CalendarInterval.fromYearMonthString(value)
+          case ("day", "hour") =>
+            CalendarInterval.fromDayTimeString(value, "day", "hour")
+          case ("day", "minute") =>
+            CalendarInterval.fromDayTimeString(value, "day", "minute")
+          case ("day", "second") =>
+            CalendarInterval.fromDayTimeString(value, "day", "second")
+          case ("hour", "minute") =>
+            CalendarInterval.fromDayTimeString(value, "hour", "minute")
+          case ("hour", "second") =>
+            CalendarInterval.fromDayTimeString(value, "hour", "second")
+          case ("minute", "second") =>
+            CalendarInterval.fromDayTimeString(value, "minute", "second")
+          case _ =>
+            throw new ParseException(s"Intervals $from TO $to are not supported.", ctx)
+        }
+        validate(interval != null, "No interval can be constructed", ctx)
 
 Review comment:
   Which of `fromYearMonthString` or `fromDayTimeString` can return `null`? They either throw an exception or return an interval.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org