dongjoon-hyun commented on PR #58314:
URL: https://github.com/apache/spark/pull/58314#issuecomment-5449673477
Thanks for the quick turnaround — all six items from the previous round are
addressed, and CI is fully green on `fcf6236`. `NOT_APPLICABLE` is the right
binding policy here too: the test is whether the config changes what a view/UDF
body *resolves to*, and this one only affects a display name.
Two follow-ups, one of which is a correction of something I said last round.
### 1. Correction: the `lazy val` does change the default-path name
I said making `cachedName` lazy has "no behavior change". That is not quite
right, and it is worth a line in the PR description.
`cached.setName(cachedName)` runs *after* the plan has been executed:
```scala
val cb = try {
...
serializer.convertInternalRowToCachedBatch(buildInputRDD(cachedPlan.execute()),
...)
...
}
val cached = cb.mapPartitionsWithIndexInternal { ... }.persist(storageLevel)
cached.setName(cachedName) // <- forced here, post-execution
```
For an AQE plan, `AdaptiveSparkPlanExec.generateTreeString` renders the
initial plan while `isFinalPlan=false`, and switches to a `Final Plan` section
once execution has completed. So with the config off (the default), the
Storage-tab name of an anonymous cache is now derived from the **final** AQE
plan rather than the pre-execution plan it used to show.
Harmless in itself — it is a display string, and arguably the more useful
one — but it does mean "Does this PR introduce any user-facing change? No" is
slightly overstated. Suggest noting that the anonymous cached name is now
rendered at materialization time.
### 2. Same for the config read
Because the conf is read inside the lazy val, it is evaluated at first
materialization, not at `cache()` time:
```scala
df.cache() // config = false
spark.conf.set("spark.sql.dataframeCache.sequentialName.enabled", "true")
df.count() // -> "CachedRDD 7"
```
That seems fine (and is the only sensible option given the laziness), but a
short sentence in the conf `doc` or the comment would save the next reader the
trip.
### 3. Optional: `SparkPlan.id` already gives you a sequential id
`SparkPlan` carries a globally unique, monotonically increasing id:
```scala
// SparkPlan.scala
private val nextPlanId = new AtomicInteger(0)
private[execution] def newPlanId(): Int = nextPlanId.getAndIncrement()
...
val id: Int = SparkPlan.newPlanId()
```
So the sequential branch could be just:
```scala
s"CachedRDD ${cachedPlan.id}"
```
which drops the `CachedRDDBuilder` companion object, the extra `AtomicLong`,
and the "a copy of the builder would draw a new id" caveat entirely — the id
then belongs to the plan, not to the forcing of a lazy val.
The trade-off is that the numbers are no longer dense (they interleave with
every other physical operator's id), so `CachedRDD 0`, `CachedRDD 41`,
`CachedRDD 97` instead of `0, 1, 2`. If the dense numbering is what you want
for readability, the current form is fine — your call.
### 4. Optional: the test does not cover the actual win
The new test asserts the *value* of `cachedName`, but the reason for this PR
is that the tree string is never rendered for a cache that is never
materialized. That is cheap to pin down with a wrapper plan that records
whether `toString` was called:
```scala
val plan = new PlanWithCountedToString(child) // increments a counter in
toString
val r = InMemoryRelation(StorageLevel.MEMORY_ONLY, ..., None)
assert(plan.toStringCount == 0) // not rendered at
construction
```
Otherwise a future change that re-eagerizes the name would pass the suite.
Nothing blocking. With (1) reflected in the description this LGTM.
--
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]
