szehon-ho commented on code in PR #59036:
URL: https://github.com/apache/spark/pull/59036#discussion_r4150555028
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogV2Util.scala:
##########
@@ -677,6 +677,12 @@ private[sql] object CatalogV2Util {
builder
}
+ /** Returns non-reserved table properties with additional display-only
metadata. */
+ def tablePropertiesForDisplay(table: Table): Map[String, String] = {
+ (table.displayProperties().asScala.toMap ++ table.properties().asScala)
Review Comment:
Documented that SET/UNSET only changes `properties()` and cannot remove
catalog-controlled display entries. Extended `ShowTblPropertiesSuite` to check
unsetting a colliding stored key, unsetting a display-only key, and SET
followed by UNSET. It also checks that `getTableProperties` returns the display
value after UNSET while the key is absent from `getCreateTableString`.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableInfo.java:
##########
@@ -59,19 +62,40 @@ public Map<String, String> properties() {
return properties;
}
+ /**
+ * Returns the additional display-only properties described by {@link
Table#displayProperties()}.
+ *
+ * @since 4.4.0
+ */
+ public Map<String, String> displayProperties() {
Review Comment:
Clarified both Javadocs: display properties describe metadata returned by a
catalog, and Spark leaves them empty when calling `createTable`,
`createTableLike`, `stageCreate`, `stageReplace`, or `stageCreateOrReplace`.
Removed the create-time display-property setters from the two in-memory
catalogs; the preservation test now sets the entries on the live table returned
by `createTable`.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/DescribeTableSuite.scala:
##########
@@ -220,6 +222,38 @@ class DescribeTableSuite extends
command.DescribeTableSuiteBase
}
}
+ gridTest("display properties preserve table type and reserved
metadata")(Seq(false, true)) {
+ external =>
+ withNamespaceAndTable("ns", "table") { tbl =>
+ sql(s"CREATE TABLE $tbl (id bigint) $defaultUsing " +
+ "COMMENT 'table comment' LOCATION 'file:/tmp/display-properties' " +
+ "TBLPROPERTIES ('persisted' = 'stored')")
+ val table = loadTable(catalog, "ns", "table")
+ if (external) {
+ table.properties.put(TableCatalog.PROP_EXTERNAL, "true")
+ }
+ table.setDisplayProperties(Map(
Review Comment:
Added a shared fixture in `CommandSuiteBase` that supplies every key in
`TABLE_RESERVED_PROPERTIES`. Both DESCRIBE and SHOW TABLE EXTENDED now test
managed/external tables with and without a stored LOCATION. The DESCRIBE
assertion includes all reserved rows, and SHOW TABLE EXTENDED checks the
complete output.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/DelegatingTable.java:
##########
@@ -74,6 +74,11 @@ public Map<String, String> properties() {
return Collections.unmodifiableMap(info.properties());
}
+ @Override
+ public Map<String, String> displayProperties() {
Review Comment:
Added an end-to-end test in `DataSourceV2MetadataTableSuite`. Its catalog
builds `TableInfo` with display properties in `loadTable` and returns a
`DelegatingTable`. The test exercises DESCRIBE TABLE EXTENDED, SHOW
TBLPROPERTIES, SHOW TABLE EXTENDED, and `getTableProperties`, and verifies that
`getCreateTableString` excludes the display-only entry.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/ShowTblPropertiesSuite.scala:
##########
@@ -17,10 +17,35 @@
package org.apache.spark.sql.execution.command.v2
+import scala.jdk.CollectionConverters._
+
+import org.apache.spark.sql.Row
+import org.apache.spark.sql.connector.catalog.TableCatalog
import org.apache.spark.sql.execution.command
/**
* The class contains tests for the `SHOW TBLPROPERTIES` command to check V2
table catalogs.
*/
class ShowTblPropertiesSuite extends command.ShowTblPropertiesSuiteBase with
CommandSuiteBase {
+ test("display properties supplement current properties and exclude reserved
keys") {
Review Comment:
Added a display-only `password` to the shared fixture and asserted
`*********(redacted)` in DESCRIBE TABLE EXTENDED, SHOW TBLPROPERTIES (including
single-key lookup), and SHOW TABLE EXTENDED. The catalog API test also verifies
redaction in `getTableProperties`.
##########
sql/api/src/main/scala/org/apache/spark/sql/catalog/Catalog.scala:
##########
@@ -846,6 +846,10 @@ abstract class Catalog {
/**
* Returns all table properties as a map (same as `SHOW TBLPROPERTIES`).
*
+ * For V2 tables, this also includes the table's display-only properties.
These entries may be
Review Comment:
Added the same note to the Connect Scala override. The Scala and PySpark
docs now state directly that display-only entries may be transient and are
excluded from the DDL returned by `getCreateTableString`.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/Table.java:
##########
@@ -95,6 +95,24 @@ default Map<String, String> properties() {
return Collections.emptyMap();
}
+ /**
+ * Returns additional, display-only table properties.
+ *
+ * <p>Spark adds these entries to the non-reserved properties displayed by
V2 table metadata
Review Comment:
Added the specific limitation to `Table.displayProperties()`: for now, SHOW
TABLE EXTENDED omits these entries for the built-in session catalog and its
CatalogExtensions. Changing that routing needs separate compatibility work for
mixed V1/V2 tables and existing V1 output.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/InMemoryTableCatalog.scala:
##########
@@ -168,8 +168,11 @@ class BasicInMemoryTableCatalog extends TableCatalog {
}
override def createTable(ident: Identifier, tableInfo: TableInfo): Table = {
- createTable(ident, tableInfo.columns(), tableInfo.partitions(),
tableInfo.properties(),
+ val table = createTable(
+ ident, tableInfo.columns(), tableInfo.partitions(),
tableInfo.properties(),
Distributions.unspecified(), Array.empty, None, None,
tableInfo.constraints())
+
table.asInstanceOf[InMemoryBaseTable].setDisplayProperties(tableInfo.displayProperties())
Review Comment:
Removed both create-time setters and the cast from
`BasicInMemoryTableCatalog`. The Catalyst preservation test now sets display
properties on the live table returned by `createTable`.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/ShowTblPropertiesSuite.scala:
##########
@@ -17,10 +17,35 @@
package org.apache.spark.sql.execution.command.v2
+import scala.jdk.CollectionConverters._
+
+import org.apache.spark.sql.Row
+import org.apache.spark.sql.connector.catalog.TableCatalog
import org.apache.spark.sql.execution.command
/**
* The class contains tests for the `SHOW TBLPROPERTIES` command to check V2
table catalogs.
*/
class ShowTblPropertiesSuite extends command.ShowTblPropertiesSuiteBase with
CommandSuiteBase {
+ test("display properties supplement current properties and exclude reserved
keys") {
+ withNamespaceAndTable("ns", "table") { tbl =>
+ sql(s"CREATE TABLE $tbl (id bigint) $defaultUsing " +
+ "TBLPROPERTIES ('persisted' = 'stored')")
+ loadTable(catalog, "ns", "table").setDisplayProperties(Map(
+ "catalog-label" -> "catalog-value",
+ "persisted" -> "display-value",
+ TableCatalog.PROP_EXTERNAL -> "true",
+ TableCatalog.PROP_LOCATION -> "file:/display-location").asJava)
+
+ checkAnswer(sql(s"SHOW TBLPROPERTIES $tbl"),
+ Seq(Row("catalog-label", "catalog-value"), Row("persisted", "stored")))
+ checkAnswer(sql(s"SHOW TBLPROPERTIES $tbl ('persisted')"),
Row("persisted", "stored"))
+ checkAnswer(sql(s"SHOW TBLPROPERTIES $tbl ('catalog-label')"),
+ Row("catalog-label", "catalog-value"))
+
+ sql(s"ALTER TABLE $tbl SET TBLPROPERTIES ('persisted' = 'updated')")
Review Comment:
Added `setDisplayProperties(table.displayProperties())` when the
session-catalog fixture reconstructs a table. A new regression test checks SHOW
TBLPROPERTIES and DESCRIBE TABLE EXTENDED after SET, and that the display entry
survives a subsequent UNSET.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
