[SPARK-10855] [SQL] Add a JDBC dialect for Apache Derby #8982
Conversation
|
;Jenkins, this is ok to test |
|
Test build #43250 has finished for PR 8982 at commit
|
|
Tests seem to have passed cleanly. Any comments? Suggestions for improvements? Thanks. |
| case object DerbyDialect extends JdbcDialect { | ||
| override def canHandle(url: String): Boolean = url.startsWith("jdbc:derby") | ||
| override def getCatalystType( | ||
| sqlType: Int, typeName: String, size: Int, md: MetadataBuilder): Option[DataType] = { |
|
Test build #43436 has finished for PR 8982 at commit
|
…can create Derby tables with DECIMAL columns.
…of a DECIMAL if the max Derby precision is exceeded.
…tion, make an if expression more compact.
| override def canHandle(url: String): Boolean = url.startsWith("jdbc:derby") | ||
| override def getCatalystType( | ||
| sqlType: Int, typeName: String, size: Int, md: MetadataBuilder): Option[DataType] = { | ||
| if (sqlType == Types.REAL) Some(FloatType) else None |
There was a problem hiding this comment.
I have changed the original code so that it matches the pattern followed by the other getCatalystType() overloads: The code returns a Some(FloatType) rather than an Option(FloatType). I am new to programming in Scala, but the former expression is the pattern which I see all over the code. However, Reynold recommended the latter expression. If Option(FloatType) is the correct value to return, then we should probably change all of the other getCatalystType() overloads to return Option(...) rather than Some(...). I would appreciate your guidance on this point. Thanks.
There was a problem hiding this comment.
Thanks for the review comments, Reynold and Michael. Hopefully, my last commit (e6022ed) addresses your concerns:
o I adjusted the indentation of a method signature, per Reynold's advice.
o I simplified an "if" expression, also per Reynold's advice. However, I have a question about this change (see my comment above on the line in question).
Thanks,
-Rick
There was a problem hiding this comment.
In this case (where you are clearly not passing null) they are equivalent.
Option will return None in the case of null, which is why I prefer using
that method of construction.
On Oct 9, 2015 9:28 AM, "Rick Hillegas" notifications@github.com wrote:
In sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala
#8982 (comment):@@ -287,3 +288,30 @@ case object MsSqlServerDialect extends JdbcDialect {
case _ => None
}
}
+
+/**
- * :: DeveloperApi ::
- * Default Apache Derby dialect, mapping real on read
- * and string/byte/short/boolean/decimal on write.
- */
+@DeveloperAPI
+case object DerbyDialect extends JdbcDialect {- override def canHandle(url: String): Boolean = url.startsWith("jdbc:derby")
- override def getCatalystType(
sqlType: Int, typeName: String, size: Int, md: MetadataBuilder): Option[DataType] = {- if (sqlType == Types.REAL) Some(FloatType) else None
I have changed the original code so that it matches the pattern followed
by the other getCatalystType() overloads: The code returns a
Some(FloatType) rather than an Option(FloatType). I am new to programming
in Scala, but the former expression is the pattern which I see all over the
code. However, Reynold recommended the latter expression. If
Option(FloatType) is the correct value to return, then we should probably
change all of the other getCatalystType() overloads to return Option(...)
rather than Some(...). I would appreciate your guidance on this point.
Thanks.—
Reply to this email directly or view it on GitHub
https://github.com/apache/spark/pull/8982/files#r41649575.
There was a problem hiding this comment.
Thanks for the quick response, Michael. Should I adjust the other getCatalystType() overloads to follow the Option(...) pattern? I could do that as part of this pull-request or I could open another JIRA if you think that is worth doing. Thanks.
|
Test build #43471 has finished for PR 8982 at commit
|
|
My latest commit (d56897e) adjusts the Option usage per Reynold and Michael's advice. I believe that I have addressed all issues with this pull request. We can file a new JIRA if we want to harmonize the usage of Option in JdbcDialects.scala. Thanks. |
|
Test build #43478 has finished for PR 8982 at commit
|
|
Test build #43481 has finished for PR 8982 at commit
|
|
Thanks - I've merged this. |
@marmbrus
@rxin
This patch adds a JdbcDialect class, which customizes the datatype mappings for Derby backends. The patch also adds unit tests for the new dialect, corresponding to the existing tests for other JDBC dialects.
JDBCSuite runs cleanly for me with this patch. So does JDBCWriteSuite, although it produces noise as described here: https://issues.apache.org/jira/browse/SPARK-10890
This patch is my original work, which I license to the ASF. I am a Derby contributor, so my ICLA is on file under SVN id "rhillegas": http://people.apache.org/committer-index.html
Touches the following files:
org.apache.spark.sql.jdbc.JdbcDialects
Adds a DerbyDialect.
org.apache.spark.sql.jdbc.JDBCSuite
Adds unit tests for the new DerbyDialect.