[SPARK-31828][SQL] Retain table properties at CreateTableLikeCommand #28647

ulysses-you · 2020-05-27T01:22:57Z

What changes were proposed in this pull request?

At CreateTableLikeCommand, we use the new tblproperties with merge source tblproperties.

Why are the changes needed?

We should retain the useful tblproperties, e.g. parquet.compression. And hive also retain the tblproperties.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Add UT.

ulysses-you · 2020-05-27T01:38:06Z

One point.
Hive only retain the useful properties through serde class annotation.
Should we remove the properties like transient_lastDdlTime simply?
Or follow the hive process ?

cc @cloud-fan

SparkQA · 2020-05-27T03:21:55Z

Test build #123155 has finished for PR 28647 at commit 37ba3b3.

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

cloud-fan · 2020-05-27T08:25:21Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala

      assert(source.properties("a") == "apple")
      sql("CREATE TABLE t LIKE s STORED AS parquet TBLPROPERTIES('f'='foo', 'b'='bar')")
      val table = catalog.getTableMetadata(TableIdentifier("t"))
-      assert(table.properties.get("a") === None)


Seems like we intentionally don't keep the table properties from the original table. @maropu @dongjoon-hyun @viirya are you OK with the behavior change proposed by this PR?

Based on the description of CreateTableLikeCommand:

The CatalogTable attributes copied from the source table are storage(inputFormat, outputFormat, serde, compressed, properties), schema, provider, partitionColumnNames, bucketSpec by default.

So we don't say table properties are copied from source table too. Not sure about why we didn't copy table properties.

I feel it is OK as seems copying original table properties should not be harmful. And we already copy storage properties.

But we should update the doc together.

hm, I have the same opinion with @viirya; I couldn't find any strong reason not to copy the properites. Rather, is this a kind of bugs? Anyway, yea, we should clearly descibe the behaivour in our doc...

ulysses-you · 2020-05-28T04:54:03Z

In HiveDDLSuite, exists the code

    assert(targetTable.properties.filterKeys(!metastoreGeneratedProperties.contains(_)).isEmpty,
      "the table properties of source tables should not be copied in the created table")

If use this behavior, we should change the test code.

cloud-fan · 2020-05-28T09:18:54Z

let's update the doc of CreateTableLikeCommand to make it clear that we copy table properties as well.

ulysses-you · 2020-05-28T09:53:52Z

I see the enum properties key in HiveDDLSuite.

val metastoreGeneratedProperties = Seq(
      "CreateTime",
      "transient_lastDdlTime",
      "grantTime",
      "lastUpdateTime",
      "last_modified_by",
      "last_modified_time",
      "Owner:",
      "totalNumberFiles",
      "maxFileSize",
      "minFileSize"
    )

Shall we copy the tblproperties without these?

maropu · 2020-05-29T00:41:54Z

Could you update the SQL doc for the behaivour, too? https://github.com/apache/spark/blame/master/docs/sql-ref-syntax-ddl-create-table-like.md

maropu · 2020-05-29T00:54:53Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala

      CatalogTableType.EXTERNAL
    }

+    val newProperties =


plz leave some comments about why the metastore props are remvoed here? #28647 (comment)

maropu · 2020-05-29T01:01:31Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala

 object DDLUtils {
  val HIVE_PROVIDER = "hive"

+  val METASTORE_GENERATED_PROPERTIES: Set[String] = Set(


Just to check; does the latest hive metastore have the same set of these props here?

I searched somewhere, but I think there is no determinate way to check this.

Can we reuse HiveClientImpl.HiveStatisticsProperties? They are the hive specific properties we explicitly ignore.

HiveClientImpl.HiveStatisticsProperties has been used at HiveClientImpl.getTable(), we needn't remove them again.

I don't feel comfortable maintaining this list here. Is it possible to remove Hive specific properties in HiveClientImpl? So that the hive related stuff remains in hive related classes.

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala

SparkQA · 2020-05-29T02:08:59Z

Test build #123255 has finished for PR 28647 at commit b30d62c.

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

SparkQA · 2020-05-29T04:07:59Z

Test build #123256 has finished for PR 28647 at commit 162627a.

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

SparkQA · 2020-05-29T04:21:00Z

Test build #123257 has finished for PR 28647 at commit 9eacf1e.

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

ulysses-you · 2020-05-29T07:38:42Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala

+    val newProperties = sourceTableDesc.tableType match {
+      case VIEW =>
+        // For view, we just use new properties
+        properties


Keep view behavior as before. Hive also does not copy view properties.

nit format;

val newProperties = sourceTableDesc.tableType match { case MANAGED | EXTERNAL => // Hive only retain the useful properties through serde class annotation. // For better compatible with Hive, we remove the metastore properties. sourceTableDesc.properties -- DDLUtils.METASTORE_GENERATED_PROPERTIES ++ properties case VIEW => // For view, we just use new properties properties }

SparkQA · 2020-05-29T11:04:44Z

Test build #123272 has finished for PR 28647 at commit 175d0e2.

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

ulysses-you · 2020-05-29T14:50:50Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala

-      "maxFileSize",
-      "minFileSize"
-    )
-    assert(targetTable.properties.filterKeys(!metastoreGeneratedProperties.contains(_)).isEmpty,


Remove this check. And we can't check all meta properties here. For example transient_lastDdlTime, hive will add this properties when table created. So the properties is always exists.

Add some meta properties test at new UT.

SparkQA · 2020-05-29T19:21:53Z

Test build #123291 has finished for PR 28647 at commit ed0877e.

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

ulysses-you · 2020-05-29T23:22:56Z

cc @cloud-fan @viirya @maropu

maropu · 2020-05-30T00:51:33Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala

+  test("SPARK-31828: Retain table properties at CreateTableLikeCommand") {
+    val catalog = spark.sessionState.catalog
+    withTable("t1", "t2", "t3") {
+      sql(s"CREATE TABLE t1(c1 int) TBLPROPERTIES('k1'='v1', 'k2'='v2', 'totalNumberFiles'='meta')")


Just in case, could you test all the prpos in METASTORE_GENERATED_PROPERTIES? Probably, its beetter to split it into two test units like add a new test unit test("SPARK-31828: Filters out Hive metastore properties in CreateTableLikeCommand") {.

github-actions · 2020-10-19T00:55:03Z

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

ulysses-you · 2020-11-02T00:26:00Z

I believe this is useful, can we revisit PR ? cc @maropu @cloud-fan @viirya @dilipbiswal

dongjoon-hyun · 2020-11-02T16:11:03Z

This is reopened to the author (@ulysses-you )'s request.

dongjoon-hyun · 2020-11-02T16:11:50Z

@ulysses-you . Please rebase your PR regularly to the master to avoid STALE label.

SparkQA · 2020-11-02T16:59:34Z

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

SparkQA · 2020-11-02T17:20:52Z

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

SparkQA · 2020-11-02T20:29:54Z

Test build #130531 has finished for PR 28647 at commit 03966ae.

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

ulysses-you · 2020-11-02T23:50:08Z

thank you @dongjoon-hyun

…1828

SparkQA · 2020-11-03T00:44:29Z

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

SparkQA · 2020-11-03T00:54:38Z

Test build #130535 has finished for PR 28647 at commit dc00260.

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

SparkQA · 2020-11-03T01:12:17Z

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

SparkQA · 2020-11-09T01:29:21Z

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

SparkQA · 2020-11-09T01:53:38Z

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

SparkQA · 2020-11-09T05:07:21Z

Test build #130760 has finished for PR 28647 at commit 2d470fc.

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

ulysses-you · 2020-11-09T05:47:06Z

cc @maropu @cloud-fan @viirya do you have time to review this thanks !

…1828

SparkQA · 2020-11-17T07:26:42Z

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

SparkQA · 2020-11-17T07:57:30Z

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

SparkQA · 2020-11-17T08:05:02Z

Test build #131201 has finished for PR 28647 at commit 4b55575.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds the following public classes (experimental):
public static class NoOpMergedShuffleFileManager implements MergedShuffleFileManager
public class RemoteBlockPushResolver implements MergedShuffleFileManager
static class PushBlockStreamCallback implements StreamCallbackWithID
public static class AppShuffleId
public static class AppShufflePartitionInfo
trait HasMaxBlockSizeInMB extends Params
>>> class VectorAccumulatorParam(AccumulatorParam):
fully qualified classname of key Writable class (e.g. \"org.apache.hadoop.io.Text\")
fully qualified classname of key Writable class (e.g. \"org.apache.hadoop.io.Text\")
fully qualified classname of key Writable class (e.g. \"org.apache.hadoop.io.Text\")
fully qualified classname of key Writable class (e.g. \"org.apache.hadoop.io.Text\")
class HasMaxBlockSizeInMB(Params):
trait SQLConfHelper
class Analyzer(override val catalogManager: CatalogManager)
case class UnresolvedTableOrView(
case class UnresolvedPartitionSpec(
case class ResolvedPartitionSpec(
case class ElementAt(
case class GetArrayItem(
case class GetMapValue(
case class Elt(
trait OffsetWindowFunction extends WindowFunction
class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with SQLConfHelper with Logging
abstract class AbstractSqlParser extends ParserInterface with SQLConfHelper with Logging
class CatalystSqlParser extends AbstractSqlParser
case class AnalyzeTable(
case class AnalyzeColumn(
case class AlterTableAddPartition(
case class AlterTableDropPartition(
case class LoadData(
case class ShowCreateTable(child: LogicalPlan, asSerde: Boolean = false) extends Command
abstract class Rule[TreeType <: TreeNode[_]] extends SQLConfHelper with Logging
implicit class PartitionSpecsHelper(partSpecs: Seq[PartitionSpec])
class SparkPlanner(val session: SparkSession, val experimentalMethods: ExperimentalMethods)
class SparkSqlParser extends AbstractSqlParser
class SparkSqlAstBuilder extends AstBuilder
case class CoalesceShufflePartitions(session: SparkSession) extends Rule[SparkPlan]
class FindDataSourceTable(sparkSession: SparkSession) extends Rule[LogicalPlan]
class FallBackFileSourceV2(sparkSession: SparkSession) extends Rule[LogicalPlan]
class ResolveSQLOnFile(sparkSession: SparkSession) extends Rule[LogicalPlan]
case class PreprocessTableCreation(sparkSession: SparkSession) extends Rule[LogicalPlan]
case class AlterTableAddPartitionExec(
case class AlterTableDropPartitionExec(
case class DropTableExec(
class V2SessionCatalog(catalog: SessionCatalog)
case class PlanDynamicPruningFilters(sparkSession: SparkSession)
class HDFSBackedReadStateStore(val version: Long, map: MapType)
trait ReadStateStore
trait StateStore extends ReadStateStore
class WrappedReadStateStore(store: StateStore) extends ReadStateStore
abstract class BaseStateStoreRDD[T: ClassTag, U: ClassTag](
class ReadStateStoreRDD[T: ClassTag, U: ClassTag](
case class PlanSubqueries(sparkSession: SparkSession) extends Rule[SparkPlan]
class VariableSubstitution extends SQLConfHelper
abstract class JdbcDialect extends Serializable with Logging
class ResolveHiveSerdeTable(session: SparkSession) extends Rule[LogicalPlan]
class DetermineTableStats(session: SparkSession) extends Rule[LogicalPlan]

…1828

SparkQA · 2020-11-24T14:12:00Z

Test build #131644 has finished for PR 28647 at commit c45489a.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class ExecutorSource(
case class GetShufflePushMergerLocations(numMergersNeeded: Int, hostsToFilter: Set[String])
case class RemoveShufflePushMergerLocation(host: String) extends ToBlockManagerMaster
case class UnresolvedTable(
class SubExprEvaluationRuntime(cacheMaxEntries: Int)
case class ExpressionProxy(
case class ResultProxy(result: Any)
case class CurrentTimeZone() extends LeafExpression with Unevaluable
abstract class LikeAllBase extends UnaryExpression with ImplicitCastInputTypes with NullIntolerant
case class LikeAll(child: Expression, patterns: Seq[UTF8String]) extends LikeAllBase
case class NotLikeAll(child: Expression, patterns: Seq[UTF8String]) extends LikeAllBase
case class ParseUrl(children: Seq[Expression], failOnError: Boolean = SQLConf.get.ansiEnabled)
implicit class MetadataColumnsHelper(metadata: Array[MetadataColumn])
trait PathFilterStrategy extends Serializable
trait StrategyBuilder
class PathGlobFilter(filePatten: String) extends PathFilterStrategy
abstract class ModifiedDateFilter extends PathFilterStrategy
class ModifiedBeforeFilter(thresholdTime: Long, val timeZoneId: String)
class ModifiedAfterFilter(thresholdTime: Long, val timeZoneId: String)

github-actions · 2021-03-05T01:09:47Z

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

init

61dfe05

probot-autolabeler bot added the SQL label May 27, 2020

fix

37ba3b3

cloud-fan reviewed May 27, 2020

View reviewed changes

update doc

594a1bb

remove metastore proerties

b30d62c

maropu reviewed May 29, 2020

View reviewed changes

update doc

29aba43

maropu reviewed May 29, 2020

View reviewed changes

add comment

162627a

probot-autolabeler bot added the DOCS label May 29, 2020

maropu reviewed May 29, 2020

View reviewed changes

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala Show resolved Hide resolved

add meta test

9eacf1e

fix view

175d0e2

ulysses-you commented May 29, 2020

View reviewed changes

fix

ed0877e

ulysses-you commented May 29, 2020

View reviewed changes

maropu reviewed May 30, 2020

View reviewed changes

github-actions bot added the Stale label Oct 19, 2020

github-actions bot closed this Oct 20, 2020

dongjoon-hyun reopened this Nov 2, 2020

dongjoon-hyun removed the Stale label Nov 2, 2020

Merge branch 'master' of https://github.com/apache/spark into SPARK-3…

dc00260

…1828

minor

2d470fc

Merge branch 'master' of https://github.com/apache/spark into SPARK-3…

4b55575

…1828

Merge branch 'master' of https://github.com/apache/spark into SPARK-3…

c45489a

…1828

github-actions bot added the Stale label Mar 5, 2021

github-actions bot closed this Mar 6, 2021

[SPARK-31828][SQL] Retain table properties at CreateTableLikeCommand #28647

[SPARK-31828][SQL] Retain table properties at CreateTableLikeCommand #28647

Uh oh!

Conversation

ulysses-you commented May 27, 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!

ulysses-you commented May 27, 2020

Uh oh!

SparkQA commented May 27, 2020

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!

ulysses-you commented May 28, 2020

Uh oh!

cloud-fan commented May 28, 2020

Uh oh!

ulysses-you commented May 28, 2020

Uh oh!

maropu commented May 29, 2020

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!

Uh oh!

SparkQA commented May 29, 2020

Uh oh!

SparkQA commented May 29, 2020

Uh oh!

SparkQA commented May 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 29, 2020

Uh oh!

ulysses-you May 29, 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!

SparkQA commented May 29, 2020

Uh oh!

ulysses-you commented May 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 19, 2020

Uh oh!

ulysses-you commented Nov 2, 2020

Uh oh!

dongjoon-hyun commented Nov 2, 2020

Uh oh!

dongjoon-hyun commented Nov 2, 2020

Uh oh!

SparkQA commented Nov 2, 2020

Uh oh!

SparkQA commented Nov 2, 2020

Uh oh!

SparkQA commented Nov 2, 2020

Uh oh!

ulysses-you commented Nov 2, 2020

Uh oh!

SparkQA commented Nov 3, 2020

ulysses-you May 29, 2020 •

edited

Loading