-
Notifications
You must be signed in to change notification settings - Fork 902
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
Avoid trigger execution when getting result schema #6688
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6688 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 684 684
Lines 42237 42277 +40
Branches 5755 5765 +10
======================================
- Misses 42237 42277 +40 ☔ View full report in Codecov by Sentry. |
cc @iodone |
@@ -59,7 +59,7 @@ class PlanOnlyStatement( | |||
override protected def resultSchema: StructType = { | |||
if (result == null) { | |||
new StructType().add("plan", "string") | |||
} else if (result.isEmpty) { | |||
} else if (result.schema.isEmpty) { |
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.
If I get it correctly, when we go into thie else if branch, that means the query is in planExcludes
? We return the result rather than the plan. Can we add a new flag to distinguish them ? I'm not sure if the result schema of planExcludes
is always empty.
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.
I think we don't need to add a flag to distinguish them, because we handle all cases:
if (result == null) {
...
else if (result.schema.isEmpty) {
...
} else {
result.schema
}
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.
And it is consistent with ExecuteStatement
Lines 53 to 58 in 635c793
override protected def resultSchema: StructType = { | |
if (result == null || result.schema.isEmpty) { | |
new StructType().add("Result", "string") | |
} else { | |
result.schema | |
} |
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.
ah, I see. How about reduce the else if and else by using super.resultSchema()
?
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.
Sounds good, thanks, I will change that
🔍 Description
Issue References 🔗
DataFrame.isEmpty
may trigger execution again, we should avoid it.Describe Your Solution 🔧
Types of changes 🔖
Test Plan 🧪
Behavior Without This Pull Request ⚰️
Behavior With This Pull Request 🎉
Related Unit Tests
Checklist 📝
Be nice. Be informative.