Skip to content
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.

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

[SPARK-32512][SQL] add alter table add/drop partition command for datasourcev2 #29339

Closed
wants to merge 36 commits into from
Closed
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
a57aafc
add table partition API
jackylee-ch Aug 3, 2020
6efca68
add AlterTableAddPartitionExec and AlterTableDropPartitionExec
jackylee-ch Aug 3, 2020
f0bc357
redefine AlterTableAddPartitionExec and AlterTableDropPartitionExec
jackylee-ch Aug 4, 2020
61cae52
fix test failed
jackylee-ch Aug 5, 2020
b1fc84b
change the warning for purge
jackylee-ch Aug 5, 2020
67fcb12
add SupportsAtomicPartitionManagement API support
jackylee-ch Aug 6, 2020
f4a6ee3
reorder match cases
jackylee-ch Aug 6, 2020
9bd20ba
fix errors
jackylee-ch Aug 6, 2020
6327ead
change match cases
jackylee-ch Aug 6, 2020
800c51a
add alter table add/drop partitions suites
jackylee-ch Aug 7, 2020
60b0a12
Merge branch 'master' into SPARK-32512-new
jackylee-ch Aug 13, 2020
a4c29a2
restart git action
jackylee-ch Aug 21, 2020
a6caf68
change code for comments
jackylee-ch Sep 27, 2020
3405f5b
use UnresolvedTableOrView to resolve the identifier
jackylee-ch Sep 27, 2020
9bc2e76
add children for AlterTableAddPartitionsStatement and AlterTableDropP…
jackylee-ch Sep 28, 2020
0740ef5
add ResolvedView analyzed
jackylee-ch Sep 28, 2020
ad40d7b
add ResolvePartitionSpec rule
jackylee-ch Sep 30, 2020
8fce669
change scala style
jackylee-ch Oct 19, 2020
af4b50b
change scala style
jackylee-ch Oct 19, 2020
b046909
fix scala-2.13 compile failed
jackylee-ch Oct 19, 2020
fbc2b58
add more implicit method
jackylee-ch Oct 19, 2020
dedb32a
remove unused imports
jackylee-ch Oct 21, 2020
41cf069
Merge branch 'master'
jackylee-ch Nov 6, 2020
96a62be
redefine ResolvePartitionSpec
jackylee-ch Nov 6, 2020
d5d8f13
fix test failed
jackylee-ch Nov 7, 2020
6bf49bd
add check in CheckAnalysis
jackylee-ch Nov 7, 2020
cdd7085
restart test
jackylee-ch Nov 9, 2020
0545538
change UnresolveTableOrView to UnresolvedTable
jackylee-ch Nov 10, 2020
f1fcac1
remove implicit class
jackylee-ch Nov 10, 2020
7014ba1
Merge branch 'master' into SPARK-32512-new
jackylee-ch Nov 10, 2020
69bbbd5
fix build failed
jackylee-ch Nov 10, 2020
effd0ed
Merge branch 'SPARK-32512-new' of https://github.com/stczwd/spark int…
jackylee-ch Nov 10, 2020
7377469
use ResolvedV1TableIdentifier
jackylee-ch Nov 10, 2020
c6711cd
fix suite test failed
jackylee-ch Nov 10, 2020
dcd5060
fix suite test failed
jackylee-ch Nov 10, 2020
d316e56
fix test failed
jackylee-ch Nov 11, 2020
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
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,12 @@

package org.apache.spark.sql.catalyst.analysis

import scala.collection.JavaConverters._

import org.apache.spark.sql.AnalysisException
import org.apache.spark.sql.catalyst.plans.logical._
import org.apache.spark.sql.catalyst.rules.Rule
import org.apache.spark.sql.connector.catalog.{CatalogManager, CatalogPlugin, LookupCatalog, SupportsNamespaces, TableCatalog, TableChange}
import org.apache.spark.sql.connector.catalog.{CatalogManager, CatalogPlugin, LookupCatalog, SupportsNamespaces, SupportsPartitionManagement, TableCatalog, TableChange}

/**
* Resolves catalogs from the multi-part identifiers in SQL statements, and convert the statements
Expand All @@ -30,6 +32,7 @@ class ResolveCatalogs(val catalogManager: CatalogManager)
extends Rule[LogicalPlan] with LookupCatalog {
import org.apache.spark.sql.connector.catalog.CatalogV2Implicits._
import org.apache.spark.sql.connector.catalog.CatalogV2Util._
import org.apache.spark.sql.execution.datasources.v2.DataSourceV2Implicits._

override def apply(plan: LogicalPlan): LogicalPlan = plan resolveOperators {
case AlterTableAddColumnsStatement(
Expand Down Expand Up @@ -228,6 +231,30 @@ class ResolveCatalogs(val catalogManager: CatalogManager)

case ShowCurrentNamespaceStatement() =>
ShowCurrentNamespace(catalogManager)

case c @ AlterTableAddPartitionStatement(
ResolvedTable(_, _, table: SupportsPartitionManagement), _, _) =>
val partitions = c.partitionSpecsAndLocs.map { case (spec, location) =>
val tableProperties = table.properties().asScala.toMap
val partParams =
location.map(locationUri => tableProperties + ("location" -> locationUri))
.getOrElse(tableProperties)
(spec.asPartitionIdentifier(table.partitionSchema()), partParams)
}
AlterTableAddPartition(
table,
partitions,
c.ifNotExists)

case c @ AlterTableDropPartitionStatement(
ResolvedTable(_, _, table: SupportsPartitionManagement), _, _, _, _) =>
if (c.purge) {
logWarning("PURGE won't take effect here, please put it in table properties")
}
AlterTableDropPartition(
table,
c.specs.map(_.asPartitionIdentifier(table.partitionSchema())),
c.ifExists)
}

object NonSessionCatalogAndTable {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3418,7 +3418,7 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging
spec -> location
}
AlterTableAddPartitionStatement(
visitMultipartIdentifier(ctx.multipartIdentifier),
UnresolvedTableOrView(visitMultipartIdentifier(ctx.multipartIdentifier)),
Copy link
Contributor

Choose a reason for hiding this comment

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

this doesn't support view, we should use UnresolvedTable

Copy link
Contributor Author

@jackylee-ch jackylee-ch Nov 6, 2020

Choose a reason for hiding this comment

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

Em. Maybe it is a good idea to do this in another PR, as there're other commands use this, too.

Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't change behavior in this PR. If AlterTableAddPartition fails before if the name refers to a view, we should use UnresolvedTable to keep the failing behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

specsAndLocs.toSeq,
ctx.EXISTS != null)
}
Expand Down Expand Up @@ -3458,7 +3458,7 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging
operationNotAllowed("ALTER VIEW ... DROP PARTITION", ctx)
}
AlterTableDropPartitionStatement(
visitMultipartIdentifier(ctx.multipartIdentifier),
UnresolvedTableOrView(visitMultipartIdentifier(ctx.multipartIdentifier)),
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

ctx.partitionSpec.asScala.map(visitNonOptionalPartitionSpec).toSeq,
ifExists = ctx.EXISTS != null,
purge = ctx.PURGE != null,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ case class AlterTableRecoverPartitionsStatement(
* ALTER TABLE ... ADD PARTITION command, as parsed from SQL
*/
case class AlterTableAddPartitionStatement(
tableName: Seq[String],
child: LogicalPlan,
partitionSpecsAndLocs: Seq[(TablePartitionSpec, Option[String])],
ifNotExists: Boolean) extends ParsedStatement

Expand All @@ -237,7 +237,7 @@ case class AlterTableRenamePartitionStatement(
* ALTER TABLE ... DROP PARTITION command, as parsed from SQL
*/
case class AlterTableDropPartitionStatement(
tableName: Seq[String],
child: LogicalPlan,
specs: Seq[TablePartitionSpec],
ifExists: Boolean,
purge: Boolean,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

package org.apache.spark.sql.catalyst.plans.logical

import org.apache.spark.sql.catalyst.InternalRow
import org.apache.spark.sql.catalyst.analysis.{NamedRelation, UnresolvedException}
import org.apache.spark.sql.catalyst.catalog.CatalogTypes.TablePartitionSpec
import org.apache.spark.sql.catalyst.expressions.{Attribute, AttributeReference, Expression, Unevaluable}
Expand Down Expand Up @@ -551,3 +552,31 @@ case class ShowFunctions(
pattern: Option[String]) extends Command {
override def children: Seq[LogicalPlan] = child.toSeq
}

/**
* The logical plan of the ALTER TABLE ADD PARTITION command that works for v2 tables.
*
* The syntax of this command is:
* {{{
* ALTER TABLE table ADD [IF NOT EXISTS]
* PARTITION spec1 [LOCATION 'loc1'][, PARTITION spec2 [LOCATION 'loc2'], ...];
* }}}
*/
case class AlterTableAddPartition(
table: SupportsPartitionManagement,
Copy link
Contributor

Choose a reason for hiding this comment

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

You should make this LogicalPlan and create AlterTableAddPartition(UnresolvedTableOrView, ...) in AstBuilder.scala, and you can remove*Statement classes.

Then you can move the logics in ResolveCatalog to DataSourceV2Strategy using this class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason why I keep AlterTableAddPartitionStatement and AlterTableAddPartition is we still need analyze partitionIdents and make it qualified in Analyzer. It is different from RefreshTable, which only need to analyze UnresolvedTableOrView.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good point and does reveal a flaw in the current framework.

We can add some util classes to help us resolve partition spec

trait PartitionSpec

case class UnresolvedPartitionSpec(spec: TablePartitionSpec, location: Option[String]) extends PartitionSpec

case class ResolvedPartitionSpec(spec: InternalRow, location: Option[String]) extends PartitionSpec

then the command can be defined as

case class AlterTableAddPartition(table: LogicalPlan, partitions: Seq[PartitionSpec], ifExists: Boolean, ...)

And we add a rule to match AlterTableAddPartition, and resolve the UnresolvedPartitionSpec to ResolvedPartitionSpec.

parts: Seq[(InternalRow, Map[String, String])],
ignoreIfExists: Boolean) extends Command

/**
* The logical plan of the ALTER TABLE DROP PARTITION command that works for v2 tables.
* This may remove the data and metadata for this partition.
*
* The syntax of this command is:
* {{{
* ALTER TABLE table DROP [IF EXISTS] PARTITION spec1[, PARTITION spec2, ...];
* }}}
*/
case class AlterTableDropPartition(
table: SupportsPartitionManagement,
partIdents: Seq[InternalRow],
ignoreIfNotExists: Boolean) extends Command
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,10 @@ package org.apache.spark.sql.execution.datasources.v2
import scala.collection.JavaConverters._

import org.apache.spark.sql.AnalysisException
import org.apache.spark.sql.connector.catalog.{SupportsDelete, SupportsRead, SupportsWrite, Table, TableCapability}
import org.apache.spark.sql.catalyst.InternalRow
import org.apache.spark.sql.catalyst.catalog.CatalogTypes.TablePartitionSpec
import org.apache.spark.sql.connector.catalog.{SupportsAtomicPartitionManagement, SupportsDelete, SupportsPartitionManagement, SupportsRead, SupportsWrite, Table, TableCapability}
import org.apache.spark.sql.types.{ByteType, DoubleType, FloatType, IntegerType, LongType, ShortType, StringType, StructType}
import org.apache.spark.sql.util.CaseInsensitiveStringMap

object DataSourceV2Implicits {
Expand Down Expand Up @@ -52,6 +55,26 @@ object DataSourceV2Implicits {
}
}

def asPartitionable: SupportsPartitionManagement = {
table match {
case support: SupportsPartitionManagement =>
support
case _ =>
throw new AnalysisException(
s"Table does not support partition management: ${table.name}")
}
}

def asAtomicPartitionable: SupportsAtomicPartitionManagement = {
table match {
case support: SupportsAtomicPartitionManagement =>
support
case _ =>
throw new AnalysisException(
s"Table does not support atomic partition management: ${table.name}")
}
}

def supports(capability: TableCapability): Boolean = table.capabilities.contains(capability)

def supportsAny(capabilities: TableCapability*): Boolean = capabilities.exists(supports)
Expand All @@ -62,4 +85,42 @@ object DataSourceV2Implicits {
new CaseInsensitiveStringMap(options.asJava)
}
}

implicit class TablePartitionSpecHelper(partSpec: TablePartitionSpec) {
def asPartitionIdentifier(partSchema: StructType): InternalRow = {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's move it to ResolvePartitionSpec and turn it into a method not implicit class.

val conflictKeys = partSpec.keys.toSeq.diff(partSchema.map(_.name))
if (conflictKeys.nonEmpty) {
throw new AnalysisException(s"Partition key ${conflictKeys.mkString(",")} not exists")
}

val partValues = partSchema.map { part =>
val partValue = partSpec.get(part.name).orNull
if (partValue == null) {
null
} else {
// TODO: Support other datatypes, such as DateType
part.dataType match {
Copy link
Member

Choose a reason for hiding this comment

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

How about other types like DateType. If you are not going to support them in this PR, please, open an JIRA and add TODO here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a TODO sounds good to me.

case _: ByteType =>
partValue.toByte
case _: ShortType =>
partValue.toShort
case _: IntegerType =>
partValue.toInt
case _: LongType =>
partValue.toLong
case _: FloatType =>
partValue.toFloat
case _: DoubleType =>
partValue.toDouble
case _: StringType =>
partValue
case _ =>
throw new AnalysisException(
s"Type ${part.dataType.typeName} is not supported for partition.")
}
}
}
InternalRow.fromSeq(partValues)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1708,13 +1708,13 @@ class DDLParserSuite extends AnalysisTest {
val parsed2 = parsePlan(sql2)

val expected1 = AlterTableAddPartitionStatement(
Seq("a", "b", "c"),
UnresolvedTableOrView(Seq("a", "b", "c")),
Seq(
(Map("dt" -> "2008-08-08", "country" -> "us"), Some("location1")),
(Map("dt" -> "2009-09-09", "country" -> "uk"), None)),
ifNotExists = true)
val expected2 = AlterTableAddPartitionStatement(
Seq("a", "b", "c"),
UnresolvedTableOrView(Seq("a", "b", "c")),
Seq((Map("dt" -> "2008-08-08"), Some("loc"))),
ifNotExists = false)

Expand Down Expand Up @@ -1781,7 +1781,7 @@ class DDLParserSuite extends AnalysisTest {
assertUnsupported(sql2_view)

val expected1_table = AlterTableDropPartitionStatement(
Seq("table_name"),
UnresolvedTableOrView(Seq("table_name")),
Seq(
Map("dt" -> "2008-08-08", "country" -> "us"),
Map("dt" -> "2009-09-09", "country" -> "uk")),
Expand All @@ -1797,7 +1797,7 @@ class DDLParserSuite extends AnalysisTest {

val sql3_table = "ALTER TABLE a.b.c DROP IF EXISTS PARTITION (ds='2017-06-10')"
val expected3_table = AlterTableDropPartitionStatement(
Seq("a", "b", "c"),
UnresolvedTableOrView(Seq("a", "b", "c")),
Seq(Map("ds" -> "2017-06-10")),
ifExists = true,
purge = false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,4 +92,8 @@ class InMemoryPartitionTable(

override def partitionExists(ident: InternalRow): Boolean =
memoryTablePartitions.containsKey(ident)

def clearPartitions(): Unit = {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove it now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

memoryTablePartitions.clear()
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.spark.sql.connector

import java.util

import org.apache.spark.sql.catalyst.analysis.TableAlreadyExistsException
import org.apache.spark.sql.connector.catalog.{CatalogV2Implicits, Identifier, Table}
import org.apache.spark.sql.connector.expressions.Transform
import org.apache.spark.sql.types.StructType

class InMemoryPartitionTableCatalog extends InMemoryTableCatalog {
import CatalogV2Implicits._

override def createTable(
ident: Identifier,
schema: StructType,
partitions: Array[Transform],
properties: util.Map[String, String]): Table = {
if (tables.containsKey(ident)) {
throw new TableAlreadyExistsException(ident)
}

InMemoryTableCatalog.maybeSimulateFailedTableCreation(properties)

val table = new InMemoryAtomicPartitionTable(
s"$name.${ident.quoted}", schema, partitions, properties)
tables.put(ident, table)
namespaces.putIfAbsent(ident.namespace.toList, Map())
table
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import org.apache.spark.sql.catalyst.{FunctionIdentifier, TableIdentifier}
import org.apache.spark.sql.catalyst.catalog.{BucketSpec, CatalogStorageFormat, CatalogTable, CatalogTableType, CatalogUtils}
import org.apache.spark.sql.catalyst.plans.logical._
import org.apache.spark.sql.catalyst.rules.Rule
import org.apache.spark.sql.connector.catalog.{CatalogManager, CatalogPlugin, CatalogV2Util, LookupCatalog, SupportsNamespaces, TableCatalog, TableChange, V1Table}
import org.apache.spark.sql.connector.catalog.{CatalogManager, CatalogPlugin, CatalogV2Util, LookupCatalog, SupportsNamespaces, SupportsPartitionManagement, TableCatalog, TableChange, V1Table}
import org.apache.spark.sql.connector.expressions.Transform
import org.apache.spark.sql.execution.command._
import org.apache.spark.sql.execution.datasources.{CreateTable, DataSource, RefreshTable}
Expand Down Expand Up @@ -497,10 +497,11 @@ class ResolveSessionCatalog(
v1TableName.asTableIdentifier,
"ALTER TABLE RECOVER PARTITIONS")

case AlterTableAddPartitionStatement(tbl, partitionSpecsAndLocs, ifNotExists) =>
val v1TableName = parseV1Table(tbl, "ALTER TABLE ADD PARTITION")
case AlterTableAddPartitionStatement(
r @ ResolvedTable(_, _, _: V1Table), partitionSpecsAndLocs, ifNotExists)
if isSessionCatalog(r.catalog) =>
AlterTableAddPartitionCommand(
v1TableName.asTableIdentifier,
r.identifier.asTableIdentifier,
partitionSpecsAndLocs,
ifNotExists)

Expand All @@ -511,10 +512,11 @@ class ResolveSessionCatalog(
from,
to)

case AlterTableDropPartitionStatement(tbl, specs, ifExists, purge, retainData) =>
val v1TableName = parseV1Table(tbl, "ALTER TABLE DROP PARTITION")
case AlterTableDropPartitionStatement(
r @ ResolvedTable(_, _, _: V1Table), specs, ifExists, purge, retainData)
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

if isSessionCatalog(r.catalog) =>
AlterTableDropPartitionCommand(
v1TableName.asTableIdentifier,
r.identifier.asTableIdentifier,
specs,
ifExists,
purge,
Expand Down
Loading