Skip to content

[SPARK-21338][SQL][FOLLOW-UP] Implement isCascadingTruncateTable() method in AggregatedDialect #19286

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.

Sign up for GitHub

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

Jump to bottom
Closed
viirya wants to merge 3 commits into from
Closed

[SPARK-21338][SQL][FOLLOW-UP] Implement isCascadingTruncateTable() method in AggregatedDialect #19286

viirya wants to merge 3 commits into from

Conversation

@viirya
Copy link
Member

@viirya viirya commented Sep 20, 2017

What changes were proposed in this pull request?

The implemented isCascadingTruncateTable in AggregatedDialect is wrong. When no dialect claims cascading, once there is an unknown cascading truncate in the dialects, we should return unknown cascading, instead of false.

How was this patch tested?

Added test.

@viirya
Copy link
Member Author

viirya commented Sep 20, 2017

cc @gatorsmile @huaxingao

gatorsmile
gatorsmile reviewed Sep 20, 2017
View reviewed changes
sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala Outdated
}
override def isCascadingTruncateTable(): Option[Boolean] = Some(false)
}, testH2Dialect))
assert(agg2.isCascadingTruncateTable() === None)
Copy link
Member

@gatorsmile gatorsmile Sep 20, 2017

Choose a reason for hiding this comment

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

Let us add test cases to enumerate all the combinations of None, true and false

Copy link
Member Author

@viirya viirya Sep 20, 2017

Choose a reason for hiding this comment

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

Added a test case for it.

@SparkQA
Copy link

SparkQA commented Sep 20, 2017

Test build #81973 has finished for PR 19286 at commit d4fadbb.

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

@SparkQA
Copy link

SparkQA commented Sep 20, 2017

Test build #81965 has finished for PR 19286 at commit 803b196.

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

@viirya
Copy link
Member Author

viirya commented Sep 20, 2017

retest this please.

@SparkQA
Copy link

SparkQA commented Sep 20, 2017

Test build #81976 has finished for PR 19286 at commit d4fadbb.

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

@viirya
Copy link
Member Author

viirya commented Sep 20, 2017

retest this please.

@SparkQA
Copy link

SparkQA commented Sep 20, 2017

Test build #81985 has finished for PR 19286 at commit d4fadbb.

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

@viirya
Copy link
Member Author

viirya commented Sep 21, 2017

@gatorsmile Does the added test look good for you? Thanks.

@viirya
Copy link
Member Author

viirya commented Sep 23, 2017

ping @gatorsmile

gatorsmile
gatorsmile reviewed Sep 23, 2017
View reviewed changes
sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala Outdated

val expectedCascading = Seq(Some(true), Some(true), None, Some(true), Some(false), None)

dialectCombination.zip(expectedCascading).foreach { case (dialects, cascading) =>
Copy link
Member

@gatorsmile gatorsmile Sep 23, 2017

Choose a reason for hiding this comment

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

Could we combine dialectCombination and expectedCascading together? Or we can create a separate helper function?

gatorsmile
gatorsmile reviewed Sep 23, 2017
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/jdbc/AggregatedDialect.scala Outdated
if (cascading.getOrElse(false)) {
cascading
} else {
if (dialects.exists(_.isCascadingTruncateTable().isEmpty)) {
Copy link
Member

@gatorsmile gatorsmile Sep 23, 2017

Choose a reason for hiding this comment

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

combine line 51 and 52?

gatorsmile
gatorsmile reviewed Sep 23, 2017
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/jdbc/AggregatedDialect.scala Outdated
// If any dialect claims cascading truncate, this dialect is also cascading truncate.
// Otherwise, if any dialect has unknown cascading truncate, this dialect is also unknown.
val cascading = dialects.flatMap(_.isCascadingTruncateTable()).reduceOption(_ || _)
if (cascading.getOrElse(false)) {
Copy link
Member

@gatorsmile gatorsmile Sep 23, 2017

Choose a reason for hiding this comment

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

Use case-match?

Copy link
Member Author

@viirya viirya Sep 24, 2017

Choose a reason for hiding this comment

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

Done.

@dilipbiswal
Copy link
Contributor

dilipbiswal commented Sep 23, 2017

@viirya Hey simon, thanks for catching this. Will it be little easier to follow if we wrote like this ?

override def isCascadingTruncateTable(): Option[Boolean] = {
   def compute(left: Option[Boolean], right: Option[Boolean]): Option[Boolean] = {
     (left, right) match {
       case (_, Some(true)) => Some(true)
       case (Some(true), _) => Some(true)
       case (Some(false), Some(false)) => Some(false)
       case (_, _) => None
     }
   }
   // If any dialect claims cascading truncate, this dialect is also cascading truncate.
   // Otherwise, if any dialect has unknown cascading truncate, this dialect is also unknown.
   dialects.map(_.isCascadingTruncateTable()).reduce(compute(_, _))

@viirya
Copy link
Member Author

viirya commented Sep 24, 2017

@dilipbiswal Thanks for the suggestion. However, it looks more complicated, IMO.

@dilipbiswal
Copy link
Contributor

dilipbiswal commented Sep 24, 2017

@viirya No problem. The newer version you have looks clean as well.

@SparkQA
Copy link

SparkQA commented Sep 24, 2017

Test build #82123 has finished for PR 19286 at commit 7e5a57c.

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

@gatorsmile
Copy link
Member

gatorsmile commented Sep 24, 2017

LGTM

@gatorsmile
Copy link
Member

gatorsmile commented Sep 24, 2017

Thanks! Merged to master

@asfgit asfgit closed this in 2274d84 Sep 24, 2017
@viirya viirya deleted the SPARK-21338-followup branch December 27, 2023 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gatorsmile gatorsmile gatorsmile left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@viirya @SparkQA @dilipbiswal @gatorsmile