sunchao commented on code in PR #5292:
URL: https://github.com/apache/datafusion-comet/pull/5292#discussion_r4124863799
##########
spark/src/main/scala/org/apache/comet/serde/datetime.scala:
##########
@@ -884,30 +884,14 @@ 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 getCompatibleNotes(): Seq[String] = Seq(
- "Both the default JVM codegen-dispatch path and the native path currently
limit the" +
- " elapsed-time component to about 292 years in either direction. This
only affects" +
- " extreme intervals and is tracked in" +
- " [#5279](https://github.com/apache/datafusion-comet/issues/5279).")
-
- override def getIncompatibleReasons(): Seq[String] = Seq(incompatReason)
-
- override def getSupportLevel(expr: MakeInterval): SupportLevel =
- Incompatible(Some(incompatReason))
+object CometMakeInterval extends CometExpressionSerde[MakeInterval] {
+ override def getSupportLevel(expr: MakeInterval): SupportLevel = Compatible()
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.secs, DoubleType))
- val childExprs = children.map(exprToProtoInternal(_, inputs, binding))
+ val childExprs = expr.children.map(exprToProtoInternal(_, inputs, binding))
Review Comment:
[P2] Preserve NULL short-circuiting when switching to native execution. With
ANSI enabled and a Parquet table `t(y INT, s STRING)` containing `(NULL,
'bad')`, `SELECT make_interval(y, CAST(s AS INT)) FROM t` returns NULL in Spark
and the former default dispatcher. Serializing the children independently makes
`ScalarFunctionExpr` evaluate the invalid cast before `SparkMakeInterval`
checks the NULL years, so the query now fails. This affects ordinary execution
after the incompatibility gate is removed. Could the lowering preserve Spark’s
ordered short-circuiting, or retain dispatch for these expressions?
Evidence: Spark 4.1.3 executed the Parquet query and returned NULL, with the
expression retained in its optimized plan. The dispatcher’s whole-expression
codegen shape also returned NULL. A disposable integration test compiled
against this exact checkout constructed the same native `ScalarFunctionExpr`,
Comet ANSI `Cast`, and `SparkMakeInterval`. `(NULL, '0')` returned NULL, while
`(NULL, 'bad')` returned `Err(External(CastInvalidValue { value: "bad",
from_type: "STRING", to_type: "INT" }))`. The reproduction passed after
asserting that error. Native results:
`/tmp/comet-5292-current-shortcircuit.log`. Spark/codegen results:
`/tmp/comet-5292-recheck-2f2ncg4n/validated-results.log`.
--
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]
