Skip to content

[SPARK-58204][SQL] Fix unprotected mutable state in RegExpReplace, RegExpExtractBase, StringTranslate, FormatNumber, and NamedLambdaVariable by marking them stateful#57353

Open
vinodkc wants to merge 1 commit into
apache:masterfrom
vinodkc:fix-spark58154-stateful-expressions

Conversation

@vinodkc

@vinodkc vinodkc commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Override stateful: Boolean = true on five Catalyst expressions that maintain mutable
@transient state but were not marked stateful:

  • RegExpReplace
  • RegExpExtractBase (RegExpExtract, RegExpExtractAll)
  • StringTranslate
  • FormatNumber
  • NamedLambdaVariable

For NamedLambdaVariable, also override withNewChildrenInternal to reset the AtomicReference on copy while preserving exprId. LeafLike.withNewChildrenInternal returns this by default, which would make freshCopyIfContainsStatefulExpression() a no-op even with the stateful flag set.

Why are the changes needed?

freshCopyIfContainsStatefulExpression() only copies subtrees containing a stateful = true node. Without the override, these expressions are shared across projections. When two projections evaluate concurrently — e.g. via ConvertToLocalRelation on the driver or pipelined execution on executors — threads race on the shared @transient cache fields and silently corrupt results.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Added one test per affected group in the existing suites (HigherOrderFunctionsSuite, RegexpExpressionsSuite, StringExpressionsSuite), each asserting stateful == true and that freshCopyIfContainsStatefulExpression() returns a distinct instance with independent state.

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

No

…gExpExtractBase, StringTranslate, FormatNumber, and NamedLambdaVariable by marking them stateful

@HyukjinKwon HyukjinKwon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 blocking, 0 non-blocking, 0 nits.
Clean, well-targeted fix that follows the established stateful pattern and correctly handles the LeafExpression special case. No findings.

Verification

Traced the correctness of marking these five expressions stateful:

  • No side effects beyond freshCopyIfContainsStatefulExpression — a repo-wide sweep found no other behavioral consumer of the expression .stateful flag (subexpression elimination keys on deterministic, not stateful), so there is no CSE / constant-folding / optimizer regression; every other .stateful hit is the streaming package name.
  • NamedLambdaVariable (leaf) equivalence — with the new withNewChildrenInternal, a fresh copy gets an independent AtomicReference but the same exprId. Interpreted eval re-links body vars to the bound argument instances by exprId (functionsForEval), and codegen looks up lambda vars by exprId.id (getLambdaVar), so the split reference is re-unified at eval/codegen time — results are unchanged. exprId preservation is essential and correctly distinguishes this from newInstance(). Ordinary tree transforms don't reset the reference (leaf short-circuits withNewChildren), only freshCopy does.
  • Fix completeness — swept every catalyst expression file for @transient ... var mutable state: the five are the complete previously-unmarked set. StringRegexExpression (Like/RLike) uses an immutable lazy val cache and correctly stays unflagged; RegExpCount/RegExpSubStr are RuntimeReplaceable delegating to the now-stateful extractors; RegExpInStr inherits from RegExpExtractBase; RDG/Shuffle were already stateful.
  • Tests are meaningful — the NamedLambdaVariable test asserts copy.value ne lv.value, which is exactly the assertion that would fail without the withNewChildrenInternal override, so it guards the leaf no-op the PR fixes.

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