andygrove commented on code in PR #5878: URL: https://github.com/apache/datafusion-comet/pull/5878#discussion_r3996935615
########## spark/src/test/resources/sql-tests/expressions/math/round.sql: ########## @@ -93,3 +97,17 @@ SELECT l, round(l, -19), round(l, -20) FROM test_round_long_overflow query SELECT round(5000000000000000000L, -19), round(-5000000000000000000L, -19) + +-- Disabling the dispatcher leaves supported types native and sends float/double inputs to Spark, +-- even with allowIncompatible enabled. +statement +SET spark.comet.exec.scalaUDF.codegen.enabled=false Review Comment: This is the first fixture in `sql-tests/` that changes config with a mid-file `SET`. It works because the `Config: ...codegen.enabled=true` line at the top makes `withSQLConf` restore the flag after the file, but nothing in the fixture says that, and the top-of-file directive now reads as if the dispatcher is on for the whole file. If someone later removes that seemingly redundant `Config` line, the `SET` leaks and every fixture that runs afterwards silently loses dispatcher coverage. Could we follow the existing pattern instead and move these three queries into their own fixture with `-- Config: spark.comet.exec.scalaUDF.codegen.enabled=false` at the top? `misc/codegen_dispatch_disabled_fallback.sql` is the precedent. That keeps each file under a single config. ########## spark/src/test/resources/sql-tests/expressions/math/round.sql: ########## @@ -15,9 +15,13 @@ -- specific language governing permissions and limitations -- under the License. +-- Config: spark.comet.exec.scalaUDF.codegen.enabled=true +-- ConfigMatrix: spark.comet.expression.round.allowIncompatible=false,true Review Comment: I don't think this matrix can observe anything. `CometRound.getSupportLevel` only returns `Unsupported` or `Compatible`, and the `Unsupported` arm in `exprToProtoInternal` never reads `allowIncompatible`. So both matrix values run the exact same code, and the whole file (including the overflow tables and every existing query) runs twice. The sql-file-tests doc asks us not to add `ConfigMatrix` speculatively. Could we drop the matrix here? If the goal is to guard against float/double being reclassified as `Incompatible` in the future, that would be better handled by whichever PR makes that change adding the row 2 coverage at that point. -- 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]
