The CLI panics when passing an invalid explain query

[comphead]

Which issue does this PR close?

Closes #4378 .

Rationale for this change

What changes are included in this PR?

Replace panics in favor if Error

Are these changes tested?

Are there any user-facing changes?

[comphead]

Hi @HaoYang670 please check the PR
But tbh the optimizer doesn't respect errors now so the error message looks like

DataFusion CLI v14.0.0
❯ explain explain select 1;
Internal("Unsupported logical plan: Explain must be root of the plan")

[HaoYang670]

Could we add a test for this?

[HaoYang670]

Hi @HaoYang670 please check the PR But tbh the optimizer doesn't respect errors now so the error message looks like

DataFusion CLI v14.0.0
❯ explain explain select 1;
Internal("Unsupported logical plan: Explain must be root of the plan")

Thank you for your contribution @comphead .
This is because the error is generated in the logical optimizer, but it is ignored as the configuration datafusion.optimizer.skip_failed_rules is set to `true by default:
https://github.com/apache/arrow-datafusion/blob/49166ea55f317722ab7a37fbfc253bcd497c1672/datafusion/optimizer/src/optimizer.rs#L248-L265

And the error message printed in CLI is created when generating physical plan:
https://github.com/apache/arrow-datafusion/blob/49166ea55f317722ab7a37fbfc253bcd497c1672/datafusion/core/src/physical_plan/planner.rs#L1093-L1095

Generally, we don't want to return an internal error to user, because it means that there are bugs in our project.
For the expain explain ... query, if it is invalid, we should return an error when parsing it, instead of when optimizing it.

cc @andygrove. Should this bug be fixed in sqlparser-rs or in datafusion?

[jackwener]

Agree with @HaoYang670 , look like it should be fixed in sqlparser-rs.

It's error during parse.

[andygrove]

I agree that the parser bug should be fixed in sqlparser-rs. It would also be nice to have a unit test for this change.

[comphead]

Thanks @HaoYang670 @andygrove @jackwener added test

[alamb]

Thank you @HaoYang670

[ursabot]

Benchmark runs are scheduled for baseline = f8ec401 and contender = 0253459. 0253459 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java