andygrove commented on code in PR #5039:
URL: https://github.com/apache/datafusion-comet/pull/5039#discussion_r3715572781
##########
spark/src/main/scala/org/apache/comet/serde/datetime.scala:
##########
@@ -968,6 +968,33 @@ object CometMakeYMInterval extends
CometCodegenDispatch[MakeYMInterval]
object CometMakeDTInterval extends CometCodegenDispatch[MakeDTInterval]
+object CometMakeInterval extends CometExpressionSerde[MakeInterval] {
Review Comment:
One structural thought. Since `getSupportLevel` is unconditionally
`Incompatible`, the default path here is a full fallback to Spark, which is the
same as the behavior today without this PR. Users only benefit if they flip
`allowIncompatible=true` and take on the #5131 divergences.
Would you be open to mixing in `CodegenDispatchFallback`?
```scala
object CometMakeInterval
extends CometExpressionSerde[MakeInterval]
with CodegenDispatchFallback {
```
That routes the non-opt-in `Incompatible` case through the JVM codegen
dispatcher (`QueryPlanSerde.scala:874`), so the projection stays in the Comet
pipeline with exact Spark semantics by default, and your native kernel becomes
the fast opt-in. It is the same shape as `CometConvertTimezone` and
`CometFromUTCTimestamp` above.
This would also let #5260 and this PR land together rather than one
replacing the other. I opened #5260 as the codegen-dispatch route before seeing
how far this one had come. If you would rather keep them separate I am happy to
close #5260 and let you carry the dispatch mixin here.
##########
spark/src/main/scala/org/apache/comet/serde/datetime.scala:
##########
@@ -968,6 +968,33 @@ object CometMakeYMInterval extends
CometCodegenDispatch[MakeYMInterval]
object CometMakeDTInterval extends CometCodegenDispatch[MakeDTInterval]
+object CometMakeInterval extends CometExpressionSerde[MakeInterval] {
+ private val incompatReason =
+ "The native implementation converts seconds to `Float64`, which can lose
microsecond" +
+ " precision, and stores time in nanoseconds, which overflows for large
seconds values" +
+ " that Spark can represent."
+
Review Comment:
The reason attributes the nanosecond overflow to seconds, but hours and
minutes hit it too, and at much lower values. Spark's
`IntervalUtils.makeInterval` accumulates microseconds while the DataFusion
kernel accumulates nanoseconds, so every time component has a 1000x smaller
range.
`make_interval(0, 0, 0, 0, 2562048)` is enough to show it. Spark computes
`2562048 * 3_600_000_000 = 9_223_372_800_000_000` micros and returns a valid
interval. The kernel computes `2562048 * 3_600_000_000_000 =
9_223_372_800_000_000_000` nanos, which exceeds `i64::MAX`, so `checked_mul`
fails and it returns NULL, or throws under ANSI. The cutoffs are hours >=
2,562,048 and mins >= 153,722,868.
Could the reason say "time components (hours, minutes, seconds)" rather than
just seconds? This string is what renders on the generated compat page, so it
is the only warning a user gets. It would be good to widen #5131's description
the same way.
##########
native/core/src/execution/jni_api.rs:
##########
@@ -623,6 +624,7 @@ fn register_datafusion_spark_function(session_ctx:
&SessionContext) {
session_ctx.register_udf(ScalarUDF::new_from_impl(SparkDateSub::default()));
session_ctx.register_udf(ScalarUDF::new_from_impl(SparkFromUtcTimestamp::default()));
session_ctx.register_udf(ScalarUDF::new_from_impl(SparkLastDay::default()));
+
session_ctx.register_udf(ScalarUDF::new_from_impl(SparkMakeInterval::default()));
Review Comment:
I do not think this registration is reachable. Because the serde sets the
return type, `create_scalar_function_expr` skips the `session_ctx.udf(...)`
lookup (`planner.rs:3337`) and goes to `create_comet_physical_fun`, where the
`"make_interval"` arm builds `SparkMakeInterval::new(fail_on_error)` directly.
If you want to keep a registry entry for safety, `SparkMakeDate` is the
precedent and it lives in `all_scalar_functions()` in `comet_scalar_funcs.rs`.
Putting a Comet wrapper here is a little misleading, since everything else in
`register_datafusion_spark_function` is a raw upstream UDF, and this one
hardcodes `fail_on_error = false` via `Default`. If it ever did get used it
would silently ignore ANSI.
##########
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(overflow)
+SELECT make_interval(2147483647)
+
+query expect_error(overflow)
+SELECT make_interval(0, 0, 2147483647)
Review Comment:
Would you tighten these to `expect_error(ARITHMETIC_OVERFLOW)`? Both engines
produce that class on every supported profile. Spark 3.5 and 4.1 both route
through `QueryExecutionErrors.arithmeticOverflowError`, and Comet's
`SparkError::ArithmeticOverflow` renders `[ARITHMETIC_OVERFLOW] interval
overflow. ...`. The bare `overflow` would also match an unrelated failure, and
`ARITHMETIC_OVERFLOW` is the more common convention in the existing fixtures.
Note the message bodies still differ. Spark says `integer overflow` or `long
overflow` depending on which `Math.*Exact` tripped, Comet always says `interval
overflow`. That is unavoidable given the wrapper only sees the result null
mask, but a short comment in `make_interval.rs` noting it would save the next
reader the investigation.
##########
spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala:
##########
@@ -303,6 +303,7 @@ object QueryPlanSerde extends Logging with CometExprShim
with CometTypeShim {
classOf[MakeTimestamp] -> CometMakeTimestamp,
classOf[MakeYMInterval] -> CometMakeYMInterval,
classOf[MakeDTInterval] -> CometMakeDTInterval,
+ classOf[MakeInterval] -> CometMakeInterval,
Review Comment:
`TryMakeInterval` is `RuntimeReplaceable` and its replacement is
`MakeInterval(..., failOnError = false)`, so `try_make_interval` reaches this
handler after `ReplaceExpressions`. That means this PR enables it too.
Two follow-ons. The `try_make_interval` row in `expressions.md` (line 308)
still says `🔜` with the #5061 note, so it needs the same update as the
`make_interval` row. And there is a combination neither fixture covers:
`failOnError = false` with `spark.sql.ansi.enabled = true`, where
`try_make_interval(2147483647)` must return NULL instead of throwing. Would you
add a small fixture for it? It needs `-- MinSparkVersion: 4.0`, since
`try_make_interval` is not registered in 3.5.
##########
spark/src/test/resources/sql-tests/expressions/datetime/make_interval.sql:
##########
@@ -0,0 +1,64 @@
+-- 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.comet.expression.MakeInterval.allowIncompatible=true
+
+statement
+CREATE TABLE test_make_interval(
+ years int,
+ months int,
+ weeks int,
+ days int,
+ hours int,
+ mins int,
+ secs decimal(18, 6)) USING parquet
+
+statement
+INSERT INTO test_make_interval 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),
+ (-2147483648, 0, 0, 0, 0, 0, 0.000000)
+
+query
+SELECT make_interval(years, months, weeks, days, hours, mins, secs)
+FROM test_make_interval
+ORDER BY years
+
+query
+SELECT make_interval(1, 2), make_interval(3), make_interval()
+
+query
+SELECT make_interval(0, 1, 0, 1, 0, 0, 100.000001)
+
+query
+SELECT make_interval(2147483647)
+
+query ignore(https://github.com/apache/datafusion-comet/issues/5131)
+SELECT make_interval(1, 2, 3, 4, 0, 0, 123456789012.123456)
+
+query
+SELECT make_interval(0, 0, 0, 0, 0, 0, 999999999.999999)
+
+query ignore(https://github.com/apache/datafusion-comet/issues/5131)
+SELECT make_interval(0, 0, 0, 0, 0, 0, 999999999.000001)
+
+query
+SELECT make_interval(0, 0, 0, 0, 0, 0, 1234567890123456789)
Review Comment:
Could you add an hours case alongside the seconds ones? It is the same #5131
nanosecond overflow but on a component the fixture does not touch, and at a
value a real query is much more likely to produce than a 12-digit seconds
decimal.
```sql
query ignore(https://github.com/apache/datafusion-comet/issues/5131)
SELECT make_interval(0, 0, 0, 0, 2562048)
```
--
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]
