andygrove commented on code in PR #5039:
URL: https://github.com/apache/datafusion-comet/pull/5039#discussion_r3722969763
##########
spark/src/main/scala/org/apache/comet/serde/datetime.scala:
##########
@@ -963,6 +963,33 @@ object CometMakeYMInterval extends
CometCodegenDispatch[MakeYMInterval]
object CometMakeDTInterval extends CometCodegenDispatch[MakeDTInterval]
+object CometMakeInterval extends CometExpressionSerde[MakeInterval] with
CodegenDispatchFallback {
+ private val incompatReason =
+ "The native implementation converts seconds to `Float64`, which can lose
microsecond" +
+ " precision, and stores time in nanoseconds, which overflows for large
time components" +
+ " (hours, minutes, seconds) that Spark can represent."
+
+ override def getIncompatibleReasons(): Seq[String] = Seq(incompatReason)
+
+ override def getSupportLevel(expr: MakeInterval): SupportLevel =
+ Incompatible(Some(incompatReason))
+
+ override def convert(
+ expr: MakeInterval,
+ 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))
Review Comment:
`MakeInterval` exposes `secs` as a named field, so `expr.children.updated(6,
Cast(expr.secs, DoubleType))` would say what it means without pinning the child
ordering. Same result, one less thing to break if the case class ever gains a
field.
##########
spark/src/main/scala/org/apache/comet/serde/datetime.scala:
##########
@@ -963,6 +963,33 @@ object CometMakeYMInterval extends
CometCodegenDispatch[MakeYMInterval]
object CometMakeDTInterval extends CometCodegenDispatch[MakeDTInterval]
+object CometMakeInterval extends CometExpressionSerde[MakeInterval] with
CodegenDispatchFallback {
Review Comment:
The default dispatch path has a range limit of its own, and I do not think
the checkmark in `expressions.md` covers it.
I built this branch and ran `SELECT make_interval(0, 0, 0, 0, h) FROM t`
over a Parquet column holding `2562048`, with no configs set, so the default
route. Spark returns a valid interval. Comet aborts the stage.
```
Exception thrown while executing query in Comet:
...
+- CometNativeScan parquet spark_catalog.default.zzprobe[h#8] ...
Caused by: java.lang.ArithmeticException: long overflow
at java.base/java.lang.Math.multiplyExact(Math.java:1004)
at ...GeneratedClass$SpecificCometBatchKernel.process(Unknown Source)
at
org.apache.comet.udf.codegen.CometScalaUDFCodegen.evaluate(CometScalaUDFCodegen.scala:129)
at org.apache.comet.udf.CometUdfBridge.evaluate(CometUdfBridge.java:122)
```
The source is `CometBatchKernelCodegenOutput.scala:222-229`, which writes
`CalendarIntervalType` into an `IntervalMonthDayNanoVector` as
`Math.multiplyExact(interval.microseconds, 1000L)`. That caps micros at
9,223,372,036,854,775, about 292 years, while Spark's
`IntervalUtils.makeInterval` accumulates micros with no such ceiling. It is the
same #5131 root cause on the compatible path, at a 1000x higher threshold, and
it surfaces as a raw uncaught `ArithmeticException` rather than a NULL. Note it
happens with `spark.sql.ansi.enabled=false`, and it is not a Spark-classed
error, so it does not read as an overflow to a user.
This is a constraint of Comet's `CalendarIntervalType` Arrow representation
rather than something this PR introduced, but this PR is the first thing that
lets a user construct an arbitrary `CalendarInterval`, so it is where the
caveat becomes reachable. Could you file an issue for it, widen the
`expressions.md` note to mention the default path's ceiling, and add the case
to the dispatch fixture as `query ignore(<new issue>)` so it is pinned for the
next reader?
##########
spark/src/test/resources/sql-tests/expressions/datetime/make_interval_ansi.sql:
##########
@@ -0,0 +1,38 @@
+-- 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.
+
+-- Native ANSI execution must preserve Spark's overflow exception.
+-- Config: spark.sql.ansi.enabled=true
+-- Config: spark.comet.expression.MakeInterval.allowIncompatible=true
+
+statement
+CREATE TABLE test_make_interval_ansi(years int) USING parquet
+
+statement
+INSERT INTO test_make_interval_ansi VALUES (NULL)
+
+query
+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)
+SELECT make_interval(2147483647)
+
+query expect_error(ARITHMETIC_OVERFLOW)
+SELECT make_interval(0, 0, 2147483647)
Review Comment:
My earlier suggestion to tighten these to `ARITHMETIC_OVERFLOW` was wrong,
sorry about that. The error class prefix only appears in the rendered message
from Spark 4.0 onward. On 3.4 and 3.5,
`QueryExecutionErrors.arithmeticOverflowError` comes out as a bare `integer
overflow. If necessary set "spark.sql.ansi.enabled" to false to bypass this
error.`, which is what the two red `[expressions]` jobs are hitting.
`overflow. If necessary set` is present in all four Spark profiles and in
Comet's `SparkError::ArithmeticOverflow`, so it should be green everywhere
while still being specific enough not to swallow an unrelated failure.
```sql
query expect_error(overflow. If necessary set)
SELECT make_interval(2147483647)
```
Worth also adding an `ignore` row here for the ANSI side of #5131, since the
fixture only covers cases where Comet and Spark agree that an overflow
happened. `make_interval(0, 0, 0, 0, 2562048)` is a value Spark accepts but the
native kernel throws on.
##########
spark/src/test/resources/sql-tests/expressions/datetime/try_make_interval.sql:
##########
@@ -0,0 +1,23 @@
+-- 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.
+
+-- MinSparkVersion: 4.0
+-- Config: spark.sql.ansi.enabled=true
+-- Config: spark.comet.exec.scalaUDF.codegen.enabled=true
+
+query
+SELECT try_make_interval(2147483647)
Review Comment:
This is the only query in the file, and both engines return NULL for it, so
a wrong-value bug would not show up here. Could you add a case that returns a
real interval, and one under non-ANSI? Something over a column would also cover
the non-literal path.
```sql
query
SELECT try_make_interval(1, 2, 3, 4, 5, 6, 7.123456)
```
Also, `spark.comet.exec.scalaUDF.codegen.enabled` already defaults to true,
so that directive on line 20 can go unless it is there to document intent.
##########
spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala:
##########
@@ -1131,9 +1132,16 @@ object QueryPlanSerde extends Logging with CometExprShim
with CometTypeShim {
}
def scalarFunctionExprToProto(funcName: String, args: Option[Expr]*):
Option[Expr] = {
+ scalarFunctionExprToProto(funcName, false, args: _*)
+ }
+
+ def scalarFunctionExprToProto(
Review Comment:
This new overload does not have a caller. `CometMakeInterval` goes through
`scalarFunctionExprToProtoWithReturnType`, which already accepted `failOnError`
before this PR, and the only reference to this one is the two-arg version above
delegating with `false`. Looks like a leftover from an earlier revision. Could
you drop it?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
