szehon-ho commented on code in PR #59036:
URL: https://github.com/apache/spark/pull/59036#discussion_r4331100523
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2DataFrameSessionCatalogSuite.scala:
##########
@@ -53,6 +54,25 @@ class DataSourceV2DataFrameSessionCatalogSuite
override protected val catalogAndNamespace: String = ""
+ test("display properties survive ALTER TABLE in the session catalog") {
+ val t = "display_properties"
+ withTable(t) {
+ sql(s"CREATE TABLE $t (id bigint) USING $v2Format")
+ val table = sessionCatalog.loadTable(Identifier.of(Array("default"), t))
+ .asInstanceOf[InMemoryTable]
+ table.setDisplayProperties(util.Map.of("catalog-label", "catalog-value"))
+
+ sql(s"ALTER TABLE $t SET TBLPROPERTIES ('k' = 'v')")
+ checkAnswer(sql(s"SHOW TBLPROPERTIES $t"),
+ Seq(Row("catalog-label", "catalog-value"), Row("k", "v")))
+ checkAnswer(sql(s"DESCRIBE TABLE EXTENDED $t").where("col_name = 'Table
Properties'"),
+ Row("Table Properties", "[catalog-label=catalog-value,k=v]", ""))
Review Comment:
Added the `SHOW TABLE EXTENDED` assertion to the session-catalog test. It
checks that the table is listed and its metadata omits `catalog-label`, pinning
the documented limitation.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/CatalogV2UtilSuite.scala:
##########
@@ -35,6 +37,47 @@ import org.apache.spark.sql.util.CaseInsensitiveStringMap
class CatalogV2UtilSuite extends SparkFunSuite {
+ test("tablePropertiesForDisplay preserves properties when a mock has no
display properties") {
+ val table = mock(classOf[Table])
+ when(table.properties()).thenReturn(util.Map.of(
+ "persisted", "stored", TableCatalog.PROP_EXTERNAL, "true"))
+
+ assert(CatalogV2Util.tablePropertiesForDisplay(
+ table.properties(), table.displayProperties(), new SQLConf) ===
Map("persisted" -> "stored"))
+ }
+
+ test("tablePropertiesForDisplay ignores reserved display keys and preserves
stored values") {
+ val displayProperties = new util.HashMap[String, String]()
+ CatalogV2Util.TABLE_RESERVED_PROPERTIES.foreach(displayProperties.put(_,
"display-value"))
+ displayProperties.put("persisted", "display-value")
+ displayProperties.put("catalog-label", "catalog-value")
+ val table = new DelegatingTable(new TableInfo.Builder()
+ .withProperties(util.Map.of("persisted", "stored"))
+ .withDisplayProperties(displayProperties)
+ .build(), "table")
+
+ assert(CatalogV2Util.tablePropertiesForDisplay(
+ table.properties(), table.displayProperties(), new SQLConf) ===
+ Map("persisted" -> "stored", "catalog-label" -> "catalog-value"))
+ assert(table.properties() === util.Map.of("persisted", "stored"))
+ }
+
+ test("tablePropertiesForDisplay redacts stored and display-only values") {
+ val conf = new SQLConf
+ conf.setConf(SQLConf.SQL_OPTIONS_REDACTION_PATTERN, "(?i)catalog-label".r)
+ val properties = util.Map.of("password", "stored-password", "persisted",
"stored")
+ val displayProperties = util.Map.of(
+ "catalog-label", "catalog-value", "secret", "display-secret",
"persisted", "display-value")
+
+ assert(CatalogV2Util.tablePropertiesForDisplay(properties,
displayProperties, conf) === Map(
+ "password" -> "*********(redacted)",
+ "catalog-label" -> "*********(redacted)",
+ "secret" -> "*********(redacted)",
+ "persisted" -> "stored"))
+ assert(properties.get("password") === "stored-password")
Review Comment:
All three helper tests now pass plain maps directly. Removed the mock and
DelegatingTable setup and the redundant stored-map assertion. The redaction
test now uses mutable HashMap inputs and compares both complete maps afterward,
so in-place redaction would fail the input-preservation checks.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/ShowTblPropertiesSuite.scala:
##########
@@ -17,10 +17,48 @@
package org.apache.spark.sql.execution.command.v2
+import org.apache.spark.sql.Row
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')")
+ setDisplayProperties(loadTable(catalog, "ns", "table"))
+
+ checkAnswer(sql(s"SHOW TBLPROPERTIES $tbl"),
+ Seq(Row("catalog-label", "catalog-value"), Row("password",
"*********(redacted)"),
+ Row("persisted", "stored")))
+ checkAnswer(sql(s"SHOW TBLPROPERTIES $tbl ('persisted')"),
Row("persisted", "stored"))
Review Comment:
Added an exact `checkAnswer` for `SHOW TBLPROPERTIES ... ('location')`,
expecting the missing-property message even though the display map contains a
location entry.
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2MetadataTableSuite.scala:
##########
@@ -152,8 +191,10 @@ class TestingDataSourceTableCatalog extends TableCatalog {
throw new RuntimeException("shouldn't be called")
override def renameTable(oldIdent: Identifier, newIdent: Identifier): Unit =
throw new RuntimeException("shouldn't be called")
- override def listTables(namespace: Array[String]): Array[Identifier] =
- throw new RuntimeException("shouldn't be called")
+ override def listTables(namespace: Array[String]): Array[Identifier] = {
+ assert(namespace.toSeq == Seq("display_properties"))
Review Comment:
Changed `listTables` to match the supported namespace and retain the
RuntimeException guard otherwise. Updated the class Scaladoc to describe the V2
metadata fixture and its listing scope.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/CommandSuiteBase.scala:
##########
@@ -77,4 +79,31 @@ trait CommandSuiteBase extends SharedSparkSession {
catalogPlugin.asTableCatalog.loadTable(Identifier.of(Array(schema), table))
.asInstanceOf[InMemoryTable]
}
+
+ def createTableWithDisplayProperties(
+ namespace: String,
+ table: String,
+ external: Boolean = false,
+ withLocation: Boolean = false): Unit = {
+ val location = if (withLocation) "LOCATION 'file:/tmp/display-properties'
" else ""
+ sql(s"CREATE TABLE $catalog.$namespace.$table (id bigint) $defaultUsing " +
+ s"COMMENT 'table comment' $location" +
+ "TBLPROPERTIES ('persisted' = 'stored')")
+ setDisplayProperties(loadTable(catalog, namespace, table), external)
Review Comment:
The shared fixture now obtains the underlying table through
`BasicInMemoryTableCatalog.liveTable` before setting display properties, so the
setup also works for catalogs that copy on load.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/CommandSuiteBase.scala:
##########
@@ -77,4 +79,31 @@ trait CommandSuiteBase extends SharedSparkSession {
catalogPlugin.asTableCatalog.loadTable(Identifier.of(Array(schema), table))
.asInstanceOf[InMemoryTable]
}
+
+ def createTableWithDisplayProperties(
+ namespace: String,
+ table: String,
+ external: Boolean = false,
+ withLocation: Boolean = false): Unit = {
+ val location = if (withLocation) "LOCATION 'file:/tmp/display-properties'
" else ""
+ sql(s"CREATE TABLE $catalog.$namespace.$table (id bigint) $defaultUsing " +
+ s"COMMENT 'table comment' $location" +
+ "TBLPROPERTIES ('persisted' = 'stored')")
+ setDisplayProperties(loadTable(catalog, namespace, table), external)
+ }
+
+ /**
+ * Sets display-only test metadata. When external is true, also marks the
stored table external.
+ */
+ def setDisplayProperties(table: InMemoryTable, external: Boolean = false):
Unit = {
+ if (external) {
+ table.properties.put(TableCatalog.PROP_EXTERNAL, "true")
Review Comment:
The fixture now uses `CREATE EXTERNAL TABLE` when requested. Removed the
direct write to stored `external`; `setDisplayProperties` now sets only display
entries, and its Scaladoc reflects that.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/ShowTblPropertiesSuite.scala:
##########
@@ -17,10 +17,48 @@
package org.apache.spark.sql.execution.command.v2
+import org.apache.spark.sql.Row
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 " +
Review Comment:
Reused `createTableWithDisplayProperties("ns", "table")` in the SHOW
TBLPROPERTIES test, consolidating its setup with the other display-property
tests.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/InMemoryBaseTable.scala:
##########
@@ -85,14 +87,22 @@ abstract class InMemoryBaseTable(
tableColumns = newColumns
}
+ override def displayProperties(): util.Map[String, String] = {
+ Option(tableDisplayProperties).getOrElse(super.displayProperties())
Review Comment:
Wrapped the configured display map in `Collections.unmodifiableMap`, keeping
the accessor read-only after setting or copying display properties as well as
in its default state.
--
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]
