Jefffrey commented on code in PR #9330:
URL: https://github.com/apache/arrow-datafusion/pull/9330#discussion_r1501335629
##########
datafusion/core/src/dataframe/mod.rs:
##########
@@ -1044,6 +1044,9 @@ impl DataFrame {
/// # }
/// ```
pub fn explain(self, verbose: bool, analyze: bool) -> Result<DataFrame> {
+ if matches!(self.plan, LogicalPlan::Explain(_)) {
+ return plan_err!("Nested EXPLAINs are not supported");
+ }
Review Comment:
:+1:
##########
datafusion/core/src/dataframe/mod.rs:
##########
@@ -2974,4 +2977,21 @@ mod tests {
Ok(())
}
+ #[tokio::test]
+ async fn recursion_explain() -> Result<()> {
+ let ctx = SessionContext::new();
+ // must be error
+ let mut result = ctx.sql("explain select 1").await?.explain(false,
false);
+ assert!(
+ result.is_err(),
+ "Expected an error, but operation succeeded."
+ );
+ // must be error
+ result = ctx.sql("explain explain select 1").await;
+ assert!(
+ result.is_err(),
+ "Expected an error, but operation succeeded."
+ );
Review Comment:
Just a few notes, you can avoid the redundant comments and the assert
messages, as just doing `assert!(result.is_err())` is descriptive enough.
Usually you would add an assert message to provide additional context, such as
`Should've failed when doing nested explain`, but for a test case like this I
think it's not necessary.
Small things like this can help keep code a little cleaner
:slightly_smiling_face:
(Also test case may be better named as `nested_explain_should_fail` for
example
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
