[SPARK-28139][SQL] Add v2 ALTER TABLE implementation. #24937

rdblue · 2019-06-22T23:02:05Z

What changes were proposed in this pull request?

Implement ALTER TABLE for v2 tables:

Add AlterTable logical plan and AlterTableExec physical plan
Convert ALTER TABLE parsed plans to AlterTable when a v2 catalog is responsible for an identifier
Validate that columns to alter exist in analyzer checks
Fix nested type handling in CatalogV2Util

How was this patch tested?

Add extensive tests in DataSourceV2SQLSuite

rdblue · 2019-06-23T01:49:53Z

@dongjoon-hyun, this PR implements ALTER TABLE for DataSourceV2. Do you have time to review it?

SparkQA · 2019-06-23T02:04:19Z

Test build #106796 has finished for PR 24937 at commit 9f6f8e9.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
final class AddColumn implements ColumnChange
final class RenameColumn implements ColumnChange
final class UpdateColumnType implements ColumnChange
final class UpdateColumnComment implements ColumnChange
final class DeleteColumn implements ColumnChange
case class UnresolvedRelation(
case class AlterTable(
case class AlterTableExec(

gengliangwang · 2019-06-25T15:36:10Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

how about

import org.apache.spark.sql.catalyst.plans.logical.sql._

No, I've noted in several places that this is a bad practice because it causes namespace problems (duplicate names accidentally imported) and commit conflicts.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala

rdblue · 2019-06-27T20:32:25Z

@jose-torres, this is the alter table PR, if you have time to review.

SparkQA · 2019-06-27T23:44:24Z

Test build #106976 has finished for PR 24937 at commit 72b139a.

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

jose-torres

Looks generally good to me. Only one major question.

jose-torres · 2019-07-01T17:00:18Z

sql/core/src/test/scala/org/apache/spark/sql/sources/v2/DataSourceV2SQLSuite.scala

nit: I'd prefer to have a test for compatible-but-truncating types too, even if they currently have the same behavior.

Can you give an example?

I'm thinking cases like LONG to INT, but maybe I'm missing context on how they're abstracted.

There is a test case for this that rejects unsafe casts: AlterTable: update column type must be compatible. Long to int is not allowed.

jose-torres · 2019-07-01T17:04:25Z

sql/core/src/test/scala/org/apache/spark/sql/sources/v2/DataSourceV2SQLSuite.scala

General question more related to the parser than here: are there tests ensuring that something dangerous doesn't happen if you try to rename/alter/whatever a special identifier like "points.value" directly?

It is possible to alter points.value, so the parser and analysis checks allow it. For example, the "AlterTable: update column map value type" test updates a map's value type from int to long.

Implementations may also support renaming here, since some formats, like Parquet, use an array of structs to store map data. (Similarly, Parquet has a struct to capture nullability of array elements.) So it may be valid to rename points.value, but most implementations would reject it.

Makes sense.

SparkQA · 2019-07-04T23:03:23Z

Test build #107246 has finished for PR 24937 at commit b7df0e6.

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

SparkQA · 2019-07-05T21:57:31Z

Test build #107291 has finished for PR 24937 at commit c40115e.

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

SparkQA · 2019-07-09T20:18:53Z

Test build #107410 has finished for PR 24937 at commit c022b37.

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

cloud-fan · 2019-07-11T08:24:05Z

sql/catalyst/src/main/java/org/apache/spark/sql/catalog/v2/utils/CatalogV2Util.scala

+
+      case (Seq("element", names @ _*), array @ ArrayType(elementStruct: StructType, _)) =>
+        Some(field.copy(dataType = array.copy(elementType = replace(elementStruct, names, update))))
+


This looks a pretty cool feature to me. Can we document it in TableChange? Tell users that they can use a.b.mapField.key to change map type's key type, etc.

cloud-fan · 2019-07-11T08:29:15Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

+   * has a catalog specified, like prod_catalog.db.table, or when a default v2 catalog is set and
+   * the table identifier does not include a catalog.
+   */
+  object ResolveAlterTable extends Rule[LogicalPlan] {


can you remind me where we handle the v1 code path for ALTER TABLE?

cloud-fan · 2019-07-11T08:37:00Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/AlterTableExec.scala

+
+  override protected def doExecute(): RDD[InternalRow] = {
+    try {
+      catalog.alterTable(ident, changes: _*)


not related to this PR but just a thought: for API that's implemented by users and called by Spark, it seems not a good idea to use var-length argument list, as Spark always call this API with all the parameters at hand.

cloud-fan · 2019-07-11T08:38:18Z

sql/core/src/test/scala/org/apache/spark/sql/sources/v2/DataSourceV2SQLSuite.scala

    }
  }
+
+  test("AlterTable: table does not exist") {


can we create a new test suite for alter table? I'm afraid this test suite will get too big if we put all the DDL tests here.

cloud-fan · 2019-07-11T08:46:56Z

LGTM, can you resolve the conflict please?

brkyvz

LGTM. I have a couple questions around how case sensitivity is handled in column name resolution. I also think that ALTER TABLE SET LOCATION merits its own TableChange operation (minor)

brkyvz · 2019-07-11T18:20:45Z

sql/catalyst/src/main/java/org/apache/spark/sql/catalog/v2/TableChange.java

    }
  }

+  interface ColumnChange extends TableChange {


Could you add docs for this please?

brkyvz · 2019-07-11T18:24:35Z

sql/catalyst/src/main/java/org/apache/spark/sql/catalog/v2/utils/CatalogV2Util.scala

    val pos = struct.getFieldIndex(fieldNames.head)
        .getOrElse(throw new IllegalArgumentException(s"Cannot find field: ${fieldNames.head}"))
    val field = struct.fields(pos)
-    val replacement: Option[StructField] = if (fieldNames.tail.isEmpty) {


Do you need to perform name resolution according to case sensitivity instead of strict equality when calling getFieldIndex?

brkyvz · 2019-07-11T18:29:16Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

+    import org.apache.spark.sql.catalog.v2.CatalogV2Implicits._
+    override def apply(plan: LogicalPlan): LogicalPlan = plan resolveOperators {
+      case alter @ AlterTableAddColumnsStatement(
+          CatalogObjectIdentifier(Some(v2Catalog), ident), cols) =>


Do we have a JIRA to follow up on changing these to use the V2SessionCatalog if a catalog is not defined?

brkyvz · 2019-07-11T18:32:05Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

+        AlterTable(
+          v2Catalog.asTableCatalog, ident,
+          UnresolvedRelation(alter.tableName),
+          Seq(TableChange.setProperty("location", newLoc)))


Hmm, I feel changing the location of a table deserves its own special TableChange operator, like ChangeLocation rather than a simple setProperty. When I think property, I think TBLPROPERTIES, and the location is separate from that

Not all tables have locations and we're using the convention of passing optional metadata as table properties. I think that given that we don't want to build special support for everything, this is the right way to pass the change.

brkyvz · 2019-07-11T18:32:57Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala

+            val table = alter.table
+            def findField(operation: String, fieldName: Array[String]): StructField = {
+              // include collections because structs nested in maps and arrays may be altered
+              val field = table.schema.findNestedField(fieldName, includeCollections = true)


how is case sensitivity handled here?

I believe it uses the resolver, so it should be case sensitive if the analyzer is.

brkyvz · 2019-07-11T19:03:13Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala

              case _ =>
            }

+          case alter: AlterTable if alter.childrenResolved =>


Is there a reason that some of these checks are here, and some in the resolved of AlterTable? It'd be nice to hide the complexity inside AlterTable if possible, and do a simpler check here on whether AlterTable is resolved.
However, I would much prefer the case where we can throw a better error message.

Yes. The meaning of resolved is that references are satisfied and there are no unresolved placeholders -- that's why resolved checks that the referenced fields exist. This checks whether that resolved plan has valid and reasonable changes, like whether the change updates a column from a string to a boolean.

SparkQA · 2019-07-11T20:43:54Z

Test build #107545 has finished for PR 24937 at commit 5b10bd0.

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

cloud-fan · 2019-07-12T03:55:31Z

Since it's a pretty big PR and I'd like to merge it first. @rdblue please send a followup PR to add docs to ColumnChange and support v2 session catalog, thanks!

rdblue · 2019-07-12T16:21:31Z

Thanks, @cloud-fan!

## What changes were proposed in this pull request? Implement `ALTER TABLE` for v2 tables: * Add `AlterTable` logical plan and `AlterTableExec` physical plan * Convert `ALTER TABLE` parsed plans to `AlterTable` when a v2 catalog is responsible for an identifier * Validate that columns to alter exist in analyzer checks * Fix nested type handling in `CatalogV2Util` ## How was this patch tested? * Add extensive tests in `DataSourceV2SQLSuite` Closes apache#24937 from rdblue/SPARK-28139-add-v2-alter-table. Lead-authored-by: Ryan Blue <[email protected]> Co-authored-by: Ryan Blue <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

gengliangwang reviewed Jun 26, 2019

View reviewed changes

jose-torres reviewed Jul 1, 2019

View reviewed changes

dongjoon-hyun added the SQL label Jul 4, 2019

[SPARK-28139][SQL] Add v2 ALTER TABLE implementation.

ecab485

rdblue force-pushed the SPARK-28139-add-v2-alter-table branch from b7df0e6 to c40115e Compare July 5, 2019 18:42

Do not allow replacing complex types.

c022b37

rdblue force-pushed the SPARK-28139-add-v2-alter-table branch from c40115e to c022b37 Compare July 9, 2019 17:15

cloud-fan reviewed Jul 11, 2019

View reviewed changes

Merge branch 'master' into SPARK-28139-add-v2-alter-table

5b10bd0

brkyvz approved these changes Jul 11, 2019

View reviewed changes

cloud-fan closed this in 507b745 Jul 12, 2019

cloud-fan mentioned this pull request Jul 12, 2019

[SPARK-28238][SQL] Implement DESCRIBE TABLE for Data Source V2 Tables. #25040

Closed


		case (Seq("element", names @ _*), array @ ArrayType(elementStruct: StructType, _)) =>
		Some(field.copy(dataType = array.copy(elementType = replace(elementStruct, names, update))))

[SPARK-28139][SQL] Add v2 ALTER TABLE implementation. #24937

[SPARK-28139][SQL] Add v2 ALTER TABLE implementation. #24937

Uh oh!

Conversation

rdblue commented Jun 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

rdblue commented Jun 23, 2019

Uh oh!

SparkQA commented Jun 23, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rdblue commented Jun 27, 2019

Uh oh!

SparkQA commented Jun 27, 2019

Uh oh!

jose-torres left a comment

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 Jul 4, 2019

Uh oh!

SparkQA commented Jul 5, 2019

Uh oh!

SparkQA commented Jul 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan Jul 11, 2019 • 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!

cloud-fan commented Jul 11, 2019

Uh oh!

brkyvz left a comment

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!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 11, 2019

Uh oh!

rdblue commented Jun 22, 2019 •

edited

Loading

cloud-fan Jul 11, 2019 •

edited

Loading