You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "andygrove (via GitHub)" <gi...@apache.org> on 2026/01/29 16:07:02 UTC
[PR] tests: Add ~115 SQL test files covering edge cases for every Comet-supported expression [WIP] [datafusion-comet]
andygrove opened a new pull request, #3328:
URL: https://github.com/apache/datafusion-comet/pull/3328
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
N/A
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
Improve testing.
These new tests exposed some bugs:
- [#3326](https://github.com/apache/datafusion-comet/issues/3326) — `space(-1)` causes native crash ("failed to round upto multiple of 64")
- [#3327](https://github.com/apache/datafusion-comet/issues/3327) — `map_from_arrays(NULL, NULL)` causes native crash ("map key cannot be null")
Both bugs have reproducing tests in the SQL files using the new `query ignore(...)` directive, so they'll be easy to re-enable once fixed.
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
### What's included
- **~115 new SQL test files** in `spark/src/test/resources/sql-tests/expressions/` covering math, string, conditional, predicate, datetime, array, map, struct, JSON, aggregate, and cast expressions
- **Two new query modes** in the test framework:
- `query expect_fallback(reason)` — verifies both that the fallback reason matches AND results match Spark
- `query ignore(reason)` — skips the query with a logged reason (for known bugs with issue links)
- **Happy-path `_enabled` tests** for incompatible expressions with configs enabled (`caseConversion`, `regexp.allowIncompatible`, `InitCap.allowIncompatible`, `DateFormatClass.allowIncompatible`, `FromUnixTime.allowIncompatible`)
- **Updates to 3 existing files**: `arithmetic.sql`, `bitwise.sql`, `datetime.sql` with additional edge cases
### Edge cases covered per expression
- NULL inputs
- Boundary values (MIN/MAX for int types, +/-Infinity, NaN for doubles)
- Empty strings, empty collections
- Type variety (int, long, float, double, decimal, string)
- Special values (negative zero, overflow, division by zero)
## How are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] tests: Add SQL test files covering edge cases for (almost) every Comet-supported expression [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #3328:
URL: https://github.com/apache/datafusion-comet/pull/3328#discussion_r2743151024
##########
spark/src/test/scala/org/apache/comet/SqlFileTestParser.scala:
##########
@@ -0,0 +1,159 @@
+/*
+ * 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.io.File
+
+import scala.io.Source
+
+/** A record in a SQL test file: either a statement (DDL/DML) or a query (SELECT). */
+sealed trait SqlTestRecord
+
+/** A SQL statement to execute (CREATE TABLE, INSERT, etc.). */
+case class SqlStatement(sql: String) extends SqlTestRecord
+
+/** A SQL query whose results are compared between Spark and Comet. */
+case class SqlQuery(sql: String, mode: QueryMode = CheckOperator) extends SqlTestRecord
+
+sealed trait QueryMode
+case object CheckOperator extends QueryMode
+case object SparkAnswerOnly extends QueryMode
+case class WithTolerance(tol: Double) extends QueryMode
+case class ExpectFallback(reason: String) extends QueryMode
+case class Ignore(reason: String) extends QueryMode
+
+/**
+ * Parsed representation of a .sql test file.
+ *
+ * @param configs
+ * Spark SQL configs to set for this test file.
+ * @param configMatrix
+ * Map of config key to list of values. The test will run once per combination.
+ * @param records
+ * Ordered list of statements and queries.
+ * @param tables
+ * Table names extracted from CREATE TABLE statements (for cleanup).
+ * @param minSparkVersion
+ * Optional minimum Spark version required to run this test (e.g. "3.5").
+ */
+case class SqlTestFile(
+ configs: Seq[(String, String)],
+ configMatrix: Seq[(String, Seq[String])],
+ records: Seq[SqlTestRecord],
+ tables: Seq[String],
+ minSparkVersion: Option[String] = None)
+
+object SqlFileTestParser {
+
+ private val ConfigPattern = """--\s*Config:\s*(.+)=(.+)""".r
+ private val ConfigMatrixPattern = """--\s*ConfigMatrix:\s*(.+)=(.+)""".r
+ private val MinSparkVersionPattern = """--\s*MinSparkVersion:\s*(.+)""".r
+ private val CreateTablePattern = """(?i)CREATE\s+TABLE\s+(\w+)""".r.unanchored
+
+ def parse(file: File): SqlTestFile = {
+ val source = Source.fromFile(file, "UTF-8")
+ try {
+ parse(source.getLines().toSeq)
+ } finally {
+ source.close()
+ }
+ }
+
+ def parse(lines: Seq[String]): SqlTestFile = {
+ var configs = Seq.empty[(String, String)]
+ var configMatrix = Seq.empty[(String, Seq[String])]
+ var minSparkVersion: Option[String] = None
+ val records = Seq.newBuilder[SqlTestRecord]
+ val tables = Seq.newBuilder[String]
+
+ var i = 0
Review Comment:
Thanks. I addressed this and your other comments
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] tests: Add SQL test files covering edge cases for (almost) every Comet-supported expression [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #3328:
URL: https://github.com/apache/datafusion-comet/pull/3328#discussion_r2743036747
##########
spark/src/test/resources/sql-tests/expressions/aggregate/avg.sql:
##########
@@ -0,0 +1,30 @@
+-- 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.
+
+-- ConfigMatrix: parquet.enable.dictionary=false,true
Review Comment:
Many of the tests run with dictionary encoding on and off, and this is because this is what we do in existing Scala-based tests, so I left it for now. However, this is pointless because most of these tests only insert a few rows. I will fix this in a future PR.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] tests: Add SQL test files covering edge cases for (almost) every Comet-supported expression [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #3328:
URL: https://github.com/apache/datafusion-comet/pull/3328#discussion_r2742847584
##########
spark/src/test/scala/org/apache/comet/SqlFileTestParser.scala:
##########
@@ -0,0 +1,159 @@
+/*
+ * 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.io.File
+
+import scala.io.Source
+
+/** A record in a SQL test file: either a statement (DDL/DML) or a query (SELECT). */
+sealed trait SqlTestRecord
+
+/** A SQL statement to execute (CREATE TABLE, INSERT, etc.). */
+case class SqlStatement(sql: String) extends SqlTestRecord
+
+/** A SQL query whose results are compared between Spark and Comet. */
+case class SqlQuery(sql: String, mode: QueryMode = CheckOperator) extends SqlTestRecord
+
+sealed trait QueryMode
+case object CheckOperator extends QueryMode
Review Comment:
`checkCoverageAndAnswer`
?
##########
spark/src/test/scala/org/apache/comet/SqlFileTestParser.scala:
##########
@@ -0,0 +1,159 @@
+/*
+ * 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.io.File
+
+import scala.io.Source
+
+/** A record in a SQL test file: either a statement (DDL/DML) or a query (SELECT). */
+sealed trait SqlTestRecord
+
+/** A SQL statement to execute (CREATE TABLE, INSERT, etc.). */
+case class SqlStatement(sql: String) extends SqlTestRecord
+
+/** A SQL query whose results are compared between Spark and Comet. */
+case class SqlQuery(sql: String, mode: QueryMode = CheckOperator) extends SqlTestRecord
+
+sealed trait QueryMode
Review Comment:
```suggestion
sealed trait QueryAssertionMode
```
##########
spark/src/test/scala/org/apache/comet/SqlFileTestParser.scala:
##########
@@ -0,0 +1,159 @@
+/*
+ * 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.io.File
+
+import scala.io.Source
+
+/** A record in a SQL test file: either a statement (DDL/DML) or a query (SELECT). */
+sealed trait SqlTestRecord
+
+/** A SQL statement to execute (CREATE TABLE, INSERT, etc.). */
+case class SqlStatement(sql: String) extends SqlTestRecord
+
+/** A SQL query whose results are compared between Spark and Comet. */
+case class SqlQuery(sql: String, mode: QueryMode = CheckOperator) extends SqlTestRecord
+
+sealed trait QueryMode
+case object CheckOperator extends QueryMode
+case object SparkAnswerOnly extends QueryMode
+case class WithTolerance(tol: Double) extends QueryMode
+case class ExpectFallback(reason: String) extends QueryMode
+case class Ignore(reason: String) extends QueryMode
+
+/**
+ * Parsed representation of a .sql test file.
+ *
+ * @param configs
+ * Spark SQL configs to set for this test file.
+ * @param configMatrix
+ * Map of config key to list of values. The test will run once per combination.
+ * @param records
+ * Ordered list of statements and queries.
+ * @param tables
+ * Table names extracted from CREATE TABLE statements (for cleanup).
+ * @param minSparkVersion
+ * Optional minimum Spark version required to run this test (e.g. "3.5").
+ */
+case class SqlTestFile(
+ configs: Seq[(String, String)],
+ configMatrix: Seq[(String, Seq[String])],
+ records: Seq[SqlTestRecord],
+ tables: Seq[String],
+ minSparkVersion: Option[String] = None)
+
+object SqlFileTestParser {
+
+ private val ConfigPattern = """--\s*Config:\s*(.+)=(.+)""".r
+ private val ConfigMatrixPattern = """--\s*ConfigMatrix:\s*(.+)=(.+)""".r
+ private val MinSparkVersionPattern = """--\s*MinSparkVersion:\s*(.+)""".r
+ private val CreateTablePattern = """(?i)CREATE\s+TABLE\s+(\w+)""".r.unanchored
+
+ def parse(file: File): SqlTestFile = {
+ val source = Source.fromFile(file, "UTF-8")
+ try {
+ parse(source.getLines().toSeq)
+ } finally {
+ source.close()
+ }
+ }
+
+ def parse(lines: Seq[String]): SqlTestFile = {
+ var configs = Seq.empty[(String, String)]
+ var configMatrix = Seq.empty[(String, Seq[String])]
+ var minSparkVersion: Option[String] = None
+ val records = Seq.newBuilder[SqlTestRecord]
+ val tables = Seq.newBuilder[String]
+
+ var i = 0
Review Comment:
can we name `i` better?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] tests: Add SQL test files covering edge cases for (almost) every Comet-supported expression [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #3328:
URL: https://github.com/apache/datafusion-comet/pull/3328#discussion_r2742832407
##########
spark/src/test/scala/org/apache/comet/CometSqlFileTestSuite.scala:
##########
@@ -0,0 +1,133 @@
+/*
+ * 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.io.File
+
+import org.scalactic.source.Position
+import org.scalatest.Tag
+
+import org.apache.spark.sql.CometTestBase
+import org.apache.spark.sql.execution.adaptive.AdaptiveSparkPlanHelper
+
+class CometSqlFileTestSuite extends CometTestBase with AdaptiveSparkPlanHelper {
+
+ override protected def test(testName: String, testTags: Tag*)(testFun: => Any)(implicit
+ pos: Position): Unit = {
+ super.test(testName, testTags: _*) {
+ withSQLConf(CometConf.COMET_NATIVE_SCAN_IMPL.key -> CometConf.SCAN_AUTO) {
+ testFun
+ }
+ }
+ }
+
+ /** Check if the current Spark version meets a minimum version requirement. */
+ private def meetsMinSparkVersion(minVersion: String): Boolean = {
+ val current = org.apache.spark.SPARK_VERSION.split("[.-]").take(2).map(_.toInt)
+ val required = minVersion.split("[.-]").take(2).map(_.toInt)
+ (current(0) > required(0)) ||
+ (current(0) == required(0) && current(1) >= required(1))
+ }
+
+ private val testResourceDir = {
+ val url = getClass.getClassLoader.getResource("sql-tests")
+ assert(url != null, "Could not find sql-tests resource directory")
+ new File(url.toURI)
+ }
+
+ private def discoverTestFiles(dir: File): Seq[File] = {
+ if (!dir.exists()) return Seq.empty
+ val files = dir.listFiles().toSeq
+ val sqlFiles = files.filter(f => f.isFile && f.getName.endsWith(".sql"))
+ val subDirFiles = files.filter(_.isDirectory).flatMap(discoverTestFiles)
+ sqlFiles ++ subDirFiles
+ }
+
+ /** Generate all config combinations from a ConfigMatrix specification. */
+ private def configCombinations(
+ matrix: Seq[(String, Seq[String])]): Seq[Seq[(String, String)]] = {
+ if (matrix.isEmpty) return Seq(Seq.empty)
+ val (key, values) = matrix.head
+ val rest = configCombinations(matrix.tail)
+ for {
+ value <- values
+ combo <- rest
+ } yield (key, value) +: combo
+ }
+
+ private def runTestFile(file: SqlTestFile): Unit = {
+ val allConfigs = file.configs
+ withSQLConf(allConfigs: _*) {
+ withTable(file.tables: _*) {
+ file.records.foreach {
+ case SqlStatement(sql) =>
+ spark.sql(sql)
+ case SqlQuery(sql, mode) =>
+ mode match {
+ case CheckOperator =>
+ checkSparkAnswerAndOperator(sql)
+ case SparkAnswerOnly =>
+ checkSparkAnswer(sql)
+ case WithTolerance(tol) =>
+ checkSparkAnswerWithTolerance(sql, tol)
+ case ExpectFallback(reason) =>
+ checkSparkAnswerAndFallbackReason(sql, reason)
+ case Ignore(reason) =>
+ logInfo(s"IGNORED query (${reason}): $sql")
+ }
+ }
+ }
+ }
+ }
+
+ // Discover and register all .sql test files
+ discoverTestFiles(testResourceDir).foreach { file =>
Review Comment:
we can parallelize the `foreach` and run tests in parallel
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] tests: Add SQL test files covering edge cases for (almost) every Comet-supported expression [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #3328:
URL: https://github.com/apache/datafusion-comet/pull/3328#issuecomment-3824449160
I will add ansi tests in a follow on PR
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] tests: Add SQL test files covering edge cases for (almost) every Comet-supported expression [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #3328:
URL: https://github.com/apache/datafusion-comet/pull/3328#issuecomment-3819634420
@shehabgamin you may be interested in this
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] tests: Add SQL test files covering edge cases for every Comet-supported expression [WIP] [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #3328:
URL: https://github.com/apache/datafusion-comet/pull/3328#discussion_r2742772395
##########
pr-description.md:
##########
@@ -0,0 +1,72 @@
+## Summary
+
+This PR introduces a SQL file-based test framework for Comet expression tests and adds 123 declarative `.sql` test files covering all Comet-supported expressions.
+
+### Motivation
+
+Expression tests in `CometExpressionSuite` were written as individual Scala test methods with significant boilerplate: creating tables, inserting data, running queries, and cleaning up — all repeated for each expression. This made tests verbose, hard to review, and inconsistent in coverage.
+
+### SQL File Test Framework
+
+Two new Scala files form the framework:
+
+- **`SqlFileTestParser.scala`** — Parses `.sql` test files into a structured `SqlTestFile` representation. Each file can contain:
+ - `-- Config: key=value` — Spark SQL configs to set for the entire file
+ - `-- ConfigMatrix: key=value1,value2` — Generates one test per combination of values (e.g., dictionary encoding on/off)
Review Comment:
correct, we can also (later) add config parameters when running the whole suite, maybe via maven profiles
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] tests: Add SQL test files covering edge cases for every Comet-supported expression [WIP] [datafusion-comet]
Posted by "hsiang-c (via GitHub)" <gi...@apache.org>.
hsiang-c commented on code in PR #3328:
URL: https://github.com/apache/datafusion-comet/pull/3328#discussion_r2742767072
##########
pr-description.md:
##########
@@ -0,0 +1,72 @@
+## Summary
+
+This PR introduces a SQL file-based test framework for Comet expression tests and adds 123 declarative `.sql` test files covering all Comet-supported expressions.
+
+### Motivation
+
+Expression tests in `CometExpressionSuite` were written as individual Scala test methods with significant boilerplate: creating tables, inserting data, running queries, and cleaning up — all repeated for each expression. This made tests verbose, hard to review, and inconsistent in coverage.
+
+### SQL File Test Framework
+
+Two new Scala files form the framework:
+
+- **`SqlFileTestParser.scala`** — Parses `.sql` test files into a structured `SqlTestFile` representation. Each file can contain:
+ - `-- Config: key=value` — Spark SQL configs to set for the entire file
+ - `-- ConfigMatrix: key=value1,value2` — Generates one test per combination of values (e.g., dictionary encoding on/off)
Review Comment:
IIUC, we can toggle ANSI mode by `ConfigMatrix: spark.sql.ansi.enabled=true,false`?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] tests: Add SQL test files covering edge cases for (almost) every Comet-supported expression [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #3328:
URL: https://github.com/apache/datafusion-comet/pull/3328#discussion_r2742840290
##########
spark/src/test/scala/org/apache/comet/CometSqlFileTestSuite.scala:
##########
@@ -0,0 +1,133 @@
+/*
+ * 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.io.File
+
+import org.scalactic.source.Position
+import org.scalatest.Tag
+
+import org.apache.spark.sql.CometTestBase
+import org.apache.spark.sql.execution.adaptive.AdaptiveSparkPlanHelper
+
+class CometSqlFileTestSuite extends CometTestBase with AdaptiveSparkPlanHelper {
+
+ override protected def test(testName: String, testTags: Tag*)(testFun: => Any)(implicit
+ pos: Position): Unit = {
+ super.test(testName, testTags: _*) {
+ withSQLConf(CometConf.COMET_NATIVE_SCAN_IMPL.key -> CometConf.SCAN_AUTO) {
+ testFun
+ }
+ }
+ }
+
+ /** Check if the current Spark version meets a minimum version requirement. */
+ private def meetsMinSparkVersion(minVersion: String): Boolean = {
+ val current = org.apache.spark.SPARK_VERSION.split("[.-]").take(2).map(_.toInt)
+ val required = minVersion.split("[.-]").take(2).map(_.toInt)
+ (current(0) > required(0)) ||
+ (current(0) == required(0) && current(1) >= required(1))
+ }
+
+ private val testResourceDir = {
+ val url = getClass.getClassLoader.getResource("sql-tests")
+ assert(url != null, "Could not find sql-tests resource directory")
+ new File(url.toURI)
+ }
+
+ private def discoverTestFiles(dir: File): Seq[File] = {
+ if (!dir.exists()) return Seq.empty
+ val files = dir.listFiles().toSeq
+ val sqlFiles = files.filter(f => f.isFile && f.getName.endsWith(".sql"))
+ val subDirFiles = files.filter(_.isDirectory).flatMap(discoverTestFiles)
+ sqlFiles ++ subDirFiles
+ }
+
+ /** Generate all config combinations from a ConfigMatrix specification. */
+ private def configCombinations(
Review Comment:
would it be good to name `configMatrix` ?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] tests: Add ~115 SQL test files covering edge cases for every Comet-supported expression [WIP] [datafusion-comet]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #3328:
URL: https://github.com/apache/datafusion-comet/pull/3328#issuecomment-3818730532
## [Codecov](https://app.codecov.io/gh/apache/datafusion-comet/pull/3328?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 16.41%. Comparing base ([`f09f8af`](https://app.codecov.io/gh/apache/datafusion-comet/commit/f09f8af64c6599255e116a376f4f008f2fd63b43?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`1c6e475`](https://app.codecov.io/gh/apache/datafusion-comet/commit/1c6e4753d6d6009cf766d02d5ad3742645efe4ca?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 905 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #3328 +/- ##
=============================================
- Coverage 56.12% 16.41% -39.71%
+ Complexity 976 411 -565
=============================================
Files 119 175 +56
Lines 11743 16153 +4410
Branches 2251 2656 +405
=============================================
- Hits 6591 2652 -3939
- Misses 4012 13079 +9067
+ Partials 1140 422 -718
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/datafusion-comet/pull/3328?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] tests: Add SQL test files covering edge cases for every Comet-supported expression [WIP] [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #3328:
URL: https://github.com/apache/datafusion-comet/pull/3328#issuecomment-3819229006
> > Are we able to use the standard .slt format or do we require additional features? There's an MIT-licensed parser from the Apache Calcite team here: https://github.com/hydromatic/sql-logic-test
>
> This is inspired by slt but there are some key differences.
>
> * We do not record expected results in the file. Instead, we compare to Spark (and we run for multiple Spark versions)
>
> * There are specific features to support minimum Spark version, and support settings Spark configs, including combinatorial matrices (so that we can migrate some existing tests to this approach)
>
> * It creates individual scala unit tests per file, fitting into the existing testing approach
It is also worth pointing out that the test runner is ~300 LOC and extends `CometTestBase`, so this is tightly integrated with existing test infrastructure
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] tests: Add SQL test files covering edge cases for (almost) every Comet-supported expression [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #3328:
URL: https://github.com/apache/datafusion-comet/pull/3328#issuecomment-3828913841
Thanks for the reviews @mbutrovich @comphead. I will get the related documentation PR ready to review next.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] tests: Add SQL test files covering edge cases for every Comet-supported expression [WIP] [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on PR #3328:
URL: https://github.com/apache/datafusion-comet/pull/3328#issuecomment-3819181265
Are we able to use the standard .slt format or do we require additional features? There's an MIT-licensed parser from the Apache Calcite team here: https://github.com/hydromatic/sql-logic-test
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] tests: Add SQL test files covering edge cases for every Comet-supported expression [WIP] [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #3328:
URL: https://github.com/apache/datafusion-comet/pull/3328#issuecomment-3819197007
> Are we able to use the standard .slt format or do we require additional features? There's an MIT-licensed parser from the Apache Calcite team here: https://github.com/hydromatic/sql-logic-test
This is inspired by slt but there are some key differences.
- We do not record expected results in the file. Instead, we compare to Spark (and we run for multiple Spark versions)
- There are specific features to support minimum Spark version, and support settings Spark configs, including combinatorial matrices (so that we can migrate some existing tests to this approach)
- It individual scala unit tests per file, fitting into the existing testing approach
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] tests: Add SQL test files covering edge cases for (almost) every Comet-supported expression [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #3328:
URL: https://github.com/apache/datafusion-comet/pull/3328#discussion_r2742848342
##########
spark/src/test/scala/org/apache/comet/CometSqlFileTestSuite.scala:
##########
@@ -0,0 +1,133 @@
+/*
+ * 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.io.File
+
+import org.scalactic.source.Position
+import org.scalatest.Tag
+
+import org.apache.spark.sql.CometTestBase
+import org.apache.spark.sql.execution.adaptive.AdaptiveSparkPlanHelper
+
+class CometSqlFileTestSuite extends CometTestBase with AdaptiveSparkPlanHelper {
+
+ override protected def test(testName: String, testTags: Tag*)(testFun: => Any)(implicit
+ pos: Position): Unit = {
+ super.test(testName, testTags: _*) {
+ withSQLConf(CometConf.COMET_NATIVE_SCAN_IMPL.key -> CometConf.SCAN_AUTO) {
+ testFun
+ }
+ }
+ }
+
+ /** Check if the current Spark version meets a minimum version requirement. */
+ private def meetsMinSparkVersion(minVersion: String): Boolean = {
+ val current = org.apache.spark.SPARK_VERSION.split("[.-]").take(2).map(_.toInt)
+ val required = minVersion.split("[.-]").take(2).map(_.toInt)
+ (current(0) > required(0)) ||
+ (current(0) == required(0) && current(1) >= required(1))
+ }
+
+ private val testResourceDir = {
+ val url = getClass.getClassLoader.getResource("sql-tests")
+ assert(url != null, "Could not find sql-tests resource directory")
+ new File(url.toURI)
+ }
+
+ private def discoverTestFiles(dir: File): Seq[File] = {
+ if (!dir.exists()) return Seq.empty
+ val files = dir.listFiles().toSeq
+ val sqlFiles = files.filter(f => f.isFile && f.getName.endsWith(".sql"))
+ val subDirFiles = files.filter(_.isDirectory).flatMap(discoverTestFiles)
+ sqlFiles ++ subDirFiles
+ }
+
+ /** Generate all config combinations from a ConfigMatrix specification. */
+ private def configCombinations(
+ matrix: Seq[(String, Seq[String])]): Seq[Seq[(String, String)]] = {
+ if (matrix.isEmpty) return Seq(Seq.empty)
+ val (key, values) = matrix.head
+ val rest = configCombinations(matrix.tail)
+ for {
+ value <- values
+ combo <- rest
+ } yield (key, value) +: combo
+ }
+
+ private def runTestFile(file: SqlTestFile): Unit = {
+ val allConfigs = file.configs
+ withSQLConf(allConfigs: _*) {
+ withTable(file.tables: _*) {
+ file.records.foreach {
+ case SqlStatement(sql) =>
+ spark.sql(sql)
+ case SqlQuery(sql, mode) =>
+ mode match {
+ case CheckOperator =>
+ checkSparkAnswerAndOperator(sql)
+ case SparkAnswerOnly =>
+ checkSparkAnswer(sql)
+ case WithTolerance(tol) =>
+ checkSparkAnswerWithTolerance(sql, tol)
+ case ExpectFallback(reason) =>
+ checkSparkAnswerAndFallbackReason(sql, reason)
+ case Ignore(reason) =>
+ logInfo(s"IGNORED query (${reason}): $sql")
+ }
+ }
+ }
+ }
+ }
+
+ // Discover and register all .sql test files
+ discoverTestFiles(testResourceDir).foreach { file =>
Review Comment:
I will try this as follow on PR if that is ok.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] tests: Add SQL test files covering edge cases for (almost) every Comet-supported expression [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #3328:
URL: https://github.com/apache/datafusion-comet/pull/3328
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org