Skip to content
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-22932] [SQL] Refactor AnalysisContext #20127

Closed
wants to merge 1 commit into from

Conversation

gatorsmile
Copy link
Member

What changes were proposed in this pull request?

Add a reset function to ensure the state in AnalysisContext is per-query.

How was this patch tested?

The existing test cases

@gatorsmile
Copy link
Member Author

cc @cloud-fan @jiangxb1987

@SparkQA
Copy link

SparkQA commented Dec 31, 2017

Test build #85557 has finished for PR 20127 at commit f158a95.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@kiszk
Copy link
Member

kiszk commented Dec 31, 2017

retest this please

@SparkQA
Copy link

SparkQA commented Dec 31, 2017

Test build #85562 has finished for PR 20127 at commit f158a95.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@kiszk
Copy link
Member

kiszk commented Jan 1, 2018

retest this please

@SparkQA
Copy link

SparkQA commented Jan 1, 2018

Test build #85570 has finished for PR 20127 at commit f158a95.

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

Copy link
Member

@viirya viirya left a comment

Choose a reason for hiding this comment

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

I think AnalysisContext is already per-query for now, because only withAnalysisContext can change it.

Seems this just adds another safety guard.

}
}

private def executeSameContext(plan: LogicalPlan): LogicalPlan = super.execute(plan)
Copy link
Member

Choose a reason for hiding this comment

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

executeWithSameContext?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

@@ -70,6 +71,8 @@ object AnalysisContext {
}

def get: AnalysisContext = value.get()
def reset(): Unit = value.remove()
Copy link
Member

Choose a reason for hiding this comment

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

private

Copy link
Member Author

Choose a reason for hiding this comment

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

Will be resolved by the future PR.

@jiangxb1987
Copy link
Contributor

lgtm

@gatorsmile
Copy link
Member Author

Thanks! Merged to master

@asfgit asfgit closed this in e0c090f Jan 2, 2018
@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Jan 2, 2018

Hi, @gatorsmile .
This PR seems to land into only master, but is marked as 2.3.0 in JIRA.
Could you merge this into branch-2.3 or fix the version into 2.4 or 3.0?

asfgit pushed a commit that referenced this pull request Jan 3, 2018
## What changes were proposed in this pull request?
Add a `reset` function to ensure the state in `AnalysisContext ` is per-query.

## How was this patch tested?
The existing test cases

Author: gatorsmile <gatorsmile@gmail.com>

Closes #20127 from gatorsmile/refactorAnalysisContext.
@gatorsmile
Copy link
Member Author

Yeah, I manually merged it to 2.3 branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants