szehon-ho commented on code in PR #59036:
URL: https://github.com/apache/spark/pull/59036#discussion_r4139215461
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/ShowTablePropertiesExec.scala:
##########
@@ -34,7 +34,7 @@ case class ShowTablePropertiesExec(
import scala.jdk.CollectionConverters._
// The reserved properties are accessible through DESCRIBE
- val properties = conf.redactOptions(catalogTable.properties.asScala.toMap)
+ val properties =
conf.redactOptions(catalogTable.displayProperties.asScala.toMap)
Review Comment:
Kept `getTableProperties()` consistent with `SHOW TBLPROPERTIES`, including
the additional display-only entries. The Scala and Python API docs and PR
description now explain that these entries may be transient and unsuitable for
copying into DDL, and point to `getCreateTableString()`. Added a V2 test in
`org.apache.spark.sql.internal.CatalogSuite` covering the returned properties,
unchanged table type/description, and exclusion of the display-only entry from
generated DDL. Existing connectors retain their behavior because the new method
defaults to an empty map.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DescribeTableExec.scala:
##########
@@ -189,7 +189,8 @@ case class DescribeTableExec(
rows += toCatalystRow("# Detailed Table Information", "", "")
addIdentifierRows(rows, catalogName, identifier, entityLabel = "Table")
- val tableType = if
(table.properties().containsKey(TableCatalog.PROP_EXTERNAL)) {
+ val displayProperties = table.displayProperties()
Review Comment:
Changed both commands so Type and all reserved metadata rows continue to
read `properties()`. The shared `CatalogV2Util.tablePropertiesForDisplay()`
helper merges the additional entries with `properties()` (existing values win),
then removes reserved keys from the free-form property list. The Javadoc now
defines that contract, and the command tests cover managed/external tables and
conflicting reserved display keys.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/ShowTablesExtendedExec.scala:
##########
@@ -82,7 +82,8 @@ case class ShowTablesExtendedExec(
results.put("Catalog", catalogName)
results.put("Namespace", identifier.namespace().quoted)
results.put("Table", identifier.name())
- val tableType = if
(table.properties().containsKey(TableCatalog.PROP_EXTERNAL)) {
+ val displayProperties = table.displayProperties()
Review Comment:
Documented this scope in the PR description. The new properties are used
only by the V2 table execution paths. `SHOW TABLE EXTENDED` for
`spark_catalog`, including session-catalog extensions, still uses the V1
command. The description also calls out that `DESCRIBE TABLE EXTENDED` and
`SHOW TBLPROPERTIES` can use the new API when an extension returns a V2 table.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/Table.java:
##########
@@ -95,6 +95,18 @@ default Map<String, String> properties() {
return Collections.emptyMap();
}
+ /**
+ * Returns the string map of properties exposed by commands that display
table metadata.
+ *
+ * <p>The default implementation returns {@link #properties()}.
Implementations may include
+ * transient metadata that should not be treated as persisted table
properties.
+ *
+ * @since 4.4.0
+ */
+ default Map<String, String> displayProperties() {
+ return properties();
Review Comment:
The additive API addresses this: an empty display map now contributes no
extra entries, while Spark still reads `properties()`. Added a
`CatalogV2UtilSuite` regression test with a Mockito mock that stubs only
`properties()` and leaves `displayProperties()` unstubbed. No
`thenCallRealMethod()` is needed for the existing properties to remain visible;
the PR description also notes this.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/Table.java:
##########
@@ -95,6 +95,18 @@ default Map<String, String> properties() {
return Collections.emptyMap();
}
+ /**
+ * Returns the string map of properties exposed by commands that display
table metadata.
+ *
+ * <p>The default implementation returns {@link #properties()}.
Implementations may include
+ * transient metadata that should not be treated as persisted table
properties.
Review Comment:
Adopted the additive form. `displayProperties()` now returns only extra
entries and defaults to an empty map. Spark merges them with the current
`properties()`, gives existing values precedence, and filters reserved keys in
one shared helper. The `SHOW TBLPROPERTIES` test includes a colliding key and
verifies that an `ALTER TABLE ... SET TBLPROPERTIES` update is reflected while
the additional entry is preserved.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/Table.java:
##########
@@ -95,6 +95,18 @@ default Map<String, String> properties() {
return Collections.emptyMap();
}
+ /**
+ * Returns the string map of properties exposed by commands that display
table metadata.
+ *
+ * <p>The default implementation returns {@link #properties()}.
Implementations may include
+ * transient metadata that should not be treated as persisted table
properties.
+ *
+ * @since 4.4.0
+ */
+ default Map<String, String> displayProperties() {
Review Comment:
The API remains table-scoped, and the PR description now states that
explicitly. I added `TableInfo.Builder.withDisplayProperties()`, the
corresponding metadata accessor, and delegation in `DelegatingTable`, with
tests, so the metadata-only table path can use it without subclassing. Views
retain their existing behavior; extending the API to `Relation` would also
require defining and testing the view display contract.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/DescribeTableSuite.scala:
##########
@@ -220,6 +222,37 @@ class DescribeTableSuite extends
command.DescribeTableSuiteBase
}
}
+ test("display commands use display properties without exposing them to SHOW
CREATE TABLE") {
+ withNamespaceAndTable("ns", "table") { tbl =>
+ sql(s"CREATE TABLE $tbl (id bigint) $defaultUsing " +
+ "TBLPROPERTIES ('persisted' = 'stored')")
+
+ val table = loadTable(catalog, "ns", "table")
+ val displayProperties = new util.HashMap[String,
String](table.properties)
+ displayProperties.put("catalog-label", "catalog-value")
+ table.setDisplayProperties(displayProperties)
Review Comment:
Added separate `DESCRIBE TABLE EXTENDED` and `SHOW TABLE EXTENDED` tests for
managed and external tables. Their display maps contain conflicting `external`,
`location`, `comment`, `provider`, and `owner` entries. The assertions verify
that Type and the reserved rows retain the values from `properties()`, while
only the non-reserved additional entry appears in the property list.
--
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]
