[SPARK-21743][SQL][follow-up] top-most limit should not cause memory leak #18993
Conversation
| // Do not enable whole stage codegen for a single limit. | ||
| override def supportCodegen: Boolean = child match { | ||
| case plan: CodegenSupport => plan.supportCodegen | ||
| case _ => false |
There was a problem hiding this comment.
This is wrong, we may have more operators above Limit, so it's not a single Limit.
| override def apply(plan: LogicalPlan): Seq[SparkPlan] = plan match { | ||
| case logical.ReturnAnswer(rootPlan) => rootPlan match { | ||
| case logical.Limit(IntegerLiteral(limit), logical.Sort(order, true, child)) => | ||
| execution.TakeOrderedAndProjectExec(limit, order, child.output, planLater(child)) :: Nil |
There was a problem hiding this comment.
kinda unrelated, remove these logical and execution prefix to shorten the code.
| // query plan are consumed. It's possible that `CollectLimitExec` only consumes a little | ||
| // data from child plan and finishes the query without releasing resources. Here we wrap | ||
| // the child plan with `LocalLimitExec`, to stop the processing of whole stage codegen and | ||
| // trigger the resource releasing work, after we consume `limit` rows. |
There was a problem hiding this comment.
comments updated.
| projection.initialize(0) | ||
| LocalRelation(projectList.map(_.toAttribute), data.map(projection)) | ||
|
|
||
| case Limit(IntegerLiteral(limit), LocalRelation(output, data)) => |
There was a problem hiding this comment.
This is to fix SQLQuerySuite.SPARK-19650: An action on a Command should not trigger a Spark job, limit over local relation should not trigger a spark job.
There was a problem hiding this comment.
This kinda violates the idea that we shouldn't rely on optimization for correctness, but I suppose this is ok.
There was a problem hiding this comment.
technically this is not about correctness, An action on a Command should not trigger a Spark job is also kind of optimization.
There was a problem hiding this comment.
Yeah, you are right about that.
|
LGTM pending jenkins. |
|
Test build #80843 has finished for PR 18993 at commit
|
|
retest this please. |
|
LGTM |
|
Test build #80848 has finished for PR 18993 at commit
|
|
LGTM Thanks! Merging to master. |
5 participants
What changes were proposed in this pull request?
This is a follow-up of #18955 , to fix a bug that we break whole stage codegen for
Limit.How was this patch tested?
existing tests.