[GitHub] spark pull request #19286: [SPARK-21338][SQL][FOLLOW-UP] Implement isCascadi...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/spark/pull/19286


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19286: [SPARK-21338][SQL][FOLLOW-UP] Implement isCascadi...

[viirya]

Github user viirya commented on a diff in the pull request:

https://github.com/apache/spark/pull/19286#discussion_r140644507
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/jdbc/AggregatedDialect.scala ---
@@ -43,6 +43,17 @@ private class AggregatedDialect(dialects: 
List[JdbcDialect]) extends JdbcDialect
   }
 
   override def isCascadingTruncateTable(): Option[Boolean] = {
-dialects.flatMap(_.isCascadingTruncateTable()).reduceOption(_ || _)
+// 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)) {
--- End diff --

Done.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19286: [SPARK-21338][SQL][FOLLOW-UP] Implement isCascadi...

[gatorsmile]

Github user gatorsmile commented on a diff in the pull request:

https://github.com/apache/spark/pull/19286#discussion_r140638171
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/jdbc/AggregatedDialect.scala ---
@@ -43,6 +43,17 @@ private class AggregatedDialect(dialects: 
List[JdbcDialect]) extends JdbcDialect
   }
 
   override def isCascadingTruncateTable(): Option[Boolean] = {
-dialects.flatMap(_.isCascadingTruncateTable()).reduceOption(_ || _)
+// 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)) {
--- End diff --

Use case-match?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19286: [SPARK-21338][SQL][FOLLOW-UP] Implement isCascadi...

[gatorsmile]

Github user gatorsmile commented on a diff in the pull request:

https://github.com/apache/spark/pull/19286#discussion_r140638141
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/jdbc/AggregatedDialect.scala ---
@@ -43,6 +43,17 @@ private class AggregatedDialect(dialects: 
List[JdbcDialect]) extends JdbcDialect
   }
 
   override def isCascadingTruncateTable(): Option[Boolean] = {
-dialects.flatMap(_.isCascadingTruncateTable()).reduceOption(_ || _)
+// 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)) {
+  cascading
+} else {
+  if (dialects.exists(_.isCascadingTruncateTable().isEmpty)) {
--- End diff --

combine line 51 and 52?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19286: [SPARK-21338][SQL][FOLLOW-UP] Implement isCascadi...

[gatorsmile]

Github user gatorsmile commented on a diff in the pull request:

https://github.com/apache/spark/pull/19286#discussion_r140638128
  
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala 
---
@@ -749,6 +749,34 @@ class JDBCSuite extends SparkFunSuite
 assert(agg.isCascadingTruncateTable() === Some(true))
   }
 
+  test("Aggregated dialects: isCascadingTruncateTable") {
+def genDialect(cascadingTruncateTable: Option[Boolean]): JdbcDialect = 
new JdbcDialect {
+  override def canHandle(url: String): Boolean = true
+  override def getCatalystType(
+sqlType: Int,
+typeName: String,
+size: Int,
+md: MetadataBuilder): Option[DataType] = None
+  override def isCascadingTruncateTable(): Option[Boolean] = 
cascadingTruncateTable
+}
+
+val dialectCombination = Seq(
+  List(genDialect(Some(true)), genDialect(Some(false)), 
genDialect(None)),
+  List(genDialect(Some(true)), genDialect(Some(true)), 
genDialect(None)),
+  List(genDialect(Some(false)), genDialect(Some(false)), 
genDialect(None)),
+  List(genDialect(Some(true)), genDialect(Some(true))),
+  List(genDialect(Some(false)), genDialect(Some(false))),
+  List(genDialect(None), genDialect(None))
+)
+
+val expectedCascading = Seq(Some(true), Some(true), None, Some(true), 
Some(false), None)
+
+dialectCombination.zip(expectedCascading).foreach { case (dialects, 
cascading) =>
--- End diff --

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


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19286: [SPARK-21338][SQL][FOLLOW-UP] Implement isCascadi...

[viirya]

Github user viirya commented on a diff in the pull request:

https://github.com/apache/spark/pull/19286#discussion_r139887244
  
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala 
---
@@ -747,6 +747,19 @@ class JDBCSuite extends SparkFunSuite
 assert(agg.getCatalystType(0, "", 1, null) === Some(LongType))
 assert(agg.getCatalystType(1, "", 1, null) === Some(StringType))
 assert(agg.isCascadingTruncateTable() === Some(true))
+
+val agg2 = new AggregatedDialect(List(new JdbcDialect {
+  override def canHandle(url: String) : Boolean = 
url.startsWith("jdbc:h2:")
+  override def getCatalystType(
+  sqlType: Int, typeName: String, size: Int, md: MetadataBuilder): 
Option[DataType] =
+if (sqlType % 2 == 0) {
+  Some(LongType)
+} else {
+  None
+}
+  override def isCascadingTruncateTable(): Option[Boolean] = 
Some(false)
+}, testH2Dialect))
+assert(agg2.isCascadingTruncateTable() === None)
--- End diff --

Added a test case for it.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19286: [SPARK-21338][SQL][FOLLOW-UP] Implement isCascadi...

[gatorsmile]

Github user gatorsmile commented on a diff in the pull request:

https://github.com/apache/spark/pull/19286#discussion_r139878221
  
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala 
---
@@ -747,6 +747,19 @@ class JDBCSuite extends SparkFunSuite
 assert(agg.getCatalystType(0, "", 1, null) === Some(LongType))
 assert(agg.getCatalystType(1, "", 1, null) === Some(StringType))
 assert(agg.isCascadingTruncateTable() === Some(true))
+
+val agg2 = new AggregatedDialect(List(new JdbcDialect {
+  override def canHandle(url: String) : Boolean = 
url.startsWith("jdbc:h2:")
+  override def getCatalystType(
+  sqlType: Int, typeName: String, size: Int, md: MetadataBuilder): 
Option[DataType] =
+if (sqlType % 2 == 0) {
+  Some(LongType)
+} else {
+  None
+}
+  override def isCascadingTruncateTable(): Option[Boolean] = 
Some(false)
+}, testH2Dialect))
+assert(agg2.isCascadingTruncateTable() === None)
--- End diff --

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


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19286: [SPARK-21338][SQL][FOLLOW-UP] Implement isCascadi...

[viirya]

GitHub user viirya opened a pull request:

https://github.com/apache/spark/pull/19286

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

## 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.


You can merge this pull request into a Git repository by running:

$ git pull https://github.com/viirya/spark-1 SPARK-21338-followup

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/spark/pull/19286.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #19286


commit 803b1961a34d4d9f4c8ebcbe5544dd23fbaa720a
Author: Liang-Chi Hsieh 
Date:   2017-09-20T04:52:08Z

Fix isCascadingTruncateTable for AggregatedDialect.




---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org