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

thomastechs · 2016-01-26T02:42:47Z

Adding a getJDBCType() method to the JdbcDialects.scala which would create a VARCHAR type for Oracle create table operations.Currently the type TEXT is throwing exceptions which is incompatible to Oracle

thomastechs · 2016-01-26T02:45:44Z

@yhuai fyr

yhuai · 2016-01-26T15:22:31Z

Can you also create a pr for master?

Can you add a docker integration test (like MySQLIntegrationSuite)?

yhuai · 2016-01-26T15:22:36Z

test this please

SparkQA · 2016-01-26T15:41:06Z

Test build #50107 has finished for PR 10912 at commit a479fce.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

yhuai · 2016-01-26T16:30:47Z

please fix the style error

Updated the style error-remove whitespace

thomastechs · 2016-01-27T05:41:49Z

@yhuai The style check error has been fixed and committed.

yhuai · 2016-01-27T05:51:46Z

Can you add a test? Also, can you create a pr for master?

Adding test case for the getJDBCType() method for the datatype-StringType as a part of OracleDialect

thomastechs · 2016-01-27T06:47:26Z

@yhuai Added the testcase for 1.4. For the master PR, would it be good to raise another bug in reference to this and upload the fix as a new PR for that bug.? In that case, we could close this quickly (once tests passed) , and release this fix for Jose who raised this bug, for his purpose.

yhuai · 2016-01-27T06:51:13Z

sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala

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.

yhuai · 2016-01-27T06:53:25Z

Please create a PR against master. You do not need to create a new jira. In the title of the new PR, you just use the same JIRA number. For this PR, it is better to add [BRANCH-1.4] in the title.

yhuai · 2016-01-27T06:54:39Z

btw, just want to double check. Have you tested your fix with Oracle?

yhuai · 2016-01-27T06:56:34Z

Also, I'd like to get the change in master first. Thanks.

thomastechs · 2016-01-27T07:04:05Z

@yhuai Yes;tested. So, as you mentioned, will create a new PR against master with the OracleIntegrationSuite.

yhuai · 2016-01-27T17:57:39Z

test this please

SparkQA · 2016-01-27T18:12:48Z

Test build #50199 has finished for PR 10912 at commit 06bb080.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

thomastechs · 2016-01-27T19:17:49Z

@yhuai Can the below line wrap pass the style check?
assert(oracleDialect.getJDBCType(StringType)
.map(_.databaseTypeDefinition).get == "VARCHAR2(255)")

thomastechs · 2016-01-27T19:18:15Z

Read it as 2 spaces before .map

yhuai · 2016-01-27T21:30:40Z

If you look at the link, you will see (https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/50199/consoleFull)

Scalastyle checks failed at following occurrences:
[error] /home/jenkins/workspace/SparkPullRequestBuilder/sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala:450: File line length exceeds 100 characters
[error] (sql/test:scalastyle) errors exist
[error] Total time: 7 s, completed Jan 27, 2016 10:12:08 AM
[error] /home/jenkins/workspace/SparkPullRequestBuilder/sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala:450: File line length exceeds 100 characters
[error] (sql/test:scalastyle) errors exist
[error] Total time: 6 s, completed Jan 27, 2016 10:12:47 AM
[error] Got a return code of 1 on line 133 of the run-tests script.

Also, I do not think the test in this PR will work. Please put that in the OracleIntegrationSuite in the PR for master.

thomastechs · 2016-01-28T15:32:17Z

@yhuai working on the OracleIntegrationSuite (In the process of docker creation using Oracle linux)
Regarding the style check, I had already looked into the style check error which mentioned 100 chars per line. So thought of doing a double check with you by splitting assert statement like I mentioned earlier; would be a right pass for the style check.

thomastechs · 2016-02-19T03:50:46Z

@yhuai and @JoshRosen
Sorry for the delay. I was working with @jayadevanmurali to bring up the docker based test suite for master branch.
We are able to create the test case with OracleIntegrationSuite and successfully tested this fix in our local environment.The version we used is Oracle 11g:, and used the docker image from wnameless/oracle-xe-11g.
We tried to build using maven repository for the ojdbc jar required for testing this.Added the dependency like below in the pom file (docker integration project).
<dependency> <groupId>com.oracle</groupId> <artifactId>ojdbc6</artifactId> <version>11.2.0.2.0</version> <scope>test</scope> </dependency>

But there was no corresponding ojdbc jar in the maven repository. So we had to manually download it to the local and build it.
So, if we create a pull request for the master branch fix, how these jar to be uploaded?
Please let us know your thoughts.

thomastechs · 2016-02-22T18:11:13Z

@yhuai
Created the PR https://github.com/apache/spark/pull/11306/files for the master branch

Updated for the style fix.

thomastechs · 2016-02-29T05:52:36Z

@yhuai The style check error for this PR has been fixed in the last commit. This is for branch 1.4. You may request a retest please.

yhuai · 2016-02-29T07:02:33Z

sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala

+    assert(oracleDialect.getJDBCType(StringType).
+      map(_.databaseTypeDefinition).get == "VARCHAR2(255)")
+  }
+


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

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.

yhuai · 2016-02-29T18:02:31Z

test this please

yhuai · 2016-02-29T18:08:04Z

sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala

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


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.

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

SparkQA · 2016-02-29T20:35:10Z

Test build #52189 has finished for PR 10912 at commit b46f1b3.

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

yhuai · 2016-02-29T20:37:22Z

sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala

    assert(agg.getCatalystType(1, "", 1, null) == Some(StringType))
  }

+  test("OracleDialect type mapping") {


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

yhuai · 2016-02-29T20:37:57Z

Merging to 1.4 and 1.5

… map string datatypes to Oracle VARCHAR datatype Adding a getJDBCType() method to the JdbcDialects.scala which would create a VARCHAR type for Oracle create table operations.Currently the type TEXT is throwing exceptions which is incompatible to Oracle Author: thomastechs <[email protected]> Closes #10912 from thomastechs/thomastechs-12941-branch.

… map string datatypes to Oracle VARCHAR datatype Adding a getJDBCType() method to the JdbcDialects.scala which would create a VARCHAR type for Oracle create table operations.Currently the type TEXT is throwing exceptions which is incompatible to Oracle Author: thomastechs <[email protected]> Closes #10912 from thomastechs/thomastechs-12941-branch. (cherry picked from commit 9d8404b) Signed-off-by: Yin Huai <[email protected]> Conflicts: sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala

yhuai · 2016-02-29T20:56:28Z

ok I have merged it into branch 1.4 and branch 1.5. Can you close this PR? Thanks!

Update for [SPARK-12941][SQL]

a479fce

Adding a getJDBCType() method to the JdbcDialects.scala which would create a VARCHAR type for Oracle create table operations.Currently the type TEXT is throwing exceptions which is incompatible to Oracle

thomastechs mentioned this pull request Jan 26, 2016

[SPARK-12941][SQL] Spark-SQL JDBC Oracle dialect fails to map string datatypes to Oracle VARCHAR datatype thomastechs/spark#1

Open

Update JdbcDialects.scala

e2e842d

Updated the style error-remove whitespace

Update for [SPARK-12941][SQL] JDBCSuite.scala

06bb080

Adding test case for the getJDBCType() method for the datatype-StringType as a part of OracleDialect

yhuai reviewed Jan 27, 2016
View reviewed changes

thomastechs changed the title ~~[SPARK-12941][SQL] Spark-SQL JDBC Oracle dialect fails to map string datatypes to Oracle VARCHAR datatype~~ [SPARK-12941][SQL][BRANCH-1.4] Spark-SQL JDBC Oracle dialect fails to map string datatypes to Oracle VARCHAR datatype Jan 27, 2016

Update JDBCSuite.scala [SPARK-12941][Branch-1.4]

c92064d

Updated for the style fix.

yhuai reviewed Feb 29, 2016
View reviewed changes

Update JDBCSuite.scala

b46f1b3

yhuai reviewed Feb 29, 2016
View reviewed changes

asfgit closed this in c37bbb3 Mar 1, 2016

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

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

Uh oh!

Conversation

thomastechs commented Jan 26, 2016

Uh oh!

thomastechs commented Jan 26, 2016

Uh oh!

yhuai commented Jan 26, 2016

Uh oh!

yhuai commented Jan 26, 2016

Uh oh!

SparkQA commented Jan 26, 2016

Uh oh!

yhuai commented Jan 26, 2016

Uh oh!

thomastechs commented Jan 27, 2016

Uh oh!

yhuai commented Jan 27, 2016

Uh oh!

thomastechs commented Jan 27, 2016

Uh oh!

yhuai Jan 27, 2016

Choose a reason for hiding this comment

Uh oh!

yhuai commented Jan 27, 2016

Uh oh!

yhuai commented Jan 27, 2016

Uh oh!

yhuai commented Jan 27, 2016

Uh oh!

thomastechs commented Jan 27, 2016

Uh oh!

yhuai commented Jan 27, 2016

Uh oh!

SparkQA commented Jan 27, 2016

Uh oh!

thomastechs commented Jan 27, 2016

Uh oh!

thomastechs commented Jan 27, 2016

Uh oh!

yhuai commented Jan 27, 2016

Uh oh!

thomastechs commented Jan 28, 2016

Uh oh!

thomastechs commented Feb 19, 2016

Uh oh!

thomastechs commented Feb 22, 2016

Uh oh!

thomastechs commented Feb 29, 2016

Uh oh!

yhuai Feb 29, 2016

Choose a reason for hiding this comment

Uh oh!

thomastechs Feb 29, 2016

Choose a reason for hiding this comment

Uh oh!

yhuai commented Feb 29, 2016

Uh oh!

yhuai Feb 29, 2016

Choose a reason for hiding this comment

Uh oh!

thomastechs Mar 1, 2016

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Feb 29, 2016

Uh oh!

yhuai Feb 29, 2016

Choose a reason for hiding this comment

Uh oh!

yhuai commented Feb 29, 2016

Uh oh!

yhuai commented Feb 29, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development