Copilot commented on code in PR #12776:
URL: https://github.com/apache/gluten/pull/12776#discussion_r3990679293
##########
gluten-ut/spark35/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala:
##########
@@ -402,35 +391,35 @@ class VeloxTestSettings extends BackendTestSettings {
.exclude("SPARK-35640: read binary as timestamp should throw schema
incompatible error")
// Exception msg.
.exclude("SPARK-35640: int as long should throw schema incompatible error")
- // Velox parquet reader not allow offset zero.
- .exclude("SPARK-40128 read DELTA_LENGTH_BYTE_ARRAY encoded strings")
enableSuite[GlutenParquetV1PartitionDiscoverySuite]
enableSuite[GlutenParquetV2PartitionDiscoverySuite]
enableSuite[GlutenParquetProtobufCompatibilitySuite]
enableSuite[GlutenParquetV1QuerySuite]
+ // No fix needed: INT read as LONG is allowed.
.exclude("row group skipping doesn't overflow when reading into larger
type")
// Unsupport spark.sql.files.ignoreCorruptFiles.
.exclude("Enabling/disabling ignoreCorruptFiles")
Review Comment:
The new comment 'No fix needed: INT read as LONG is allowed.' doesn’t
clearly explain why the referenced test is still excluded (the test name is
about row-group skipping overflow rather than type widening correctness).
Please revise the comment to reflect the actual reason the test remains
excluded for Velox (or remove the exclusion if it now passes).
##########
gluten-ut/spark41/src/test/scala/org/apache/spark/sql/execution/datasources/parquet/GlutenParquetFilterSuite.scala:
##########
@@ -68,44 +59,6 @@ abstract class GlutenParquetFilterSuite extends
ParquetFilterSuite with GlutenSQ
getWorkspaceFilePath("sql", "core", "src", "test", "resources").toString
+ "/" + name)
}
- testGluten("filter pushdown - timestamp") {
- Seq(true, false).foreach {
- java8Api =>
- Seq(CORRECTED, LEGACY).foreach {
- rebaseMode =>
- val millisData = Seq(
- "1000-06-14 08:28:53.123",
- "1582-06-15 08:28:53.001",
- "1900-06-16 08:28:53.0",
- "2018-06-17 08:28:53.999")
- // INT96 doesn't support pushdown
- withSQLConf(
- SQLConf.DATETIME_JAVA8API_ENABLED.key -> java8Api.toString,
- SQLConf.PARQUET_INT96_REBASE_MODE_IN_WRITE.key ->
rebaseMode.toString,
- SQLConf.PARQUET_OUTPUT_TIMESTAMP_TYPE.key -> INT96.toString
- ) {
- import testImplicits._
- withTempPath {
- file =>
- millisData
- .map(i => Tuple1(Timestamp.valueOf(i)))
- .toDF
- .write
- .format(dataSourceName)
- .save(file.getCanonicalPath)
- readParquetFile(file.getCanonicalPath) {
- df =>
- val schema = new
SparkToParquetSchemaConverter(conf).convert(df.schema)
- assertResult(None) {
-
createParquetFilters(schema).createFilter(sources.IsNull("_1"))
- }
- }
- }
- }
- }
- }
- }
-
testGluten("SPARK-12218: 'Not' is included in Parquet filter pushdown") {
import testImplicits._
Review Comment:
The Gluten-local Parquet tests `filter pushdown - timestamp` and `filter
pushdown - date` were removed in this suite (and similarly in other
Spark-version copies). If the intent is to rely on inherited Spark
`ParquetFilterSuite` coverage instead, it would help to (a) confirm those
specific scenarios are still exercised for Gluten/Velox and (b) keep at least
one targeted assertion for Velox-specific semantics (e.g., INT96 pushdown
behavior) if Spark coverage does not validate the native path.
##########
gluten-ut/spark41/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala:
##########
@@ -516,9 +516,6 @@ class VeloxTestSettings extends BackendTestSettings {
enableSuite[GlutenParquetV1FilterSuite]
// Rewrite.
.exclude("SPARK-23852: Broken Parquet push-down for partially-written
stats")
- // Rewrite for supported INT96 - timestamp.
- .exclude("filter pushdown - timestamp")
- .exclude("filter pushdown - date")
// Exception bebaviour.
.exclude("SPARK-25207: exception when duplicate fields in case-insensitive
mode")
Review Comment:
The PR description is currently left as the default template and doesn’t
explain the rationale for removing multiple Parquet unit tests and changing
suite exclusions (which can materially affect CI signal). Please update the PR
description to summarize which tests were removed/reenabled and why (e.g.,
relying on upstream Spark tests, backend behavior changes, or prior rewrites no
longer needed).
##########
gluten-ut/spark41/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala:
##########
@@ -550,6 +544,7 @@ class VeloxTestSettings extends BackendTestSettings {
.exclude("filter pushdown - StringPredicate")
.exclude("SPARK-38825: in and notIn filters")
enableSuite[GlutenParquetInteroperabilitySuite]
+ // Disabled because Velox does not implement Spark’s legacy Impala INT96
timestamp conversion semantics.
Review Comment:
The comment uses a typographic apostrophe (`Spark’s`). To avoid potential
encoding/tooling issues and keep source comments consistent, consider using a
plain ASCII apostrophe (`Spark's`).
--
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]
