[SPARK-56505][SQL][TESTS][FOLLOWUP] Make the classic session in SparkSessionBinderBase an overridable seam#56963
Closed
cloud-fan wants to merge 3 commits into
Closed
Conversation
cloud-fan
changed the title
Contributor
Author
cloud-fan
force-pushed
the
cloud-fan/session-binder-classic-accessor
branch
from
f186973 to
ae7ee1e
Compare
…derBase an overridable seam Follow-up to the SessionQueryTest refactor. SparkSessionBinderBase exposes the bound session via the abstract `spark`, but its per-test afterEach hook needs the classic session (sharedState.cacheManager) and reads the trait's private _spark field directly. That hard-codes the assumption that the shared beforeAll session is the one the hooks should act on, so a suite cannot bind a per-test session (e.g. under OneInstancePerTest, where beforeAll never runs on the per-test instance and _spark stays null). Promote the classic session to an overridable `protected def classicSpark` (default = _spark) and route afterEach through it. connect.SparkSessionBinder's existing private classicSpark becomes an override of it (no behavior change). Add PerTestSessionBinderSuite demonstrating a per-test cloned session via the override. No user-facing change; test-only.
cloud-fan
force-pushed
the
cloud-fan/session-binder-classic-accessor
branch
from
ae7ee1e to
5a21790
Compare
…rkSessionBinder Reflow the classicSpark doc comment so no line exceeds 100 characters. Co-authored-by: Isaac
…ionBinder Reflow the connect binder doc comment to satisfy scalafmt's maxColumn=98 (the connect modules use a stricter limit than scalastyle's 100). Co-authored-by: Isaac
HyukjinKwon
approved these changes
Jul 5, 2026
zhengruifeng
approved these changes
Jul 6, 2026
Contributor
Author
|
thanks for the review, merging to master/4.x |
cloud-fan
added a commit
that referenced
this pull request
Jul 7, 2026
…essionBinderBase an overridable seam ### What changes were proposed in this pull request? This is a follow-up to #56190 (which added `SessionQueryTest` and the `SparkSessionBinder` / `SparkSessionBinderBase` traits). This PR makes the classic `SparkSession` used by the test-session lifecycle hooks in `SparkSessionBinderBase` an overridable seam, so a suite can bind its session per test instead of only through the single shared `beforeAll` session. `SparkSessionBinderBase` exposes the bound session through the abstract `spark` (typed as the session-agnostic `SparkSession`, so the connect binder can narrow it to a connect session). Its per-test `afterEach` hook, however, needs the classic session (`sharedState.cacheManager`), and today reads the trait's `private var _spark` directly. That is fine for the common case where the session is created in `beforeAll`, but it hard-codes the assumption that the shared field is the session the hooks should act on. Changes: - Add `protected def classicSpark: classic.SparkSession = _spark` to `SparkSessionBinderBase` and route the per-test hook through it (`afterEach`'s `cacheManager.clearCache()`). - `connect.SparkSessionBinder` already computed the same value as a `private def classicSpark`; change it to `override protected def classicSpark` of the promoted accessor (no behavior change). - Add `PerTestSessionBinderSuite`, a small example suite that runs each test in a fresh instance (`OneInstancePerTest`) bound to a per-test cloned session, by overriding `classicSpark`. This both demonstrates the new flexibility and guards the seam. ### Why are the changes needed? The current binder can only bind the session created in `beforeAll`. A suite that wants an isolated session per test -- e.g. one mixing in `OneInstancePerTest`, where ScalaTest runs each test in a fresh suite instance that executes only `beforeEach`/`afterEach` and never `beforeAll` -- has no way to tell the inherited hooks which session to operate on; those hooks read the private shared field, which is `null` in the per-test instance. Making `classicSpark` overridable removes that limitation while leaving the default (shared-session) behavior unchanged. ### Does this PR introduce _any_ user-facing change? No. This is test-only; the default behavior of `SparkSessionBinderBase` and the connect binder is unchanged. ### How was this patch tested? New `PerTestSessionBinderSuite` exercises the per-test binding path and passes. Existing binder-based suites compile and run unchanged (the connect binder change is a `private def` -> `override protected def` with the same body). ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 4.8) Closes #56963 from cloud-fan/cloud-fan/session-binder-classic-accessor. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit dad5db7) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
This is a follow-up to #56190 (which added
SessionQueryTestand theSparkSessionBinder/SparkSessionBinderBasetraits).This PR makes the classic
SparkSessionused by the test-session lifecycle hooks inSparkSessionBinderBasean overridable seam, so a suite can bind its session per test instead ofonly through the single shared
beforeAllsession.SparkSessionBinderBaseexposes the bound session through the abstractspark(typed as thesession-agnostic
SparkSession, so the connect binder can narrow it to a connect session). Itsper-test
afterEachhook, however, needs the classic session (sharedState.cacheManager), andtoday reads the trait's
private var _sparkdirectly. That is fine for the common case where thesession is created in
beforeAll, but it hard-codes the assumption that the shared field is thesession the hooks should act on.
Changes:
protected def classicSpark: classic.SparkSession = _sparktoSparkSessionBinderBaseandroute the per-test hook through it (
afterEach'scacheManager.clearCache()).connect.SparkSessionBinderalready computed the same value as aprivate def classicSpark;change it to
override protected def classicSparkof the promoted accessor (no behavior change).PerTestSessionBinderSuite, a small example suite that runs each test in a fresh instance(
OneInstancePerTest) bound to a per-test cloned session, by overridingclassicSpark. This bothdemonstrates the new flexibility and guards the seam.
Why are the changes needed?
The current binder can only bind the session created in
beforeAll. A suite that wants an isolatedsession per test -- e.g. one mixing in
OneInstancePerTest, where ScalaTest runs each test in afresh suite instance that executes only
beforeEach/afterEachand neverbeforeAll-- has no wayto tell the inherited hooks which session to operate on; those hooks read the private shared field,
which is
nullin the per-test instance. MakingclassicSparkoverridable removes that limitationwhile leaving the default (shared-session) behavior unchanged.
Does this PR introduce any user-facing change?
No. This is test-only; the default behavior of
SparkSessionBinderBaseand the connect binder isunchanged.
How was this patch tested?
New
PerTestSessionBinderSuiteexercises the per-test binding path and passes. Existing binder-basedsuites compile and run unchanged (the connect binder change is a
private def->override protected defwith the same body).Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 4.8)