Skip to content

[SPARK-48353][FOLLOW UP][SQL] Exception handling improvements #51034

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

davidm-db
Copy link
Contributor

What changes were proposed in this pull request?

This pull request proposes an improvement in how conditions are matched with exception handlers. Previously, condition would match with an exception handler only if the match was full/complete - i.e. condition <MAIN>.<SUBCLASS> would match only to the handler defined for <MAIN>.<SUBCLASS>. With this improvement, <MAIN>.<SUBCLASS> condition would match to the handlers defined for <MAIN> condition as well, with correct precedence.

This pull requests also proposes a number of fixes for different missing things:

  • CompoundBodyExec.reset() is not resetting the scopeStatus field.
  • Exception handler body is never reset (this includes internal iterator reset). This causes issues if the handler is defined in a loop and gets matched multiple times.
  • When searching for a place to inject LEAVE statement, after the EXIT handler has been executed, the logic considered only CompoundBodyExec nodes, whereas it should have included all non-leaf statements.
  • Exception handling for exceptions that happen in conditions (for each statement type - if/else, while, case, etc) is not working properly because the injected LEAVE statement is not recognized properly.
  • Exception handling for exceptions that happen in the last statement of the body of if/else and searched case statement is not working properly because the injected LEAVE statement is not recognized properly.
  • hasNext() in ForStatementExec is executing the query. This causes the issues in cases when FOR is the first statement in the compound and the query fails. It means that exception would happen during the hasNext() checks instead of the actual iteration. In such case, the parent compound (of the FOR statement) is not entered (because that happens during the iteration) and call stack is not properly setup for exception handling.

Changes corresponding to this problems, in the same order:

  • Reset the scopeStatus field in CompoundBodyExec.reset().
  • Call reset() on handler before starting its execution in SqlScriptingExecution.handleException().
  • Move curr pointer to NonLeafStatementExec and use that in SqlScriptingExecution. getNextStatement() in case when EXIT handler is finished and removed from the stack.
  • Add special case handling for LeaveStatementExec in *.Condition cases in all of the relevant statement types. When exception happens during the condition execution, the LeaveStatementExec is injected into curr field, but the state hasn't changed. This means that when the LEAVE statement is to be executed, the state would correspond to the condition. I don't know how to do this better, so any suggestions are more than welcome!
  • Add special case handling for LeaveStatementExec in *Body cases of if/else and searched case statement - equivalent to the previous bullet.
  • Reorder conditions in ForStatementExec.hasNext().

Why are the changes needed?

These changes are fixing wrong logic and improving some of the already existing exception handling mechanisms.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

New unit tests are added for to test/guard all of the introduced improvements.

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

No.

@github-actions github-actions bot added the SQL label May 27, 2025
@davidm-db davidm-db changed the title Exception handling improvements [SPARK-48353][FOLLOW UP][SQL] Exception handling improvements May 27, 2025
@davidm-db davidm-db marked this pull request as ready for review May 27, 2025 19:56
@davidm-db
Copy link
Contributor Author

cc @cloud-fan @dejankrak-db @miland-db @dusantism-db please review

@cloud-fan
Copy link
Contributor

I'm fixing the mima checks at #51038 , please rebase after the fix is merged

@davidm-db davidm-db force-pushed the exception_handling_improvements branch from 73da2bb to 15b8b9d Compare May 29, 2025 06:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants