sunchao commented on code in PR #5329: URL: https://github.com/apache/datafusion-comet/pull/5329#discussion_r3816936739
########## spark/src/test/scala/org/apache/comet/CometFallbackInvarianceSuite.scala: ########## @@ -0,0 +1,305 @@ +/* + * 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 scala.collection.mutable +import scala.util.{Failure, Success, Try} + +import org.apache.spark.sql.Row +import org.apache.spark.sql.execution.{ProjectExec, SparkPlan} +import org.apache.spark.sql.internal.SQLConf + +/** + * Checks that forcing an expression from Comet's native path back to Spark does not change the + * outcome of a query. + * + * Comet exposes `spark.comet.expression.<Name>.enabled` for every registered expression, and + * `QueryPlanSerde` honours it as forced fallback. For any expression Comet rates compatible, that + * gives a free invariant: the outcome of a query -- its rows, or the error it raises -- must be + * identical whether the expression is evaluated natively or by Spark. + * + * Two properties of this suite are load-bearing: + * + * 1. "Outcome" is three-valued. A leg either produces rows or throws. A value on one leg and an + * exception on the other is a failure with a named witness, not an error in the harness -- + * exception parity is where several historical divergences have lived. + * + * 2. Every comparison is gated on evidence that the config flip actually moved execution. A query + * whose executed plan proves nothing is reported SKIPPED-VACUOUS and never counted as a pass. + * Without that gate an invariance sweep silently overstates its own coverage. + * + * Queries deliberately avoid `ORDER BY`: a shuffle roots the executed plan at + * `AdaptiveSparkPlanExec`, whose `children` is empty, so plan inspection would read zero for + * everything and the gate above would be inoperative. Row order is canonicalised in the + * comparator instead. + */ +class CometFallbackInvarianceSuite extends CometFuzzTestBase { Review Comment: [P2] Register the suite in both CI workflows Could you add `org.apache.comet.CometFallbackInvarianceSuite` to the suite matrices in both `.github/workflows/pr_build_linux.yml` and `.github/workflows/pr_build_macos.yml`? They use explicit suite lists. `python3 dev/ci/check-suites.py` passes on the base but exits 255 on this head, and the [Preflight job](https://github.com/apache/datafusion-comet/actions/runs/31517356712/job/96231037450) fails on this class before the downstream tests can run. ########## spark/src/test/scala/org/apache/comet/CometFallbackInvarianceSuite.scala: ########## @@ -0,0 +1,305 @@ +/* + * 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 scala.collection.mutable +import scala.util.{Failure, Success, Try} + +import org.apache.spark.sql.Row +import org.apache.spark.sql.execution.{ProjectExec, SparkPlan} +import org.apache.spark.sql.internal.SQLConf + +/** + * Checks that forcing an expression from Comet's native path back to Spark does not change the + * outcome of a query. + * + * Comet exposes `spark.comet.expression.<Name>.enabled` for every registered expression, and + * `QueryPlanSerde` honours it as forced fallback. For any expression Comet rates compatible, that + * gives a free invariant: the outcome of a query -- its rows, or the error it raises -- must be + * identical whether the expression is evaluated natively or by Spark. + * + * Two properties of this suite are load-bearing: + * + * 1. "Outcome" is three-valued. A leg either produces rows or throws. A value on one leg and an + * exception on the other is a failure with a named witness, not an error in the harness -- + * exception parity is where several historical divergences have lived. + * + * 2. Every comparison is gated on evidence that the config flip actually moved execution. A query + * whose executed plan proves nothing is reported SKIPPED-VACUOUS and never counted as a pass. + * Without that gate an invariance sweep silently overstates its own coverage. + * + * Queries deliberately avoid `ORDER BY`: a shuffle roots the executed plan at + * `AdaptiveSparkPlanExec`, whose `children` is empty, so plan inspection would read zero for + * everything and the gate above would be inoperative. Row order is canonicalised in the + * comparator instead. + */ +class CometFallbackInvarianceSuite extends CometFuzzTestBase { + + /** + * Expressions Comet documents as not guaranteed to match Spark exactly. A divergence here is + * reported as EXCUSED: logged for the record, never counted as a pass. + */ + private val incompatibleRated: Set[String] = + Set( + "DateFormatClass", + "FromUTCTimestamp", + "GetJsonObject", + "RLike", Review Comment: [P2] Do not excuse compatible RLike mismatches Could you remove this exemption or derive it from the active implementation and configuration? At this head, [`CometRLike.getSupportLevel`](https://github.com/apache/datafusion-comet/blob/e3c6fea350e44558194090945f3a0b0bd604e2af/spark/src/main/scala/org/apache/comet/serde/strings.scala#L378-L399) rates the default JVM-codegen path compatible. The incompatible Rust regex implementation requires `spark.comet.expression.RLike.allowIncompatible=true`, which this suite does not enable. With valid execution-plan evidence, opposite Boolean results are nevertheless recorded as `EXCUSED`, leaving the test green. I reproduced that with the unchanged comparator on Scala 2.12.18 and 2.13.17. The same mismatch for `EqualTo` correctly produces `FAIL-VALUE`. -- 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]
