Copilot commented on code in PR #3390: URL: https://github.com/apache/datafusion-comet/pull/3390#discussion_r2763733937
########## spark/src/test/scala/org/apache/comet/SqlFileTestParser.scala: ########## @@ -26,11 +26,30 @@ 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 statement to execute (CREATE TABLE, INSERT, etc.). + * + * @param sql + * The SQL text. + * @param line + * 1-based line number in the original .sql file where the statement starts. + */ +case class SqlStatement(sql: String, line: Int = -1) extends SqlTestRecord Review Comment: The Scaladoc says `line` is a 1-based line number, but the default value is `-1`. Either document the sentinel value (e.g., "-1 if unknown"), or consider making it an `Option[Int]` to avoid an invalid value for the documented contract. ########## spark/src/test/scala/org/apache/comet/SqlFileTestParser.scala: ########## @@ -26,11 +26,30 @@ 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 statement to execute (CREATE TABLE, INSERT, etc.). + * + * @param sql + * The SQL text. + * @param line + * 1-based line number in the original .sql file where the statement starts. + */ +case class SqlStatement(sql: String, line: Int = -1) extends SqlTestRecord -/** A SQL query whose results are compared between Spark and Comet. */ -case class SqlQuery(sql: String, mode: QueryAssertionMode = CheckCoverageAndAnswer) +/** + * A SQL query whose results are compared between Spark and Comet. + * + * @param sql + * The SQL text. + * @param mode + * How to validate the query. + * @param line + * 1-based line number in the original .sql file where the query starts. + */ +case class SqlQuery( + sql: String, + mode: QueryAssertionMode = CheckCoverageAndAnswer, + line: Int = -1) Review Comment: The Scaladoc for `line` says it is a 1-based line number, but the default is `-1`. Please either mention that `-1` means "unknown" or switch to an `Option[Int]` to keep the type aligned with the documented meaning. -- 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]
