[SPARK-52767][SQL] Optimize maxRows and maxRowsPerPartition for join and union

[zml1206]

What changes were proposed in this pull request?

Make the maxRows and maxRowsPerPartition only calculated at most once.

Why are the changes needed?

Improve performance, especially when there are dozens of joins and unions.
Before pr, the number of maxRows executions of join/union increases exponentially with the number of joins/unions.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Local test, 28 tables join before pr 36s, after pr 4s, 29 tables join before pr 67s, after pr 5s

 Seq(1).toDF("a").write.mode("overwrite").parquet("tmp/t1")
spark.read.parquet("tmp/t1").createOrReplaceTempView("t")
val t1 = System.currentTimeMillis()
spark.sql(
"""
|select a,count(1) from (
|select t1.a from (select distinct a from t) t1
|join t t2 on t1.a=t2.a
|join t t3 on t1.a=t3.a
|join t t4 on t1.a=t4.a
|join t t5 on t1.a=t5.a
|join t t6 on t1.a=t6.a
|join t t7 on t1.a=t7.a
|join t t8 on t1.a=t8.a
|join t t9 on t1.a=t9.a
|join t t10 on t1.a=t10.a
|join t t11 on t1.a=t11.a
|join t t12 on t1.a=t12.a
|join t t13 on t1.a=t13.a
|join t t14 on t1.a=t14.a
|join t t15 on t1.a=t15.a
|join t t16 on t1.a=t16.a
|join t t17 on t1.a=t17.a
|join t t18 on t1.a=t18.a
|join t t19 on t1.a=t19.a
|join t t20 on t1.a=t20.a
|join t t21 on t1.a=t21.a
|join t t22 on t1.a=t22.a
|join t t23 on t1.a=t23.a
|join t t24 on t1.a=t24.a
|join t t25 on t1.a=t25.a
|join t t26 on t1.a=t26.a
|join t t27 on t1.a=t27.a
|join t t28 on t1.a=t28.a
|) group by a
|""".stripMargin).show

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

No.

[zml1206]

cc @yaooqinn@ulysses-you Thanks.

[wForget]

nit: keep short-circuit

Suggested change

	valrightMaxRowsOption= right.maxRows
	valrightMaxRowsOption=if (leftMaxRowsOption.isDefined) right.maxRowselseNone

[zml1206]

Good catch! This can avoid unnecessary right maxRows calculations, thanks.

[zml1206]

@cloud-fan Can you help take a look? Thanks.

[JacobZheng0927]

Would it be better to use lazy val instead of def here for potential performance improvement?
For example:
override def maxRows: Option[Long] = { // implementation }
could be changed to:
override lazy val maxRows: Option[Long] = { // implementation }
If the computation is expensive and the result doesn’t change, caching it with lazy val might help avoid recomputation.

[zml1206]

Would it be better to use lazy val instead of def here for potential performance improvement? For example: override def maxRows: Option[Long] = { // implementation } could be changed to: override lazy val maxRows: Option[Long] = { // implementation } If the computation is expensive and the result doesn’t change, caching it with lazy val might help avoid recomputation.

No, it may change. For example, it was None before AQE, but it has a value during AQE.

[JacobZheng0927]

Would it be better to use lazy val instead of def here for potential performance improvement? For example: override def maxRows: Option[Long] = { // implementation } could be changed to: override lazy val maxRows: Option[Long] = { // implementation } If the computation is expensive and the result doesn’t change, caching it with lazy val might help avoid recomputation.

No, it may change. For example, it was None before AQE, but it has a value during AQE.

child is a parameter of a case class, so it’s immutable — it won’t change after construction.
If child does need to change, the case class itself would be reconstructed.

[zml1206]

Would it be better to use lazy val instead of def here for potential performance improvement? For example: override def maxRows: Option[Long] = { // implementation } could be changed to: override lazy val maxRows: Option[Long] = { // implementation } If the computation is expensive and the result doesn’t change, caching it with lazy val might help avoid recomputation.

No, it may change. For example, it was None before AQE, but it has a value during AQE.

child is a parameter of a case class, so it’s immutable — it won’t change after construction. If child does need to change, the case class itself would be reconstructed.

This seems to make sense, thanks. What do you think? @cloud-fan

[github-actions]

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

[cloud-fan]

using lazy val makes sense to me

[zml1206]

Updated, please check again, thank you @cloud-fan

[yaooqinn]

+1, LTGM

[cloud-fan]

thanks, merging to master/4.1 (it's a long standing PR and it fixes a perf bug)

[dongjoon-hyun]

thanks, merging to master/4.1 (it's a long standing PR and it fixes a perf bug)

As a release manager, +1 for @cloud-fan 's backporting decision.