Address make_interval review feedback

Author: peterxcli

docs/source/user-guide/latest/expressions.md

@@ -277,7 +277,7 @@ The type-name conversion functions (`bigint`, `binary`, `boolean`, `date`, `deci
 | `localtimestamp` | ✅ | — |  |
 | `make_date` | ✅ | Native |  |
 | `make_dt_interval` | ✅ | Codegen dispatch |  |
-| `make_interval` | ✅ | Hybrid | Routes through the JVM codegen dispatcher by default; the native path is opt-in via allowIncompatible ([details](compatibility/expressions/datetime.md)) |
+| `make_interval` | ✅ | Hybrid | Routes through the JVM codegen dispatcher by default; intervals outside Arrow's nanosecond range are tracked by [#5279](https://github.com/apache/datafusion-comet/issues/5279); the native path is opt-in via allowIncompatible ([details](compatibility/expressions/datetime.md)) |
 | `make_time` | 🔜 | — | Spark 4.1 TIME type; tracked by [#4288](https://github.com/apache/datafusion-comet/issues/4288) |
 | `make_timestamp` | ✅ | Hybrid |  |
 | `make_timestamp_ltz` | ✅ | — | 2-arg TIME form falls back |

spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala

@@ -1132,16 +1132,9 @@ object QueryPlanSerde extends Logging with CometExprShim with CometTypeShim {
   }
 
   def scalarFunctionExprToProto(funcName: String, args: Option[Expr]*): Option[Expr] = {
-    scalarFunctionExprToProto(funcName, false, args: _*)
-  }
-
-  def scalarFunctionExprToProto(
-      funcName: String,
-      failOnError: Boolean,
-      args: Option[Expr]*): Option[Expr] = {
     val builder = ExprOuterClass.ScalarFunc.newBuilder()
     builder.setFunc(funcName)
-    builder.setFailOnError(failOnError)
+    builder.setFailOnError(false)
     scalarFunctionExprToProto0(builder, args: _*)
   }
 

spark/src/main/scala/org/apache/comet/serde/datetime.scala

@@ -969,6 +969,12 @@ object CometMakeInterval extends CometExpressionSerde[MakeInterval] with Codegen
       " precision, and stores time in nanoseconds, which overflows for large time components" +
       " (hours, minutes, seconds) that Spark can represent."
 
+  override def getCompatibleNotes(): Seq[String] = Seq(
+    "Both the default JVM codegen-dispatch path and the native path encode elapsed time as" +
+      " Arrow nanoseconds. Spark intervals whose microseconds cannot be multiplied by 1,000" +
+      " in an `i64` are not supported" +
+      " ([#5279](https://github.com/apache/datafusion-comet/issues/5279)).")
+
   override def getIncompatibleReasons(): Seq[String] = Seq(incompatReason)
 
   override def getSupportLevel(expr: MakeInterval): SupportLevel =
@@ -979,7 +985,7 @@ object CometMakeInterval extends CometExpressionSerde[MakeInterval] with Codegen
       inputs: Seq[Attribute],
       binding: Boolean): Option[Expr] = {
     // The explicit return type skips DataFusion's registry coercion, but its kernel needs Float64.
-    val children = expr.children.updated(6, Cast(expr.children(6), DoubleType))
+    val children = expr.children.updated(6, Cast(expr.secs, DoubleType))
     val childExprs = children.map(exprToProtoInternal(_, inputs, binding))
     val optExpr = scalarFunctionExprToProtoWithReturnType(
       "make_interval",

spark/src/test/resources/sql-tests/expressions/datetime/make_interval_ansi.sql

@@ -31,8 +31,11 @@ SELECT make_interval(1, 2, 3, 4, 5, 6, 7.123456)
 query
 SELECT make_interval(years) FROM test_make_interval_ansi
 
-query expect_error(ARITHMETIC_OVERFLOW)
+query expect_error(overflow. If necessary set)
 SELECT make_interval(2147483647)
 
-query expect_error(ARITHMETIC_OVERFLOW)
+query expect_error(overflow. If necessary set)
 SELECT make_interval(0, 0, 2147483647)
+
+query ignore(https://github.com/apache/datafusion-comet/issues/5131)
+SELECT make_interval(0, 0, 0, 0, 2562048)

spark/src/test/resources/sql-tests/expressions/datetime/make_interval_dispatch.sql

@@ -0,0 +1,49 @@
+-- Licensed to the Apache Software Foundation (ASF) under one
+-- or more contributor license agreements.  See the NOTICE file
+-- distributed with this work for additional information
+-- regarding copyright ownership.  The ASF licenses this file
+-- to you under the Apache License, Version 2.0 (the
+-- "License"); you may not use this file except in compliance
+-- with the License.  You may obtain a copy of the License at
+--
+--   http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing,
+-- software distributed under the License is distributed on an
+-- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+-- KIND, either express or implied.  See the License for the
+-- specific language governing permissions and limitations
+-- under the License.
+
+-- With allowIncompatible unset, MakeInterval uses Spark's JVM codegen dispatcher.
+
+statement
+CREATE TABLE test_make_interval_dispatch(
+  years int,
+  months int,
+  weeks int,
+  days int,
+  hours int,
+  mins int,
+  secs decimal(18, 6)) USING parquet
+
+statement
+INSERT INTO test_make_interval_dispatch VALUES
+  (1, 2, 3, 4, 5, 6, 7.123456),
+  (0, 1, 0, 1, 0, 0, 100.000001),
+  (-1, -2, -1, -1, -1, -1, -1.500000),
+  (NULL, 1, 2, 3, 4, 5, 6.000000),
+  (2, NULL, 2, 3, 4, 5, 6.000000),
+  (3, 1, 2, 3, 4, 5, NULL),
+  (0, 0, 0, 0, 2562048, 0, 0.000000)
+
+query
+SELECT make_interval(years, months, weeks, days, hours, mins, secs)
+FROM test_make_interval_dispatch
+WHERE hours != 2562048
+ORDER BY years
+
+query ignore(https://github.com/apache/datafusion-comet/issues/5279)
+SELECT make_interval(0, 0, 0, 0, hours)
+FROM test_make_interval_dispatch
+WHERE hours = 2562048

spark/src/test/resources/sql-tests/expressions/datetime/make_interval_dispatch_ansi.sql

@@ -0,0 +1,48 @@
+-- Licensed to the Apache Software Foundation (ASF) under one
+-- or more contributor license agreements.  See the NOTICE file
+-- distributed with this work for additional information
+-- regarding copyright ownership.  The ASF licenses this file
+-- to you under the Apache License, Version 2.0 (the
+-- "License"); you may not use this file except in compliance
+-- with the License.  You may obtain a copy of the License at
+--
+--   http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing,
+-- software distributed under the License is distributed on an
+-- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+-- KIND, either express or implied.  See the License for the
+-- specific language governing permissions and limitations
+-- under the License.
+
+-- Config: spark.sql.ansi.enabled=true
+
+statement
+CREATE TABLE test_make_interval_dispatch_ansi(years int, weeks int, hours int) USING parquet
+
+statement
+INSERT INTO test_make_interval_dispatch_ansi VALUES
+  (1, 0, 5),
+  (2147483647, 0, 0),
+  (0, 2147483647, 0),
+  (0, 0, 2562048)
+
+query
+SELECT make_interval(years, 0, weeks, 0, hours)
+FROM test_make_interval_dispatch_ansi
+WHERE years = 1
+
+query expect_error(overflow. If necessary set)
+SELECT make_interval(years)
+FROM test_make_interval_dispatch_ansi
+WHERE years = 2147483647
+
+query expect_error(overflow. If necessary set)
+SELECT make_interval(0, 0, weeks)
+FROM test_make_interval_dispatch_ansi
+WHERE weeks = 2147483647
+
+query ignore(https://github.com/apache/datafusion-comet/issues/5279)
+SELECT make_interval(0, 0, 0, 0, hours)
+FROM test_make_interval_dispatch_ansi
+WHERE hours = 2562048

spark/src/test/resources/sql-tests/expressions/datetime/try_make_interval.sql

@@ -16,8 +16,24 @@
 -- under the License.
 
 -- MinSparkVersion: 4.0
--- Config: spark.sql.ansi.enabled=true
--- Config: spark.comet.exec.scalaUDF.codegen.enabled=true
+-- ConfigMatrix: spark.sql.ansi.enabled=true,false
+
+statement
+CREATE TABLE test_try_make_interval(
+  years int,
+  months int,
+  weeks int,
+  days int,
+  hours int,
+  mins int,
+  secs decimal(18, 6)) USING parquet
+
+statement
+INSERT INTO test_try_make_interval VALUES
+  (1, 2, 3, 4, 5, 6, 7.123456),
+  (2147483647, 0, 0, 0, 0, 0, 0.000000)
 
 query
-SELECT try_make_interval(2147483647)
+SELECT try_make_interval(years, months, weeks, days, hours, mins, secs)
+FROM test_try_make_interval
+ORDER BY years