Skip to content

Conversation

@beliefer
Copy link
Contributor

@beliefer beliefer commented Dec 9, 2021

What changes were proposed in this pull request?

#34543 provides the parse method parseQuery. So we can reduce the time spent on executing PlanParserSuite by replace parsePlan.

Why are the changes needed?

Improve the performance of PlanParserSuite.

Does this PR introduce any user-facing change?

'No'.
Just update tests.

How was this patch tested?

Jenkins test.

@github-actions github-actions bot added the SQL label Dec 9, 2021
@SparkQA
Copy link

SparkQA commented Dec 9, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50498/

@SparkQA
Copy link

SparkQA commented Dec 9, 2021

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50498/

@SparkQA
Copy link

SparkQA commented Dec 9, 2021

Test build #146023 has finished for PR 34841 at commit 3b08e4a.

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

@beliefer
Copy link
Contributor Author

beliefer commented Dec 9, 2021

ping @cloud-fan

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.

Could you give us some improvement numbers in the PR description, @beliefer ?

So we can reduce the time spent on executing PlanParserSuite by replace parsePlan.

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-37589][SQL] Improve the performance of PlanParserSuite [SPARK-37589][SQL][TESTS] Improve the performance of PlanParserSuite Dec 9, 2021
comparePlans(parsePlan(sqlCommand), plan, checkAnalysis = false)
}

private def assertQueryEqual(query: String, plan: LogicalPlan): Unit = {
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes it hard to write tests as people need to pick between assertEqual and assertQueryEqual.

How much we can speed up this test suite by doing so?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

About tens of milliseconds

@beliefer
Copy link
Contributor Author

beliefer commented Dec 9, 2021

Maybe it not worth.

@beliefer beliefer closed this Dec 9, 2021
@dongjoon-hyun
Copy link
Member

Thank you for closing, @beliefer .

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.

4 participants