dongjoon-hyun commented on PR #58314:
URL: https://github.com/apache/spark/pull/58314#issuecomment-5429556991
Thanks for working on this — the underlying problem is real, and defaulting
the config to `false` keeps this safe. A few comments.
### 1. Part of this can be fixed without a config
`cachedName` has only two consumers:
- `InMemoryTableScanExec#nodeName` — but only in the `case Some(_) =>`
branch, i.e. **named** tables only.
- `CachedRDDBuilder#buildBuffers` → `cached.setName(cachedName)`.
So for anonymous caches, `cachedName` feeds exactly one thing: the RDD
display name in the Storage tab. Yet it is a `val` in the case class body, so
`Utils.abbreviate(cachedPlan.toString, 1024)` is evaluated at **every**
`CachedRDDBuilder` construction — including caches that are never materialized
(e.g. `df.cache()` that is never triggered, or a plan AQE ends up not using).
Making it `lazy val` removes that cost unconditionally, with no config and
no behavior change:
```scala
lazy val cachedName: String = tableName.map(n => s"In-memory table
$n").getOrElse { ... }
```
One thing to confirm if you take this: `cachedPlan` is `@transient`, so a
`lazy val` forced after deserialization would NPE. Both consumers above look
driver-side (`setName`, and `override val nodeName` which is itself eager), so
it seems safe — but worth stating explicitly.
This would also narrow what the new config has to justify, down to "large
anonymous caches that *are* materialized".
### 2. Off-by-one between the doc and the behavior
```scala
private val _nextCachedRDDId = new AtomicLong(0)
def nextCachedRDDId(): Long = _nextCachedRDDId.getAndIncrement
```
`AtomicLong(0)` + `getAndIncrement` means the first name is `CachedRDD 0`,
but both the config `doc` and the PR description say `'CachedRDD 1'`. Either
use `incrementAndGet` or fix the doc.
Minor: the closest precedent in this area is `SparkPlan.newPlanId()`:
```scala
private val nextPlanId = new AtomicInteger(0)
private[execution] def newPlanId(): Int = nextPlanId.getAndIncrement()
```
`private val nextId` reads better here than the underscore-prefixed name.
### 3. The config should be `.internal()`, and the name is off-convention
Since the only observable effect for anonymous caches is an RDD display
name, this is a debugging/tuning knob — `.internal()` seems right. As written
it will show up in the public SQL config docs table.
The name also doesn't match its neighbors in `SQLConf`, which is where it is
(correctly) placed:
- `spark.sql.defaultCacheStorageLevel`
- `spark.sql.dataframeCache.logLevel` ← directly above the new entry
- `spark.sql.useSequentialCacheName` ← new
Something like `spark.sql.dataframeCache.sequentialName.enabled` would be
more consistent, and follows the usual `.enabled` suffix for boolean confs.
### 4. Prefer `cachedPlan.conf` over `cachedPlan.session.conf`
```scala
if (cachedPlan.session.conf.get(SQLConf.USE_SEQUENTIAL_CACHE_NAME)) {
```
`SparkPlan.conf` already resolves to `session.sessionState.conf` when a
session is active and falls back to `SQLConf.get` otherwise, and it is what
this very file uses a few lines down (`cachedPlan.conf.clone()` in
`buildBuffers`):
```scala
if (cachedPlan.conf.getConf(SQLConf.USE_SEQUENTIAL_CACHE_NAME)) {
```
(`cachedPlan.session` is `getActiveSession.orNull`, so the current form NPEs
on a null session. `newPartitionStats()` already assumes non-null, so this
isn't a new risk — but no reason to add another one.)
### 5. Side-effecting `val` in a case class body
`CachedRDDBuilder` is a case class, and `cachedName` now increments a global
counter as a side effect of construction. Any future `copy(...)` would silently
change the name and burn an id. There are no `copy` call sites on the builder
today (`InMemoryRelation.copy()` shares the builder reference), so this is
latent — but a short comment would help.
For what it's worth, the equality side is fine: `cachedName` is a body
`val`, not a constructor param, so `equals`/`hashCode`/canonicalization are
unaffected and `sameResult` / plan reuse can't be perturbed by this.
### 6. Test
- The other two tests in `InMemoryRelationSuite` are prefixed
(`SPARK-46779:`, `SPARK-47177:`); please add `SPARK-59024:` for consistency.
- The disabled-config assertion is weak:
```scala
assert(!r4.cacheBuilder.cachedName.startsWith("CachedRDD "))
```
This only checks it isn't the new format, not that it *is* the abbreviated
plan tree string. Asserting equality against `Utils.abbreviate(...)` (or at
least that it starts with the plan's first line) would actually protect the
fallback path.
### Nits confirmed OK
- `.version("4.4.0")` matches `branch-4.x`, which is right for a
normally-backported change.
- Placement inside the cache-related config cluster in `SQLConf` is good.
- Default `false` means no golden-file or explain-output impact.
--
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]
