[SPARK-19004][SQL] Fix JDBCWriteSuite.testH2Dialect by removing getCatalystType
#16409
Conversation
|
Hi, @gatorsmile . |
|
Test build #70612 has finished for PR 16409 at commit
|
| @@ -44,9 +44,6 @@ class JDBCWriteSuite extends SharedSQLContext with BeforeAndAfter { | |||
|
|
|||
| val testH2Dialect = new JdbcDialect { | |||
| override def canHandle(url: String) : Boolean = url.startsWith("jdbc:h2") | |||
| override def getCatalystType( | |||
There was a problem hiding this comment.
If we get rid of this, how can it pass the test case "Remap types via JdbcDialects". At least, it failed in my local environement.
This overwrite was intentionally introduced for testing JdbcDialect. See the PR: #5498
There was a problem hiding this comment.
Thank you for review, @gatorsmile .
JdbcDialect should handle a few DBMS specific types and should return None for general types. The schema is handled in general in the following fallback logic.
The current implementation is wrong because it returns always StringType for all kind types of columns.
There was a problem hiding this comment.
I know that is wrong, but this customized getCatalystType is used to test whether the customized getCatalystType are taking effects.
There was a problem hiding this comment.
I'll check that again for test.
There was a problem hiding this comment.
For the failure, I see that it's due to complaining the correct result.
List(StructField(THEID,IntegerType,false)) was not empty
ScalaTestFailureLocation: org.apache.spark.sql.jdbc.JDBCSuite$$anonfun$39 at (JDBCSuite.scala:616)
There was a problem hiding this comment.
First of all, it's my bad that I didn't notice that there were two testH2Dialect.
Now, I understand that this is intentional for the specific test case Remap types via JdbcDialects, but it seems to be only valid for JDBCSuite.scala.
I mean we can use a normal JdbcDialect in JDBCWriteSuite because it's a different suite. Actually, while making #15664, I got stuck by this bug.
Is it possible to fix this correctly in JDBCWriterSuite only as this PR proposed?
There was a problem hiding this comment.
uh, I see. you are removing testH2Dialect in JDBCWriteSuite.scala. I am fine about this change. Please update the PR title, and the PR/JIRA description.
There was a problem hiding this comment.
Thank you so much, @gatorsmile .
I update the title and description of this PR and JIRA together.
dongjoon-hyun
changed the title
testH2Dialect by removing getCatalystTypeJDBCWriteSuite.testH2Dialect by removing getCatalystType
|
Thanks! Merging to master. |
|
Thank you for review and merging! |
…tCatalystType` ## What changes were proposed in this pull request? `JDBCSuite` and `JDBCWriterSuite` have their own `testH2Dialect`s for their testing purposes. This PR fixes `testH2Dialect` in `JDBCWriterSuite` by removing `getCatalystType` implementation in order to return correct types. Currently, it always returns `Some(StringType)` incorrectly. Note that, for the `testH2Dialect` in `JDBCSuite`, it's intentional because of the test case `Remap types via JdbcDialects`. ## How was this patch tested? This is a test only update. Author: Dongjoon Hyun <dongjoon@apache.org> Closes apache#16409 from dongjoon-hyun/SPARK-H2-DIALECT.
…tCatalystType` ## What changes were proposed in this pull request? `JDBCSuite` and `JDBCWriterSuite` have their own `testH2Dialect`s for their testing purposes. This PR fixes `testH2Dialect` in `JDBCWriterSuite` by removing `getCatalystType` implementation in order to return correct types. Currently, it always returns `Some(StringType)` incorrectly. Note that, for the `testH2Dialect` in `JDBCSuite`, it's intentional because of the test case `Remap types via JdbcDialects`. ## How was this patch tested? This is a test only update. Author: Dongjoon Hyun <dongjoon@apache.org> Closes apache#16409 from dongjoon-hyun/SPARK-H2-DIALECT.
What changes were proposed in this pull request?
JDBCSuiteandJDBCWriterSuitehave their owntestH2Dialects for their testing purposes.This PR fixes
testH2DialectinJDBCWriterSuiteby removinggetCatalystTypeimplementation in order to return correct types. Currently, it always returnsSome(StringType)incorrectly. Note that, for thetestH2DialectinJDBCSuite, it's intentional because of the test caseRemap types via JdbcDialects.How was this patch tested?
This is a test only update.