dongjoon-hyun commented on code in PR #59036:
URL: https://github.com/apache/spark/pull/59036#discussion_r4151303971
##########
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:
**These two sentences describe the catalog's behavior as Spark's.** On this
path Spark never reads the display entries: `SetTableProperties` and
`UnsetTableProperties` only produce `TableChange.setProperty` and
`TableChange.removeProperty`, and `AlterTableExec.scala` L41 passes them to
`TableCatalog.alterTable`. Whether a display entry survives `SET`, or
disappears on `UNSET`, is up to the catalog:
- `InMemoryTableSessionCatalog.alterTable` dropped the display state on
every `ALTER TABLE` until R2-9 was fixed.
- A catalog that handles `removeProperty("label")` by deleting its label
contradicts "cannot be removed with UNSET TBLPROPERTIES".
Also, the second half of R2-1, that a display value shows through once a
colliding stored key is unset, is only in the PR description. Could this
paragraph be phrased as guidance for implementors? For example:
```java
<p>Spark passes {@code ALTER TABLE ... SET/UNSET TBLPROPERTIES} to
{@link TableCatalog#alterTable} as {@link TableChange#setProperty} and
{@link TableChange#removeProperty} without consulting this map.
Implementations should apply
them to {@code properties()} only and keep display-only entries unchanged.
Then
{@code UNSET TBLPROPERTIES} of a display-only key succeeds but the key is
still listed, and
unsetting a stored key that collides with a display-only key exposes the
display-only value.
```
##########
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:
Minor: R2-2 also asked to say that catalogs may ignore this field on the
create path, and that half is missing here and in the setter (L96-99). The
default `TableCatalog.createTable(Identifier, TableInfo)` and the
`StagingTableCatalog.stage*` defaults drop it, but a catalog that stores the
whole `TableInfo`, such as `InMemoryRelationCatalog`, persists whatever a
non-Spark caller put there, e.g. `catB.createTable(ident,
delegatingTable.getTableInfo())`. Without that sentence, implementors can't
tell whether to ignore it or store it. Could we add something like "Catalogs
may ignore them in these methods."? The setter could then just refer to `{@link
TableInfo#displayProperties()}`, so the five-method list lives in one place.
##########
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:
**The new API has no null contract, and this merge passes null values
through.** `TableInfo.Builder.withDisplayProperties` copies the map with `new
HashMap<>(...)`, which keeps null values, and `Utils.redact` passes them
through. Take a connector that returns a `DelegatingTable` whose display map
has `"last_refresh" -> null` for a value it doesn't know yet:
- `SHOW TBLPROPERTIES t`, `SHOW TBLPROPERTIES t ('last_refresh')` and
`spark.catalog.getTableProperties` fail, because `ShowTableProperties` declares
`key` and `value` as non-nullable (`v2Commands.scala` L1583-1584) and
`toCatalystRow` rejects the null.
- `DESCRIBE TABLE EXTENDED` and `SHOW TABLE EXTENDED` print
`last_refresh=null`.
If `displayProperties()` itself returns null, `asScala` returns null and
`.toMap` throws an NPE in all three commands. `ShowTablesExtendedExec` has no
per-table error handling, so one such table fails `SHOW TABLE EXTENDED IN
cat.ns LIKE '*'` for the whole namespace.
`properties()` has the same exposure, but the SQL path rejects null property
values, while these entries are computed by the catalog and may be transient.
Could the `Table#displayProperties()` Javadoc say that the map and its keys and
values must not be null, as `TableCatalog#tableStateOptionKeys()` does with "a
non-null set"? Alternatively, this helper could drop null entries.
##########
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:
nit: the other v2 suites use the shared `setDisplayProperties` helper from
`CommandSuiteBase`, which this suite also mixes in. `ShowCreateTableExec` reads
only `table.properties`, so `setDisplayProperties(loadTable(catalog, ns,
table))` keeps the expected DDL, also covers display-only `provider`,
`collation`, `owner` and `password`, and lets us drop
`scala.jdk.CollectionConverters._` (L20) and `TableCatalog` (L25). Separately,
could the helper's Scaladoc mention that `external = true` writes `external`
into the stored properties? It shares its name with
`InMemoryBaseTable.setDisplayProperties`, which only sets display entries.
##########
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:
nit: the two modes are named after different things, an option and a catalog
class. L106 passes `copyOnLoad=true` to both, but
`InMemoryRowLevelOperationTableCatalog.loadTable` always returns `rlot.copy()`,
so the option has no effect in the `rowLevel` case, and the names don't say
what actually differs. Could the modes be named after the catalogs, e.g.
`Seq("InMemoryTableCatalog", "InMemoryRowLevelOperationTableCatalog")`, with a
short comment on the option?
##########
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:
nit: after the create-time `setDisplayProperties` was removed for R2-8, this
re-wrap is the only change left in this method, and the arguments are
unchanged. Could we restore the original single line to keep the diff and `git
blame` clean?
##########
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:
nit: L191 hoists `table.properties()`, but
`tablePropertiesForDisplay(table)` reads `table.properties()` again, so this
command still reads it twice per table, and so does
`ShowTablesExtendedExec.scala` (L85 and L102). Could the helper take the map
the caller already has, e.g. `tablePropertiesForDisplay(tableProperties,
table.displayProperties())`? Then the `Type` and reserved rows also come from
the same snapshot as the `Table Properties` row.
##########
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:
Minor, pre-existing: this still decides `Type` by
`containsKey(PROP_EXTERNAL)` alone and ignores `PROP_TABLE_TYPE`, although
`withTableType` is the only type setter on `TableInfo.Builder`. For a
`DelegatingTable` built with
`.withTableType(TableSummary.EXTERNAL_TABLE_TYPE)`, like the existing
`test_json` fixture in `DataSourceV2MetadataTableSuite`:
- `DESCRIBE TABLE EXTENDED` prints both `Type | MANAGED` and `Table_type |
EXTERNAL`.
- `SHOW TABLE EXTENDED` prints `Type: MANAGED`
(`ShowTablesExtendedExec.scala` L86).
- `SHOW TABLE EXTENDED ... AS JSON` (through `listTableSummaries`) and
`V1Table.toCatalogTable` treat it as `EXTERNAL`.
This is the `external=false` case from R1-2 seen from another side. It isn't
introduced by this PR, and fixing it changes pinned outputs, so could we file a
follow-up JIRA to derive `Type` from `table_type` first and fall back to
`external`?
##########
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:
nit: all three callers wrap this helper in the same
`conf.redactOptions(...)`: `DescribeTableExec.scala` L206,
`ShowTablePropertiesExec.scala` L35 and `ShowTablesExtendedExec.scala` L102.
Since the name says "for display", a future caller could print the result as
is, and these entries can hold secrets, as the display-only `password` in the
new fixture shows. Could the helper take a `conf: SQLConf`, like
`lookupCachedRelationWithStateOptions` in this object, and redact inside? If
you prefer to keep redaction at the call sites, could the Scaladoc say that the
values are not redacted?
##########
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:
nit: this gridTest and the one at `ShowTablesSuite.scala` L72-80 differ only
in the table name. The grid, the `LOCATION` literal, the `CREATE TABLE`
statement and the `setDisplayProperties(loadTable(...), external)` call are
copies, so the shared helper from R2-3 keeps only the display map in sync.
Also, the two axes are independent (`Type` depends only on the stored
`external`, and `Location` only on the stored `location`), so `(false, true)`
and `(true, false)` add no coverage, while `(true, true)` still checks that a
display `external=false` and a display `location` don't override the stored
values. Could the table creation move into the `CommandSuiteBase` helper, and
the grid become `Seq((false, false), (true, true))`?
--
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]
