Skip to content

[SPARK-50491][SQL] Fix bug where empty BEGIN END blocks throw an error#49064

Closed
dusantism-db wants to merge 3 commits intoapache:masterfrom
dusantism-db:scripting-noop-statement
Closed

[SPARK-50491][SQL] Fix bug where empty BEGIN END blocks throw an error#49064
dusantism-db wants to merge 3 commits intoapache:masterfrom
dusantism-db:scripting-noop-statement

Conversation

@dusantism-db
Copy link
Copy Markdown
Contributor

@dusantism-db dusantism-db commented Dec 4, 2024

This PR depends on #48989

What changes were proposed in this pull request?

There is a bug in SQL scripting which causes empty compound statements to throw an error if their body consists solely of empty BEGIN END blocks. Examples:

WHILE 1 = 1 DO
  BEGIN
  END;
END WHILE;
BEGIN
  BEGIN
    BEGIN
    END;
  END;
END;

This PR fixes this by introducing a NO-OP statement for SQL scripting, which empty BEGIN END blocks will return.

Why are the changes needed?

Currenty, compound bodies declare they have the next element even if their body is consisted only of empty blocks. This is because it only checks for existence of statements in the body, not whether there is at least one statement which is not an empty block.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Unit tests were added to existing suites.

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

No.

@github-actions github-actions bot added the SQL label Dec 4, 2024
@dusantism-db dusantism-db changed the title [SPARK-50491][SQL] Fix bug where empty BEGIN END blocks throw an error [WIP][SPARK-50491][SQL] Fix bug where empty BEGIN END blocks throw an error Dec 4, 2024
@dusantism-db dusantism-db force-pushed the scripting-noop-statement branch from 813f785 to 5bc7482 Compare December 6, 2024 12:46
@dusantism-db dusantism-db changed the title [WIP][SPARK-50491][SQL] Fix bug where empty BEGIN END blocks throw an error [SPARK-50491][SQL] Fix bug where empty BEGIN END blocks throw an error Dec 6, 2024
@miland-db
Copy link
Copy Markdown
Contributor

Good job on testing different NonLeafStatements with empty compounds!

@dusantism-db dusantism-db force-pushed the scripting-noop-statement branch from a268135 to 55a679d Compare December 9, 2024 11:51
@cloud-fan
Copy link
Copy Markdown
Contributor

This approach LGTM, but do you know why CompoundBodyExec can't handle Nil statements?

@dusantism-db
Copy link
Copy Markdown
Contributor Author

This approach LGTM, but do you know why CompoundBodyExec can't handle Nil statements?

@cloud-fan
The problem is that hasNext for compound bodies is true for non-empty bodies, however a non-empty body with only empty BEGIN END blocks does not have a leaf statement to return. So Compound body expects a leaf statement because hasNext is true, but since there is no actual statement we will return the NO-OP.

@cloud-fan
Copy link
Copy Markdown
Contributor

thanks, merging to master!

@cloud-fan cloud-fan closed this in e70f8ab Dec 10, 2024
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.

5 participants