-
Notifications
You must be signed in to change notification settings - Fork 29k
[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.
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-25497][SQL] Limit operation within whole stage codegen should not consume all the inputs #22630
Changes from 1 commit
13d882a
0a6c79a
51ce7be
2188b27
e0bc621
dc2dfa5
e61078b
9114107
eac31b2
4fc4301
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -360,6 +360,10 @@ trait CodegenSupport extends SparkPlan { | |
| * limit-not-reached checks. | ||
| */ | ||
| 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 (parent.limitNotReachedChecks.isEmpty) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just one thought: since we propagate (correctly) the 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 { | ||
|
|
@@ -368,6 +372,29 @@ trait CodegenSupport extends SparkPlan { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * A special kind of operators which support whole stage codegen. Blocking means these operators | ||
| * will consume all the inputs first, before producing output. Typical blocking operators are | ||
| * sort and aggregate. | ||
| */ | ||
| trait BlockingOperatorWithCodegen extends CodegenSupport { | ||
|
|
||
| // Blocking operators usually have some kind of buffer to keep the data before producing them, so | ||
| // then don't to copy its result even if its child does. | ||
| override def needCopyResult: Boolean = false | ||
|
|
||
| // Blocking operators always consume all the input first, so its upstream operators don't need a | ||
| // stop check. | ||
| override def needStopCheck: Boolean = false | ||
|
|
||
| // Blocking operators need to consume all the inputs before producing any output. This means, | ||
| // Limit operator after this blocking operator will never reach its limit during the execution of | ||
| // this blocking operator's upstream operators. Here we override this method to return Nil, so | ||
| // that upstream operators will not generate useless conditions (which are always evaluated to | ||
| // false) for the Limit operators after this blocking operator. | ||
| override def limitNotReachedChecks: Seq[String] = Nil | ||
| } | ||
|
|
||
|
|
||
| /** | ||
| * InputAdapter is used to hide a SparkPlan from a subtree that supports codegen. | ||
|
|
||
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.
nit: shall we do this only if
Utils.isTestingand otherwise just emit a warning maybe?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 good idea!