dongjoon-hyun commented on code in PR #59036:
URL: https://github.com/apache/spark/pull/59036#discussion_r4118738185
##########
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:
This also changes the public API `spark.catalog.getTableProperties`, because
`classic.Catalog.getTableProperties` is implemented by executing a
`ShowTableProperties` plan. PySpark and Spark Connect
(`SparkConnectPlanner.transformGetTableProperties`) go through the same path.
The API was released in 4.2.0 and is documented as "Returns all table
properties as a map (same as `SHOW TBLPROPERTIES`)".
Take a connector whose `displayProperties()` returns `properties() +
{"catalog-label" -> "catalog-value"}`:
- `spark.catalog.getTableProperties("cat.ns.t")` contains `catalog-label`.
- `spark.catalog.getCreateTableString("cat.ns.t")` doesn't.
User code that copies table properties via `getTableProperties(src)` into
`CREATE TABLE ... TBLPROPERTIES`, `ALTER TABLE ... SET TBLPROPERTIES` or
`DataFrameWriterV2.tableProperty` would then persist the transient label on the
target table. That is the leak this PR prevents for `SHOW CREATE TABLE`. The
existing `getTableProperties` tests only use V1 tables, so they don't cover
this.
Could you either keep `getTableProperties` on `properties()`, or document
this behavior change in the PR description and the API doc, with a test?
##########
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:
This switches not only the free-form `Table Properties` list but also the
`Type` row (EXTERNAL/MANAGED via `PROP_EXTERNAL`) and the reserved-property
rows (Comment, Collation, Location, Provider, Owner, ...) to
`displayProperties()`. `ShowTablesExtendedExec` does the same.
However, the new Javadoc doesn't say that `displayProperties()` must be a
superset of `properties()`. Suppose a connector takes "display-only properties"
literally and returns only its labels, e.g. `Map.of("label", "x")`. For an
external table:
- `DESCRIBE TABLE EXTENDED` and `SHOW TABLE EXTENDED` show `Type MANAGED`
and drop the Location/Provider/Owner rows.
- `SHOW TBLPROPERTIES t ('k')` returns "does not have property: k" for a
persisted key `k`.
Meanwhile `SHOW CREATE TABLE` and
`spark.catalog.getTable(...).tableType`/`description` still read
`properties()`, so they disagree. Also, a display key named like a reserved key
(`owner`, `location`, `external`, ...) silently overrides that row. Because the
check is `containsKey(PROP_EXTERNAL)`, even `external=false` shows `EXTERNAL`.
Could we keep `Type` and the reserved rows on `table.properties()` and use
`displayProperties()` only for the `Table Properties` list? If not, please
document the superset and reserved-key contract in the Javadoc.
##########
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:
Minor: the display commands now call this new default method instead of
`properties()`. Under Mockito's default answer (`RETURNS_DEFAULTS`), a mock
doesn't run default methods, so a `Table` mock returns an empty `HashMap` here.
Take a mock that only stubs `when(t.properties()).thenReturn(Map("external" ->
"true", "k" -> "v").asJava)`:
- `DescribeTableExec` now gives `Type MANAGED` and `Table Properties []`.
- `SHOW TBLPROPERTIES` returns 0 rows.
Spark's own tests don't do this. Downstream connector tests may change
without warning, though, and would need `thenCallRealMethod()`. A note in the
PR description may be worth it.
##########
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:
The display map here is a full copy of `table.properties` plus one
non-reserved key, so this test can't tell whether the `Type` row and the
reserved rows come from `displayProperties()` or from `properties()`. Reverting
`DescribeTableExec` (L193, L202-203) or `ShowTablesExtendedExec` (L86, L96-97)
back to `table.properties()` still passes. Could you add a display-only
reserved key (e.g. `external` or `location`) and assert the `Type`/`Location`
rows? Alternatively, pin whichever behavior we settle on in the
`DescribeTableExec` comment.
##########
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:
This per-instance state doesn't survive when the in-memory test infra copies
or rebuilds a table:
- `InMemoryTable.copy()` and the other `copy()` implementations only call
`setVersionAndValidatedVersionFrom`.
- `InMemoryTableCatalog.alterTable` rebuilds the table via
`newInMemoryTable`.
- `pinTable` copies the table.
`setDisplayProperties` is then silently lost and `displayProperties()` falls
back to `properties`. That happens with:
- `copyOnLoad=true` catalogs, e.g. `CachedTableSuite` and
`DataSourceV2DataFrameSuite`.
- `row_level_op_catalog`, which always returns `rlot.copy()` and is
registered in the same v2 `CommandSuiteBase`.
- Any `ALTER TABLE`.
The new test passes only because `test_catalog` returns the live instance.
Could you document this limitation, or store only the extra entries and
propagate them on copy?
##########
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:
Question: `properties()` is declared on `Relation`, which `View` also
implements, and v2 views have the same problem. `ShowCreateV2ViewExec` renders
the non-reserved `View.properties()` into `TBLPROPERTIES`, and
`DescribeV2ViewExec` and `ShowV2ViewPropertiesExec` read `viewInfo.properties`
directly.
The documented metadata-only path (`TableInfo.Builder` -> `DelegatingTable`)
can't carry display-only metadata either. `DelegatingTable` doesn't override
this method and `TableInfo` has no such field, so a catalog would have to
subclass `DelegatingTable`.
Is it intentional to scope this to `Table` only? Declaring it on `Relation`,
with a setter on `RelationBuilder` (shared by `TableInfo.Builder` and
`View.Builder`), would cover both paths. Adding it later would also be
compatible, but it would help to state the intended scope.
##########
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:
nit: these two members are inserted between `version()` and `setVersion()`,
which splits the version-related members (`version`, `setVersion`,
`setVersionAndValidatedVersionFrom`, ..., `setValidatedVersion`). `AGENTS.md`
also asks us to follow the file's existing sectioning when adding members.
Could we move them before `version()` (after `updateColumns`, following the
`columns`/`properties`/`displayProperties` order in `Table`), or after
`setValidatedVersion`?
##########
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:
nit: delegating to the interface default makes the intent clearer: behave
like a regular connector when the display properties are unset.
```suggestion
Option(tableDisplayProperties).getOrElse(super.displayProperties())
```
##########
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:
FYI, the session catalog never reaches this exec. For `spark_catalog`
(builtin, or a `CatalogExtension` such as Delta/Iceberg-style session
catalogs), `ResolveSessionCatalog` always rewrites
`ShowTablesExtended(ResolvedV1Database(db), ...)` to the v1
`ShowTablesCommand`; there is no `useV1Command` guard. That command prints
`CatalogTable.simpleString` and never calls `loadTable()` or
`displayProperties()`.
`DESCRIBE TABLE EXTENDED` and `SHOW TBLPROPERTIES`, by contrast, fall back
to v1 only for `V1Table`. When the extension returns its own `Table`
implementation, those two use the v2 execs. So for the same session-catalog
table, the display-only keys appear in `DESCRIBE TABLE EXTENDED` and `SHOW
TBLPROPERTIES` but not in `SHOW TABLE EXTENDED`.
The PR description says the metadata appears in `SHOW TABLE EXTENDED` with
no conditions. Could you document the scope, or add a session-catalog test that
pins the current behavior?
##########
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:
Design question: this models display-only metadata as a second full property
map that defaults to `properties()`.
- An implementation has to copy the persisted properties just to add one
label, as the test does with `new util.HashMap[String,
String](table.properties)`.
- There are two sources of truth for the same keys. A cached display map can
go stale after `ALTER TABLE ... SET TBLPROPERTIES` while `SHOW CREATE TABLE`
shows the new value.
Have you considered an additive form instead? For example, a map of
catalog-owned extras that defaults to empty, which Spark merges as
`properties() ++ extras` (persisted/reserved keys win) or renders as separate
rows. That matches existing additive patterns such as `SupportsMetadataColumns`
and the `Statistics`/`Metadata Columns`/`Constraints` sections of `DESCRIBE`,
and it removes the superset-contract question. Since this is a new public API
(`@since 4.4.0`), it would be good to settle the shape before it is released.
##########
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:
nit: `checkAnswer` gives an exact comparison and a diff on failure.
`assert(properties.exists(...))` only reports "... was false", and the `toMap`
+ per-key asserts can't detect unexpected extra rows such as a leaked reserved
key. For example:
```scala
checkAnswer(
sql(s"DESCRIBE TABLE EXTENDED $tbl").where("col_name = 'Table
Properties'"),
Row("Table Properties", "[catalog-label=catalog-value,persisted=stored]",
""))
checkAnswer(
sql(s"SHOW TBLPROPERTIES $tbl"),
Seq(Row("catalog-label", "catalog-value"), Row("persisted", "stored")))
```
##########
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:
nit: this test checks `SHOW TBLPROPERTIES`, `SHOW TABLE EXTENDED` and `SHOW
CREATE TABLE` inside the `DESCRIBE TABLE` suite ("contains tests for the
`DESCRIBE TABLE` command"). It runs as "DESCRIBE TABLE using V2 catalog V2
command: ...", so people working on `ShowTblPropertiesSuite`, `ShowTablesSuite`
or `ShowCreateTableSuite` can easily miss it. `loadTable` is available in all
v2 command suites, so could we split this per command? The `SHOW CREATE TABLE`
part could then reuse `getShowCreateDDL`.
--
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]
