[SPARK-55005][SQL] Fix CONTINUE HANDLER to continue loop execution after handling exceptions in loop body#53759
Closed
miland-db wants to merge 2 commits intoapache:masterfrom
Closed
Conversation
JIRA Issue Information=== Sub-task SPARK-55005 === This comment was automatically generated by GitHub Actions |
Contributor
Author
|
Adding @cloud-fan and @srielau. Please review |
Contributor
|
thanks, merging to master/4.1! |
cloud-fan
changed the title
cloud-fan
pushed a commit
that referenced
this pull request
Jan 12, 2026
…ter handling exceptions in loop body ### What changes were proposed in this pull request? This PR fixes a critical bug in `CONTINUE HANDLER` execution within loops. When a `CONTINUE HANDLER` handled an exception that occurred inside a loop body, the loop would exit completely instead of continuing to the next iteration. #### Root cause The `interruptConditionalStatements` method in `SqlScriptingExecution.scala` was designed to skip conditional statements when exceptions occurred in their condition evaluation (e.g., WHILE 1/0 > 0). However, it didn't distinguish between: - Exception in condition → loop should be skipped - Exception in body → loop should continue to next iteration The method unconditionally set `interrupted = true` on all conditional statements, causing loops to exit even when the error occurred during body execution. #### The fix The fix was to add `isInCondition: Boolean` method to `ConditionalStatementExec` trait to track whether a conditional statement is currently evaluating its condition or executing its body. It was also needed to implement `isInCondition` for all 6 `ConditionalStatementExec`: - IfElseStatementExec - WhileStatementExec - RepeatStatementExec - ForStatementExec - SearchedCaseStatementExec - SimpleCaseStatementExec ### Why are the changes needed? This ensures `CONTINUE HANDLER` only interrupts conditional statements when exceptions occur during condition evaluation, allowing loops to continue normally when exceptions occur in their bodies. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Added comprehensive test coverage for `CONTINUE HANDLER` across all conditional statement types. These tests verify that when an exception occurs inside a loop body and is handled by a `CONTINUE HANDLER`, the loop continues to the next iteration rather than exiting. ### Was this patch authored or co-authored using generative AI tooling? Closes #53759 from miland-db/milan-dankovic_data/continue-handler-fix-loops. Authored-by: Milan Dankovic <milan.dankovic@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 7a40891) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
srielau
added a commit
to srielau/spark
that referenced
this pull request
Jan 14, 2026
Removed setReturnCurrentWithoutAdvancing() method and returnCurrentWithoutAdvancing flag as they are no longer needed after PR apache#53759. PR apache#53759 fixed CONTINUE HANDLER to properly handle exceptions in loop bodies by introducing isInCondition to distinguish between: - Exception in condition → loop should be interrupted - Exception in body → loop should continue to next iteration The returnCurrentWithoutAdvancing flag was an earlier workaround that attempted to solve iterator advancement issues after CONTINUE handlers. With the proper fix in place via isInCondition, this code is now redundant. Tested: All continue handler and cursor tests pass without this code.
srielau
added a commit
to srielau/spark
that referenced
this pull request
Jan 14, 2026
…PEAT+CONTINUE HANDLER The code removed in commit 3b1dccf was NOT dead code - it was needed for REPEAT loops with CONTINUE handlers. Root Cause Analysis: 1. Before FETCH variable resolution fix: Tests had 58 UNRESOLVED_COLUMN errors, never reached the iterator logic 2. After FETCH variable resolution fix (dad9930): Variables resolved correctly, tests progressed into REPEAT loop execution 3. This exposed a pre-existing bug: 7 'No more elements' INTERNAL_ERROR in REPEAT+CONTINUE HANDLER tests 4. The UNRESOLVED_COLUMN errors were MASKING the iterator bug The returnCurrentWithoutAdvancing flag handles a specific case: - When a CONTINUE handler fires within a REPEAT loop body (e.g., from FETCH) - After the handler completes, curr already points to the correct next statement - Without the flag, the iterator incorrectly advances again, causing 'No more elements' PR apache#53759's isInCondition fix handles exceptions in loop CONDITIONS vs BODIES, but doesn't cover the case of handlers firing from statements (like FETCH) within REPEAT loop bodies. This restores the removed code with a whitespace fix. Results: - All 7 'No more elements' INTERNAL_ERROR fixed ✅ - All cursor tests now pass with proper REPEAT+CONTINUE HANDLER behavior - 0 INTERNAL_ERROR in cursors.sql.out
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
3 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 PR fixes a critical bug in
CONTINUE HANDLERexecution within loops. When aCONTINUE HANDLERhandled an exception that occurred inside a loop body, the loop would exit completely instead of continuing to the next iteration.Root cause
The
interruptConditionalStatementsmethod inSqlScriptingExecution.scalawas designed to skip conditional statements when exceptions occurred in their condition evaluation (e.g., WHILE 1/0 > 0). However, it didn't distinguish between:The method unconditionally set
interrupted = trueon all conditional statements, causing loops to exit even when the error occurred during body execution.The fix
The fix was to add
isInCondition: Booleanmethod toConditionalStatementExectrait to track whether a conditional statement is currently evaluating its condition or executing its body. It was also needed to implementisInConditionfor all 6ConditionalStatementExec:Why are the changes needed?
This ensures
CONTINUE HANDLERonly interrupts conditional statements when exceptions occur during condition evaluation, allowing loops to continue normally when exceptions occur in their bodies.Does this PR introduce any user-facing change?
No.
How was this patch tested?
Added comprehensive test coverage for
CONTINUE HANDLERacross all conditional statement types. These tests verify that when an exception occurs inside a loop body and is handled by aCONTINUE HANDLER, the loop continues to the next iteration rather than exiting.Was this patch authored or co-authored using generative AI tooling?