You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "szehon-ho (via GitHub)" <gi...@apache.org> on 2026/10/01 00:18:05 UTC
Re: [PR] [SPARK-59778][SQL] Add display-only properties to the Table API [spark]
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: 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