Copilot commented on code in PR #12776:
URL: https://github.com/apache/gluten/pull/12776#discussion_r3905409389
##########
gluten-ut/spark35/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala:
##########
@@ -478,16 +466,13 @@ class VeloxTestSettings extends BackendTestSettings {
enableSuite[GlutenParquetV1AggregatePushDownSuite]
enableSuite[GlutenParquetV2AggregatePushDownSuite]
// TODO: Timestamp columns stats will lost if using int64 in parquet
writer.
- .exclude("aggregate push down - different data types")
+ .include("aggregate push down - different data types")
Review Comment:
Same concern as above: `.include(...)` may unintentionally restrict the
suite to a single test (or otherwise alter selection behavior). If the goal is
to start running this test again, remove the prior exclusion and avoid adding
`.include(...)` unless you explicitly want an allow-list for
`GlutenParquetV2AggregatePushDownSuite`.
##########
gluten-ut/spark35/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala:
##########
@@ -395,43 +383,43 @@ class VeloxTestSettings extends BackendTestSettings {
.exclude("filter pushdown - StringPredicate")
.exclude("SPARK-38825: in and notIn filters")
enableSuite[GlutenParquetInteroperabilitySuite]
- .exclude("parquet timestamp conversion")
+ .include("parquet timestamp conversion")
Review Comment:
Switching from `.exclude(...)` to `.include(...)` can change semantics from
'run everything except X' to 'run only X' (depending on the
`BackendTestSettings` DSL). If the intent is simply to re-enable this
previously excluded test, prefer removing the `.exclude(\"parquet timestamp
conversion\")` line without adding `.include(...)`, to avoid accidentally
filtering out the rest of `GlutenParquetInteroperabilitySuite`.
##########
gluten-ut/spark35/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala:
##########
@@ -395,43 +383,43 @@ class VeloxTestSettings extends BackendTestSettings {
.exclude("filter pushdown - StringPredicate")
.exclude("SPARK-38825: in and notIn filters")
enableSuite[GlutenParquetInteroperabilitySuite]
- .exclude("parquet timestamp conversion")
+ .include("parquet timestamp conversion")
enableSuite[GlutenParquetIOSuite]
// Velox doesn't write file metadata into parquet file.
.exclude("Write Spark version into Parquet metadata")
// Exception.
.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")
- // decimal failed ut
+ // No fix needed: narrowing the scale should not be allowed.
.exclude("SPARK-34212 Parquet should read decimals correctly")
// new added in spark-3.3 and need fix later, random failure may caused by
memory free
.exclude("SPARK-39833: pushed filters with project without filter columns")
.exclude("SPARK-39833: pushed filters with count()")
// Rewrite because the filter after datasource is not needed.
.exclude(
"SPARK-26677: negated null-safe equality comparison should not filter
matched row groups")
- // Velox currently does not distinguish `isAdjustedToUTC` in Parquet.
+ // No fix needed: read TimestampLTZ as TimestampNTZ is allowed.
.exclude("SPARK-36182: can't read TimestampLTZ as TimestampNTZ")
Review Comment:
The new comment is ambiguous given the test is still excluded. Consider
clarifying the rationale (e.g., that Velox intentionally allows this behavior
and the Spark test asserts the opposite), so it’s clear why the exclusion
remains.
##########
gluten-ut/spark41/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala:
##########
@@ -518,9 +518,6 @@ class VeloxTestSettings extends BackendTestSettings {
enableSuite[GlutenParquetV1FilterSuite]
Review Comment:
The PR description is still the unfilled template and doesn’t describe what
changed (e.g., removal of filter pushdown tests and updates to Velox
exclusions/includes). Please update the PR description with a short summary and
how it was tested so reviewers/CI owners can validate the intent and expected
impact.
--
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]
