sunchao commented on code in PR #5031: URL: https://github.com/apache/datafusion-comet/pull/5031#discussion_r3837574539
########## spark/src/test/scala/org/apache/comet/CometExpressionCoverageSuite.scala: ########## @@ -0,0 +1,214 @@ +/* + * 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. + */ + +package org.apache.comet + +import java.nio.charset.StandardCharsets +import java.nio.file.{Files, Paths} + +import scala.util.{Failure, Success, Try} + +import org.apache.spark.sql.CometTestBase +import org.apache.spark.sql.internal.SQLConf + +/** + * A runtime coverage audit that finds Spark scalar expressions which Comet does not accelerate. + * + * Static grep-based audits are unreliable because Comet dispatches expressions through several + * mechanisms (serde maps, the `StaticInvoke` method map, `RuntimeReplaceable` rewrites to other + * handled expressions, and the Spark 4.x expression shims). This suite probes behavior directly: + * for every function in Spark's registry it runs the function's own documented example over a + * Comet-scanned table and inspects the physical plan. If the projection stays fully Comet native + * the expression is covered; otherwise the Comet fallback reason (from [[ExtendedExplainInfo]]) + * is recorded. + * + * Two categories of fallback are NOT gaps and are filtered out: + * - "all arguments are foldable": several serdes intentionally decline all-constant inputs so + * Spark can constant-fold them. The probe examples are all-literal, so this is a probe + * artifact, not a gap. `spark.comet.expr.allowIncompatible` is enabled so that expressions + * that are merely incompatible-by-default do not show up as gaps either. + * + * A gap whose fallback operator is a `Project` is an actionable scalar-expression gap, versus + * generators, aggregates, and window functions, which fall back through their own operators. + * + * The report is written to `spark/target/expression-coverage-report.md`. Run with: + * {{{ + * ./mvnw test -Dsuites="org.apache.comet.CometExpressionCoverageSuite" -Dtest=none + * }}} + */ +class CometExpressionCoverageSuite extends CometTestBase { + + private val probe = "comet_coverage_probe" + + private case class Probe( + function: String, + className: String, + status: String, + operator: String, + reason: String, + query: String) + + /** + * Pull the first self-contained `SELECT` example out of a function's `ExpressionInfo`. Examples + * that reference tables, values lists, windows, or grouping need setup this probe does not + * provide, so they are skipped. + */ + private def firstProjectionExample(examples: String): Option[String] = { + if (examples == null) return None + examples.linesIterator + .map(_.trim) + .filter(_.startsWith("> SELECT ")) + .map(_.stripPrefix(">").trim.stripSuffix(";").trim) + .find { q => + val u = q.toUpperCase + !u.contains(" FROM ") && !u.contains(" OVER ") && !u.contains("GROUP BY") && + !u.contains("VALUES") && !u.contains(" JOIN ") && !u.contains("LATERAL") && + !u.contains("(SELECT") + } + } + + test("expression coverage report") { + withTable(probe) { + sql(s"CREATE TABLE $probe(x int) USING parquet") + sql(s"INSERT INTO $probe VALUES (1)") + + val explain = new ExtendedExplainInfo() + val registry = spark.sessionState.functionRegistry + val functions = registry.listFunction().sortBy(_.funcName).distinct + + val results: Seq[Probe] = functions.flatMap { fi => + registry.lookupFunction(fi).map { info => + val className = info.getClassName + firstProjectionExample(info.getExamples) match { + case None => + Probe(fi.funcName, className, "SKIP", "", "no self-contained example", "") + case Some(projection) => + val query = s"$projection FROM $probe" + val outcome = Try { + withSQLConf( Review Comment: [P1] Preserve compilation with Spark 3.x's Unit-valued withSQLConf Could you capture the probe result inside the callback and return it outside `withSQLConf`? On Spark 3.4 and 3.5, the inherited `SQLTestUtils.withSQLConf` returns `Unit`, unlike the generic helper in Spark 4.x. Consequently `outcome` is `Try[Unit]` and the tuple patterns at lines 117 and 119 do not compile. This breaks test compilation for the supported Spark 3.x profiles, not just execution of this audit. The exact-head [Spark 3.5 CI job](https://github.com/apache/datafusion-comet/actions/runs/30128927096/job/89603660200) reports `found: (T1, T2), required: Unit` at both patterns. The canonical Spark 3.4/3.5 helper signatures confirm the cause. -- 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]
