lyne7-sc commented on code in PR #2379:
URL: https://github.com/apache/auron/pull/2379#discussion_r3525127407
##########
thirdparty/auron-iceberg/src/main/scala/org/apache/spark/sql/auron/iceberg/IcebergScanSupport.scala:
##########
@@ -82,35 +85,54 @@ object IcebergScanSupport extends Logging {
}
}
- def plan(exec: BatchScanExec): Option[IcebergScanPlan] = {
- exec.getTagValue(scanPlanTag) match {
+ def plan(exec: BatchScanExec, useRuntimeFilters: Boolean = false):
Option[IcebergScanPlan] = {
+ val tag =
+ if (useRuntimeFilters && exec.runtimeFilters.nonEmpty) {
+ runtimeFilteredScanPlanTag
+ } else {
+ scanPlanTag
+ }
+ exec.getTagValue(tag) match {
case Some(cached) => cached
case None =>
- val planned = planUncached(exec)
- exec.setTagValue(scanPlanTag, planned)
+ val planned = planUncached(exec, useRuntimeFilters)
+ exec.setTagValue(tag, planned)
planned
}
}
- private def planUncached(exec: BatchScanExec): Option[IcebergScanPlan] = {
+ def withRuntimeFilters(
+ exec: BatchScanExec,
+ runtimeFilters: Seq[SparkExpression]): BatchScanExec = {
+ if (exec.runtimeFilters == runtimeFilters) {
Review Comment:
Good catch. You are right that in the current built-in Spark path,
NativeIcebergTableScanExec is initialized from the same BatchScanExec, so
runtimeFilters normally matches basedScan.runtimeFilters and the copy branch is
not exercised by this PR.
I would still prefer to keep this boundary. I do not want to bake in the
assumption that the original basedScan.runtimeFilters is always the final
filter sequence the native scan should use. In the future, if Auron rebuilds
native scans with an updated runtime filter sequence (for example, from a
native physical/AQE-stage rule), the native scan's runtimeFilters may differ
from basedScan.runtimeFilters. In that case, withRuntimeFilters lets us plan
runtime-filtered partitions using the native scan's filters instead of silently
falling back to the original basedScan.runtimeFilters.
I added a short comment to make this intention explicit and adjusted the
doCanonicalize comment so it no longer implies that the copy branch is always
taken.
I do not think we can add a meaningful integration test for the copy branch
in this PR because the built-in Spark path does not currently produce filters
different from basedScan.runtimeFilters. Covering that branch would require an
artificial/native rewrite rule that rebuilds the native scan with a different
filter sequence, which feels broader than this PR?
For the version-specific copy signatures, I verified the Spark 4.1 shims
profile compiles successfully.
--
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]
