Copilot commented on code in PR #12962:
URL: https://github.com/apache/gluten/pull/12962#discussion_r3926717112
##########
gluten-iceberg/src/test/scala/org/apache/gluten/execution/IcebergSuite.scala:
##########
@@ -60,6 +64,70 @@ abstract class IcebergSuite extends
WholeStageTransformerSuite {
}
}
+ test("rewrite_data_files uses an iceberg staged scan transformer") {
+ val tableName = "iceberg_rewrite_tb"
+ withTable(tableName) {
+ withSQLConf("spark.sql.adaptive.enabled" -> "false") {
+ spark.sql(s"CREATE TABLE $tableName (id INT, data STRING) USING
iceberg")
+ (1 to 5).foreach {
+ id => spark.sql(s"INSERT INTO $tableName VALUES ($id, 'value-$id')")
+ }
+
+ def dataFileCount: Long =
+ spark.table(s"spark_catalog.default.$tableName.files").count()
+
+ assert(dataFileCount == 5)
+ val stagedScanSeen = new CountDownLatch(1)
+ val listener = new QueryExecutionListener {
+ override def onSuccess(
+ funcName: String,
+ qe: QueryExecution,
+ durationNs: Long): Unit = {
+ if (
+ qe.executedPlan.exists {
+ case scan: IcebergScanTransformer =>
+ scan.scan.getClass.getSimpleName == "SparkStagedScan"
+ case _ => false
+ }
Review Comment:
Avoid checking the scan type via `getSimpleName` string comparison; it’s
brittle (anonymous subclasses can have an empty simpleName) and won’t match
subclasses. Prefer an `isInstanceOf` check against the concrete Iceberg scan
class.
##########
gluten-iceberg/src/main/scala/org/apache/iceberg/spark/source/GlutenIcebergSourceUtil.scala:
##########
@@ -42,8 +42,13 @@ object GlutenIcebergSourceUtil {
private val InputFileBlockStartCol = "input_file_block_start"
private val InputFileBlockLengthCol = "input_file_block_length"
- def getClassOfSparkBatchQueryScan(): Class[SparkBatchQueryScan] = {
- classOf[SparkBatchQueryScan]
+ def supportsScan(sparkScan: Scan): Boolean = sparkScan match {
+ case _: SparkBatchQueryScan => true
+ case scan: SparkStagedScan =>
+ val tasks = getScanTasks(scan)
+ tasks.nonEmpty &&
+ (tasks.forall(_.isFileScanTask) ||
tasks.forall(_.isInstanceOf[CombinedScanTask]))
+ case _ => false
}
Review Comment:
`supportsScan` materializes all staged-scan tasks into a `List` just to test
emptiness/type, which can be expensive for large rewrites. This can be done
with a single iterator pass without allocating the full task list.
--
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]
