Copilot commented on code in PR #12776: URL: https://github.com/apache/gluten/pull/12776#discussion_r3988472391
########## gluten-ut/spark33/src/test/scala/org/apache/spark/sql/execution/datasources/parquet/GlutenParquetFilterSuite.scala: ########## @@ -0,0 +1,383 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.spark.sql.execution.datasources.parquet + +import org.apache.spark.SparkConf +import org.apache.spark.sql._ +import org.apache.spark.sql.catalyst.dsl.expressions._ +import org.apache.spark.sql.catalyst.expressions._ +import org.apache.spark.sql.catalyst.optimizer.InferFiltersFromConstraints +import org.apache.spark.sql.catalyst.planning.PhysicalOperation +import org.apache.spark.sql.connector.catalog.CatalogV2Implicits.parseColumnPath +import org.apache.spark.sql.execution.datasources.{DataSourceStrategy, HadoopFsRelation, LogicalRelation, PushableColumnAndNestedColumn} +import org.apache.spark.sql.execution.datasources.v2.DataSourceV2ScanRelation +import org.apache.spark.sql.execution.datasources.v2.parquet.ParquetScan +import org.apache.spark.sql.internal.SQLConf +import org.apache.spark.sql.types._ +import org.apache.spark.tags.ExtendedSQLTest +import org.apache.spark.util.Utils + +import org.apache.hadoop.fs.Path +import org.apache.parquet.filter2.predicate.{FilterApi, FilterPredicate} +import org.apache.parquet.filter2.predicate.FilterApi._ +import org.apache.parquet.hadoop.{ParquetFileReader, ParquetInputFormat, ParquetOutputFormat} +import org.apache.parquet.hadoop.util.HadoopInputFile + +abstract class GlutenParquetFilterSuite extends ParquetFilterSuite with GlutenSQLTestsBaseTrait { Review Comment: Adding this wrapper enables every inherited `ParquetFilterSuite` test for Spark 3.3. Unlike the Spark 3.4/3.5 backend settings, `spark33`'s Bolt settings do not exclude the inherited `filter pushdown - timestamp` and `filter pushdown - date` cases, even though the removed backend-specific rewrites avoided the same legacy rebase/INT96 behavior. Add those exclusions or provide Bolt-specific rewrites before enabling this suite. ########## gluten-ut/spark34/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala: ########## @@ -443,6 +431,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. .exclude("parquet timestamp conversion") enableSuite[GlutenParquetIOSuite] Review Comment: Removing this exclusion re-enables `SPARK-40128`, but the test exercises DELTA_LENGTH_BYTE_ARRAY data with an offset-zero page and the Velox reader support has not changed in this PR. The test will fail rather than merely update coverage; retain the exclusion until the reader limitation is fixed. ########## gluten-ut/spark35/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala: ########## @@ -394,6 +382,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. .exclude("parquet timestamp conversion") enableSuite[GlutenParquetIOSuite] Review Comment: Removing this exclusion re-enables `SPARK-40128`, but the test exercises DELTA_LENGTH_BYTE_ARRAY data with an offset-zero page and the Velox reader support has not changed in this PR. The test will fail rather than merely update coverage; retain the exclusion until the reader limitation is fixed. ########## gluten-ut/spark40/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala: ########## @@ -560,9 +557,6 @@ class VeloxTestSettings extends BackendTestSettings { enableSuite[GlutenParquetV2FilterSuite] Review Comment: The removed plain exclusions re-enable Spark's inherited `filter pushdown - timestamp/date` cases after the backend-specific rewritten cases were deleted. Those upstream cases exercise the legacy date-rebase/INT96 behavior that this suite's previous rewrite avoided, and no corresponding Velox support change is included; keep the exclusions or port backend-safe tests before removing them. ########## 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. .exclude("parquet timestamp conversion") enableSuite[GlutenParquetIOSuite] Review Comment: Removing this exclusion re-enables `SPARK-40128`, but the test exercises DELTA_LENGTH_BYTE_ARRAY data with an offset-zero page and the Velox reader support has not changed in this PR. The test will fail rather than merely update coverage; retain the exclusion until the reader limitation is fixed. ########## gluten-ut/spark35/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala: ########## @@ -342,27 +342,18 @@ class VeloxTestSettings extends BackendTestSettings { enableSuite[GlutenOrcV1SchemaPruningSuite] enableSuite[GlutenOrcV2SchemaPruningSuite] enableSuite[GlutenParquetColumnIndexSuite] - // Rewrite by just removing test timestamp. - .exclude("test reading unaligned pages - test all types") - // Rewrite by converting smaller integral value to timestamp. - .exclude("test reading unaligned pages - test all types (dict encode)") enableSuite[GlutenParquetCompressionCodecPrecedenceSuite] enableSuite[GlutenParquetDeltaByteArrayEncodingSuite] enableSuite[GlutenParquetDeltaEncodingInteger] enableSuite[GlutenParquetDeltaEncodingLong] enableSuite[GlutenParquetDeltaLengthByteArrayEncodingSuite] enableSuite[GlutenParquetEncodingSuite] - // Velox does not support rle encoding, but it can pass when native writer enabled. - .exclude("parquet v2 pages - rle encoding for boolean value columns") enableSuite[GlutenParquetFieldIdIOSuite] enableSuite[GlutenParquetFileFormatV1Suite] enableSuite[GlutenParquetFileFormatV2Suite] enableSuite[GlutenParquetV1FilterSuite] Review Comment: The removed plain exclusions re-enable Spark's inherited `filter pushdown - timestamp/date` cases after the backend-specific rewritten cases were deleted. Those upstream cases exercise the legacy date-rebase/INT96 behavior that this suite's previous rewrite avoided, and no corresponding Velox support change is included; keep the exclusions or port backend-safe tests before removing them. ########## gluten-ut/spark34/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala: ########## @@ -391,27 +391,18 @@ class VeloxTestSettings extends BackendTestSettings { enableSuite[GlutenOrcV1SchemaPruningSuite] enableSuite[GlutenOrcV2SchemaPruningSuite] enableSuite[GlutenParquetColumnIndexSuite] - // Rewrite by just removing test timestamp. - .exclude("test reading unaligned pages - test all types") - // Rewrite by converting smaller integral value to timestamp. - .exclude("test reading unaligned pages - test all types (dict encode)") enableSuite[GlutenParquetCompressionCodecPrecedenceSuite] enableSuite[GlutenParquetDeltaByteArrayEncodingSuite] enableSuite[GlutenParquetDeltaEncodingInteger] enableSuite[GlutenParquetDeltaEncodingLong] enableSuite[GlutenParquetDeltaLengthByteArrayEncodingSuite] enableSuite[GlutenParquetEncodingSuite] - // Velox does not support rle encoding, but it can pass when native writer enabled. - .exclude("parquet v2 pages - rle encoding for boolean value columns") enableSuite[GlutenParquetFieldIdIOSuite] enableSuite[GlutenParquetFileFormatV1Suite] enableSuite[GlutenParquetFileFormatV2Suite] enableSuite[GlutenParquetV1FilterSuite] Review Comment: The removed plain exclusions re-enable Spark's inherited `filter pushdown - timestamp/date` cases after the backend-specific rewritten cases were deleted. Those upstream cases exercise the legacy date-rebase/INT96 behavior that this suite's previous rewrite avoided, and no corresponding Velox support change is included; keep the exclusions or port backend-safe tests before removing them. This issue also appears on line 418 of the same file. ########## gluten-ut/spark35/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala: ########## @@ -477,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") enableSuite[GlutenOrcV1AggregatePushDownSuite] .exclude("nested column: Count(nested sub-field) not push down") enableSuite[GlutenOrcV2AggregatePushDownSuite] .exclude("nested column: Max(top level column) not push down") .exclude("nested column: Count(nested sub-field) not push down") enableSuite[GlutenParquetCodecSuite] Review Comment: Removing these exclusions re-enables the `lz4_raw` and `lz4raw` Parquet codec tests, but `VeloxBackend.validateCompressionCodec` explicitly rejects both codecs, so native writes fail with the backend validation error. Restore the exclusions until the Velox writer supports these codecs, or make these tests bypass the native writer. ########## gluten-ut/spark40/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala: ########## @@ -542,9 +542,6 @@ class VeloxTestSettings extends BackendTestSettings { enableSuite[GlutenParquetV1FilterSuite] Review Comment: The removed plain exclusions re-enable Spark's inherited `filter pushdown - timestamp/date` cases after the backend-specific rewritten cases were deleted. Those upstream cases exercise the legacy date-rebase/INT96 behavior that this suite's previous rewrite avoided, and no corresponding Velox support change is included; keep the exclusions or port backend-safe tests before removing them. ########## gluten-ut/spark40/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala: ########## @@ -576,6 +570,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. .exclude("parquet timestamp conversion") enableSuite[GlutenParquetIOSuite] Review Comment: Removing this exclusion re-enables `SPARK-40128`, but the test exercises DELTA_LENGTH_BYTE_ARRAY data with an offset-zero page and the Velox reader support has not changed in this PR. The test will fail rather than merely update coverage; retain the exclusion until the reader limitation is fixed. ########## gluten-ut/spark41/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala: ########## @@ -516,9 +516,6 @@ class VeloxTestSettings extends BackendTestSettings { enableSuite[GlutenParquetV1FilterSuite] Review Comment: The removed plain exclusions re-enable Spark's inherited `filter pushdown - timestamp/date` cases after the backend-specific rewritten cases were deleted. Those upstream cases exercise the legacy date-rebase/INT96 behavior that this suite's previous rewrite avoided, and no corresponding Velox support change is included; keep the exclusions or port backend-safe tests before removing them. ########## gluten-ut/spark41/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala: ########## @@ -534,9 +531,6 @@ class VeloxTestSettings extends BackendTestSettings { enableSuite[GlutenParquetV2FilterSuite] Review Comment: The removed plain exclusions re-enable Spark's inherited `filter pushdown - timestamp/date` cases after the backend-specific rewritten cases were deleted. Those upstream cases exercise the legacy date-rebase/INT96 behavior that this suite's previous rewrite avoided, and no corresponding Velox support change is included; keep the exclusions or port backend-safe tests before removing them. -- 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]
