You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "imback82 (via GitHub)" <gi...@apache.org> on 2023/08/21 02:37:39 UTC
[GitHub] [spark] imback82 opened a new pull request, #42577: [SPARK-XXXXX][SQL] Introduce CLUSTER BY clause for CREATE/REPLACE TABLE
imback82 opened a new pull request, #42577:
URL: https://github.com/apache/spark/pull/42577
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'core/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
### Was this patch authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has been used in the process of authoring this patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-44886][SQL] Introduce CLUSTER BY clause for CREATE/REPLACE TABLE [spark]
Posted by "imback82 (via GitHub)" <gi...@apache.org>.
imback82 commented on code in PR #42577:
URL: https://github.com/apache/spark/pull/42577#discussion_r1383855097
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/interface.scala:
##########
@@ -170,6 +170,23 @@ case class CatalogTablePartition(
}
}
+/**
+ * A container for clustering information.
+ *
+ * @param columnNames the names of the columns used for clustering.
+ */
+case class ClusterBySpec(columnNames: Seq[UnresolvedAttribute]) {
+ override def toString: String = columnNames.map(_.name).mkString(",")
+
+ lazy val toDDL: String = if (columnNames.nonEmpty) s"CLUSTER BY ($toString)" else ""
+}
+
+object ClusterBySpec {
+ def apply(columns: String): ClusterBySpec = columns match {
Review Comment:
this is used for parsing property value back to ClusterBySpec. I renamed this to `fromProperty`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-44886][SQL] Introduce CLUSTER BY clause for CREATE/REPLACE TABLE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42577:
URL: https://github.com/apache/spark/pull/42577#discussion_r1382781093
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/interface.scala:
##########
@@ -170,6 +170,23 @@ case class CatalogTablePartition(
}
}
+/**
+ * A container for clustering information.
+ *
+ * @param columnNames the names of the columns used for clustering.
+ */
+case class ClusterBySpec(columnNames: Seq[UnresolvedAttribute]) {
+ override def toString: String = columnNames.map(_.name).mkString(",")
+
+ lazy val toDDL: String = if (columnNames.nonEmpty) s"CLUSTER BY ($toString)" else ""
+}
+
+object ClusterBySpec {
+ def apply(columns: String): ClusterBySpec = columns match {
Review Comment:
this looks weird, where do we use it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-44886][SQL] Introduce CLUSTER BY clause for CREATE/REPLACE TABLE [spark]
Posted by "imback82 (via GitHub)" <gi...@apache.org>.
imback82 commented on code in PR #42577:
URL: https://github.com/apache/spark/pull/42577#discussion_r1382761816
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/interface.scala:
##########
@@ -253,7 +270,8 @@ case class CatalogTable(
tracksPartitionsInCatalog: Boolean = false,
schemaPreservesCase: Boolean = true,
ignoredProperties: Map[String, String] = Map.empty,
- viewOriginalText: Option[String] = None) {
+ viewOriginalText: Option[String] = None,
+ clusterBySpec: Option[ClusterBySpec] = None) {
Review Comment:
We haven't added any field to `CatalogTable` in the last 5 years and should we _not_ add this new field? Alternatively, we can store clustering columns in the property and make the property as reserved?
##########
sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala:
##########
@@ -502,13 +502,16 @@ private[hive] class HiveClientImpl(
"comment",
// For EXTERNAL_TABLE, the table properties has a particular field "EXTERNAL". This is added
// in the function toHiveTable.
- "EXTERNAL"
+ "EXTERNAL",
+ // Used to store clusterBySpec.
+ "clusteringColumns"
)
val filteredProperties = properties.filterNot {
case (key, _) => excludedTableProperties.contains(key)
}
val comment = properties.get("comment")
+ val clusterBySpec = properties.get("clusteringColumns").map(ClusterBySpec(_))
Review Comment:
should we make this property as reserved?
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/command/CreateTableClusterBySuite.scala:
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.hive.execution.command
+
+import org.apache.spark.sql.execution.command.v1
+
+/**
+ * The class contains tests for the `CREATE TABLE ... CLUSTER BY` command to check V1 Hive external
+ * table catalog.
+ */
+class CreateTableClusterBySuite extends v1.CreateTableClusterBySuiteBase with CommandSuiteBase {
+ // Hive doesn't support nested column names with space and dot.
Review Comment:
Actually, the exception stack trace looks like the following:
```
[CANNOT_RECOGNIZE_HIVE_TYPE] Cannot recognize hive type string: "STRUCT<COL4.1:INT>", column: `col3`. The specified data type for the field cannot be recognized by Spark SQL. Please check the data type of the specified field and ensure that it is a valid Spark SQL data type. Refer to the Spark SQL documentation for a list of valid data types and their format. If the data type is correct, please ensure that you are using a supported version of Spark SQL. SQLSTATE: 429BB
org.apache.spark.SparkException: [CANNOT_RECOGNIZE_HIVE_TYPE] Cannot recognize hive type string: "STRUCT<COL4.1:INT>", column: `col3`. The specified data type for the field cannot be recognized by Spark SQL. Please check the data type of the specified field and ensure that it is a valid Spark SQL data type. Refer to the Spark SQL documentation for a list of valid data types and their format. If the data type is correct, please ensure that you are using a supported version of Spark SQL. SQLSTATE: 429BB
at org.apache.spark.sql.errors.QueryExecutionErrors$.cannotRecognizeHiveTypeError(QueryExecutionErrors.scala:1637)
at org.apache.spark.sql.hive.client.HiveClientImpl$.getSparkSQLDataType(HiveClientImpl.scala:1067)
at org.apache.spark.sql.hive.client.HiveClientImpl$.$anonfun$verifyColumnDataType$1(HiveClientImpl.scala:1082)
at scala.collection.IterableOnceOps.foreach(IterableOnce.scala:576)
at scala.collection.IterableOnceOps.foreach$(IterableOnce.scala:574)
at org.apache.spark.sql.types.StructType.foreach(StructType.scala:102)
```
`COL4.1` in `"STRUCT<COL4.1:INT>"` is not quoted, so the parser fails. We prob. need to fix `DataType.catalogString`...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-44886][SQL] Introduce CLUSTER BY clause for CREATE/REPLACE TABLE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42577:
URL: https://github.com/apache/spark/pull/42577#discussion_r1384616988
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/CreateTableClusterBySuite.scala:
##########
@@ -0,0 +1,68 @@
+/*
+ * 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.execution.command.v1
+
+import org.apache.spark.sql.AnalysisException
+import org.apache.spark.sql.catalyst.TableIdentifier
+import org.apache.spark.sql.catalyst.analysis.UnresolvedAttribute
+import org.apache.spark.sql.catalyst.catalog.ClusterBySpec
+import org.apache.spark.sql.connector.catalog.CatalogManager.SESSION_CATALOG_NAME
+import org.apache.spark.sql.execution.command
+
+/**
+ * This base suite contains unified tests for the `CREATE TABLE ... CLUSTER BY` command that
+ * checks V1 table catalogs. The tests that cannot run for all V1 catalogs are located in more
+ * specific test suites:
+ *
+ * - V1 In-Memory catalog: `org.apache.spark.sql.execution.command.v1.CreateTableClusterBySuite`
+ * - V1 Hive External catalog:
+ * `org.apache.spark.sql.hive.execution.command.CreateTableClusterBySuite`
+ */
+trait CreateTableClusterBySuiteBase extends command.CreateTableClusterBySuiteBase
+ with command.TestsV1AndV2Commands {
+ override def validateClusterBy(
+ tableIdent: TableIdentifier, clusteringColumns: Seq[String]): Unit = {
+ val catalog = spark.sessionState.catalog
+ val table = catalog.getTableMetadata(tableIdent)
+ assert(table.clusterBySpec ===
+ Some(ClusterBySpec(clusteringColumns.map(UnresolvedAttribute(_)))))
+ }
+}
+
+/**
+ * The class contains tests for the `CREATE TABLE ... CLUSTER BY` command to check V1 In-Memory
+ * table catalog.
+ */
+class CreateTableClusterBySuite extends CreateTableClusterBySuiteBase
+ with CommandSuiteBase {
+ override def commandVersion: String = super[CreateTableClusterBySuiteBase].commandVersion
+
+ test("clustering columns not defined in schema") {
+ withNamespaceAndTable("ns", "table") { tbl =>
+ checkError(
+ exception = intercept[AnalysisException](
+ sql(s"CREATE TABLE $tbl (id bigint, data string) $defaultUsing CLUSTER BY (unknown)")),
+ errorClass = "COLUMN_NOT_DEFINED_IN_TABLE",
Review Comment:
oh I see https://github.com/apache/spark/pull/42577/files#diff-d96524deae4f3cbf30881f842ab6091aea1de9d3e5c6810f4f35d3dc697dd3d3R58 It's better if we can unify the error between v1 and v2.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-44886][SQL] Introduce CLUSTER BY clause for CREATE/REPLACE TABLE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42577:
URL: https://github.com/apache/spark/pull/42577#discussion_r1384591410
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/interface.scala:
##########
@@ -170,6 +170,25 @@ case class CatalogTablePartition(
}
}
+/**
+ * A container for clustering information.
+ *
+ * @param columnNames the names of the columns used for clustering.
+ */
+case class ClusterBySpec(columnNames: Seq[UnresolvedAttribute]) {
+ override def toString: String = columnNames.map(_.name).mkString(",")
+}
+
+object ClusterBySpec {
+ def fromProperty(columns: String): ClusterBySpec = columns match {
+ case "" => ClusterBySpec(Seq.empty[UnresolvedAttribute])
+ case _ => ClusterBySpec(columns.split(",").map(_.trim).map(UnresolvedAttribute.quotedString))
Review Comment:
hmm what if the column name contains `,`? shall we use JSON format?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-44886][SQL] Introduce CLUSTER BY clause for CREATE/REPLACE TABLE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42577:
URL: https://github.com/apache/spark/pull/42577#discussion_r1384608602
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/CreateTableClusterBySuiteBase.scala:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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.execution.command
+
+import org.apache.spark.sql.QueryTest
+import org.apache.spark.sql.catalyst.TableIdentifier
+
+/**
+ * This base suite contains unified tests for the `CREATE/REPLACE TABLE ... CLUSTER BY` command
+ * that check V1 and V2 table catalogs. The tests that cannot run for all supported catalogs are
+ * located in more specific test suites:
+ *
+ * - V2 table catalog tests: `org.apache.spark.sql.execution.command.v2.CreateTableClusterBySuite`
+ * - V1 table catalog tests:
+ * `org.apache.spark.sql.execution.command.v1.CreateTableClusterBySuiteBase`
+ * - V1 In-Memory catalog: `org.apache.spark.sql.execution.command.v1.CreateTableClusterBySuite`
+ * - V1 Hive External catalog:
+ * `org.apache.spark.sql.hive.execution.command.CreateTableClusterBySuite`
+ */
+trait CreateTableClusterBySuiteBase extends QueryTest with DDLCommandTestUtils {
+ override val command = "CREATE/REPLACE TABLE CLUSTER BY"
+
+ protected val nestedColumnSchema: String =
+ "col1 INT, col2 STRUCT<col3 INT, `col4 1` INT>, col3 STRUCT<`col4.1` INT>"
+ protected val nestedClusteringColumns: Seq[String] =
+ Seq("col2.col3", "col2.`col4 1`", "col3.`col4.1`")
+
+ def validateClusterBy(tableIdent: TableIdentifier, clusteringColumns: Seq[String]): Unit
Review Comment:
`Seq[String]` should be good.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-44886][SQL] Introduce CLUSTER BY clause for CREATE/REPLACE TABLE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42577:
URL: https://github.com/apache/spark/pull/42577#discussion_r1386075909
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/interface.scala:
##########
@@ -170,6 +174,55 @@ case class CatalogTablePartition(
}
}
+/**
+ * A container for clustering information.
+ *
+ * @param columnNames the names of the columns used for clustering.
+ */
+case class ClusterBySpec(columnNames: Seq[NamedReference]) {
+ override def toString: String = toJson
+
+ def toJson: String = ClusterBySpec.mapper.writeValueAsString(columnNames.map(_.fieldNames))
+}
+
+object ClusterBySpec {
+ private val mapper = {
+ val ret = new ObjectMapper() with ClassTagExtensions
+ ret.setSerializationInclusion(Include.NON_ABSENT)
+ ret.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false)
+ ret.registerModule(DefaultScalaModule)
+ ret
+ }
+
+ def fromProperty(columns: String): ClusterBySpec = {
+ ClusterBySpec(mapper.readValue[Seq[Seq[String]]](columns).map(FieldReference(_)))
+ }
+
+ def toProperty(
+ schema: StructType,
+ clusterBySpec: ClusterBySpec,
+ resolver: Resolver): (String, String) = {
+ CatalogTable.PROP_CLUSTERING_COLUMNS ->
+ normalizeClusterBySpec(schema, clusterBySpec, resolver).toJson
+ }
+
+ private def normalizeClusterBySpec(
+ schema: StructType,
+ clusterBySpec: ClusterBySpec,
+ resolver: Resolver): ClusterBySpec = {
+ val normalizedColumns = clusterBySpec.columnNames.map { columnName =>
+ val position = SchemaUtils.findColumnPosition(
Review Comment:
This checks column existence but we only hit it for v1 path (cover to v1 command), where do we check column existence for pure v2 path?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-44886][SQL] Introduce CLUSTER BY clause for CREATE/REPLACE TABLE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42577:
URL: https://github.com/apache/spark/pull/42577#discussion_r1384596211
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/interface.scala:
##########
@@ -170,6 +170,25 @@ case class CatalogTablePartition(
}
}
+/**
+ * A container for clustering information.
+ *
+ * @param columnNames the names of the columns used for clustering.
+ */
+case class ClusterBySpec(columnNames: Seq[UnresolvedAttribute]) {
Review Comment:
It's weird to use `UnresolvedAttribute` here as we are not going to resolve it in the analyzer. How about just `Seq[Seq[String]]` or `Seq[FieldReference]`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-44886][SQL] Introduce CLUSTER BY clause for CREATE/REPLACE TABLE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #42577:
URL: https://github.com/apache/spark/pull/42577#issuecomment-1803654432
thanks, merging to master!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-44886][SQL] Introduce CLUSTER BY clause for CREATE/REPLACE TABLE [spark]
Posted by "imback82 (via GitHub)" <gi...@apache.org>.
imback82 commented on code in PR #42577:
URL: https://github.com/apache/spark/pull/42577#discussion_r1385921893
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/interface.scala:
##########
@@ -170,6 +174,55 @@ case class CatalogTablePartition(
}
}
+/**
+ * A container for clustering information.
+ *
+ * @param columnNames the names of the columns used for clustering.
+ */
+case class ClusterBySpec(columnNames: Seq[NamedReference]) {
+ override def toString: String = toJson
+
+ def toJson: String = ClusterBySpec.mapper.writeValueAsString(columnNames.map(_.fieldNames))
+}
+
+object ClusterBySpec {
+ private val mapper = {
+ val ret = new ObjectMapper() with ClassTagExtensions
+ ret.setSerializationInclusion(Include.NON_ABSENT)
+ ret.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false)
+ ret.registerModule(DefaultScalaModule)
+ ret
+ }
+
+ def fromProperty(columns: String): ClusterBySpec = {
+ ClusterBySpec(mapper.readValue[Seq[Seq[String]]](columns).map(FieldReference(_)))
Review Comment:
Alternatively I could have serialized `FieldReference`, but this approach is more generic.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/CreateTableClusterBySuite.scala:
##########
@@ -0,0 +1,68 @@
+/*
+ * 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.execution.command.v1
+
+import org.apache.spark.sql.AnalysisException
+import org.apache.spark.sql.catalyst.TableIdentifier
+import org.apache.spark.sql.catalyst.analysis.UnresolvedAttribute
+import org.apache.spark.sql.catalyst.catalog.ClusterBySpec
+import org.apache.spark.sql.connector.catalog.CatalogManager.SESSION_CATALOG_NAME
+import org.apache.spark.sql.execution.command
+
+/**
+ * This base suite contains unified tests for the `CREATE TABLE ... CLUSTER BY` command that
+ * checks V1 table catalogs. The tests that cannot run for all V1 catalogs are located in more
+ * specific test suites:
+ *
+ * - V1 In-Memory catalog: `org.apache.spark.sql.execution.command.v1.CreateTableClusterBySuite`
+ * - V1 Hive External catalog:
+ * `org.apache.spark.sql.hive.execution.command.CreateTableClusterBySuite`
+ */
+trait CreateTableClusterBySuiteBase extends command.CreateTableClusterBySuiteBase
+ with command.TestsV1AndV2Commands {
+ override def validateClusterBy(
+ tableIdent: TableIdentifier, clusteringColumns: Seq[String]): Unit = {
+ val catalog = spark.sessionState.catalog
+ val table = catalog.getTableMetadata(tableIdent)
+ assert(table.clusterBySpec ===
+ Some(ClusterBySpec(clusteringColumns.map(UnresolvedAttribute(_)))))
+ }
+}
+
+/**
+ * The class contains tests for the `CREATE TABLE ... CLUSTER BY` command to check V1 In-Memory
+ * table catalog.
+ */
+class CreateTableClusterBySuite extends CreateTableClusterBySuiteBase
+ with CommandSuiteBase {
+ override def commandVersion: String = super[CreateTableClusterBySuiteBase].commandVersion
+
+ test("clustering columns not defined in schema") {
+ withNamespaceAndTable("ns", "table") { tbl =>
+ checkError(
+ exception = intercept[AnalysisException](
+ sql(s"CREATE TABLE $tbl (id bigint, data string) $defaultUsing CLUSTER BY (unknown)")),
+ errorClass = "COLUMN_NOT_DEFINED_IN_TABLE",
Review Comment:
now that the logic follows the same as in `PreprocessTableCreation`, the error message is unitifed: https://github.com/apache/spark/pull/42577/files#diff-f2a04f920c41d18a7d387216f86405bfdc6fb09c44ebe1bb09312ba7dde55333R216
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-44886][SQL] Introduce CLUSTER BY clause for CREATE/REPLACE TABLE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42577:
URL: https://github.com/apache/spark/pull/42577#discussion_r1386077119
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/CreateTableClusterBySuiteBase.scala:
##########
@@ -0,0 +1,85 @@
+/*
+ * 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.execution.command
+
+import org.apache.spark.sql.{AnalysisException, QueryTest}
+
+/**
+ * This base suite contains unified tests for the `CREATE/REPLACE TABLE ... CLUSTER BY` command
+ * that check V1 and V2 table catalogs. The tests that cannot run for all supported catalogs are
+ * located in more specific test suites:
+ *
+ * - V2 table catalog tests: `org.apache.spark.sql.execution.command.v2.CreateTableClusterBySuite`
+ * - V1 table catalog tests:
+ * `org.apache.spark.sql.execution.command.v1.CreateTableClusterBySuiteBase`
+ * - V1 In-Memory catalog: `org.apache.spark.sql.execution.command.v1.CreateTableClusterBySuite`
+ * - V1 Hive External catalog:
+ * `org.apache.spark.sql.hive.execution.command.CreateTableClusterBySuite`
+ */
+trait CreateTableClusterBySuiteBase extends QueryTest with DDLCommandTestUtils {
+ override val command = "CREATE/REPLACE TABLE CLUSTER BY"
+
+ protected val nestedColumnSchema: String =
+ "col1 INT, col2 STRUCT<col3 INT, `col4 1` INT>, col3 STRUCT<`col4.1` INT>"
+ protected val nestedClusteringColumns: Seq[String] =
+ Seq("col2.col3", "col2.`col4 1`", "col3.`col4.1`")
+
+ def validateClusterBy(tableName: String, clusteringColumns: Seq[String]): Unit
+
+ test("test basic CREATE TABLE with clustering columns") {
+ withNamespaceAndTable("ns", "table") { tbl =>
+ spark.sql(s"CREATE TABLE $tbl (id INT, data STRING) $defaultUsing CLUSTER BY (id, data)")
+ validateClusterBy(tbl, Seq("id", "data"))
+ }
+ }
+
+ test("test clustering columns with comma") {
+ assume(!catalogVersion.contains("Hive")) // Hive catalog doesn't support column names with dots.
Review Comment:
we can override `def excluded` in the hive suite to exclude this test case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-44886][SQL] Introduce CLUSTER BY clause for CREATE/REPLACE TABLE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42577:
URL: https://github.com/apache/spark/pull/42577#discussion_r1382780703
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -3973,18 +4000,31 @@ class AstBuilder extends DataTypeAstBuilder with SQLConfHelper with Logging {
*/
override def visitReplaceTable(ctx: ReplaceTableContext): LogicalPlan = withOrigin(ctx) {
val orCreate = ctx.replaceTableHeader().CREATE() != null
- val (partTransforms, partCols, bucketSpec, properties, options, location, comment, serdeInfo) =
- visitCreateTableClauses(ctx.createTableClauses())
+ val (partTransforms, partCols, bucketSpec, properties, options, location, comment, serdeInfo,
+ clusterBySpec) = visitCreateTableClauses(ctx.createTableClauses())
val columns = Option(ctx.createOrReplaceTableColTypeList())
.map(visitCreateOrReplaceTableColTypeList).getOrElse(Nil)
+
+ if (clusterBySpec.isDefined) {
+ if (partCols.nonEmpty || partTransforms.nonEmpty) {
+ throw QueryParsingErrors.clusterByWithPartitionedBy(ctx)
Review Comment:
shall we check this inside `visitCreateTableClauses`? Then CREATE and REPLACE TABLE can share the code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-44886][SQL] Introduce CLUSTER BY clause for CREATE/REPLACE TABLE [spark]
Posted by "imback82 (via GitHub)" <gi...@apache.org>.
imback82 commented on code in PR #42577:
URL: https://github.com/apache/spark/pull/42577#discussion_r1386812960
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/CreateTableClusterBySuiteBase.scala:
##########
@@ -0,0 +1,85 @@
+/*
+ * 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.execution.command
+
+import org.apache.spark.sql.{AnalysisException, QueryTest}
+
+/**
+ * This base suite contains unified tests for the `CREATE/REPLACE TABLE ... CLUSTER BY` command
+ * that check V1 and V2 table catalogs. The tests that cannot run for all supported catalogs are
+ * located in more specific test suites:
+ *
+ * - V2 table catalog tests: `org.apache.spark.sql.execution.command.v2.CreateTableClusterBySuite`
+ * - V1 table catalog tests:
+ * `org.apache.spark.sql.execution.command.v1.CreateTableClusterBySuiteBase`
+ * - V1 In-Memory catalog: `org.apache.spark.sql.execution.command.v1.CreateTableClusterBySuite`
+ * - V1 Hive External catalog:
+ * `org.apache.spark.sql.hive.execution.command.CreateTableClusterBySuite`
+ */
+trait CreateTableClusterBySuiteBase extends QueryTest with DDLCommandTestUtils {
+ override val command = "CREATE/REPLACE TABLE CLUSTER BY"
+
+ protected val nestedColumnSchema: String =
+ "col1 INT, col2 STRUCT<col3 INT, `col4 1` INT>, col3 STRUCT<`col4.1` INT>"
+ protected val nestedClusteringColumns: Seq[String] =
+ Seq("col2.col3", "col2.`col4 1`", "col3.`col4.1`")
+
+ def validateClusterBy(tableName: String, clusteringColumns: Seq[String]): Unit
+
+ test("test basic CREATE TABLE with clustering columns") {
+ withNamespaceAndTable("ns", "table") { tbl =>
+ spark.sql(s"CREATE TABLE $tbl (id INT, data STRING) $defaultUsing CLUSTER BY (id, data)")
+ validateClusterBy(tbl, Seq("id", "data"))
+ }
+ }
+
+ test("test clustering columns with comma") {
+ assume(!catalogVersion.contains("Hive")) // Hive catalog doesn't support column names with dots.
Review Comment:
done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-44886][SQL] Introduce CLUSTER BY clause for CREATE/REPLACE TABLE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42577:
URL: https://github.com/apache/spark/pull/42577#discussion_r1382780377
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -3958,6 +3983,8 @@ class AstBuilder extends DataTypeAstBuilder with SQLConfHelper with Logging {
* replace_table_clauses (order insensitive):
* [OPTIONS table_property_list]
* [PARTITIONED BY (partition_fields)]
+ * [CLUSTER BY (col_name, col_name, ...)]
+ * [CLUSTER BY (col_name, col_name, ...)]
Review Comment:
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-44886][SQL] Introduce CLUSTER BY clause for CREATE/REPLACE TABLE [spark]
Posted by "imback82 (via GitHub)" <gi...@apache.org>.
imback82 commented on code in PR #42577:
URL: https://github.com/apache/spark/pull/42577#discussion_r1383858771
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -3973,18 +4000,31 @@ class AstBuilder extends DataTypeAstBuilder with SQLConfHelper with Logging {
*/
override def visitReplaceTable(ctx: ReplaceTableContext): LogicalPlan = withOrigin(ctx) {
val orCreate = ctx.replaceTableHeader().CREATE() != null
- val (partTransforms, partCols, bucketSpec, properties, options, location, comment, serdeInfo) =
- visitCreateTableClauses(ctx.createTableClauses())
+ val (partTransforms, partCols, bucketSpec, properties, options, location, comment, serdeInfo,
+ clusterBySpec) = visitCreateTableClauses(ctx.createTableClauses())
val columns = Option(ctx.createOrReplaceTableColTypeList())
.map(visitCreateOrReplaceTableColTypeList).getOrElse(Nil)
+
+ if (clusterBySpec.isDefined) {
+ if (partCols.nonEmpty || partTransforms.nonEmpty) {
+ throw QueryParsingErrors.clusterByWithPartitionedBy(ctx)
Review Comment:
+1, done.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -3958,6 +3983,8 @@ class AstBuilder extends DataTypeAstBuilder with SQLConfHelper with Logging {
* replace_table_clauses (order insensitive):
* [OPTIONS table_property_list]
* [PARTITIONED BY (partition_fields)]
+ * [CLUSTER BY (col_name, col_name, ...)]
+ * [CLUSTER BY (col_name, col_name, ...)]
Review Comment:
fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-44886][SQL] Introduce CLUSTER BY clause for CREATE/REPLACE TABLE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42577:
URL: https://github.com/apache/spark/pull/42577#discussion_r1384616351
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/CreateTableClusterBySuite.scala:
##########
@@ -0,0 +1,61 @@
+/*
+ * 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.execution.command.v2
+
+import org.apache.spark.sql.AnalysisException
+import org.apache.spark.sql.catalyst.TableIdentifier
+import org.apache.spark.sql.connector.catalog.{Identifier, InMemoryPartitionTable}
+import org.apache.spark.sql.connector.catalog.CatalogV2Implicits.CatalogHelper
+import org.apache.spark.sql.connector.expressions.{ClusterByTransform, FieldReference}
+import org.apache.spark.sql.execution.command
+
+/**
+ * The class contains tests for the `CREATE TABLE ... CLUSTER BY` command to check V2 table
+ * catalogs.
+ */
+class CreateTableClusterBySuite extends command.CreateTableClusterBySuiteBase
+ with CommandSuiteBase {
+ override def validateClusterBy(
+ tableIdent: TableIdentifier, clusteringColumns: Seq[String]): Unit = {
+ val catalogPlugin = spark.sessionState.catalogManager.catalog(tableIdent.catalog.get)
+ val partTable = catalogPlugin.asTableCatalog
+ .loadTable(Identifier.of(Array(tableIdent.database.get), tableIdent.table))
+ .asInstanceOf[InMemoryPartitionTable]
+ assert(partTable.partitioning ===
+ Array(ClusterByTransform(clusteringColumns.map(FieldReference(_)))))
+ }
+
+ test("test basic CREATE/REPLACE TABLE with clustering columns") {
+ withNamespaceAndTable("ns", "table") { tbl =>
+ spark.sql(s"CREATE TABLE $tbl (id INT) $defaultUsing CLUSTER BY (id)")
Review Comment:
is this already covered in the base suite?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-44886][SQL] Introduce CLUSTER BY clause for CREATE/REPLACE TABLE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42577:
URL: https://github.com/apache/spark/pull/42577#discussion_r1384604339
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/rules.scala:
##########
@@ -297,6 +297,7 @@ case class PreprocessTableCreation(catalog: SessionCatalog) extends Rule[Logical
val normalizedPartCols = normalizePartitionColumns(schema, table)
val normalizedBucketSpec = normalizeBucketSpec(schema, table)
+ val normalizedClusterBySpec = normalizeClusterBySpec(schema, table)
Review Comment:
instead of normalizing it here, shall we normalize it before we convert cluster spec to table properties?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-44886][SQL] Introduce CLUSTER BY clause for CREATE/REPLACE TABLE [spark]
Posted by "imback82 (via GitHub)" <gi...@apache.org>.
imback82 commented on code in PR #42577:
URL: https://github.com/apache/spark/pull/42577#discussion_r1386788291
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/interface.scala:
##########
@@ -170,6 +174,55 @@ case class CatalogTablePartition(
}
}
+/**
+ * A container for clustering information.
+ *
+ * @param columnNames the names of the columns used for clustering.
+ */
+case class ClusterBySpec(columnNames: Seq[NamedReference]) {
+ override def toString: String = toJson
+
+ def toJson: String = ClusterBySpec.mapper.writeValueAsString(columnNames.map(_.fieldNames))
+}
+
+object ClusterBySpec {
+ private val mapper = {
+ val ret = new ObjectMapper() with ClassTagExtensions
+ ret.setSerializationInclusion(Include.NON_ABSENT)
+ ret.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false)
+ ret.registerModule(DefaultScalaModule)
+ ret
+ }
+
+ def fromProperty(columns: String): ClusterBySpec = {
+ ClusterBySpec(mapper.readValue[Seq[Seq[String]]](columns).map(FieldReference(_)))
+ }
+
+ def toProperty(
+ schema: StructType,
+ clusterBySpec: ClusterBySpec,
+ resolver: Resolver): (String, String) = {
+ CatalogTable.PROP_CLUSTERING_COLUMNS ->
+ normalizeClusterBySpec(schema, clusterBySpec, resolver).toJson
+ }
+
+ private def normalizeClusterBySpec(
+ schema: StructType,
+ clusterBySpec: ClusterBySpec,
+ resolver: Resolver): ClusterBySpec = {
+ val normalizedColumns = clusterBySpec.columnNames.map { columnName =>
+ val position = SchemaUtils.findColumnPosition(
Review Comment:
it's happening here for v2 path: https://github.com/apache/spark/blob/6abc4a1a58ef4e5d896717b10b2314dae2af78af/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/rules.scala#L273-L279
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-44886][SQL] Introduce CLUSTER BY clause for CREATE/REPLACE TABLE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42577:
URL: https://github.com/apache/spark/pull/42577#discussion_r1382784624
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/interface.scala:
##########
@@ -253,7 +270,8 @@ case class CatalogTable(
tracksPartitionsInCatalog: Boolean = false,
schemaPreservesCase: Boolean = true,
ignoredProperties: Map[String, String] = Map.empty,
- viewOriginalText: Option[String] = None) {
+ viewOriginalText: Option[String] = None,
+ clusterBySpec: Option[ClusterBySpec] = None) {
Review Comment:
Since we have to store it in the table properties anyway at the Hive layer, I think it's simpler to just do it ahead here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-44886][SQL] Introduce CLUSTER BY clause for CREATE/REPLACE TABLE [spark]
Posted by "imback82 (via GitHub)" <gi...@apache.org>.
imback82 commented on code in PR #42577:
URL: https://github.com/apache/spark/pull/42577#discussion_r1383854471
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/interface.scala:
##########
@@ -253,7 +270,8 @@ case class CatalogTable(
tracksPartitionsInCatalog: Boolean = false,
schemaPreservesCase: Boolean = true,
ignoredProperties: Map[String, String] = Map.empty,
- viewOriginalText: Option[String] = None) {
+ viewOriginalText: Option[String] = None,
+ clusterBySpec: Option[ClusterBySpec] = None) {
Review Comment:
Done, I guess no need to make it as a reserved property like properties in `TableCatalog`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-44886][SQL] Introduce CLUSTER BY clause for CREATE/REPLACE TABLE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42577:
URL: https://github.com/apache/spark/pull/42577#discussion_r1384607654
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/CreateTableClusterBySuiteBase.scala:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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.execution.command
+
+import org.apache.spark.sql.QueryTest
+import org.apache.spark.sql.catalyst.TableIdentifier
+
+/**
+ * This base suite contains unified tests for the `CREATE/REPLACE TABLE ... CLUSTER BY` command
+ * that check V1 and V2 table catalogs. The tests that cannot run for all supported catalogs are
+ * located in more specific test suites:
+ *
+ * - V2 table catalog tests: `org.apache.spark.sql.execution.command.v2.CreateTableClusterBySuite`
+ * - V1 table catalog tests:
+ * `org.apache.spark.sql.execution.command.v1.CreateTableClusterBySuiteBase`
+ * - V1 In-Memory catalog: `org.apache.spark.sql.execution.command.v1.CreateTableClusterBySuite`
+ * - V1 Hive External catalog:
+ * `org.apache.spark.sql.hive.execution.command.CreateTableClusterBySuite`
+ */
+trait CreateTableClusterBySuiteBase extends QueryTest with DDLCommandTestUtils {
+ override val command = "CREATE/REPLACE TABLE CLUSTER BY"
+
+ protected val nestedColumnSchema: String =
+ "col1 INT, col2 STRUCT<col3 INT, `col4 1` INT>, col3 STRUCT<`col4.1` INT>"
+ protected val nestedClusteringColumns: Seq[String] =
+ Seq("col2.col3", "col2.`col4 1`", "col3.`col4.1`")
+
+ def validateClusterBy(tableIdent: TableIdentifier, clusteringColumns: Seq[String]): Unit
Review Comment:
better to not use v1 `TableIdentifier` in base test suite that works for both v1 and v2.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-44886][SQL] Introduce CLUSTER BY clause for CREATE/REPLACE TABLE [spark]
Posted by "imback82 (via GitHub)" <gi...@apache.org>.
imback82 commented on code in PR #42577:
URL: https://github.com/apache/spark/pull/42577#discussion_r1385865564
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/interface.scala:
##########
@@ -170,6 +170,25 @@ case class CatalogTablePartition(
}
}
+/**
+ * A container for clustering information.
+ *
+ * @param columnNames the names of the columns used for clustering.
+ */
+case class ClusterBySpec(columnNames: Seq[UnresolvedAttribute]) {
+ override def toString: String = columnNames.map(_.name).mkString(",")
+}
+
+object ClusterBySpec {
+ def fromProperty(columns: String): ClusterBySpec = columns match {
+ case "" => ClusterBySpec(Seq.empty[UnresolvedAttribute])
+ case _ => ClusterBySpec(columns.split(",").map(_.trim).map(UnresolvedAttribute.quotedString))
Review Comment:
done
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/interface.scala:
##########
@@ -170,6 +170,25 @@ case class CatalogTablePartition(
}
}
+/**
+ * A container for clustering information.
+ *
+ * @param columnNames the names of the columns used for clustering.
+ */
+case class ClusterBySpec(columnNames: Seq[UnresolvedAttribute]) {
Review Comment:
I went with `Seq[NamedReference]` to be consistent with `*Transform` (including `ClusterByTransform`). Also, some of the helper function `FieldReference.unapply` returns `NamedReference`, so it's easier to work with `NamedReference`.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/connector/expressions/expressions.scala:
##########
@@ -150,6 +153,41 @@ private[sql] object BucketTransform {
}
}
+/**
+ * This class represents a transform for [[ClusterBySpec]]. This is used to bundle
+ * ClusterBySpec in CreateTable's partitioning transforms to pass it down to analyzer.
+ */
+final case class ClusterByTransform(
+ columnNames: Seq[NamedReference]) extends RewritableTransform {
+
+ override val name: String = "cluster_by"
+
+ override def references: Array[NamedReference] = {
+ arguments.collect { case named: NamedReference => named }
Review Comment:
+1
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/CreateTableClusterBySuite.scala:
##########
@@ -0,0 +1,61 @@
+/*
+ * 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.execution.command.v2
+
+import org.apache.spark.sql.AnalysisException
+import org.apache.spark.sql.catalyst.TableIdentifier
+import org.apache.spark.sql.connector.catalog.{Identifier, InMemoryPartitionTable}
+import org.apache.spark.sql.connector.catalog.CatalogV2Implicits.CatalogHelper
+import org.apache.spark.sql.connector.expressions.{ClusterByTransform, FieldReference}
+import org.apache.spark.sql.execution.command
+
+/**
+ * The class contains tests for the `CREATE TABLE ... CLUSTER BY` command to check V2 table
+ * catalogs.
+ */
+class CreateTableClusterBySuite extends command.CreateTableClusterBySuiteBase
+ with CommandSuiteBase {
+ override def validateClusterBy(
+ tableIdent: TableIdentifier, clusteringColumns: Seq[String]): Unit = {
+ val catalogPlugin = spark.sessionState.catalogManager.catalog(tableIdent.catalog.get)
+ val partTable = catalogPlugin.asTableCatalog
+ .loadTable(Identifier.of(Array(tableIdent.database.get), tableIdent.table))
+ .asInstanceOf[InMemoryPartitionTable]
+ assert(partTable.partitioning ===
+ Array(ClusterByTransform(clusteringColumns.map(FieldReference(_)))))
+ }
+
+ test("test basic CREATE/REPLACE TABLE with clustering columns") {
+ withNamespaceAndTable("ns", "table") { tbl =>
+ spark.sql(s"CREATE TABLE $tbl (id INT) $defaultUsing CLUSTER BY (id)")
Review Comment:
this is testing replace table (let me clarify the test case name), which is supported only with v2.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/rules.scala:
##########
@@ -297,6 +297,7 @@ case class PreprocessTableCreation(catalog: SessionCatalog) extends Rule[Logical
val normalizedPartCols = normalizePartitionColumns(schema, table)
val normalizedBucketSpec = normalizeBucketSpec(schema, table)
+ val normalizedClusterBySpec = normalizeClusterBySpec(schema, table)
Review Comment:
done
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/CreateTableClusterBySuiteBase.scala:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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.execution.command
+
+import org.apache.spark.sql.QueryTest
+import org.apache.spark.sql.catalyst.TableIdentifier
+
+/**
+ * This base suite contains unified tests for the `CREATE/REPLACE TABLE ... CLUSTER BY` command
+ * that check V1 and V2 table catalogs. The tests that cannot run for all supported catalogs are
+ * located in more specific test suites:
+ *
+ * - V2 table catalog tests: `org.apache.spark.sql.execution.command.v2.CreateTableClusterBySuite`
+ * - V1 table catalog tests:
+ * `org.apache.spark.sql.execution.command.v1.CreateTableClusterBySuiteBase`
+ * - V1 In-Memory catalog: `org.apache.spark.sql.execution.command.v1.CreateTableClusterBySuite`
+ * - V1 Hive External catalog:
+ * `org.apache.spark.sql.hive.execution.command.CreateTableClusterBySuite`
+ */
+trait CreateTableClusterBySuiteBase extends QueryTest with DDLCommandTestUtils {
+ override val command = "CREATE/REPLACE TABLE CLUSTER BY"
+
+ protected val nestedColumnSchema: String =
+ "col1 INT, col2 STRUCT<col3 INT, `col4 1` INT>, col3 STRUCT<`col4.1` INT>"
+ protected val nestedClusteringColumns: Seq[String] =
+ Seq("col2.col3", "col2.`col4 1`", "col3.`col4.1`")
+
+ def validateClusterBy(tableIdent: TableIdentifier, clusteringColumns: Seq[String]): Unit
Review Comment:
done
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalog.scala:
##########
@@ -19,13 +19,11 @@ package org.apache.spark.sql.execution.datasources.v2
import java.net.URI
import java.util
-
import scala.collection.mutable
import scala.jdk.CollectionConverters._
-
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-44886][SQL] Introduce CLUSTER BY clause for CREATE/REPLACE TABLE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #42577: [SPARK-44886][SQL] Introduce CLUSTER BY clause for CREATE/REPLACE TABLE
URL: https://github.com/apache/spark/pull/42577
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-44886][SQL] Introduce CLUSTER BY clause for CREATE/REPLACE TABLE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42577:
URL: https://github.com/apache/spark/pull/42577#discussion_r1384610934
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/CreateTableClusterBySuite.scala:
##########
@@ -0,0 +1,68 @@
+/*
+ * 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.execution.command.v1
+
+import org.apache.spark.sql.AnalysisException
+import org.apache.spark.sql.catalyst.TableIdentifier
+import org.apache.spark.sql.catalyst.analysis.UnresolvedAttribute
+import org.apache.spark.sql.catalyst.catalog.ClusterBySpec
+import org.apache.spark.sql.connector.catalog.CatalogManager.SESSION_CATALOG_NAME
+import org.apache.spark.sql.execution.command
+
+/**
+ * This base suite contains unified tests for the `CREATE TABLE ... CLUSTER BY` command that
+ * checks V1 table catalogs. The tests that cannot run for all V1 catalogs are located in more
+ * specific test suites:
+ *
+ * - V1 In-Memory catalog: `org.apache.spark.sql.execution.command.v1.CreateTableClusterBySuite`
+ * - V1 Hive External catalog:
+ * `org.apache.spark.sql.hive.execution.command.CreateTableClusterBySuite`
+ */
+trait CreateTableClusterBySuiteBase extends command.CreateTableClusterBySuiteBase
+ with command.TestsV1AndV2Commands {
+ override def validateClusterBy(
+ tableIdent: TableIdentifier, clusteringColumns: Seq[String]): Unit = {
+ val catalog = spark.sessionState.catalog
+ val table = catalog.getTableMetadata(tableIdent)
+ assert(table.clusterBySpec ===
+ Some(ClusterBySpec(clusteringColumns.map(UnresolvedAttribute(_)))))
+ }
+}
+
+/**
+ * The class contains tests for the `CREATE TABLE ... CLUSTER BY` command to check V1 In-Memory
+ * table catalog.
+ */
+class CreateTableClusterBySuite extends CreateTableClusterBySuiteBase
+ with CommandSuiteBase {
+ override def commandVersion: String = super[CreateTableClusterBySuiteBase].commandVersion
+
+ test("clustering columns not defined in schema") {
+ withNamespaceAndTable("ns", "table") { tbl =>
+ checkError(
+ exception = intercept[AnalysisException](
+ sql(s"CREATE TABLE $tbl (id bigint, data string) $defaultUsing CLUSTER BY (unknown)")),
+ errorClass = "COLUMN_NOT_DEFINED_IN_TABLE",
Review Comment:
shouldn't this be a general behavior? Analyzer should check the existence of clustering columns. I think it's already the case for partition and bucket cols.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-44886][SQL] Introduce CLUSTER BY clause for CREATE/REPLACE TABLE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42577:
URL: https://github.com/apache/spark/pull/42577#discussion_r1384609233
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/CreateTableClusterBySuiteBase.scala:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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.execution.command
+
+import org.apache.spark.sql.QueryTest
+import org.apache.spark.sql.catalyst.TableIdentifier
+
+/**
+ * This base suite contains unified tests for the `CREATE/REPLACE TABLE ... CLUSTER BY` command
+ * that check V1 and V2 table catalogs. The tests that cannot run for all supported catalogs are
+ * located in more specific test suites:
+ *
+ * - V2 table catalog tests: `org.apache.spark.sql.execution.command.v2.CreateTableClusterBySuite`
+ * - V1 table catalog tests:
+ * `org.apache.spark.sql.execution.command.v1.CreateTableClusterBySuiteBase`
+ * - V1 In-Memory catalog: `org.apache.spark.sql.execution.command.v1.CreateTableClusterBySuite`
+ * - V1 Hive External catalog:
+ * `org.apache.spark.sql.hive.execution.command.CreateTableClusterBySuite`
+ */
+trait CreateTableClusterBySuiteBase extends QueryTest with DDLCommandTestUtils {
+ override val command = "CREATE/REPLACE TABLE CLUSTER BY"
+
+ protected val nestedColumnSchema: String =
+ "col1 INT, col2 STRUCT<col3 INT, `col4 1` INT>, col3 STRUCT<`col4.1` INT>"
+ protected val nestedClusteringColumns: Seq[String] =
+ Seq("col2.col3", "col2.`col4 1`", "col3.`col4.1`")
+
+ def validateClusterBy(tableIdent: TableIdentifier, clusteringColumns: Seq[String]): Unit
Review Comment:
or just `String` as the qualified table name with dot.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-44886][SQL] Introduce CLUSTER BY clause for CREATE/REPLACE TABLE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42577:
URL: https://github.com/apache/spark/pull/42577#discussion_r1384604676
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalog.scala:
##########
@@ -19,13 +19,11 @@ package org.apache.spark.sql.execution.datasources.v2
import java.net.URI
import java.util
-
import scala.collection.mutable
import scala.jdk.CollectionConverters._
-
Review Comment:
nit: code style
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-44886][SQL] Introduce CLUSTER BY clause for CREATE/REPLACE TABLE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42577:
URL: https://github.com/apache/spark/pull/42577#discussion_r1384598897
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/connector/expressions/expressions.scala:
##########
@@ -150,6 +153,41 @@ private[sql] object BucketTransform {
}
}
+/**
+ * This class represents a transform for [[ClusterBySpec]]. This is used to bundle
+ * ClusterBySpec in CreateTable's partitioning transforms to pass it down to analyzer.
+ */
+final case class ClusterByTransform(
+ columnNames: Seq[NamedReference]) extends RewritableTransform {
+
+ override val name: String = "cluster_by"
+
+ override def references: Array[NamedReference] = {
+ arguments.collect { case named: NamedReference => named }
Review Comment:
it's `columnNames: Seq[NamedReference]`, we can just return `columnNames` here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-44886][SQL] Introduce CLUSTER BY clause for CREATE/REPLACE TABLE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42577:
URL: https://github.com/apache/spark/pull/42577#discussion_r1386078630
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/command/CreateTableClusterBySuite.scala:
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.hive.execution.command
+
+import org.apache.spark.sql.execution.command.v1
+
+/**
+ * The class contains tests for the `CREATE TABLE ... CLUSTER BY` command to check V1 Hive external
+ * table catalog.
+ */
+class CreateTableClusterBySuite extends v1.CreateTableClusterBySuiteBase with CommandSuiteBase {
+ // Hive doesn't support nested column names with space and dot.
Review Comment:
+1, cc @beliefer can you help to take a look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org