Skip to content

[SPARK-32537][SQL][TEST] Add a CTEHintSuite for test coverage - #29359

Closed
LantaoJin wants to merge 5 commits into
apache:masterfrom
LantaoJin:SPARK-32537
Closed

[SPARK-32537][SQL][TEST] Add a CTEHintSuite for test coverage#29359
LantaoJin wants to merge 5 commits into
apache:masterfrom
LantaoJin:SPARK-32537

Conversation

@LantaoJin

@LantaoJinLantaoJin commented Aug 5, 2020

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Add a new test suite CTEHintSuite

Why are the changes needed?

This ticket is to address the below comments to help us understand the test coverage of SQL HINT for CTE.
#29062 (comment)
#29062 (comment)

Does this PR introduce any user-facing change?

No

How was this patch tested?

Add a test suite.

@SparkQA

SparkQA commented Aug 5, 2020

Copy link
Copy Markdown

Test build #127092 has finished for PR 29359 at commit 773eacd.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class CTEHintSuite extends PlanTest with SharedSparkSession with AdaptiveSparkPlanHelper

@LantaoJin

LantaoJin commented Aug 6, 2020

Copy link
Copy Markdown
ContributorAuthor

cc @gatorsmile@cloud-fan

maropu
maropu reviewed Aug 6, 2020
View reviewed changes
Comment threadsql/core/src/test/scala/org/apache/spark/sql/CTEHintSuite.scala Outdated
import org.apache.log4j.Level

import org.apache.spark.sql.catalyst.plans.PlanTest
import org.apache.spark.sql.catalyst.plans.logical.{BROADCAST, HintInfo, Join, JoinHint, Repartition, RepartitionByExpression, ResolvedHint, SHUFFLE_HASH, SHUFFLE_MERGE, SHUFFLE_REPLICATE_NL}

@maropumaropuAug 6, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: I think its okay to fold it: import org.apache.spark.sql.catalyst.plans.logical._

Comment threadsql/core/src/test/scala/org/apache/spark/sql/CTEHintSuite.scala Outdated
import org.apache.spark.sql.execution.adaptive.AdaptiveSparkPlanHelper
import org.apache.spark.sql.test.SharedSparkSession

class CTEHintSuite extends PlanTest with SharedSparkSession with AdaptiveSparkPlanHelper {

@maropumaropuAug 6, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we need AdaptiveSparkPlanHelper?

@LantaoJinLantaoJinAug 6, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I will remove it. I just copy it from JoinHintSuite :P

Comment threadsql/core/src/test/scala/org/apache/spark/sql/CTEHintSuite.scala
Comment threadsql/core/src/test/scala/org/apache/spark/sql/CTEHintSuite.scala
cloud-fan
cloud-fan reviewed Aug 6, 2020
View reviewed changes
Comment threadsql/core/src/test/scala/org/apache/spark/sql/CTEHintSuite.scala
@SparkQA

SparkQA commented Aug 6, 2020

Copy link
Copy Markdown

Test build #127134 has finished for PR 29359 at commit 6f8a2cc.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class CTEHintSuite extends PlanTest with SharedSparkSession

@SparkQA

SparkQA commented Aug 6, 2020

Copy link
Copy Markdown

Test build #127135 has finished for PR 29359 at commit ce94127.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class CTEHintSuite extends QueryTest with SharedSparkSession

maropu
maropu reviewed Aug 6, 2020
View reviewed changes
Comment threadsql/core/src/test/scala/org/apache/spark/sql/CTEHintSuite.scala Outdated

class CTEHintSuite extends QueryTest with SharedSparkSession {

def verifyCoalesceHint(df: DataFrame): Unit = {

@maropumaropuAug 6, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: Coalesce -> Repartition?

@LantaoJinLantaoJinAug 7, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

ResolveCoalesceHints accepts names "COALESCE", "REPARTITION", and "REPARTITION_BY_RANGE".

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveHints.scala

Line 174 in 9a35b93

classResolveCoalesceHints(conf: SQLConf) extendsRule[LogicalPlan] {

@LantaoJinLantaoJinAug 7, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Changed to def verifyCoalesceOrRepartitionHint

maropu
maropu reviewed Aug 6, 2020
View reviewed changes
Comment threadsql/core/src/test/scala/org/apache/spark/sql/CTEHintSuite.scala Outdated
val repartitions = plan collect {
case r: Repartition => r
case r: RepartitionByExpression => r
case _: ResolvedHint => fail("ResolvedHint should not appear after optimize.")

@maropumaropuAug 6, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this error message correct even if the test is for an analyzed plan? https://github.com/apache/spark/pull/29359/files#diff-0d893e4fe9a621d3e084c2913c533e00R38

maropu
maropu reviewed Aug 6, 2020
View reviewed changes
Comment threadsql/core/src/test/scala/org/apache/spark/sql/CTEHintSuite.scala Outdated
sql(s"""
|WITH cte AS (SELECT /*+ REPARTITION(3) */ * FROM t)
|SELECT * FROM cte
""".stripMargin),

@maropumaropuAug 6, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit format:

 checkAnswer(
sql(
s"""
|WITH cte AS (SELECT /*+ REPARTITION(3) */ * FROM t)
|SELECT * FROM cte
|""".stripMargin),
Row(1) :: Nil)

@SparkQA

SparkQA commented Aug 7, 2020

Copy link
Copy Markdown

Test build #127176 has finished for PR 29359 at commit 4256339.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@LantaoJin

LantaoJin commented Aug 7, 2020

Copy link
Copy Markdown
ContributorAuthor

retest this please

@SparkQA

SparkQA commented Aug 7, 2020

Copy link
Copy Markdown

Test build #127187 has finished for PR 29359 at commit 4256339.

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

cloud-fan
cloud-fan reviewed Aug 7, 2020
View reviewed changes
Comment threadsql/core/src/test/scala/org/apache/spark/sql/CTEHintSuite.scala Outdated
}
}

test("SPARK-32237: Hint in CTE") {

@cloud-fancloud-fanAug 7, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actually we can remove it now. It's already covered by Resolve coalesce hint in CTE.

@SparkQA

SparkQA commented Aug 10, 2020

Copy link
Copy Markdown

Test build #127251 has finished for PR 29359 at commit a7c4d32.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@cloud-fan

cloud-fan commented Aug 10, 2020

Copy link
Copy Markdown
Contributor

github action passed, merging to master, thanks!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cloud-fancloud-fancloud-fan left review comments
@maropumaropumaropu approved these changes

Assignees

No one assigned

Labels

SQLTESTS

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@LantaoJin@SparkQA@cloud-fan@maropu@dongjoon-hyun