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

tomershaniii commented 1 month ago

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.

tomershaniii commented 1 month ago

@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 commented 1 month ago

Looks like a CI check is failing on this PR

coveralls commented 1 month ago

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%
<!--	Total:	277	279	99.28%	-->

Files with Coverage Reduction	New Missed Lines	%
tests/sqlparser_common.rs	4	89.6%
<!--	Total:	4		-->

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

💛 - Coveralls

tomershaniii commented 1 month ago

CI issues fixed, please re-run

tomershaniii commented 1 month ago

Build broken by another commit, please re-run CI

alamb commented 1 month ago

🤔 CI is still failing unfortunately

tomershaniii commented 1 month ago

yet another rebase fix @alamb plz...

alamb commented 1 month ago

🚀

alamb commented 1 month ago

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

apache / datafusion-sqlparser-rs