Skip to content

Conversation

@ulysses-you
Copy link
Contributor

What changes were proposed in this pull request?

Since JIRA SPARK-28346,PR 25111, QueryExecution will copy all node stage-by-stage. This make all node instance twice almost. So we should make all class fields lazy to avoid create more unexpected object.

Why are the changes needed?

Avoid create more unexpected object.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Exists UT.

@ulysses-you
Copy link
Contributor Author

cc @cloud-fan @viirya @gatorsmile

@dongjoon-hyun
Copy link
Member

ok to test

@SparkQA
Copy link

SparkQA commented Nov 18, 2019

Test build #113975 has finished for PR 26565 at commit 0aa4a48.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@cloud-fan
Copy link
Contributor

OK so plan copy is more expensive than we expect. How about we reduce the number of copies? What we really need is to copy the logical plan between analyzer and optimizer, other copies are not really needed, and are just there for consistency.

@ulysses-you
Copy link
Contributor Author

Yeah, it is really expensive, many plan do init action during instance.
I think we just need to copy analyzed plan what is the base plan. And for other case, we should check and copy just like what AdaptiveExecution do before.

val logicalPlan = if (sparkSession.sessionState.conf.adaptiveExecutionEnabled) {
  optimizedPlan.clone() 
} else {
  optimizedPlan
}

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a 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, @ulysses-you , @cloud-fan , @viirya .

@ulysses-you
Copy link
Contributor Author

Thanks for merging!

@ulysses-you ulysses-you deleted the make-val-lazy branch September 17, 2021 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants