Skip to content

Show current SQL recursion limit in RecursionLimitExceeded error message #15644

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

Conversation

kumarlokesh
Copy link
Contributor

@kumarlokesh kumarlokesh commented Apr 8, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

  • Added recursion_limit (available through SQL parser options) field to DFParser
    struct to track the current recursion limit
  • Updated error handling to include the current recursion limit value in error messages
  • Ensured consistent error handling for recursion limit across different parsing scenarios:
    • COPY INTO statements
    • General statement parsing
    • Native and sqlparser-rs parser paths
  • Updated error message format would be:
    SQL error: RecursionLimitExceeded (current limit: {recursion_limit})

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added the sql SQL Planner label Apr 8, 2025
@kumarlokesh kumarlokesh force-pushed the improve-error-message-for-sql-parser-recursion-limit branch from 4ed10ff to 93f771e Compare April 8, 2025 20:48
@kumarlokesh kumarlokesh requested a review from comphead April 9, 2025 19:57
@kumarlokesh kumarlokesh force-pushed the improve-error-message-for-sql-parser-recursion-limit branch from 91da480 to b1fbfe8 Compare April 18, 2025 14:35
@kumarlokesh
Copy link
Contributor Author

@comphead updated the PR after resolving merge conflicts.

Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @kumarlokesh The code now looks much more aligned.

perhaps we can factor out

                        self.parser
                             .parse_statement()
                             .map(|stmt| Statement::Statement(Box::from(stmt)))
                             .map_err(|e| match e {
                                 ParserError::RecursionLimitExceeded => {
                                     DataFusionError::SQL(
                                         ParserError::RecursionLimitExceeded,
                                         Some(format!(
                                             " (current limit: {})",
                                             self.options.recursion_limit
                                         )),
                                     )
                                 }
                                 other => DataFusionError::SQL(other, None),
                             })

into helper method

But it can be done in followup PR

@xudong963
Copy link
Member

thanks @kumarlokesh The code now looks much more aligned.

perhaps we can factor out

                        self.parser
                             .parse_statement()
                             .map(|stmt| Statement::Statement(Box::from(stmt)))
                             .map_err(|e| match e {
                                 ParserError::RecursionLimitExceeded => {
                                     DataFusionError::SQL(
                                         ParserError::RecursionLimitExceeded,
                                         Some(format!(
                                             " (current limit: {})",
                                             self.options.recursion_limit
                                         )),
                                     )
                                 }
                                 other => DataFusionError::SQL(other, None),
                             })

into helper method

But it can be done in followup PR

+1 for this to reduce duplicated code

@kumarlokesh
Copy link
Contributor Author

thanks @kumarlokesh The code now looks much more aligned.
perhaps we can factor out

                        self.parser
                             .parse_statement()
                             .map(|stmt| Statement::Statement(Box::from(stmt)))
                             .map_err(|e| match e {
                                 ParserError::RecursionLimitExceeded => {
                                     DataFusionError::SQL(
                                         ParserError::RecursionLimitExceeded,
                                         Some(format!(
                                             " (current limit: {})",
                                             self.options.recursion_limit
                                         )),
                                     )
                                 }
                                 other => DataFusionError::SQL(other, None),
                             })

into helper method
But it can be done in followup PR

+1 for this to reduce duplicated code

@comphead @xudong963 addressed here dea5fe5.

Copy link
Member

@xudong963 xudong963 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xudong963 xudong963 merged commit 392a9dd into apache:main Apr 21, 2025
27 checks passed
@kumarlokesh kumarlokesh deleted the improve-error-message-for-sql-parser-recursion-limit branch April 21, 2025 08:05
nirnayroy pushed a commit to nirnayroy/datafusion that referenced this pull request May 2, 2025
…age (apache#15644)

* Show current SQL recursion limit in RecursionLimitExceeded error message

* use recursion_limit setting from sql-parser-options

* resolve merge conflicts

* move error handling code to helper method
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sql SQL Planner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve SQL parser recursion limit error message
3 participants