felipepessoto commented on code in PR #13042:
URL: https://github.com/apache/gluten/pull/13042#discussion_r4075377572
##########
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:
Thanks for the suggestion. I’m leaving this as-is because
`DeltaParquetFileFormat.IS_ROW_DELETED_STRUCT_FIELD` defines this generated
column as `ByteType`, and the test constructs the requested field directly from
that upstream constant. `getByte` therefore verifies the same contract while
checking the values; if upstream changes the type, this test will fail at the
affected assertion rather than silently accepting it. A separate schema
assertion would duplicate the constant without adding behavioral coverage.
##########
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:
Thanks for checking this. I’m keeping the name-only predicate because it
intentionally mirrors Delta’s JVM reader contract. In Delta 3.3.2 and 4.0.1,
`DeltaParquetFileFormat.buildReaderWithPartitionValues` uses
`findColumn(name)`, which matches these generated fields by exact name without
validating `DataType`; the constants are plain `StructField`s with no separate
generated-metadata marker. Adding a stricter type requirement here could let
Gluten offload a scan that Delta would treat as generated metadata,
reintroducing the NULL/invalid-value behavior this fallback prevents.
##########
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:
Thanks. Exact equality is intentional here so the fallback matches Delta’s
JVM synthesis path. In Delta 3.3.2 and 4.0.1,
`DeltaParquetFileFormat.findColumn` checks `field.name == name` for
`__delta_internal_row_index`, `__delta_internal_is_row_deleted`, and the
temporary row-index name; it does not use Spark’s resolver. Using a
case-insensitive resolver only in Gluten would broaden the fallback beyond the
fields Delta itself recognizes and synthesizes.
--
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]
