Repository navigation
[SPARK-44462][SS][CONNECT] Fix the session passed to foreachBatch in Spark Connect - #55410
LuciferYang wants to merge 19 commits into
Conversation
Add a `dataFrameQueryIndex` map to `SessionHolder` that tracks the active cached DataFrame ID per streaming query. Before caching a new batch DataFrame in `dataFrameCachingWrapper`, we check for any stale entry from a previous batch and remove it, logging a warning. This addresses TODO 1 in StreamingForeachBatchHelper. Changes: - `SessionHolder`: Add `dataFrameQueryIndex` ConcurrentMap - `StreamingForeachBatchHelper`: Accept `queryIdRef` in `dataFrameCachingWrapper`, perform sanity check, clean up mapping in finally block. Update `scalaForeachBatchWrapper` and `pythonForeachBatchWrapper` return types to include `AtomicReference[String]`. - `SparkConnectPlanner`: Destructure new return types, set query id on the AtomicReference after query starts. - `SparkConnectSessionHolderSuite`: Update test to destructure 3-element tuple from `pythonForeachBatchWrapper`.
…nHolder for foreachBatch When a streaming query starts, `StreamExecution` clones the SparkSession. The DataFrame passed to foreachBatch has this cloned session, but the `SessionHolder` still references the original session. This causes: 1. Session mismatch: batch DataFrames operate on a different session 2. Session lifetime leak: Python worker keeps original session alive 3. CachedRemoteRelation resolves against the wrong session This commit fixes all three issues by: - Adding `registerExistingSession` to `SparkConnectSessionManager` to wrap an existing SparkSession (the stream clone) under a new session ID without creating another clone. - Adding `ForeachBatchSessionManager` that lazily creates a stream-level `SessionHolder` on the first batch invocation and reuses it for subsequent batches. - Adding `ForeachBatchCleaner` that closes both the Python runner and the stream `SessionHolder` on query termination. - Sending the stream session ID per-batch to the Python worker so it resolves `CachedRemoteRelation` against the correct session. - Updating `foreach_batch_worker.py` to read the stream session ID and create/switch the SparkSession accordingly. The stream `SessionHolder` uses `customInactiveTimeoutMs = -1` (never expire by inactivity); its lifecycle is managed by query termination via `CleanerCache`. Addresses TODO 2 and TODO 3 in StreamingForeachBatchHelper, removing all SPARK-44462 TODO comments.
…chHelper Run scalafmt to fix formatting violations detected by CI linter.
| .asInstanceOf[org.apache.spark.sql.classic.SparkSession] | ||
| val streamSessionId = UUID.randomUUID().toString | ||
| _streamSessionHolder = SparkConnectService.sessionManager | ||
| .registerExistingSession(parentSessionHolder.userId, streamSessionId, streamSession) |
There was a problem hiding this comment.
The registerExistingSession call uses parentSessionHolder.userId and a new random UUID, but nothing ties this stream session back to the parent session's artifacts or classloader. If the user's foreachBatch function relies on session-level artifacts (added jars, UDFs, etc.) from the parent session, would those be missing in the stream SessionHolder? The underlying SparkSession is the cloned one from StreamExecution, so Spark-level config should carry over, but Connect-level SessionHolder state (artifact manager, plan cache, etc.) would be fresh/empty.
There was a problem hiding this comment.
Checked this. StreamExecution clones via sparkSession.cloneSession() (StreamExecution.scala:164), and SparkSession.cloneSession() force-copies ArtifactManager and its resources (SparkSession.scala:295). Our cloned SessionHolder wraps that cloned classic SparkSession, and SessionHolder.artifactManager just delegates to session.artifactManager, so jars and the UDF classloader added to the root session before the stream starts are inherited. Artifacts added after the stream is running would not auto-sync, but that mirrors classic mode.
| # The per-batch stream session id will be received with each batch and used to create | ||
| # or switch to the correct stream-level session. | ||
| connect_url_init = connect_url + ";session_id=" + session_id | ||
| spark_connect_session = SparkSession.builder.remote(connect_url_init).getOrCreate() |
There was a problem hiding this comment.
The initial spark_connect_session is created but is never assigned to spark and never used afterward, t only serves as a validation? Is this intended to
remain open for the lifetime of the worker?
There was a problem hiding this comment.
I guess we need a session to read the FEB func, but cloned session is not yet available so we have to use the original session first
There was a problem hiding this comment.
+1 to @bogao007. We need a session to unpickle the foreachBatch function before the first batch arrives, at which point the cloned session id is not yet known. Once batches start flowing, spark is reassigned to the cloned session via .create(), so the initial one is bootstrap-only.
| log_name = "Streaming ForeachBatch worker" | ||
|
|
||
| def process(df_id, batch_id): # type: ignore[no-untyped-def] | ||
| def process(df_id, batch_id, stream_session_id): # type: ignore[no-untyped-def] |
There was a problem hiding this comment.
Can we use clone_session_id to indicate that this is a cloned session? To differentiate, maybe we could name the previous session to be root_session_id.
There was a problem hiding this comment.
Renamed to cloned_session_id / root_session_id (matches the Scala side: clonedSession* / rootSessionHolder).
| # The per-batch stream session id will be received with each batch and used to create | ||
| # or switch to the correct stream-level session. | ||
| connect_url_init = connect_url + ";session_id=" + session_id | ||
| spark_connect_session = SparkSession.builder.remote(connect_url_init).getOrCreate() |
There was a problem hiding this comment.
I guess we need a session to read the FEB func, but cloned session is not yet available so we have to use the original session first
| def curried_function(df): | ||
| def inner(batch_df, batch_id): | ||
| df.createOrReplaceTempView("updates") | ||
| batch_df.createOrReplaceTempView("batch_updates") |
There was a problem hiding this comment.
Can we still keep the createOrReplaceTempView() to verify that root session cannot access the temp view created by the cloned session? I think this is an important test case and we should make sure the behavior aligns with the classic ForeachBatch
There was a problem hiding this comment.
Added test_temp_view_is_isolated_from_root_session. It creates a temp view inside FEB, writes a sentinel table so the test can prove collect_batch actually ran, then asserts the root session hits TABLE_OR_VIEW_NOT_FOUND when reading that view. Kept test_nested_dataframes on saveAsTable — that one is about cross-session persistent tables.
| log"[session: ${MDC(SESSION_ID, effectiveHolder.sessionId)}] " + | ||
| log"Caching DataFrame with id ${MDC(DATAFRAME_ID, dfId)}") | ||
|
|
||
| // Sanity check: remove any stale DataFrame left over from a previous batch for this query. |
There was a problem hiding this comment.
Why do we need this check? We are already removing the DF from the cache after the batch completes
There was a problem hiding this comment.
It is defensive. Happy path is handled by the finally, but if a previous batch skipped its cleanup (async interruption / unchecked error path), a stale dfId could still be sitting in dataFrameQueryIndex. This also lines up with the original TODO(SPARK-44462) that the sanity-check commit replaced. I tightened the comment to spell this out. Fine to drop it if you prefer; it is strictly belt-and-suspenders.
There was a problem hiding this comment.
I see, yeah I'm okay with it since it's fixing an existing TODO.
| // Sanity check: remove any stale DataFrame left over from a previous batch for this query. | ||
| val queryId = queryIdRef.get() | ||
|
|
||
| effectiveHolder.cacheDataFrameById(dfId, df) |
There was a problem hiding this comment.
For a case where a table created outside FEB func() but used inside FEB() to perform a Spark operation, it would probably fail because it's created by the root session and would use root session id to find the cached dataframe. How do we handle that case?
There was a problem hiding this comment.
Depends on what kind of table:
- persistent / managed tables (Hive metastore etc.): fine, visible across sessions, no
CachedRemoteRelationlookup involved. - temp views from the root session: not visible in the cloned session — intentional isolation, same as classic.
- DataFrames cached via
df.persist()in the root session and captured in the closure: this one would fail because theCachedRemoteRelationid is only registered in the rootSessionHolder, not the cloned one. That is a real gap but outside the scope of this PR; I will file a follow-up JIRA. WDYT?
There was a problem hiding this comment.
Sure, I'm okay with a follow up Jira, thanks!
There was a problem hiding this comment.
AI-review / LLM disclosure: This comment was drafted by an LLM on behalf of the reviewer. Please verify the analysis independently.
[Blocker] Following up with a confirmed reproducer: using root.sql("SELECT value FROM {batch}", batch=batch_df) in foreachBatch fails on this PR head with CONNECT_INVALID_PLAN.DATAFRAME_NOT_FOUND. The worker reports the batch relation ID under the cloned session; the original session then cannot resolve that same ID. A plain uncached DataFrame captured before the query also fails when joined to batch_df, with SESSION_NOT_SAME.
Both tests first verify that the batch and captured objects work independently. See test-only PR #36 and its CI: HeartSaVioR#36 and https://github.com/HeartSaVioR/spark/actions/runs/36569505476/job/109417033218.
This is broader than a persisted-DataFrame corner case. Could we resolve or explicitly define the compatibility behavior in this PR before enabling it by default?
| extends Logging { | ||
| @volatile private var _streamSessionHolder: SessionHolder = null | ||
|
|
||
| def getOrCreateStreamSessionHolder(batchDf: DataFrame): SessionHolder = { |
There was a problem hiding this comment.
Same as above: can we use clonedSession as naming to indicate that this is a cloned session?
There was a problem hiding this comment.
Done — renamed streamSession* → clonedSession* and the ctor param parentSessionHolder → rootSessionHolder so Scala and Python stay symmetric.
- Rename streamSession/parentSessionHolder -> clonedSession/rootSessionHolder across Scala and Python for consistency with StreamExecution's semantics. - Tighten close() to clear the reference before calling closeSession so a failing close cannot leave a half-closed holder; add success/failure logs with MDC. - Eagerly close the cleaner in registerCleanerForQuery when the query is already inactive, so the cloned SessionHolder (which never expires by inactivity) cannot leak if the query terminates before cleaner registration. - Add a parity test test_temp_view_is_isolated_from_root_session that writes a sentinel table from inside foreachBatch to prove the batch ran, then asserts the root session hits TABLE_OR_VIEW_NOT_FOUND for a temp view created only in the cloned session.
- Stub `query.isActive` in StreamingForeachBatchHelperSuite's mockQuery so the eager-cleanup branch added in the previous commit doesn't immediately purge the registered cleaners. - Rewrap the ForeachBatchSessionManager docstring so no line exceeds scalafmt's maxColumn=98.
| * The returned SessionHolder has `customInactiveTimeoutMs = Some(-1)` (never expire by | ||
| * inactivity); its lifecycle is managed by the streaming query termination. | ||
| */ | ||
| private[connect] def registerExistingSession( |
There was a problem hiding this comment.
@hvanhovell could you help review the changes here? Thanks!
bogao007
left a comment
There was a problem hiding this comment.
LGTM, thanks for making the fix! I've asked @hvanhovell to help review changes as well.
|
Thank you @bogao007 ~ |
|
Any tangible resolution or path forward for this critical issue? |
Resolved: CleanerCache.registerCleanerForQuery keeps both post-insert cleanup guards (master's isClosing re-check, this branch's !query.isActive); SparkConnectSessionHolderSuite keeps master's listener capture with the ForeachBatchCleaner rename; the foreachBatch worker and parity test follow master's import layout (uuid added, unused unittest dropped).
- Make ForeachBatchSessionManager.close() terminal: a batch arriving after cleanup can no longer register another SessionHolder that never expires by inactivity, and registration is refused while the root session is closing. - Read the cloned holder under the lock so a concurrent close() cannot make getOrCreateClonedSessionHolder return null. - Close the internal cloned session with allowReconnect = true so it does not take a closedSessionsCache slot from a real client session. - Point the worker's default/active session at the cloned session, so SparkSession.active() inside the user function is the session the batch runs on, and drop the cloned-session-id assertion that can never fire. - Tests: terminal close and a closing root session, a cleaner registered for an already terminated query, and an active-session assertion in the parity test.
|
The failing #58446 guards that test on |
… in the Spark Connect compatibility test ### What changes were proposed in this pull request? Skip `StreamingForeachBatchParityTests.test_nested_dataframes` when `SPARK_SKIP_CONNECT_COMPAT_TESTS` is set. ### Why are the changes needed? The Spark Connect compatibility jobs run this branch's `pyspark-connect` tests against a server built from `master`. The `pyspark-connect-old-client` job in `build_and_test.yml` and `build_python_connect40.yml` both clone `branch-4.0` for the client side. #55410 (SPARK-44462) changes the server so the DataFrame handed to `foreachBatch` is bound to the session `StreamExecution` clones for the query, which is what classic `foreachBatch` already does. `test_nested_dataframes` creates a temp view from the batch DataFrame and reads it back through the client session, so against that server it fails: ``` File ".../connect/streaming/test_parity_foreach_batch.py", line 49, in test_nested_dataframes collected = self.spark.sql("select * from batch_updates").collect() pyspark.errors.exceptions.connect.AnalysisException: [TABLE_OR_VIEW_NOT_FOUND] The table or view `batch_updates` cannot be found. ``` Failing run: https://github.com/LuciferYang/spark/actions/runs/33346837991/job/99354964415 The assertion holds for a 4.0 server, so the guard is scoped to the compatibility jobs and this branch's own CI keeps running the test. SPARK-52077 skipped `ArrowUDTFParityTests` the same way. ### Does this PR introduce _any_ user-facing change? No, test-only. ### How was this patch tested? `test_nested_dataframes` still runs with `SPARK_SKIP_CONNECT_COMPAT_TESTS` unset and is skipped with it set to `1`. This branch's CI cannot exercise the case the guard is for, since it runs against a 4.0 server; the `pyspark-connect-old-client` job on #55410 is what verifies it, once this lands. Pass GitHub Actions. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code Closes #58446 from LuciferYang/skip-nested-dataframes-compat-40. Authored-by: YangJie <yangjie01@baidu.com> Signed-off-by: yangjie01 <yangjie01@baidu.com>
|
What changes is Python def process(batch_df, batch_id):
batch_df.createOrReplaceTempView("v")
...
spark.sql("SELECT * FROM v") # before: resolves. after: TABLE_OR_VIEW_NOT_FOUNDScala and classic are unaffected; both already hand the user function the clone. So Connect is catching up here, but working code does break. Not hypothetically: master and So: a conf selecting the session, new behavior on master and |
|
This is a critical fix, thanks for driving the fix! Would you mind giving me some time to review this? I know Bo reviewed it, but would love to give myself a review. |
OK, looking forward to your review. Thank you @HeartSaVioR |
|
Can we merge it to 4.2.1 as well to keep in sync with 4.3 : https://lists.apache.org/thread/bl3zbvbmrhz2m6jz25ogfwfd6v3wjo4q |
There was a problem hiding this comment.
AI-review / LLM disclosure: This comment was drafted by an LLM on behalf of the reviewer. Please verify the analysis independently.
I reviewed the Connect foreachBatch session change at cf3458c107dc. It fixes the stateful batch case by running batch-derived work in the stream's cloned session. The following issues remain before this can be enabled by default.
Blocker: ordinary mixed-session operations fail
A callback can capture the original SparkSession and use the batch DataFrame in SQL:
root = spark
def process(batch_df, _):
root.sql("SELECT value FROM {batch}", batch=batch_df).collect()root remains bound to the original session after callback deserialization. SQL interpolation includes batch_df's cached relation, but root.sql submits it through the original session. This patch registers that relation only in the cloned holder, so the original holder cannot resolve its ID. Changing SparkSession.active() does not rebind the captured root. The same mechanism applies to other captured session-bound objects that accept the batch as an operation's input.
An independent case uses a plain, uncached DataFrame captured before the query:
lookup = spark.range(2).selectExpr(
"CASE id WHEN 0 THEN 'hello' ELSE 'this' END AS value")
def process(batch_df, _):
batch_df.join(lookup, "value").collect()This fails at DataFrame._check_same_session with SESSION_NOT_SAME, before execution. The revised test_nested_dataframes exercises the captured and batch DataFrames separately, so it does not cover either form of composition.
The same three regression tests are available in test-only PRs against master and this PR's head. Their master-based CI and PR-head CI show:
| Test | Master-based run | This PR's head |
|---|---|---|
| Stateful AQE / stream session | Fails: root and batch share a session, with AQE enabled in both | Passes |
| Batch DataFrame in captured-session SQL | Passes | Fails: CONNECT_INVALID_PLAN.DATAFRAME_NOT_FOUND |
| Batch join with captured DataFrame | Passes | Fails: SESSION_NOT_SAME |
The SQL failure names the same batch relation ID that the worker announced under the cloned session, but reports it missing from the original session. In both failing tests, the batch and captured objects worked independently before the mixed operation failed. Please include these regression tests and resolve the mixed-session behavior in this PR.
If the operations are intentionally unsupported, the compatibility decision and migration path should be explicit before changing the default. Copying the relation into the original holder alone would avoid the lookup error but would still allow operations to bypass stream-specific configuration.
Major: clarify the scope of the configuration guarantee
Reassigning SparkSession.active() does not alter session-bound objects captured by the callback. Such objects can still run independent work under the original session's configuration. Please state that the stream-session guarantee currently applies to operations through batch_df or the active cloned session, and test that boundary.
Major: document the temp-view compatibility change
A temp view created through batch_df is now isolated from the original session. That is consistent with the cloned-session model, but changes callbacks that read the view through a previously captured session. Please document the affected behavior and a migration path for callers relying on visibility from the original session.
Minor: cleanup is not recoverable after a close failure
ForeachBatchSessionManager.close() clears its holder reference before calling closeSession; closeSession removes the holder from its store before SessionHolder.close() completes. If cleanup throws, there is no handle left for a retry. Please add a failure-path test or retain a recoverable cleanup state.
Minor: avoid an unused remote holder for Scala callbacks
The Scala callback receives the classic DataFrame directly and does not use the cached remote relation ID, yet the wrapper registers a separate SessionHolder for it. Please consider creating that holder only for paths that resolve a remote batch relation.
Nit: use the session-ID logging key
The cloned session ID is logged with STREAM_ID. Please use the session-ID key so the structured field matches the value being logged.
| // Sanity check: remove any stale DataFrame left over from a previous batch for this query. | ||
| val queryId = queryIdRef.get() | ||
|
|
||
| effectiveHolder.cacheDataFrameById(dfId, df) |
There was a problem hiding this comment.
AI-review / LLM disclosure: This comment was drafted by an LLM on behalf of the reviewer. Please verify the analysis independently.
[Blocker] Following up with a confirmed reproducer: using root.sql("SELECT value FROM {batch}", batch=batch_df) in foreachBatch fails on this PR head with CONNECT_INVALID_PLAN.DATAFRAME_NOT_FOUND. The worker reports the batch relation ID under the cloned session; the original session then cannot resolve that same ID. A plain uncached DataFrame captured before the query also fails when joined to batch_df, with SESSION_NOT_SAME.
Both tests first verify that the batch and captured objects work independently. See test-only PR #36 and its CI: HeartSaVioR#36 and https://github.com/HeartSaVioR/spark/actions/runs/36569505476/job/109417033218.
This is broader than a persisted-DataFrame corner case. Could we resolve or explicitly define the compatibility behavior in this PR before enabling it by default?
| # unpickled, which requires the active session to still be the root one. | ||
| with SparkSession._lock: | ||
| SparkSession._default_session = spark | ||
| SparkSession._active_session.session = spark |
There was a problem hiding this comment.
AI-review / LLM disclosure: This comment was drafted by an LLM on behalf of the reviewer. Please verify the analysis independently.
[Major] Updating the active/default session changes what new calls through SparkSession.active() see, but it does not rebind session-bound objects already captured and unpickled under the root session. Those objects can still run independently with root-session configuration. Please scope the stream-session guarantee accordingly and add a test for this boundary.
| q.processAllAvailable() | ||
| self.assertEqual(1, self.spark.table(sentinel_table).count()) | ||
| with self.assertRaisesRegex(AnalysisException, "TABLE_OR_VIEW_NOT_FOUND"): | ||
| self.spark.sql(f"SELECT * FROM {view_name}").collect() |
There was a problem hiding this comment.
AI-review / LLM disclosure: This comment was drafted by an LLM on behalf of the reviewer. Please verify the analysis independently.
[Major] This test establishes that a temp view created through batch_df is no longer visible through the original session. That fits the cloned-session model, but it changes callbacks that previously read the view through a captured session. Please document this compatibility change and the migration path.
| dataFrameCachingWrapper( | ||
| val sessionManager = new ForeachBatchSessionManager(sessionHolder) | ||
| val queryIdRef = new AtomicReference[String]() | ||
| val wrappedFn = dataFrameCachingWrapper( |
There was a problem hiding this comment.
AI-review / LLM disclosure: This comment was drafted by an LLM on behalf of the reviewer. Please verify the analysis independently.
[Minor] The Scala callback receives the classic DataFrame directly, and dfId is unused on this path, yet dataFrameCachingWrapper creates a remote SessionHolder and caches the batch relation. Could this holder creation be limited to paths that resolve a remote batch relation?
| allowReconnect = true) | ||
| logInfo( | ||
| log"[rootSession: ${MDC(SESSION_ID, rootSessionHolder.sessionId)}] " + | ||
| log"Closed cloned SessionHolder ${MDC(STREAM_ID, holder.sessionId)}.") |
There was a problem hiding this comment.
AI-review / LLM disclosure: This comment was drafted by an LLM on behalf of the reviewer. Please verify the analysis independently.
[Nit] holder.sessionId is a session ID but is logged with STREAM_ID. Please use the session-ID logging key for this field.
|
I think the blocker finding really matters; the query which worked would break with this change. We can't simply defer this to the out of the scope for this PR because we indicate we are going to break some queries. We wouldn't be able to make this change by default if we don't address it. |
…chBatch-session-fix
…de, holder/log cleanups - Turn the mixed-session probes into contract tests: root.sql referencing the batch DataFrame and joining it with a captured root-session DataFrame fail with DATAFRAME_NOT_FOUND / SESSION_NOT_SAME. Add migration tests that run the same work through batch_df.sparkSession. - Document the behavior change in the Structured Streaming migration guide (4.3). - Scala foreachBatch no longer registers an unused cloned SessionHolder. - Log the cloned session id under CLONED_SESSION_ID instead of STREAM_ID.
uros-b
left a comment
There was a problem hiding this comment.
Hi folks, thank you for working on this! I understand this is a blocker for Spark 4.3, however - could you please explain whether you intend to backport this to 4.0-4.2 as well? i.e. should @szehon-ho and myself treat this as a release blocker for maintenance releases, or not? It is my understanding that this is a risky change, and probably no backports. Could you please help confirm this? cc @HeartSaVioR @HyukjinKwon
|
Can we introduce a config and disable the fix by default? I don't think we want to fix something and break the other thing. If we want to make this by default, I would rather say "Let's just document what the fix does not solve" isn't an acceptable state. I don't want users to read the migration guide and change their query, till we are very sure we tried our best to not break it. Either land this partial fix with disabling by default (moving the content of migration guide to the config doc) and aim for better fix in future, or hold this fix till we address them. |
|
I'll need to lower down the tone, sorry about it. Making a decision to break the existing query isn't a cheap action. Writing the breaking change into migration guide isn't really a no-brainer solution; that only works when they can blindly change the query in the suggested way to fix the issue. If users encounter the issue, and have to think hard on how to migrate the query to unblock, we are not in a good direction to go. That decision must have taken seriously (mostly involving discussion), rather than taken by LLM. |
|
Sorry for back and forth, I hit the enter button too soon. The case we describe in migration guide is actually a behavior change from fixing the bug on Spark Connect, and it doesn't seem to be a valid pattern (even for Spark Classic). Maybe the baseline can be set to Spark Classic; if it doesn't work in Spark Classic, it's definitely no-go. If it works in Spark Classic but it's "expected" to not work in Spark Connect (due to technical issue e.g. serialization), it's also no-go. Otherwise I think we need to try fixing it. Maybe I could spend some time to validate my comment by myself. My apologize for the incomplete feedback. At least let's have a SQL config to go back to the previous behavior. It's a wrong behavior, but it's always happening that someone cares more about not seeing the breakage. |
|
No problem, I didn't leave any comment after pushing b0fad56 because I was also thinking about whether the current approach is sound, if there is a more elegant solution, or whether we need to add a new conf. |
|
Let's just have a SQL config and mention it in the migration guide. (Fix is effective by default, and they can disable it.) I confirmed that both patterns are valid to be blocked. Our diligence for this is probably just to give users a chance to save their time to migrate the query via disabling the fix while they can still upgrade the cluster. Thanks for understanding. This is really a critical issue and thanks for working on this and patiently follow this up over multiple months. |
… on) Add spark.sql.connect.streaming.foreachBatch.useClonedSession (default true, 4.3.0). When false, the batch DataFrame is bound to the root session (the pre-fix behavior), so it can be combined with the root session, at the cost of not running under the stream session's configuration. Mention it in the Structured Streaming migration guide and add a parity test covering the disabled path.
If registerNewStreamingQuery throws after a batch has already created the never-expiring cloned SessionHolder, registering the cleaner first ensures the holder is still reaped (via onQueryTerminated / cleanUpAll / the inactive-query eager close) instead of leaking until server shutdown.
|
Thanks @HeartSaVioR, that works for me. Added On the close()-recoverability nit: the cloned holder is torn down via the shared |
|
I was waiting for CI status but it was already succeeded; the result just wasn't reflected in this PR. |
|
Thanks! Merging to master/4.x/ |
…Spark Connect ### What changes were proposed in this pull request? Spark Connect's Python `foreachBatch` cached the batch DataFrame in the root session's `SessionHolder`, but the batch DataFrame belongs to `StreamExecution.sparkSessionForStream`, a clone of the session that started the query. This PR gives that clone its own `SessionHolder` and routes per-batch work through it, so the batch runs under the configuration the streaming engine pins on the clone rather than the root session's. The behavior is gated behind a new SQL config `spark.sql.connect.streaming.foreachBatch.useClonedSession` (default `true`); setting it to `false` restores the previous root-session behavior. On the server, `StreamingForeachBatchHelper` adds: - `ForeachBatchSessionManager`. On the first batch it registers `batchDf.sparkSession` under a server-generated session id through the new `SparkConnectSessionManager.registerExistingSession`, with `customInactiveTimeoutMs = -1` so it never expires by inactivity (the query owns its lifetime). The id is pinned for the whole query, so every batch resolves to the same holder. `close()` is terminal. When `useClonedSession` is `false` it registers no cloned holder and returns the root holder instead. - `ForeachBatchCleaner`, replacing `RunnerCleaner`. It closes the Python runner (when there is one) and the cloned holder. `CleanerCache.registerCleanerForQuery` also closes eagerly if the query is already inactive at registration time, since `onQueryTerminated` never fires for such a query. It is registered before `registerNewStreamingQuery` so a throw there cannot strand the never-expiring cloned holder. - A stale-DataFrame check keyed by streaming query id, so a batch DataFrame left in the cache by a previous batch is evicted. The server writes `(dfId, batchId, sessionId)` per batch, where the session id is the cloned session's when the fix is on and the root session's when it is off. On the Python side, `foreach_batch_worker` bootstraps on the root session id to unpickle the user function before the first batch, then creates a session on the provided id and repoints `SparkSession._default_session` / `_active_session` at it, so `SparkSession.active()` inside the user function returns the session the batch runs on. The Scala `foreachBatch` path receives the classic DataFrame directly and never resolves a remote batch relation, so it does not register a cloned `SessionHolder`. The cloned session id is logged under a `CLONED_SESSION_ID` key so the structured field matches the value. ### Why are the changes needed? `StreamExecution` runs the query on a clone of the session it was started with, and the engine forces onto that clone the configs stateful correctness depends on: `spark.sql.cbo.enabled=false` and `spark.sql.requireAllClusterKeysForDistribution=false` so stateful operators are not rearranged, AQE off for continuous, real-time, and stateful workloads, and the stateful shuffle partition count pinned at start. `populateStartOffsets` also restores the checkpointed conf from the offset log onto the same session. Connect resolved the batch DataFrame against the root `SessionHolder`, so operations the user function performed on it were planned and executed under the root session's configuration, with none of those settings in effect. The Python worker also kept the root session active, so the Connect session mapping never expired and the query kept running after the client disappeared. ### Does this PR introduce _any_ user-facing change? Yes, for Python `foreachBatch` under Spark Connect, with `spark.sql.connect.streaming.foreachBatch.useClonedSession` at its default of `true`. The batch DataFrame is now bound to the cloned session, matching classic `foreachBatch`, which already runs the user function on the clone (`ForeachBatchSink.addBatch` builds the batch Dataset with `ofRows(data.sparkSession, node)`). Because the batch DataFrame no longer shares a session with the client: - `SparkSession.active()` inside the function returns the batch session rather than the root one. - A temporary view created from the batch DataFrame is not visible to the root session. - Referencing the batch DataFrame from `spark.sql` on a captured root session fails with `DATAFRAME_NOT_FOUND`, and joining it with a DataFrame captured from the root session fails with `SESSION_NOT_SAME`. Combining the batch DataFrame with the root session inside the function is not supported by design: the isolation that fixes the configuration bug is the same thing that separates the two sessions. Run such operations through the batch DataFrame's own session (`batch_df.sparkSession`), and pass results out of the function through a table or a global temporary view instead of a session-local temporary view. To keep the previous behavior, set `spark.sql.connect.streaming.foreachBatch.useClonedSession` to `false`: the batch DataFrame is then bound to the root session again and can be combined with it, at the cost of not running under the stream session's configuration. The Structured Streaming migration guide documents this. ### How was this patch tested? New tests in `StreamingForeachBatchParityTests`: - `test_batch_uses_stream_session_for_stateful_aqe` pins that the batch runs on a different session than the root, with AQE off on the batch session while the root has it on. - `test_batch_dataframe_in_root_session_sql_is_unsupported` and `test_batch_dataframe_join_with_root_session_dataframe_is_unsupported` pin the contract under the default: `spark.sql` referencing the batch DataFrame from the root session fails with `DATAFRAME_NOT_FOUND`, and joining it with a captured root-session DataFrame fails with `SESSION_NOT_SAME`. - `test_batch_dataframe_in_batch_session_sql_is_supported` and `test_batch_dataframe_join_in_batch_session_is_supported` pin the documented migration: the same work run through `batch_df.sparkSession` succeeds. - `test_temp_view_is_isolated_from_root_session` pins that a temp view created inside the function resolves through the cloned session but not the root session, with a sentinel table proving the function ran. - `test_disabling_fix_restores_root_session_behavior` pins that with the config set to `false`, joining the batch DataFrame with a captured root-session DataFrame succeeds again. New tests in `StreamingForeachBatchHelperSuite`: `ForeachBatchSessionManager.close()` is terminal and a closing root session is refused; a cleaner registered for an already terminated query is closed on the spot. Updated tests: `test_nested_dataframes` now uses `saveAsTable` instead of `createOrReplaceTempView`, and `SparkConnectSessionHolderSuite` destructures the new 3-tuple from `pythonForeachBatchWrapper`. Pass GitHub Actions. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code Closes #55410 from LuciferYang/SPARK-44462-foreachBatch-session-fix. Lead-authored-by: YangJie <yangjie01@baidu.com> Co-authored-by: reviewer <reviewer@local> Signed-off-by: Jungtaek Lim <kabhwan.opensource@gmail.com> (cherry picked from commit 362ee55) Signed-off-by: Jungtaek Lim <kabhwan.opensource@gmail.com>
|
@LuciferYang Could you please submit a PR for 4.3? I see there is a merge conflict and this is valuable enough to not slip through. Thanks! |
ok, I'll do this |
|
Thanks for your reviews. @HeartSaVioR @HyukjinKwon @uros-b @bogao007 @xiaoxuandev |
…gisters the cloned session ### What changes were proposed in this pull request? Followup to #55410. The two `ForeachBatchSessionManager` cleanup tests in `StreamingForeachBatchHelperSuite` checked that `close()` is terminal and that the registered cleaner runs, but neither asserted the side effect `close()` exists to guarantee: that the cloned `SessionHolder` is removed from `SparkConnectService.sessionManager`. Because `close()` sets its `closed` flag independently of the `closeSession` call, a future change that dropped or broke the unregister step would still pass both tests while re-introducing the never-expiring-holder leak. This adds assertions that the cloned `SessionKey` is present in the session manager before `close()` and absent after. ### Why are the changes needed? To make the cloned-session leak-prevention contract regression-sensitive. Spotted in review of #59200, the branch-4.3 backport. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass GitHub Actions. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code Closes #59202 from LuciferYang/SPARK-44462-assert-cloned-session-unregister. Lead-authored-by: YangJie <yangjie01@baidu.com> Co-authored-by: reviewer <reviewer@local> Signed-off-by: yangjie01 <yangjie01@baidu.com>
…gisters the cloned session ### What changes were proposed in this pull request? Followup to #55410. The two `ForeachBatchSessionManager` cleanup tests in `StreamingForeachBatchHelperSuite` checked that `close()` is terminal and that the registered cleaner runs, but neither asserted the side effect `close()` exists to guarantee: that the cloned `SessionHolder` is removed from `SparkConnectService.sessionManager`. Because `close()` sets its `closed` flag independently of the `closeSession` call, a future change that dropped or broke the unregister step would still pass both tests while re-introducing the never-expiring-holder leak. This adds assertions that the cloned `SessionKey` is present in the session manager before `close()` and absent after. ### Why are the changes needed? To make the cloned-session leak-prevention contract regression-sensitive. Spotted in review of #59200, the branch-4.3 backport. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass GitHub Actions. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code Closes #59202 from LuciferYang/SPARK-44462-assert-cloned-session-unregister. Lead-authored-by: YangJie <yangjie01@baidu.com> Co-authored-by: reviewer <reviewer@local> Signed-off-by: yangjie01 <yangjie01@baidu.com> (cherry picked from commit 38422ae) Signed-off-by: yangjie01 <yangjie01@baidu.com>

What changes were proposed in this pull request?
Spark Connect's Python
foreachBatchcached the batch DataFrame in the root session'sSessionHolder, but the batch DataFrame belongs toStreamExecution.sparkSessionForStream, a clone of the session that started the query. This PR gives that clone its ownSessionHolderand routes per-batch work through it, so the batch runs under the configuration the streaming engine pins on the clone rather than the root session's. The behavior is gated behind a new SQL configspark.sql.connect.streaming.foreachBatch.useClonedSession(defaulttrue); setting it tofalserestores the previous root-session behavior.On the server,
StreamingForeachBatchHelperadds:ForeachBatchSessionManager. On the first batch it registersbatchDf.sparkSessionunder a server-generated session id through the newSparkConnectSessionManager.registerExistingSession, withcustomInactiveTimeoutMs = -1so it never expires by inactivity (the query owns its lifetime). The id is pinned for the whole query, so every batch resolves to the same holder.close()is terminal. WhenuseClonedSessionisfalseit registers no cloned holder and returns the root holder instead.ForeachBatchCleaner, replacingRunnerCleaner. It closes the Python runner (when there is one) and the cloned holder.CleanerCache.registerCleanerForQueryalso closes eagerly if the query is already inactive at registration time, sinceonQueryTerminatednever fires for such a query. It is registered beforeregisterNewStreamingQueryso a throw there cannot strand the never-expiring cloned holder.The server writes
(dfId, batchId, sessionId)per batch, where the session id is the cloned session's when the fix is on and the root session's when it is off. On the Python side,foreach_batch_workerbootstraps on the root session id to unpickle the user function before the first batch, then creates a session on the provided id and repointsSparkSession._default_session/_active_sessionat it, soSparkSession.active()inside the user function returns the session the batch runs on.The Scala
foreachBatchpath receives the classic DataFrame directly and never resolves a remote batch relation, so it does not register a clonedSessionHolder. The cloned session id is logged under aCLONED_SESSION_IDkey so the structured field matches the value.Why are the changes needed?
StreamExecutionruns the query on a clone of the session it was started with, and the engine forces onto that clone the configs stateful correctness depends on:spark.sql.cbo.enabled=falseandspark.sql.requireAllClusterKeysForDistribution=falseso stateful operators are not rearranged, AQE off for continuous, real-time, and stateful workloads, and the stateful shuffle partition count pinned at start.populateStartOffsetsalso restores the checkpointed conf from the offset log onto the same session. Connect resolved the batch DataFrame against the rootSessionHolder, so operations the user function performed on it were planned and executed under the root session's configuration, with none of those settings in effect. The Python worker also kept the root session active, so the Connect session mapping never expired and the query kept running after the client disappeared.Does this PR introduce any user-facing change?
Yes, for Python
foreachBatchunder Spark Connect, withspark.sql.connect.streaming.foreachBatch.useClonedSessionat its default oftrue. The batch DataFrame is now bound to the cloned session, matching classicforeachBatch, which already runs the user function on the clone (ForeachBatchSink.addBatchbuilds the batch Dataset withofRows(data.sparkSession, node)). Because the batch DataFrame no longer shares a session with the client:SparkSession.active()inside the function returns the batch session rather than the root one.spark.sqlon a captured root session fails withDATAFRAME_NOT_FOUND, and joining it with a DataFrame captured from the root session fails withSESSION_NOT_SAME.Combining the batch DataFrame with the root session inside the function is not supported by design: the isolation that fixes the configuration bug is the same thing that separates the two sessions. Run such operations through the batch DataFrame's own session (
batch_df.sparkSession), and pass results out of the function through a table or a global temporary view instead of a session-local temporary view. To keep the previous behavior, setspark.sql.connect.streaming.foreachBatch.useClonedSessiontofalse: the batch DataFrame is then bound to the root session again and can be combined with it, at the cost of not running under the stream session's configuration. The Structured Streaming migration guide documents this.How was this patch tested?
New tests in
StreamingForeachBatchParityTests:test_batch_uses_stream_session_for_stateful_aqepins that the batch runs on a different session than the root, with AQE off on the batch session while the root has it on.test_batch_dataframe_in_root_session_sql_is_unsupportedandtest_batch_dataframe_join_with_root_session_dataframe_is_unsupportedpin the contract under the default:spark.sqlreferencing the batch DataFrame from the root session fails withDATAFRAME_NOT_FOUND, and joining it with a captured root-session DataFrame fails withSESSION_NOT_SAME.test_batch_dataframe_in_batch_session_sql_is_supportedandtest_batch_dataframe_join_in_batch_session_is_supportedpin the documented migration: the same work run throughbatch_df.sparkSessionsucceeds.test_temp_view_is_isolated_from_root_sessionpins that a temp view created inside the function resolves through the cloned session but not the root session, with a sentinel table proving the function ran.test_disabling_fix_restores_root_session_behaviorpins that with the config set tofalse, joining the batch DataFrame with a captured root-session DataFrame succeeds again.New tests in
StreamingForeachBatchHelperSuite:ForeachBatchSessionManager.close()is terminal and a closing root session is refused; a cleaner registered for an already terminated query is closed on the spot.Updated tests:
test_nested_dataframesnow usessaveAsTableinstead ofcreateOrReplaceTempView, andSparkConnectSessionHolderSuitedestructures the new 3-tuple frompythonForeachBatchWrapper.Pass GitHub Actions.
Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code