HwangDongJun commented on code in PR #12833:
URL: https://github.com/apache/gluten/pull/12833#discussion_r3839949607
##########
gluten-iceberg/src/test/scala/org/apache/gluten/execution/IcebergSuite.scala:
##########
@@ -60,6 +60,30 @@ abstract class IcebergSuite extends
WholeStageTransformerSuite {
}
}
+ // SparkShims.getBatchScanExecTable returns null on Spark 3.3 (BatchScanExec
has no `table`
+ // field until Spark 3.4), so IcebergScanTransformer.table is always null
there and this
+ // improvement does not take effect; it does on Spark 3.4+.
+ testWithMinSparkVersion("iceberg getRootPathsInternal returns table
location", "3.4") {
+ // See https://github.com/apache/gluten/issues/12712: getRootPathsInternal
used to always
+ // return Seq.empty for Iceberg scans, silently skipping native filesystem
scheme validation.
+ withTable("iceberg_root_paths_tb") {
+ spark.sql("""
+ |CREATE TABLE iceberg_root_paths_tb (id INT)
+ |USING iceberg
+ |""".stripMargin)
+ spark.sql("INSERT INTO iceberg_root_paths_tb VALUES (1), (2)")
+
+ runQueryAndCompare("SELECT * FROM iceberg_root_paths_tb") {
+ df =>
+ val scans = getExecutedPlan(df).collect { case i:
IcebergScanTransformer => i }
+ assert(scans.size == 1)
+ val rootPaths = scans.head.getRootPathsInternal
+ assert(rootPaths.nonEmpty, "getRootPathsInternal should not be empty
for Iceberg tables")
+ assert(rootPaths.forall(_.nonEmpty))
+ }
+ }
+ }
+
Review Comment:
Thanks for pointing that out — you're right that `file://` gets excluded
from `distinctRootPaths`, so that test wasn't really proving anything. I wasn't
able to get a real table working on an actually-unsupported scheme (Iceberg's
FileIO fails the write itself in that case), so I took a different approach
instead.
The shared test now checks that the returned path is a real per-file path
(ends in `.parquet`, under `.../data/...`), and I added a Velox-side test that
feeds a fake unsupported scheme directly into
`VeloxFileSystemValidationJniWrapper.allSupportedByRegisteredFileSystems` (the
same function `validateScanExec` relies on) and checks that it's correctly
rejected.
Happy to adjust further if you think there's a better way to test this.
##########
gluten-iceberg/src/main/scala/org/apache/gluten/execution/IcebergScanTransformer.scala:
##########
@@ -192,8 +192,15 @@ case class IcebergScanTransformer(
override def getDataSchema: StructType = new StructType()
- // TODO: get root paths from table.
- override def getRootPathsInternal: Seq[String] = Seq.empty
+ // On Spark 3.3, SparkShims.getBatchScanExecTable always returns null
(BatchScanExec has no
+ // `table` field until Spark 3.4), so this falls back to the previous
Seq.empty behavior there;
+ // on Spark 3.4+ it returns the Iceberg table's base location.
+ override def getRootPathsInternal: Seq[String] = {
+ table match {
+ case t: SparkTable => Seq(t.table().location())
+ case _ => Seq.empty
+ }
+ }
Review Comment:
Thank you, that's a really good catch — I hadn't considered
`write.data.path` at all. I've updated the fix to derive the path from the
actual scan tasks instead
(`GlutenIcebergSourceUtil.getRootPaths(finalPartitions)`, one
`task.file().path()` per partition, following the same pattern already used in
`genSplitInfo`/`deleteExists`). As a bonus, this also resolves the Spark 3.3
gap, since it no longer relies on `BatchScanExec.table`.
One thing I'd like to flag for visibility: this means `finalPartitions` now
gets computed during validation for every scan, rather than only for format-v3
tables as before. It's the same cached value, just triggered more often. Also,
`asFileScanTask` can throw for an unexpected task type, but that's already
caught upstream and converted into a fallback rather than a crash, so it should
be safe.
Please let me know if you'd like me to handle this differently.
--
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]
