sunchao commented on code in PR #4997:
URL: https://github.com/apache/datafusion-comet/pull/4997#discussion_r4105238055
##########
spark/src/main/spark-4.1+/org/apache/comet/shims/CometExprShim.scala:
##########
@@ -46,16 +47,50 @@ trait CometExprShim extends Spark4xCometExprShim {
}
}
- // Spark 4.1 introduced TimeType and the make_time / to_time / try_to_time
functions.
- // Their planner forms differ from the shared 4.x patterns
(DateTimeUtils.makeTime
- // StaticInvoke and ToTimeParser Invoke / TryEval(Invoke)), so they live
here rather
- // than in the shared Spark4xCometExprShim parent trait. Spark 4.0 lacks
TimeType, which
- // is why this shim is shared by 4.1 and later rather than all 4.x.
+ private val integralTimePartMethods =
+ Set("getHoursOfTime", "getMinutesOfTime", "getSecondsOfTime")
+
+ // Spark 4.1 introduced TimeType and its extraction, make_time, to_time, and
try_to_time
+ // expressions. Their planner forms differ from the shared 4.x patterns
(DateTimeUtils
+ // StaticInvoke and ToTimeParser Invoke / TryEval(Invoke)), so they live
here rather than in the
+ // shared Spark4xCometExprShim parent trait. Spark 4.0 lacks TimeType, which
is why this shim is
+ // shared by 4.1 and later rather than all 4.x.
override def sparkVersionSpecificExprToProtoInternal(
expr: Expression,
inputs: Seq[Attribute],
binding: Boolean): Option[Expr] = {
+
expr match {
+ case s: StaticInvoke
+ if s.staticObject == classOf[DateTimeUtils.type] &&
+ integralTimePartMethods.contains(s.functionName) &&
+ s.arguments.size == 1 &&
+ s.dataType == IntegerType &&
+ s.arguments.head.dataType.isInstanceOf[TimeType] =>
+ val child = s.arguments.head
+ val childExpr = exprToProtoInternal(child, inputs, binding)
+
+ val optExpr = childExpr.map { childProto =>
+ timePartToProto(s.functionName, childProto)
Review Comment:
[P2] Handle scalar `TIME` inputs before routing integral extraction to these
kernels. With a Parquet table `source(id INT, h INT)` containing
`(0,0),(1,12),(2,6)`, `SELECT id, hour(make_time((SELECT max(h) FROM source),
30, 45.123456)) FROM source` should return `12` for every row. The subquery
prevents Spark constant folding. On a multi-row native batch, `make_time`
produces a scalar `Time64Nanosecond`, but `SparkHour` accepts only arrays and
raises `hour(scalar) should be fold in Spark JVM side.` The same failure
affects `minute` and `second`. This new mapping replaces the previously working
Spark fallback with a runtime query failure. Please add scalar handling to the
shared extraction kernels and cover this scalar-subquery case.
Evidence: At the reviewed SHA, `cargo test --locked -p
datafusion-comet-spark-expr --test review_4997 -- --nocapture` reproduced all
three errors using the same `ScalarFunctionExpr` composition as the native
planner over a three-row batch. `make_time` returned
`Scalar(Time64Nanosecond("45045123456000"))`. The integer scalar matches
`Subquery::evaluate`'s return representation. Spark 4.1.3 retained the
`StaticInvoke`/scalar-subquery chain and returned `(12,30,45)` for all three
rows. The relevant Spark implementations are identical in the inspected 4.1.2
and 4.2.0 sources. This is a native-component reproduction plus a Spark
reference run, not an end-to-end local Comet JVM run.
--
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]
