Copilot commented on code in PR #12964:
URL: https://github.com/apache/gluten/pull/12964#discussion_r4312786476
##########
gluten-substrait/src/main/scala/org/apache/gluten/execution/BasicScanExecTransformer.scala:
##########
@@ -177,12 +177,20 @@ trait BasicScanExecTransformer extends
LeafTransformSupport with BaseDataSource
doNativeValidation(substraitContext, relNode)
}
+ protected def isRowIndexMetadataColumn(attr: Attribute): Boolean = {
+
BackendsApiManager.getSparkPlanExecApiInstance.isRowIndexMetadataColumn(attr.name)
+ }
+
+ protected def isNativeMetadataColumn(attr: Attribute): Boolean = {
+ attr.isMetadataCol || getMetadataColumns().exists(_.exprId == attr.exprId)
Review Comment:
`isNativeMetadataColumn` calls `getMetadataColumns()` for every attribute
processed by `makeColumnTypeNode`. If `getMetadataColumns()` is non-trivial or
allocates, this can become O(n²) over the output columns. Consider caching
metadata exprIds once (e.g., a `lazy val metadataExprIdSet =
getMetadataColumns().map(_.exprId).toSet`) and using set membership here.
##########
gluten-iceberg/src/main/scala/org/apache/iceberg/spark/source/GlutenIcebergSourceUtil.scala:
##########
@@ -46,12 +46,18 @@ object GlutenIcebergSourceUtil {
private val InputFileBlockLengthCol = "input_file_block_length"
def isSupportedScan(sparkScan: Scan): Boolean = sparkScan match {
- case _: SparkBatchQueryScan | _: SparkStagedScan => true
+ case _: SparkBatchQueryScan | _: SparkStagedScan | _: SparkCopyOnWriteScan
=> true
case _ => false
}
def isSparkStagedScan(sparkScan: Scan): Boolean =
sparkScan.isInstanceOf[SparkStagedScan]
+ def isSparkCopyOnWriteScan(sparkScan: Scan): Boolean =
+ sparkScan.isInstanceOf[SparkCopyOnWriteScan]
+
+ def hasOrcFiles(sparkScan: Scan): Boolean =
+ asFileScanTask(getScanTasks(sparkScan)).exists(_.file().format() ==
FileFormat.ORC)
+
Review Comment:
`hasOrcFiles` eagerly converts all scan tasks to `FileScanTask` (and
`asFileScanTask` does multiple full passes via `forall`), then checks `exists`.
For copy-on-write MERGE/UPDATE/DELETE on large tables, this validation can
become expensive during planning. Consider a single-pass traversal that
short-circuits as soon as an ORC task is found, and avoid materializing a full
`List[FileScanTask]`.
##########
.github/workflows/util/iceberg-upstream/flaky-tests.csv:
##########
@@ -16,5 +16,7 @@
# All invocations of these methods run and are reported, but do not affect the
gate.
# Use exact class and method names; omit method arguments and invocation
indices.
class,method
-# Example
-#
org.apache.iceberg.spark.extensions.TestAlterTablePartitionFields,testDropPartitionAndSourceColumnLong
+# Tests expect BatchScanExec, native uses IcebergScanTransformer soo these
fail.
+org.apache.iceberg.spark.extensions.TestCopyOnWriteDelete,testDeleteWithVectorizedReads
+org.apache.iceberg.spark.extensions.TestCopyOnWriteMerge,testMergeWithVectorizedReads
+org.apache.iceberg.spark.extensions.TestCopyOnWriteUpdate,testUpdateWithVectorizedReads
Review Comment:
The comment indicates these tests fail deterministically due to plan-shape
expectations (not flakiness). Adding them to a 'flaky' non-gating allowlist can
mask real regressions. Prefer either (a) updating the upstream test expectation
to allow `IcebergScanTransformer`/native planning, or (b) tracking these as
known-incompatibilities with a linked issue and an explicit TODO/time horizon
(or separate 'known failures' list) so they don’t silently accumulate.
##########
gluten-iceberg/src/main/scala/org/apache/gluten/execution/IcebergScanTransformer.scala:
##########
@@ -216,13 +208,28 @@ case class IcebergScanTransformer(
!readSchemaFields.contains(name)
}
+ private lazy val copyOnWriteFilePathMetadataColumns = output.filter {
+ attr => IcebergScanTransformer.isCopyOnWriteFilePathColumn(attr.name)
+ }
+
override def getMetadataColumns(): Seq[AttributeReference] = {
- val extraMetadataColumns = inputFileRelatedMetadataColumns.filterNot {
- metadataAttr =>
metadataColumns.exists(_.name.equalsIgnoreCase(metadataAttr.name))
- }
+ val extraMetadataColumns =
+ (inputFileRelatedMetadataColumns ++
copyOnWriteFilePathMetadataColumns).filterNot {
+ metadataAttr =>
metadataColumns.exists(_.name.equalsIgnoreCase(metadataAttr.name))
+ }
metadataColumns ++ extraMetadataColumns
}
+ override protected def isRowIndexMetadataColumn(attr: Attribute): Boolean = {
+ IcebergScanTransformer.isCopyOnWriteRowPositionColumn(attr.name) ||
+ super.isRowIndexMetadataColumn(attr)
+ }
+
+ override protected def isNativeMetadataColumn(attr: Attribute): Boolean = {
+ IcebergScanTransformer.isCopyOnWriteFilePathColumn(attr.name) ||
+ super.isNativeMetadataColumn(attr)
+ }
Review Comment:
This introduces new classification rules that are critical for correctness
of Iceberg copy-on-write MERGE/UPDATE/DELETE (mapping `row_position` to
row-index and `file_path` to native metadata). Please add/extend an automated
test that exercises a copy-on-write `MERGE INTO` plan end-to-end (or at least
validates the planned scan output columns) to ensure these columns are
recognized as intended, and that unsupported formats (e.g., ORC in
copy-on-write) are rejected as expected.
##########
.github/workflows/util/iceberg-upstream/flaky-tests.csv:
##########
@@ -16,5 +16,7 @@
# All invocations of these methods run and are reported, but do not affect the
gate.
# Use exact class and method names; omit method arguments and invocation
indices.
class,method
-# Example
-#
org.apache.iceberg.spark.extensions.TestAlterTablePartitionFields,testDropPartitionAndSourceColumnLong
+# Tests expect BatchScanExec, native uses IcebergScanTransformer soo these
fail.
Review Comment:
Correct the typo 'soo' to 'so' in this comment.
--
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]
