infvg commented on code in PR #12833:
URL: https://github.com/apache/gluten/pull/12833#discussion_r3838592093
##########
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:
This test only verifies that getRootPathsInternal is non-empty, but the test
warehouse uses file:// already:
https://github.com/apache/gluten/blob/ee68b268504d405ad26275392f35ecd1977047c8/gluten-iceberg/src/test/scala/org/apache/gluten/execution/IcebergSuite.scala#L45
Velox explicitly filters file:// paths out of filesystem scheme validation:
https://github.com/apache/gluten/blob/ee68b268504d405ad26275392f35ecd1977047c8/backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxBackend.scala#L267-L276
So this does not test any regression. Can we change the test to a case with
an unsupported scheme and verify that the Iceberg scan falls back instead?
##########
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:
Could we derive the paths from the Iceberg scan instead of
`BatchScanExec.table`? `GlutenIcebergSourceUtil` already accesses
`SparkBatchQueryScan.table()`, so this also works on Spark3.3 and we wouldn't
need any version limitations:
https://github.com/apache/gluten/blob/ee68b268504d405ad26275392f35ecd1977047c8/gluten-iceberg/src/main/scala/org/apache/iceberg/spark/source/GlutenIcebergSourceUtil.scala#L115-L120
Also, `table.location()` is not necessarily the filesystem containing the
files being read. Iceberg supports a separate `write.data.path` and
`write.location-provider.impl`:
https://github.com/apache/iceberg/blob/apache-iceberg-1.10.0/docs/docs/configuration.md#write-properties
Gluten already gets the actual data path from `task.file().path()` when
building the native scan:
https://github.com/apache/gluten/blob/ee68b268504d405ad26275392f35ecd1977047c8/gluten-iceberg/src/main/scala/org/apache/iceberg/spark/source/GlutenIcebergSourceUtil.scala#L72-L85
Could we add a helper in `GlutenIcebergSourceUtil` that extracts the actual
scan file path and use that instead?
--
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]
