Skip to content

Conversation

@ScrapCodes
Copy link
Member

@ScrapCodes ScrapCodes commented Oct 13, 2020

What changes were proposed in this pull request?

Override the default SQL strings for:
ALTER TABLE UPDATE COLUMN TYPE
ALTER TABLE UPDATE COLUMN NULLABILITY
in the following MySQL JDBC dialect according to official documentation.
Write MySQL integration tests for JDBC.

Why are the changes needed?

Improved code coverage and support mysql dialect for jdbc.

Does this PR introduce any user-facing change?

Yes, Support ALTER TABLE in JDBC v2 Table Catalog: add, update type and nullability of columns (MySQL dialect)

How was this patch tested?

Added tests.

@ScrapCodes ScrapCodes changed the title [SPARK-33095] Support ALTER TABLE in JDBC v2 Table Catalog: add, update type and nullability of columns (MySQL dialect) [SPARK-33095][SQL] Support ALTER TABLE in JDBC v2 Table Catalog: add, update type and nullability of columns (MySQL dialect) Oct 13, 2020
@SparkQA
Copy link

SparkQA commented Oct 13, 2020

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

@SparkQA
Copy link

SparkQA commented Oct 13, 2020

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

@SparkQA
Copy link

SparkQA commented Oct 13, 2020

Test build #129739 has finished for PR 30025 at commit d495611.

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

@ScrapCodes ScrapCodes force-pushed the mysql-dialect branch 2 times, most recently from 78e33ec to ba7ebdb Compare October 14, 2020 07:55
@SparkQA
Copy link

SparkQA commented Oct 14, 2020

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

@SparkQA
Copy link

SparkQA commented Oct 14, 2020

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

@SparkQA
Copy link

SparkQA commented Oct 14, 2020

Test build #129750 has finished for PR 30025 at commit ba7ebdb.

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

@SparkQA
Copy link

SparkQA commented Oct 15, 2020

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

@SparkQA
Copy link

SparkQA commented Oct 15, 2020

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

@SparkQA
Copy link

SparkQA commented Oct 15, 2020

Test build #129823 has finished for PR 30025 at commit 1ee7cd6.

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


import org.apache.spark.SparkConf
import org.apache.spark.sql.AnalysisException
import org.apache.spark.sql.catalyst.parser.ParseException
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems this is not needed?

import org.apache.spark.sql.catalyst.parser.ParseException
import org.apache.spark.sql.execution.datasources.v2.jdbc.JDBCTableCatalog
import org.apache.spark.sql.jdbc.{DatabaseOnDocker, DockerJDBCIntegrationSuite}
import org.apache.spark.sql.test.SharedSparkSession
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed?

tableName: String,
columnName: String,
newDataType: String): String = {
s"ALTER TABLE $tableName MODIFY COLUMN $columnName $newDataType"
Copy link
Contributor

Choose a reason for hiding this comment

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

$columnName -> ${quoteIdentifier(columnName)}
please see #30041

@huaxingao
Copy link
Contributor

also cc @cloud-fan @MaxGekk

@SparkQA
Copy link

SparkQA commented Oct 16, 2020

Test build #129880 has finished for PR 30025 at commit dfd6d4b.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Oct 16, 2020

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

@SparkQA
Copy link

SparkQA commented Oct 16, 2020

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

@ScrapCodes
Copy link
Member Author

I have updated. First getting schema from the table and taking the type information from the schema and then passing type to alter table update nullability.

Hi @huaxingao, thanks for the help ! Please take a look again.

@cloud-fan thank you for the suggestion, Please take a look again !

@SparkQA
Copy link

SparkQA commented Oct 21, 2020

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

@SparkQA
Copy link

SparkQA commented Oct 21, 2020

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

@SparkQA
Copy link

SparkQA commented Oct 21, 2020

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

@SparkQA
Copy link

SparkQA commented Oct 21, 2020

Test build #130086 has finished for PR 30025 at commit c7f4113.

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

@SparkQA
Copy link

SparkQA commented Oct 21, 2020

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

@SparkQA
Copy link

SparkQA commented Oct 21, 2020

Test build #130091 has finished for PR 30025 at commit 230fed8.

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

JdbcUtils.alterTable(conn, getTableName(ident), changes, options)
val optionsWithTableName = new JDBCOptions(
options.parameters + (JDBCOptions.JDBC_TABLE_NAME -> getTableName(ident)))
val tableSchema: StructType = JdbcUtils.getSchemaOption(conn, optionsWithTableName).get
Copy link
Contributor

Choose a reason for hiding this comment

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

ah now I get what you mean. The AlterTable logical plan does have the table schema, but the catalog API doesn't pass it in. It's not possible to change the catalog API at this point, and it's also not worthy to add an extra table lookup here just to support update nullability in MySQL.

I think the first version is fine. Sorry for the back and forth!

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks will do so ! 👍

@SparkQA
Copy link

SparkQA commented Oct 22, 2020

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

@SparkQA
Copy link

SparkQA commented Oct 22, 2020

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

@SparkQA
Copy link

SparkQA commented Oct 22, 2020

Test build #130138 has finished for PR 30025 at commit 3fc4132.

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

@cloud-fan
Copy link
Contributor

thanks, merging to master!

@cloud-fan cloud-fan closed this in 8cae7f8 Oct 22, 2020
@ScrapCodes
Copy link
Member Author

Thanks a lot @cloud-fan :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants