szehon-ho commented on code in PR #59036:
URL: https://github.com/apache/spark/pull/59036#discussion_r4151482955
##########
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:
Added an explicit non-null map/keys/values contract to
Table.displayProperties() and the TableInfo builder parameter. This uses the
documented-contract approach rather than silently dropping invalid entries.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/Table.java:
##########
@@ -95,6 +95,31 @@ 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
+ * commands and {@code Catalog.getTableProperties}. Entries in {@link
#properties()} take
+ * precedence, and reserved keys such as {@link TableCatalog#PROP_EXTERNAL}
and
+ * {@link TableCatalog#PROP_LOCATION} are ignored in this map. Table type
and other reserved
+ * metadata are read from {@code properties()}.
+ *
+ * <p>{@code ALTER TABLE ... SET/UNSET TBLPROPERTIES} only changes {@code
properties()}.
Review Comment:
Reworded this as guidance for catalog implementations: Spark forwards
SET/UNSET to alterTable without consulting the display map. The precedence
paragraph now also states that, after a stored property is removed, Spark
displays the entry for that key if it remains in the display map.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableInfo.java:
##########
@@ -59,19 +62,50 @@ public Map<String, String> properties() {
return properties;
}
+ /**
+ * Returns the additional display-only properties described by {@link
Table#displayProperties()}.
+ *
+ * <p>These properties describe metadata returned by a catalog. Spark leaves
them empty when
Review Comment:
Removed the duplicated creation-method list from the builder documentation;
it now lives only on the getter. I kept the scope to the current Spark
behavior: these are returned metadata, and Spark leaves them empty on
creation/staging calls. Since TableInfo is internal for now, I would prefer not
to add a contract for non-Spark callers supplying this field on those paths.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DescribeTableExec.scala:
##########
@@ -198,13 +198,12 @@ case class DescribeTableExec(
CatalogV2Util.TABLE_RESERVED_PROPERTIES
.filterNot(_ == TableCatalog.PROP_EXTERNAL)
.foreach(propKey => {
- if (table.properties.containsKey(propKey)) {
- rows += toCatalystRow(propKey.capitalize,
table.properties.get(propKey), "")
+ if (tableProperties.containsKey(propKey)) {
+ rows += toCatalystRow(propKey.capitalize,
tableProperties.get(propKey), "")
}
})
val properties =
- conf.redactOptions(table.properties.asScala.toMap).toList
- .filter(kv => !CatalogV2Util.TABLE_RESERVED_PROPERTIES.contains(kv._1))
+ conf.redactOptions(CatalogV2Util.tablePropertiesForDisplay(table)).toList
Review Comment:
Changed the helper to accept the stored and display property maps. DESCRIBE
TABLE EXTENDED and SHOW TABLE EXTENDED now pass the stored map already fetched
for the reserved metadata rows, so each reads properties() only once.
##########
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] = {
Review Comment:
Moved redaction into tablePropertiesForDisplay with an explicit SQLConf
parameter and removed the three caller-side wrappers. Added a helper-level test
for stored and display-only secrets, the SQL redaction pattern, stored-value
precedence, and unchanged input maps.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/ShowCreateTableSuite.scala:
##########
@@ -33,6 +35,28 @@ class ShowCreateTableSuite extends
command.ShowCreateTableSuiteBase with Command
override def fullName: String = s"$catalog.$ns.$table"
+ test("show create table excludes display-only properties") {
+ withNamespaceAndTable(ns, table) { t =>
+ sql(s"CREATE TABLE $t (id bigint) $defaultUsing " +
+ "COMMENT 'table comment' LOCATION 'file:/tmp/display-properties' " +
+ "TBLPROPERTIES ('persisted' = 'stored')")
+ loadTable(catalog, ns, table).setDisplayProperties(Map(
Review Comment:
SHOW CREATE TABLE now uses the shared fixture, including all of its reserved
display keys and password entry, and the unused imports are removed. The setter
documentation also states that external = true updates the stored properties.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/DescribeTableSuite.scala:
##########
@@ -220,6 +220,35 @@ class DescribeTableSuite extends
command.DescribeTableSuiteBase
}
}
+ gridTest("display properties preserve table type and reserved metadata")(
Review Comment:
Moved table creation into createTableWithDisplayProperties in
CommandSuiteBase and reused it across DESCRIBE, SHOW TABLE EXTENDED, and SHOW
CREATE TABLE. Reduced both grids to (false, false) and (true, true).
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/CatalogSuite.scala:
##########
@@ -77,6 +77,56 @@ class CatalogSuite extends SparkFunSuite {
assert(loaded.getClass == catalog.getClass)
}
+ test("TableInfo and DelegatingTable carry display-only properties
separately") {
+ val displayProperties = new util.HashMap[String, String]()
+ displayProperties.put("catalog-label", "catalog-value")
+ val info = new TableInfo.Builder()
+ .withProperties(util.Map.of("persisted", "stored"))
+ .withDisplayProperties(displayProperties)
+ .build()
+ displayProperties.put("catalog-label", "changed")
+ val table = new DelegatingTable(info, "table")
+
+ assert(table.displayProperties() === util.Map.of("catalog-label",
"catalog-value"))
+ assert(table.properties() === util.Map.of("persisted", "stored"))
+ intercept[UnsupportedOperationException] {
+ table.displayProperties().put("catalog-label", "changed")
+ }
+ val defaultTable = new DelegatingTable(new TableInfo.Builder().build(),
"default")
+ assert(defaultTable.displayProperties().isEmpty)
+ }
+
+ gridTest("display properties survive copies, pinning and alterations")(
Review Comment:
Renamed the grid cases after the two catalog classes and added a comment
explaining that the row-level catalog always copies on load, while the option
enables this for InMemoryTableCatalog.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/InMemoryTableCatalog.scala:
##########
@@ -168,7 +168,8 @@ class BasicInMemoryTableCatalog extends TableCatalog {
}
override def createTable(ident: Identifier, tableInfo: TableInfo): Table = {
- createTable(ident, tableInfo.columns(), tableInfo.partitions(),
tableInfo.properties(),
+ createTable(
Review Comment:
Restored the original argument wrapping in this method.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DescribeTableExec.scala:
##########
@@ -189,7 +188,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 tableProperties = table.properties()
+ val tableType = if
(tableProperties.containsKey(TableCatalog.PROP_EXTERNAL)) {
Review Comment:
Agreed this is pre-existing and should be fixed separately by honoring
table_type before the external fallback. Leaving that behavior unchanged in
this PR. I have prepared the follow-up JIRA draft, but have not filed it yet.
--
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]
