Skip to content

[SPARK-25497][SQL] Limit operation within whole stage codegen should not consume all the inputs #22630

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.

Sign up for GitHub

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

Closed
cloud-fan wants to merge 10 commits into from
Closed

[SPARK-25497][SQL] Limit operation within whole stage codegen should not consume all the inputs #22630

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
13d882a
Limit operation within whole stage codegen should not consume all the…
cloud-fan Oct 3, 2018
0a6c79a
improve doc
cloud-fan Oct 4, 2018
51ce7be
address comment
cloud-fan Oct 5, 2018
2188b27
small update
cloud-fan Oct 5, 2018
e0bc621
address comments
cloud-fan Oct 5, 2018
dc2dfa5
add comment
cloud-fan Oct 5, 2018
e61078b
create a trait for blocking operators
cloud-fan Oct 8, 2018
9114107
address comment
cloud-fan Oct 8, 2018
eac31b2
address comments
cloud-fan Oct 8, 2018
4fc4301
address comment
cloud-fan Oct 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions sql/core/src/main/scala/org/apache/spark/sql/execution/WholeStageCodegenExec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,8 +362,15 @@ trait CodegenSupport extends SparkPlan {
final def limitNotReachedCond: String = {
// InputAdapter is also a leaf node.
val isLeafNode = children.isEmpty || this.isInstanceOf[InputAdapter]
assert(isLeafNode || this.isInstanceOf[BlockingOperatorWithCodegen],
"only leaf nodes and blocking nodes need to call this method in its data producing loop.")
if (isLeafNode || this.isInstanceOf[BlockingOperatorWithCodegen]) {
Copy link
Member

@viirya viirya Oct 8, 2018

Choose a reason for hiding this comment

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

if (!isLeafNode && !this.isInstanceOf[BlockingOperatorWithCodegen])?

val errMsg = "only leaf nodes and blocking nodes need to call 'limitNotReachedCond' " +
Copy link
Contributor

@mgaido91 mgaido91 Oct 8, 2018

Choose a reason for hiding this comment

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

nit: Only

"in its data producing loop."
if (Utils.isTesting) {
throw new IllegalStateException(errMsg)
} else {
logWarning(errMsg)
Copy link
Contributor

@mgaido91 mgaido91 Oct 8, 2018

Choose a reason for hiding this comment

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

nit: shall we also mention to report to the community if seen?

}
}
if (parent.limitNotReachedChecks.isEmpty) {
Copy link
Contributor

@mgaido91 mgaido91 Oct 5, 2018

Choose a reason for hiding this comment

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

Just one thought: since we propagate (correctly) the limitNotReachedChecks to all the children, shall we also enforce that we are calling this on a node which will not propagate the limitNotReachedChecks anymore? We may use the blocking flag proposed in the other comment maybe.

The reason I'd like to do this is to enforce that we are not introducing the same limit condition check more than once, in more than one operator, which would be useless and may cause (small) perf issue. WDYT?

Copy link
Contributor Author

@cloud-fan cloud-fan Oct 6, 2018

Choose a reason for hiding this comment

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

It's not very useful to enforce that. The consequence is so minor and I don't think it's worth the complexity. I want to have a simple and robust framework for the limit optimization first.

Copy link
Contributor

@mgaido91 mgaido91 Oct 7, 2018

Choose a reason for hiding this comment

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

I want to have a simple and robust framework

yes, I 100%, that's why I'd like to early detect all the possible situations which we are not thinking as possible but may happen in corner cases we are not considering. What I am suggesting here is to enforce and fail that for testing only of course, in production we shouldn't do anything similar.

""
} else {
Expand Down