-
Notifications
You must be signed in to change notification settings - Fork 28.1k
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
[SPARK-27439][SQL] Use analyzed plan when explaining Dataset #24415
Conversation
Test build #104747 has finished for PR 24415 at commit
|
// Because views are possibly resolved in the analyzed plan of this dataset. We use analyzed | ||
// plan in `ExplainCommand`, for consistency. Otherwise, the plans shown by explain command | ||
// might be inconsistent with the evaluated data of this dataset. | ||
val explain = ExplainCommand(queryExecution.analyzed, extended = extended) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for pinging me, @viirya . I like this PR. First of all, could you add a test case for this please? I'll take a look this more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I originally thought manual test might be enough. Will add one unit test soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. Yes. This is important fix. I want to prevent the future regressions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added an unit test. Thanks for review.
Test build #104771 has finished for PR 24415 at commit
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, LGTM. Merged to master.
Thank you, @viirya and @HyukjinKwon.
Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me too.
This is reverted due to the regression. Please see the JIRA for the detail. |
Thanks @dongjoon-hyun. I will look into how to fix it. |
Yes, it looks it should be reverted, and pass the logical plan as is. Thanks for quick action, @dongjoon-hyun. |
What changes were proposed in this pull request?
Because a review is resolved during analysis when we create a dataset, the content of the view is determined when the dataset is created, not when it is evaluated. Now the explain result of a dataset is not correctly consistent with the collected result of it, because we use pre-analyzed logical plan of the dataset in explain command. The explain command will analyzed the logical plan passed in. So if a view is changed after the dataset was created, the plans shown by explain command aren't the same with the plan of the dataset.
Before:
After:
How was this patch tested?
Manually test and unit test.