HwangDongJun commented on code in PR #12833:
URL: https://github.com/apache/gluten/pull/12833#discussion_r3986350918
##########
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:
Following up on this — I initially implemented exactly what you suggested
(task.file().path() via finalPartitions), and it does give more accurate paths.
However, while stress-testing it in CI before pushing the next round, I found
it introduces a real regression: getRootPathsInternal is invoked
unconditionally from doValidateInternal(), so forcing finalPartitions to plan
eagerly there causes Iceberg's scan to plan its input partitions before Spark
pushes down its dynamic partition pruning runtime filter. That breaks Spark's
DPP subquery-reuse detection for SupportsRuntimeV2Filtering scans —
org.apache.gluten.source.TestGlutenRuntimeFiltering (which passes 18/18 on
main) failed with 16/18 errors (IllegalStateException: Can't translate ... to
v2 Predicate, no subquery result) on spark-3.4 with that approach.
Since correctness here fundamentally requires planning the scan (which can't
safely happen before runtime filters are pushed), I've moved to a metadata-only
approach instead: Table.location() plus the write.data.path /
write.folder-storage.path properties when set. This directly addresses the
write.data.path case you flagged, and mirrors how the non-Iceberg path already
works (BatchScanExecTransformer.getRootPathsInternal reads FileIndex.rootPaths
metadata rather than planning partitions).
This doesn't fully solve the write.location-provider.impl case (a fully
custom provider that ignores both properties) — I don't see a way to get that
without planning tasks, which reintroduces the DPP issue. I think that's an
acceptable, documented limitation for now (still a strict improvement over the
current Seq.empty), but happy to discuss further if you see a better path.
Added a new test (iceberg getRootPathsInternal reflects a custom
write.data.path) covering the write.data.path property case directly, plus a
test cross-checking the returned root path is a real prefix of
input_file_name()'s actual output.
--
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]
