Copilot commented on code in PR #13042:
URL: https://github.com/apache/gluten/pull/13042#discussion_r4074921412
##########
gluten-delta/src/main/scala/org/apache/gluten/extension/OffloadDeltaScan.scala:
##########
@@ -91,12 +94,25 @@ case class OffloadDeltaScan(enableNativeDmlRowIndexScan:
Boolean) extends Offloa
}))
}
+ private def scanReadsGeneratedDeletionVectorMetadataColumn(
+ scan: FileSourceScanExec): Boolean = {
+ scanReadsColumn(
+ scan,
+ (name, _) => generatedDeletionVectorMetadataColumnNames.contains(name))
+ }
Review Comment:
`scanReadsGeneratedDeletionVectorMetadataColumn` ignores Spark’s
case-sensitivity rules by using raw `String` equality. If a query references
these synthetic columns with different casing (while Spark is
case-insensitive), this can fail to trigger fallback and reintroduce incorrect
NULL/invalid values. Use `spark.sessionState.conf.resolver` (or equivalent
resolver available here) to compare names, and apply the resolver consistently
across the synthetic-name checks.
##########
gluten-delta/src/main/scala/org/apache/gluten/extension/OffloadDeltaScan.scala:
##########
@@ -91,12 +94,25 @@ case class OffloadDeltaScan(enableNativeDmlRowIndexScan:
Boolean) extends Offloa
}))
}
+ private def scanReadsGeneratedDeletionVectorMetadataColumn(
+ scan: FileSourceScanExec): Boolean = {
+ scanReadsColumn(
+ scan,
+ (name, _) => generatedDeletionVectorMetadataColumnNames.contains(name))
+ }
Review Comment:
This predicate matches only on column name and disregards `DataType`, which
can cause unnecessary fallback (loss of native offload) if a scan includes a
user column with the same name as a synthetic DV metadata column but a
non-matching type. Since you already have `isRowIndexColumn(name, dataType)`,
consider using it for the row-index names; and for
`IS_ROW_DELETED_COLUMN_NAME`, validate the expected type (e.g., `ByteType`)
before triggering fallback.
##########
backends-velox/src-delta40/test/scala/org/apache/spark/sql/delta/DeltaDeletionVectorHandoffSuite.scala:
##########
@@ -140,6 +168,89 @@ class DeltaDeletionVectorHandoffSuite
}
}
+ test("Delta generated row-index scan should fall back when metadata row
index is disabled") {
+ withTempDir {
+ tempDir =>
+ val path = tempDir.getCanonicalPath
+ Seq(0, 1, 2)
+ .toDF("value")
+ .coalesce(1)
+ .sortWithinPartitions("value")
+ .write
+ .format("delta")
+ .save(path)
+
+ withSQLConf(DeltaSQLConf.DELETION_VECTORS_USE_METADATA_ROW_INDEX.key
-> "false") {
+ val rowIndexDf =
+ dataframeWithSyntheticColumns(path,
DeltaParquetFileFormat.ROW_INDEX_STRUCT_FIELD)
+
+
assert(!containsNativeDeltaScan(rowIndexDf.queryExecution.executedPlan))
+ checkAnswer(
+ rowIndexDf.select("value",
DeltaParquetFileFormat.ROW_INDEX_COLUMN_NAME),
+ Seq(Row(0, 0L), Row(1, 1L), Row(2, 2L)))
+ }
+ }
+ }
+
+ test("Delta generated deleted-row scan should fall back for a DV-free file")
{
+ withTempDir {
+ tempDir =>
+ val path = tempDir.getCanonicalPath
+ Seq(0, 1, 2).toDF("value").coalesce(1).write.format("delta").save(path)
+
+ withSQLConf(DeltaSQLConf.DELETION_VECTORS_USE_METADATA_ROW_INDEX.key
-> "false") {
+ val deletedRowDf =
+ dataframeWithSyntheticColumns(path,
DeltaParquetFileFormat.IS_ROW_DELETED_STRUCT_FIELD)
+
+
assert(!containsNativeDeltaScan(deletedRowDf.queryExecution.executedPlan))
+ assert(
+ deletedRowDf
+ .select(DeltaParquetFileFormat.IS_ROW_DELETED_COLUMN_NAME)
+ .collect()
+ .map(_.getByte(0))
+ .toSet === Set(0.toByte))
Review Comment:
This test assumes the deleted-row flag column is `ByteType` (via
`getByte(0)`) but doesn’t assert the schema/type, so a future upstream change
(e.g., column type becoming `BooleanType`) would fail with a less-informative
runtime error. Consider adding an explicit schema/type assertion for
`IS_ROW_DELETED_COLUMN_NAME` before collecting, to make failures clearer and
easier to diagnose.
--
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]
