sunchao commented on code in PR #5610:
URL: https://github.com/apache/datafusion-comet/pull/5610#discussion_r3910116654
##########
spark/src/test/scala/org/apache/comet/CometSqlFileTestSuite.scala:
##########
@@ -155,6 +155,10 @@ class CometSqlFileTestSuite extends CometTestBase with
AdaptiveSparkPlanHelper {
checkSparkAnswerAndOperatorWithTolerance(sql, tol)
case ExpectFallback(reason) =>
checkSparkAnswerAndFallbackReason(sql, reason)
+ case ExpectDispatch(names) =>
Review Comment:
[P2] Accept the new implementation modes as positive sentinels
Could `requireSentinelForCodegenExpectError` also recognize `ExpectDispatch`
and `ExpectNative`? Both branches call `checkSparkAnswerAndImpl`, which first
performs the same answer/operator checks as a plain query. For a file with
`spark.comet.exec.scalaUDF.codegen.enabled=true` and an `expect_error` record,
upgrading its last plain positive query to either new mode makes preflight
reject the file as missing a sentinel before any SQL runs. The stronger
assertion should be able to serve as that successful control query without
requiring a redundant plain query.
##########
spark/src/test/scala/org/apache/spark/sql/CometTestBase.scala:
##########
@@ -342,6 +342,84 @@ abstract class CometTestBase
(sparkPlan, cometPlan)
}
+ /**
+ * Check for the correct results, that Comet replaced all possible
operators, and that the named
+ * expressions ran through the mechanism the caller expects.
+ *
+ * Comet evaluates an expression one of three ways: natively (a DataFusion
expression), through
+ * the JVM codegen dispatcher (Spark's own `doGenCode` compiled into an
Arrow batch kernel), or
+ * not at all (the operator falls back to Spark). Only the third is visible
to
+ * [[checkSparkAnswerAndOperator]]; the first two produce Spark-matching
results by
+ * construction, so a serde that quietly widens from native to dispatch
(losing the native
+ * kernel) or narrows from dispatch to native (losing Spark-exact semantics)
passes every other
+ * assertion here. Use this to pin which one actually ran.
+ *
+ * Names are the expression's `prettyName` lowercased, as
[[ExtendedExplainInfo]] reports them
+ * (`bit_length`, `octet_length`, `rlike`), not necessarily the SQL alias
used to invoke it: a
+ * function registered with `setAlias` reports the invoked alias, everything
else reports its
+ * own `prettyName`.
+ *
+ * For fallback assertions use [[checkSparkAnswerAndFallbackReason]] instead.
+ */
+ protected def checkSparkAnswerAndImpl(
+ df: => DataFrame,
+ native: Seq[String] = Seq.empty,
+ dispatched: Seq[String] = Seq.empty): (SparkPlan, SparkPlan) = {
+ val (sparkPlan, cometPlan) = checkSparkAnswerAndOperator(df)
+ assertExpressionImpl(cometPlan, native, dispatched)
+ (sparkPlan, cometPlan)
+ }
+
+ /** Check for the correct results and the expected per-expression
implementation. */
+ protected def checkSparkAnswerAndImpl(
+ query: String,
+ native: Seq[String],
+ dispatched: Seq[String]): (SparkPlan, SparkPlan) = {
+ checkSparkAnswerAndImpl(sql(query), native, dispatched)
+ }
+
+ /**
+ * Assert how Comet evaluated the named expressions in an already-executed
Comet plan. Split out
+ * from [[checkSparkAnswerAndImpl]] so callers holding a plan can reuse it,
and so the assertion
+ * itself is testable.
+ *
+ * Each name must appear in its expected set and must be absent from the
other, so naming an
+ * expression is a claim about which mechanism ran it rather than a claim
that it ran somehow.
+ */
+ protected def assertExpressionImpl(
+ cometPlan: SparkPlan,
+ native: Seq[String],
+ dispatched: Seq[String]): Unit = {
+ val explainInfo = new ExtendedExplainInfo()
+ val actualNative = explainInfo.getNativeExpressions(cometPlan)
+ val actualDispatched = explainInfo.getCodegenDispatchExpressions(cometPlan)
+ def detail: String =
+ s"native=[${actualNative.mkString(", ")}] " +
+ s"codegen-dispatched=[${actualDispatched.mkString(", ")}]"
+ native.foreach { name =>
+ if (actualDispatched.contains(name)) {
Review Comment:
[P2] Account for expressions nested inside a dispatched subtree
Could the exclusion check account for descendants of a dispatched
expression? With non-null Double columns in a nonempty Parquet table, Comet
projection and codegen dispatch enabled, `SELECT abs(a), hypot(abs(b), c) FROM
t` lowers the first `abs` natively but serializes the whole `hypot(abs(b), c)`
tree to the JVM. Dispatch tags only `hypot`, so `native = Seq("abs"),
dispatched = Seq("hypot")` can pass even though the nested `abs` runs in the
JVM kernel. The new assertion therefore misses a composed case where the named
expression uses both mechanisms. A nested-expression regression case would
cover this missing classification. This is source-derived, not an executed
reproduction.
--
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]
