Skip to content

[SPARK-59024][SQL] Support sequential cached name for anonymous cached tables - #58314

Open
pan3793 wants to merge 3 commits into
apache:masterfrom
pan3793:SPARK-59024
Open

[SPARK-59024][SQL] Support sequential cached name for anonymous cached tables#58314
pan3793 wants to merge 3 commits into
apache:masterfrom
pan3793:SPARK-59024

Conversation

@pan3793

@pan3793 pan3793 commented Aug 26, 2026

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

Add a new SQL config spark.sql.useSequentialCacheName (default false). When it is true and the cached table has no name, CachedRDDBuilder uses a sequential number like CachedRDD 1 as the cached name instead of the abbreviated plan tree string:

val cachedName: String = tableName.map(n => s"In-memory table $n").getOrElse {
  if (cachedPlan.session.conf.get(SQLConf.USE_SEQUENTIAL_CACHE_NAME)) {
    s"CachedRDD ${CachedRDDBuilder.nextCachedRDDId()}"
  } else {
    Utils.abbreviate(cachedPlan.toString, 1024)
  }
}

Why are the changes needed?

For anonymous cached tables, the cached name is built from the plan's tree string (cachedPlan.toString, abbreviated to 1024 chars). Rendering the plan tree string can be expensive for large plans, so caching unnamed large DataFrames pays this cost even though the name is only used for display.

This is another spot, besides the SQL event plan description addressed in SPARK-59023, that hurts the same customer job: it constructs a huge plan whose treeString exceeds 280,000 lines, and rendering the plan tree string takes minutes per iteration and contributes to driver OOM.

Does this PR introduce any user-facing change?

Yes. A new config spark.sql.useSequentialCacheName is available. When it is enabled, anonymous cached tables get sequential names like CachedRDD 1 instead of the abbreviated plan tree string. The default behavior is unchanged.

How was this patch tested?

A new unit test in InMemoryRelationSuite (sequential cached name for anonymous cached tables) verifies:

  • anonymous cached tables get distinct CachedRDD <n> names when the config is enabled
  • named tables keep the usual In-memory table <name> name
  • the abbreviated plan tree string is kept when the config is disabled

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

Generated-by: Qwen3.8 Max

…d tables

Add spark.sql.useSequentialCacheName. When it is true and the cached table has no name, CachedRDDBuilder uses a sequential number like 'CachedRDD 1' as the cached name instead of the abbreviated plan tree string. Rendering the plan tree string can be expensive for large plans.

Assisted-by: Qwen3.8 Max
Remove .internal() from spark.sql.useSequentialCacheName, and pin the disabled case in the test with an explicit conf value instead of relying on the default.
@dongjoon-hyun

Copy link
Copy Markdown
Member

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#buildBufferscached.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:

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

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():

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

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):

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:
    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.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants