comphead commented on code in PR #5891:
URL: https://github.com/apache/datafusion-comet/pull/5891#discussion_r4274439099
##########
spark/src/test/scala/org/apache/comet/SparkErrorConverterSuite.scala:
##########
@@ -69,6 +71,23 @@ class SparkErrorConverterSuite extends AnyFunSuite {
assert(!ex.getMessage.contains("fallback.parquet"))
}
+ test("ParquetTimestampOverflow wraps the arithmetic cause in a file-read
SparkException") {
Review Comment:
Everything this test asserts is also asserted end to end by
`TIMESTAMP_MILLIS overflow fails in native scan` in `ParquetReadSuite`: the
error class per Spark version, the `path` parameter, the
`ArithmeticException("long overflow")` cause, the two-file task and the `%` and
space in the file name. Could this one be dropped together with the
`SparkException` and `isSpark40Plus` imports it adds? The counterpoint is that
the suite has per-variant converter tests for `CannotReadFile`, so keep it if
you prefer that pattern.
##########
spark/src/test/scala/org/apache/comet/parquet/ParquetReadSuite.scala:
##########
@@ -339,16 +349,29 @@ abstract class ParquetReadSuite extends CometTestBase {
}
Seq(false, true).foreach { ansiEnabled =>
- withSQLConf(SQLConf.ANSI_ENABLED.key -> ansiEnabled.toString) {
- readParquetFile(path.toString) { df =>
- Seq("ts", "ts_ntz", "s", "s.ts", "s.ts_ntz", "a", "m").foreach
{ column =>
- val selected = df.select(column)
- assert(collect(selected.queryExecution.executedPlan) {
- case _: CometNativeScanExec => true
- }.nonEmpty)
+ withSQLConf(
+ SQLConf.ANSI_ENABLED.key -> ansiEnabled.toString,
+ SQLConf.FILES_MIN_PARTITION_NUM.key -> "1",
+ SQLConf.FILES_MAX_PARTITION_BYTES.key -> "134217728") {
Review Comment:
This suite already has `forceSinglePartitionConf` (line 960), which sets
`FILES_MIN_PARTITION_NUM` to 1 and documents why. Could the test use it?
`134217728` is Spark's default for `spark.sql.files.maxPartitionBytes`
(`128MB`). I could not find anything in the test base classes that overrides
it, so that line can go.
--
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]
