Skip to content

Conversation

@liuzqt
Copy link
Contributor

@liuzqt liuzqt commented Dec 2, 2025

What changes were proposed in this pull request?

Safe type casting in QueryPlan._subqueries

Why simpling removing e.plan.asInstanceOf[PlanType] is not enough

looking at the foreachWithSubqueries API below

  def foreachWithSubqueries(f: PlanType => Unit): Unit = {
    def actualFunc(plan: PlanType): Unit = {
      f(plan)
      plan.subqueries.foreach(_.foreachWithSubqueries(f))
    }
    foreach(actualFunc)
  }

PlanExpression’s type parameter is erased at runtime. When we matched on

case planExpression: PlanExpression[PlanType @unchecked] =>
  planExpression.plan

the JVM only checked that the object was a PlanExpression; it did not verify that its plan was really a PlanType (e.g. SparkPlan). Because of @unchecked, the compiler suppressed the exhaustivity/type warning and happily treated the result as a PlanType. Later, when foreachWithSubqueries invoked the lambda f: SparkPlan => Unit, the JVM inserted a cast to SparkPlan, and we will still end up ClassCastException if the actual object is a logical plan.

Why are the changes needed?

QueryPlan._subqueries is dangerous because it force type casting (code pointer)

Imagine a SparkPlan instance invoke this API where some of its subqueries could be LogicalPlan(this could happen in AQE where logical->phyiscal planning happen respectively in main/sub queries and they could be out-of-sync at a specific point.

Reasoning why we dont' need a new API
Although this API is at critical path of the whole Spark SQL, there is no need to create a separate API since if we run into this class cast error the whole query will just fail so it's always better to fix the issue and no need to preserve the "failure" buggy behavior.

Does this PR introduce any user-facing change?

NO

How was this patch tested?

Existing UTs.

Was this patch authored or co-authored using generative AI tooling?

NO

@github-actions github-actions bot added the SQL label Dec 2, 2025
private val _subqueries = new TransientBestEffortLazyVal(() =>
expressions.filter(_.containsPattern(PLAN_EXPRESSION)).flatMap(_.collect {
case e: PlanExpression[_] => e.plan.asInstanceOf[PlanType]
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I understand, the earlier logic did not return None in case there were any subqueries regardless of the plantype. Now we do have a chance of empty sequence to be returned right? If so, should we not have UTs for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants