szehon-ho commented on code in PR #59036:
URL: https://github.com/apache/spark/pull/59036#discussion_r4139217065
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/InMemoryBaseTable.scala:
##########
@@ -75,6 +75,8 @@ abstract class InMemoryBaseTable(
// Stores the table version validated during the last `ALTER TABLE ... ADD
CONSTRAINT` operation.
private var validatedTableVersion: String = null
+ private var tableDisplayProperties: util.Map[String, String] = null
Review Comment:
The fixture now stores only the additional entries and copies them
defensively. Renamed the shared state-copy helper to `copyTableStateFrom()` and
updated its callers to propagate display metadata alongside version state;
table reconstruction during `ALTER TABLE` also preserves it. Added tests for
`copyOnLoad`, the row-level operation catalog, pinning, and alteration,
including checks that pinned properties retain their original values while the
current table reflects the update.
##########
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") {
Review Comment:
Split the coverage across `DescribeTableSuite`, `ShowTblPropertiesSuite`,
`ShowTablesSuite`, and `ShowCreateTableSuite`. The `SHOW CREATE TABLE` test now
uses `getShowCreateDDL()` and compares the full expected DDL.
##########
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)
+
+ val describeRows = sql(s"DESCRIBE TABLE EXTENDED $tbl").collect()
+ val properties = describeRows.find(_.getString(0) == "Table Properties")
+ assert(properties.exists(_.getString(1) ==
+ "[catalog-label=catalog-value,persisted=stored]"))
+
+ val shownProperties = sql(s"SHOW TBLPROPERTIES $tbl").collect()
+ .map(row => row.getString(0) -> row.getString(1)).toMap
+ assert(shownProperties("catalog-label") == "catalog-value")
+ assert(shownProperties("persisted") == "stored")
Review Comment:
Replaced the hand-rolled checks with `checkAnswer` for the selected DESCRIBE
rows and the full `SHOW TBLPROPERTIES` result, plus keyed-property checks.
`SHOW TABLE EXTENDED` and `SHOW CREATE TABLE` now compare the complete expected
output. The fixtures also include reserved keys and a collision with an
existing property.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/InMemoryBaseTable.scala:
##########
@@ -87,6 +89,14 @@ abstract class InMemoryBaseTable(
override def version(): String = tableVersion.toString
+ override def displayProperties(): util.Map[String, String] = {
+ Option(tableDisplayProperties).getOrElse(properties)
+ }
+
+ def setDisplayProperties(displayProperties: util.Map[String, String]): Unit
= {
+ tableDisplayProperties = displayProperties
+ }
Review Comment:
Moved `displayProperties()` and `setDisplayProperties()` immediately after
`updateColumns()`, before `version()`, so the version-related methods stay
together.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/InMemoryBaseTable.scala:
##########
@@ -87,6 +89,14 @@ abstract class InMemoryBaseTable(
override def version(): String = tableVersion.toString
+ override def displayProperties(): util.Map[String, String] = {
+ Option(tableDisplayProperties).getOrElse(properties)
Review Comment:
Applied: the fixture now falls back to `super.displayProperties()`. With the
additive contract, that default is an empty map.
--
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]
