Skip to content

[SPARK-12941][SQL][BRANCH-1.4] Spark-SQL JDBC Oracle dialect fails to map string datatypes to Oracle VARCHAR datatype #10912

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

Closed
thomastechs wants to merge 5 commits into from
Closed

[SPARK-12941][SQL][BRANCH-1.4] Spark-SQL JDBC Oracle dialect fails to map string datatypes to Oracle VARCHAR datatype #10912

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a479fce
Update for [SPARK-12941][SQL]
thomastechs Jan 25, 2016
e2e842d
Update JdbcDialects.scala
thomastechs Jan 27, 2016
06bb080
Update for [SPARK-12941][SQL] JDBCSuite.scala
thomastechs Jan 27, 2016
c92064d
Update JDBCSuite.scala [SPARK-12941][Branch-1.4]
thomastechs Feb 29, 2016
b46f1b3
Update JDBCSuite.scala
thomastechs Feb 29, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,4 +247,9 @@ case object OracleDialect extends JdbcDialect {
None
}
}

override def getJDBCType(dt: DataType): Option[JdbcType] = dt match {
case StringType => Some(JdbcType("VARCHAR2(255)", java.sql.Types.VARCHAR))
case _ => None
}
}
5 changes: 5 additions & 0 deletions sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -445,4 +445,9 @@ class JDBCSuite extends SparkFunSuite with BeforeAndAfter {
assert(agg.getCatalystType(1, "", 1, null) == Some(StringType))
}

test("OracleDialect type mapping") {
Copy link
Contributor

@yhuai yhuai Feb 29, 2016

Choose a reason for hiding this comment

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

Also, it will be good to make the test name more specific on what it will test.

val oracleDialect = JdbcDialects.get("jdbc:oracle://127.0.0.1/db")
assert(oracleDialect.getJDBCType(StringType).map(_.databaseTypeDefinition).get == "VARCHAR2(255)")
}
Copy link
Contributor

@yhuai yhuai Jan 27, 2016

Choose a reason for hiding this comment

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

I am not sure this test works. Let me be clear. We need to add a docker integration test like MySQLIntegrationSuite. Since spark 1.4 branch does not have the testing infrastructure. I think the the right way to do it is for your new PR (using the same JIRA id in the title) created for master, you add the test. Then, for this PR, we just keep the fix.

Copy link
Contributor

@yhuai yhuai Feb 29, 2016

Choose a reason for hiding this comment

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

OK I see. Originally, I thought it requires a oracle db. Thank you for the explanation.

Then, we should also add this test in the master. Can you submit a PR? We can reuse the jira number.

Copy link
Contributor Author

@thomastechs thomastechs Mar 1, 2016

Choose a reason for hiding this comment

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

Okei @yhuai So I shall submit another PR, for the same JIRA with the updates in the JDBCSuite.scala in the master branch. I shall also close this PR


Copy link
Contributor

@yhuai yhuai Feb 29, 2016

Choose a reason for hiding this comment

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

This test does not work, right? If so, can we remove it?

Copy link
Contributor Author

@thomastechs thomastechs Feb 29, 2016

Choose a reason for hiding this comment

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

This is similar to testcase written for MySQL,for branch 1.4; which is not actually spinning up any MySQL or Oracle instance; but for the test suite purpose, as I believe. So far, builds for this PR for branch 1.4, failed due to style checks.

}