[GitHub] [spark] MaxGekk commented on a change in pull request #32893: [SPARK-35736][SQL] Parse any day-time interval types in SQL

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

MaxGekk commented on a change in pull request #32893:
URL: https://github.com/apache/spark/pull/32893#discussion_r650556913



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
##########
@@ -2514,8 +2514,19 @@ class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with SQLConfHelper with Logg
   }
 
   override def visitDayTimeIntervalDataType(ctx: DayTimeIntervalDataTypeContext): DataType = {
-    // TODO(SPARK-35736): Parse any day-time interval types in SQL
-    DayTimeIntervalType()
+    def strToFieldIndex(str: String): Byte = str match {

Review comment:
       You could avoid hardcoded constants:
   ```scala
   val strToFieldIndex = dayTimeFields.map(i => fieldToString(i) -> i).toMap
   ```
   

##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/types/DataTypeSuite.scala
##########
@@ -258,6 +258,15 @@ class DataTypeSuite extends SparkFunSuite {
 
   checkDataTypeFromDDL(YearMonthIntervalType)
   checkDataTypeFromDDL(DayTimeIntervalType())
+  checkDataTypeFromDDL(DayTimeIntervalType(DayTimeIntervalType.DAY, DayTimeIntervalType.MINUTE))
+  checkDataTypeFromDDL(DayTimeIntervalType(DayTimeIntervalType.DAY, DayTimeIntervalType.HOUR))
+  checkDataTypeFromDDL(DayTimeIntervalType(DayTimeIntervalType.DAY, DayTimeIntervalType.DAY))
+  checkDataTypeFromDDL(DayTimeIntervalType(DayTimeIntervalType.HOUR, DayTimeIntervalType.SECOND))
+  checkDataTypeFromDDL(DayTimeIntervalType(DayTimeIntervalType.HOUR, DayTimeIntervalType.MINUTE))
+  checkDataTypeFromDDL(DayTimeIntervalType(DayTimeIntervalType.HOUR, DayTimeIntervalType.HOUR))
+  checkDataTypeFromDDL(DayTimeIntervalType(DayTimeIntervalType.MINUTE, DayTimeIntervalType.SECOND))
+  checkDataTypeFromDDL(DayTimeIntervalType(DayTimeIntervalType.MINUTE, DayTimeIntervalType.MINUTE))
+  checkDataTypeFromDDL(DayTimeIntervalType(DayTimeIntervalType.SECOND, DayTimeIntervalType.SECOND))

Review comment:
       Please, replace:
   ```suggestion
    dayTimeIntervalTypes.foreach(checkDataTypeFromDDL)
   ```

##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
##########
@@ -2514,8 +2514,19 @@ class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with SQLConfHelper with Logg
   }
 
   override def visitDayTimeIntervalDataType(ctx: DayTimeIntervalDataTypeContext): DataType = {
-    // TODO(SPARK-35736): Parse any day-time interval types in SQL
-    DayTimeIntervalType()
+    def strToFieldIndex(str: String): Byte = str match {
+      case "day" => 0

Review comment:
       Could you replace the magic numbers by the constants from:
   https://github.com/apache/spark/blob/d53831ff5cef618cfbc08dcea84ca9bb61ed9903/sql/catalyst/src/main/scala/org/apache/spark/sql/types/DayTimeIntervalType.scala#L85-L88

##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
##########
@@ -2514,8 +2514,19 @@ class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with SQLConfHelper with Logg
   }
 
   override def visitDayTimeIntervalDataType(ctx: DayTimeIntervalDataTypeContext): DataType = {
-    // TODO(SPARK-35736): Parse any day-time interval types in SQL
-    DayTimeIntervalType()
+    def strToFieldIndex(str: String): Byte = str match {
+      case "day" => 0
+      case "hour" => 1
+      case "minute" => 2
+      case "second" => 3
+    }
+    val from = strToFieldIndex(ctx.from.getText.toLowerCase(Locale.ROOT))
+    val to = if (ctx.to != null ) {
+      strToFieldIndex(ctx.to.getText.toLowerCase(Locale.ROOT))
+    } else {
+      from
+    }
+    DayTimeIntervalType(from, to)

Review comment:
       Could you use `start` and `end` as in the SQL standard.




-- 
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



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