[SPARK-33034][SQL] Support ALTER TABLE in JDBC v2 Table Catalog: add, update type and nullability of columns (Oracle dialect) #29912

MaxGekk · 2020-09-30T12:36:00Z

What changes were proposed in this pull request?

Override the default SQL strings in the Oracle Dialect for:
- ALTER TABLE ADD COLUMN
- ALTER TABLE UPDATE COLUMN TYPE
- ALTER TABLE UPDATE COLUMN NULLABILITY
Add new docker integration test suite jdbc/v2/OracleIntegrationSuite.scala

Why are the changes needed?

In SPARK-24907, we implemented JDBC v2 Table Catalog but it doesn't support some ALTER TABLE at the moment. This PR supports Oracle specific ALTER TABLE.

Does this PR introduce any user-facing change?

Yes

How was this patch tested?

By running new integration test suite:

$ ./build/sbt -Pdocker-integration-tests "test-only *.OracleIntegrationSuite"

…llability of columns

SparkQA · 2020-09-30T13:16:11Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/33890/

SparkQA · 2020-09-30T13:33:05Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/33890/

SparkQA · 2020-09-30T16:13:50Z

Test build #129273 has finished for PR 29912 at commit b7c4ea5.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class OracleIntegrationSuite extends DockerJDBCIntegrationSuite with SharedSparkSession

MaxGekk · 2020-09-30T17:39:15Z

@dongjoon-hyun @maropu @huaxingao May I ask you to review this PR.

huaxingao · 2020-09-30T22:12:35Z

...r-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/OracleIntegrationSuite.scala

+ *  $ export ORACLE_DOCKER_IMAGE_NAME=oracle/database:18.4.0-xe
+ *  $ cd $SPARK_HOME
+ *  $ ./build/sbt -Pdocker-integration-tests
+ *    "test-only org.apache.spark.sql.jdbc.OracleIntegrationSuite"


super nit: you mean "org.apache.spark.sql.jdbc.v2.OracleIntegrationSuite", right?

huaxingao · 2020-09-30T22:55:14Z

LGTM

maropu · 2020-09-30T23:42:40Z

...r-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/OracleIntegrationSuite.scala

+import org.apache.spark.sql.jdbc.{DatabaseOnDocker, DockerJDBCIntegrationSuite}
+import org.apache.spark.sql.test.SharedSparkSession
+import org.apache.spark.sql.types._
+import org.apache.spark.tags.DockerTest


plz remove all the unused imports.

maropu · 2020-09-30T23:47:11Z

...r-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/OracleIntegrationSuite.scala

+ * It has been validated with 18.4.0 Express Edition.
+ */
+@DockerTest
+class OracleIntegrationSuite extends DockerJDBCIntegrationSuite with SharedSparkSession {


Could we run the existing tests of o.a.s.s.jdbc.OracleIntegrationSuite for the V2 JDBC path? I think it is okay to fix it in a separate PR though.

I opened the JIRA ticket for that https://issues.apache.org/jira/browse/SPARK-33066, let's do that separately. Probably, we will need to split the ticket per each supported dialect.

Yea, looks okay. Thanks for opening it, Max!

maropu · 2020-10-01T00:01:44Z

...r-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/OracleIntegrationSuite.scala

+  test("SPARK-33034: alter table ... add column") {
+    withTable("oracle.alt_table") {
+      sql("CREATE TABLE oracle.alt_table (ID STRING) USING _")
+      sql("ALTER TABLE oracle.alt_table ADD COLUMNS (C1 STRING, C2 STRING)")


Does this ALTER command always succeed? What if we add a new column having the same name with the existing column? Anyway, I think it is better to add some tests for error cases.

We test here dialect specific changes:
ALTER TABLE ... ALTER COLUMN vs ALTER TABLE ... ADD
I believe the test for error handling should be added to JDBCTableCatalogSuite since error handling should be generic.

I added negative tests to the common tests #29945

maropu · 2020-10-01T00:03:48Z

...r-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/OracleIntegrationSuite.scala

+  test("SPARK-33034: alter table ... update column type") {
+    withTable("oracle.alt_table") {
+      sql("CREATE TABLE oracle.alt_table (ID INTEGER) USING _")
+      sql("ALTER TABLE oracle.alt_table ALTER COLUMN id TYPE STRING")


ditto: We can alter a column from a string type to a int one?

No:

rg.apache.spark.sql.AnalysisException: Cannot update alt_table field ID: string cannot be cast to int; line 1 pos 0; [info] AlterTable org.apache.spark.sql.execution.datasources.v2.jdbc.JDBCTableCatalog@3ebc40fe, alt_table, RelationV2[ID#25] alt_table, [org.apache.spark.sql.connector.catalog.TableChange$UpdateColumnType@ce035e83] [info] at org.apache.spark.sql.catalyst.analysis.package$AnalysisErrorAt.failAnalysis(package.scala:42) [info] at org.apache.spark.sql.catalyst.analysis.CheckAnalysis.$anonfun$checkAnalysis$31(CheckAnalysis.scala:528) [info] at scala.collection.immutable.List.foreach(List.scala:392) [info] at org.apache.spark.sql.catalyst.analysis.CheckAnalysis.$anonfun$checkAnalysis$1(CheckAnalysis.scala:489)

I will add a check for that.

maropu · 2020-10-01T00:04:38Z

...r-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/OracleIntegrationSuite.scala

+  test("SPARK-33034: alter table ... update column nullability") {
+    withTable("oracle.alt_table") {
+      sql("CREATE TABLE oracle.alt_table (ID STRING NOT NULL) USING _")
+      sql("ALTER TABLE oracle.alt_table ALTER COLUMN ID DROP NOT NULL")


ditto: What if we drop a non-existent column?

I will add negative tests.

SparkQA · 2020-10-05T08:50:26Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34015/

SparkQA · 2020-10-05T09:07:36Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34015/

SparkQA · 2020-10-05T10:41:05Z

Test build #129408 has finished for PR 29912 at commit d124fa7.

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

SparkQA · 2020-10-05T11:21:43Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34018/

SparkQA · 2020-10-05T11:38:40Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34018/

SparkQA · 2020-10-05T12:34:46Z

Test build #129411 has finished for PR 29912 at commit aa121d8.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

MaxGekk · 2020-10-05T17:56:39Z

The last build failure is not related to the changes.

maropu

I've checked that v2/OracleIntegrationSuite passed in my local env. cc: @dongjoon-hyun

cloud-fan · 2020-10-06T13:18:18Z

...r-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/OracleIntegrationSuite.scala

+  override val connectionTimeout = timeout(7.minutes)
+  override def dataPreparation(conn: Connection): Unit = {}
+
+  test("SPARK-33034: alter table ... add column") {


nit: upper case the sql keyword: ALTER TABLE ... add new columns

isn't the SQL parser case agnostic one ? ;-)

yes it is. It's a convention that people upper case the sql keyword when writing sql queries.

cloud-fan · 2020-10-06T13:18:35Z

...r-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/OracleIntegrationSuite.scala

+      expectedSchema = expectedSchema.add("C3", StringType)
+      assert(t.schema === expectedSchema)
+      // Add already existing column
+      intercept[AnalysisException] {


can we check the error message?

cloud-fan · 2020-10-06T13:18:50Z

...r-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/OracleIntegrationSuite.scala

+      }
+    }
+    // Add a column to not existing table
+    intercept[AnalysisException] {


cloud-fan · 2020-10-06T13:19:57Z

...r-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/OracleIntegrationSuite.scala

+      val expectedSchema = new StructType().add("ID", StringType)
+      assert(t.schema === expectedSchema)
+      // Update column type from STRING to INTEGER
+      intercept[AnalysisException] {


cloud-fan · 2020-10-06T13:20:28Z

...r-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/OracleIntegrationSuite.scala

+        sql("ALTER TABLE oracle.alt_table ALTER COLUMN bad_column TYPE DOUBLE")
+      }
+      // Update column to wrong type
+      intercept[AnalysisException] {


this should be ParseException?

cloud-fan

LGTM except some minor comments in tests

SparkQA · 2020-10-06T18:27:10Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34066/

SparkQA · 2020-10-06T18:44:46Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34067/

SparkQA · 2020-10-06T18:50:15Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34066/

SparkQA · 2020-10-06T19:08:40Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34067/

SparkQA · 2020-10-06T20:40:36Z

Test build #129459 has finished for PR 29912 at commit 300980a.

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

SparkQA · 2020-10-06T21:12:08Z

Test build #129460 has finished for PR 29912 at commit 5be3a7d.

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

cloud-fan · 2020-10-07T04:48:54Z

thanks, merging to master!

MaxGekk added 2 commits September 30, 2020 15:14

Support ALTER TABLE in JDBC v2 Table Catalog: add, update type and nu…

1fad3e3

…llability of columns

Add integration test suite

b7c4ea5

probot-autolabeler bot added the SQL label Sep 30, 2020

MaxGekk mentioned this pull request Sep 30, 2020

[SPARK-32523][SQL] Override alter table in JDBC dialects #29581

Closed

huaxingao reviewed Sep 30, 2020

View reviewed changes

maropu reviewed Oct 1, 2020

View reviewed changes

MaxGekk added 2 commits October 5, 2020 10:46

Remove unused imports

5d6d573

Fix the package path to the test suite

d124fa7

MaxGekk added 3 commits October 5, 2020 12:57

Add negative tests

a526470

type STRING -> INTEGER

81cbebf

type STRING -> INTEGER: catch exception

aa121d8

maropu approved these changes Oct 5, 2020

View reviewed changes

cloud-fan reviewed Oct 6, 2020

View reviewed changes

cloud-fan approved these changes Oct 6, 2020

View reviewed changes

MaxGekk mentioned this pull request Oct 6, 2020

[SPARK-33067][SQL][TESTS][FOLLOWUP] Check error messages in JDBCTableCatalogSuite #29957

Closed

MaxGekk added 3 commits October 6, 2020 19:44

Update test titles

44b8ddd

Address Wenchen's comments

300980a

AnalysisException -> ParseException

5be3a7d

cloud-fan approved these changes Oct 7, 2020

View reviewed changes

cloud-fan closed this in aea78d2 Oct 7, 2020

huaxingao mentioned this pull request Oct 8, 2020

[SPARK-33081][SQL] Support ALTER TABLE in JDBC v2 Table Catalog: update type and nullability of columns (DB2 dialect) #29972

Closed

MaxGekk deleted the jdbcv2-oracle-alter-table branch December 11, 2020 20:28

[SPARK-33034][SQL] Support ALTER TABLE in JDBC v2 Table Catalog: add, update type and nullability of columns (Oracle dialect) #29912

[SPARK-33034][SQL] Support ALTER TABLE in JDBC v2 Table Catalog: add, update type and nullability of columns (Oracle dialect) #29912

Uh oh!

Conversation

MaxGekk commented Sep 30, 2020

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

SparkQA commented Sep 30, 2020

Uh oh!

SparkQA commented Sep 30, 2020

Uh oh!

SparkQA commented Sep 30, 2020

Uh oh!

MaxGekk commented Sep 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huaxingao commented Sep 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaxGekk Oct 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 5, 2020

Uh oh!

SparkQA commented Oct 5, 2020

Uh oh!

SparkQA commented Oct 5, 2020

Uh oh!

SparkQA commented Oct 5, 2020

Uh oh!

SparkQA commented Oct 5, 2020

Uh oh!

SparkQA commented Oct 5, 2020

Uh oh!

MaxGekk commented Oct 5, 2020

Uh oh!

maropu left a comment

Choose a reason for hiding this comment

Uh oh!

cloud-fan Oct 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan left a comment

MaxGekk Oct 5, 2020 •

edited

Loading

cloud-fan Oct 6, 2020 •

edited

Loading