fix: `maybe_parse` preventing parser from erroring on recursion limit #1464

tomershaniii · 2024-10-09T11:21:38Z

This PR fixes a bug in the recursion checks, the bug is caused by maybe_parse ignoring the returned error, which (in the case of recursion depth check) prevents the Error from propagating back to the caller and may lead to unexpected parsing results.

The fix involves specific handling of the RecursionLimitExceeded error within maybe_parse and returning the error up the call stack.

iffyio

Thanks for this fix @tomershaniii! Would it be possible to include a test case that passes with this fix? If so we could add one here for example

…r stack utilization

…in poor stack utilization

tomershaniii · 2024-10-14T16:08:59Z

@iffyio took some yak shaving but it's finally here :-) check out test_parentheses_overflow
Some context for the other changes:

the parse_boxed variants were doing the boxing when exiting the recursion, which resulted in unreasonable low number of stack frames before overflowing, in the PRs i removed the parse_boxed functions and added native boxing to the respective parse_ functions

alamb

Looks great to me -- than you @tomershaniii and @iffyio

alamb · 2024-10-16T16:21:37Z

src/ast/mod.rs

@@ -3191,7 +3191,7 @@ pub enum Statement {
        /// Table confs
        options: Vec<SqlOption>,
        /// Cache table as a Query
-        query: Option<Query>,
+        query: Option<Box<Query>>,


this is a nice change

alamb · 2024-10-16T16:21:45Z

src/ast/query.rs

@@ -1103,7 +1103,7 @@ pub enum PivotValueSource {
    /// Pivot on all values returned by a subquery.
    ///
    /// See <https://docs.snowflake.com/en/sql-reference/constructs/pivot#pivot-on-column-values-using-a-subquery-with-dynamic-pivot>.
-    Subquery(Query),
+    Subquery(Box<Query>),


alamb · 2024-10-16T16:22:19Z

src/parser/mod.rs

+        match f(self) {
+            Ok(t) => Ok(Some(t)),
+            // Unwind stack if limit exceeded
+            Err(ParserError::RecursionLimitExceeded) => Err(ParserError::RecursionLimitExceeded),


alamb · 2024-10-16T16:24:55Z

Looks like a CI check is failing on this PR

coveralls · 2024-10-16T16:28:17Z

Pull Request Test Coverage Report for Build 11440876981

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

277 of 279 (99.28%) changed or added relevant lines in 17 files are covered.
4 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.001%) to 89.388%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/parser/mod.rs	61	63	96.83%

Files with Coverage Reduction	New Missed Lines	%
tests/sqlparser_common.rs	4	89.6%

Totals
Change from base Build 11429400839:	0.001%
Covered Lines:	30400
Relevant Lines:	34009

💛 - Coveralls

iffyio

LGTM! thanks @tomershaniii

tomershaniii · 2024-10-17T19:19:55Z

CI issues fixed, please re-run

tomershaniii · 2024-10-20T08:21:42Z

Build broken by another commit, please re-run CI

alamb · 2024-10-20T20:30:25Z

🤔 CI is still failing unfortunately

tomershaniii · 2024-10-21T13:13:54Z

yet another rebase fix @alamb plz...

alamb · 2024-10-21T19:41:32Z

🚀

alamb · 2024-10-21T19:41:42Z

Thanks for sticking with this @tomershaniii and for the review @iffyio

apache#1464)

iffyio reviewed Oct 13, 2024

View reviewed changes

tomershaniii added 3 commits October 14, 2024 18:58

fix for maybe_parse preventing parser from erroring on recursion limit

43680ef

parsed_box_query generates boxing on recursion tail, resulting in poo…

ae8037a

…r stack utilization

parse_boxed_query_body generates boxing on recursion tail, resulting …

94d5466

…in poor stack utilization

tomershaniii force-pushed the tomers_branch branch from e5aa255 to e474fd2 Compare October 14, 2024 16:01

alamb approved these changes Oct 16, 2024

View reviewed changes

iffyio approved these changes Oct 17, 2024

View reviewed changes

tomershaniii force-pushed the tomers_branch branch from e474fd2 to 5b86547 Compare October 17, 2024 19:06

recursion test + ability to configure recursion limit for tests

8ee028a

tomershaniii force-pushed the tomers_branch branch from 5b86547 to 8ee028a Compare October 17, 2024 19:16

tomershaniii force-pushed the tomers_branch branch from f2e6c1b to 194147d Compare October 20, 2024 08:18

Merge branch 'apache:main' into tomers_branch

edce476

tomershaniii force-pushed the tomers_branch branch from 194147d to edce476 Compare October 21, 2024 06:08

Merge branch 'apache:main' into tomers_branch

3880697

tomershaniii force-pushed the tomers_branch branch from fbfaa1e to 3880697 Compare October 21, 2024 13:11

alamb merged commit 8e0d26a into apache:main Oct 21, 2024
10 checks passed

alamb changed the title ~~fix for maybe_parse preventing parser from erroring on recursion limit~~ fix: change maybe_parse preventing parser from erroring on recursion limit Oct 21, 2024

alamb changed the title ~~fix: change maybe_parse preventing parser from erroring on recursion limit~~ fix: maybe_parse preventing parser from erroring on recursion limit Oct 21, 2024

wugeer pushed a commit to wugeer/sqlparser-rs that referenced this pull request Nov 5, 2024

fix for maybe_parse preventing parser from erroring on recursion limit (

0c8090c

apache#1464)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: `maybe_parse` preventing parser from erroring on recursion limit #1464

fix: `maybe_parse` preventing parser from erroring on recursion limit #1464

tomershaniii commented Oct 9, 2024 •

edited

Loading

iffyio left a comment

tomershaniii commented Oct 14, 2024 •

edited

Loading

alamb left a comment •

edited

Loading

alamb Oct 16, 2024

alamb Oct 16, 2024

alamb Oct 16, 2024

alamb commented Oct 16, 2024

coveralls commented Oct 16, 2024 •

edited

Loading

iffyio left a comment

tomershaniii commented Oct 17, 2024

tomershaniii commented Oct 20, 2024

alamb commented Oct 20, 2024

tomershaniii commented Oct 21, 2024

alamb commented Oct 21, 2024

alamb commented Oct 21, 2024

fix: maybe_parse preventing parser from erroring on recursion limit #1464

fix: maybe_parse preventing parser from erroring on recursion limit #1464

Conversation

tomershaniii commented Oct 9, 2024 • edited Loading

iffyio left a comment

Choose a reason for hiding this comment

tomershaniii commented Oct 14, 2024 • edited Loading

alamb left a comment • edited Loading

Choose a reason for hiding this comment

alamb Oct 16, 2024

Choose a reason for hiding this comment

alamb Oct 16, 2024

Choose a reason for hiding this comment

alamb Oct 16, 2024

Choose a reason for hiding this comment

alamb commented Oct 16, 2024

coveralls commented Oct 16, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11440876981

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

iffyio left a comment

Choose a reason for hiding this comment

tomershaniii commented Oct 17, 2024

tomershaniii commented Oct 20, 2024

alamb commented Oct 20, 2024

tomershaniii commented Oct 21, 2024

alamb commented Oct 21, 2024

alamb commented Oct 21, 2024

fix: `maybe_parse` preventing parser from erroring on recursion limit #1464

fix: `maybe_parse` preventing parser from erroring on recursion limit #1464

tomershaniii commented Oct 9, 2024 •

edited

Loading

tomershaniii commented Oct 14, 2024 •

edited

Loading

alamb left a comment •

edited

Loading

coveralls commented Oct 16, 2024 •

edited

Loading