dongjoon-hyun commented on code in PR #59036:
URL: https://github.com/apache/spark/pull/59036#discussion_r4140435786
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/DescribeTableSuite.scala:
##########
@@ -220,6 +222,38 @@ class DescribeTableSuite extends
command.DescribeTableSuiteBase
}
}
+ gridTest("display properties preserve table type and reserved
metadata")(Seq(false, true)) {
+ external =>
+ withNamespaceAndTable("ns", "table") { tbl =>
+ sql(s"CREATE TABLE $tbl (id bigint) $defaultUsing " +
+ "COMMENT 'table comment' LOCATION 'file:/tmp/display-properties' " +
+ "TBLPROPERTIES ('persisted' = 'stored')")
+ val table = loadTable(catalog, "ns", "table")
+ if (external) {
+ table.properties.put(TableCatalog.PROP_EXTERNAL, "true")
+ }
+ table.setDisplayProperties(Map(
Review Comment:
**Apart from `external`, every reserved key in this display map is also
stored on the table, so the test only checks which value wins.** The table is
created with `COMMENT`, `LOCATION`, `USING _` and the default owner, so the
display `comment`, `location`, `provider` and `owner` all collide with stored
values. A display-only `location`, `collation`, `table_type` or
`is_managed_location` is never exercised, which was the ask in
https://github.com/apache/spark/pull/59036#discussion_r4118738223.
For example, suppose the reserved-row loop (`DescribeTableExec` L198-204,
`ShowTablesExtendedExec` L93-99) were changed to read `displayProperties ++
properties` while `Type` stays on `properties()`. A table without a stored
location would then show the display `Location` (or a `Collation` or
`Table_type` row), but all the new tests would still pass because every table
they use has a stored location. The `col_name IN ('Type', 'Comment',
'Location', 'Provider', 'Owner', 'Table Properties')` filter at L246 would also
hide such a leaked row.
Could the display map be built from
`CatalogV2Util.TABLE_RESERVED_PROPERTIES`, as `CatalogV2UtilSuite` does, with
one table created without `LOCATION`, and could the filter cover all the
reserved rows? The same 15-line fixture is repeated in `ShowTablesSuite`
L77-91, so a shared helper next to `loadTable` in the v2 `CommandSuiteBase`
would keep the suites in sync.
##########
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:
**Display-only keys share the key namespace that `ALTER TABLE ... SET/UNSET
TBLPROPERTIES` manage, but those commands only change `properties()`.** With
the fixture in `ShowTblPropertiesSuite` (stored `persisted=stored`; display
`persisted=display-value` and `catalog-label=catalog-value`):
- `ALTER TABLE t UNSET TBLPROPERTIES ('persisted')` removes the stored key,
and then `SHOW TBLPROPERTIES t ('persisted')` returns `display-value` instead
of "Table ... does not have property: persisted". `getTableProperties` still
contains `persisted`, while `SHOW CREATE TABLE` no longer does.
- `ALTER TABLE t UNSET TBLPROPERTIES ('catalog-label')` succeeds, because V2
`UnsetTableProperties` only emits `TableChange.removeProperty` (a no-op for a
missing key, per `TableChange.java` L63), but the key is still listed
afterwards. No SQL can remove it, so a tool that syncs properties by diffing
`getTableProperties` against a desired set never converges.
- `SET TBLPROPERTIES ('catalog-label' = 'x')` followed by `UNSET` brings
`catalog-value` back.
Neither the Javadoc nor the tests cover this; `ShowTblPropertiesSuite` only
exercises `SET`. Could the `Table#displayProperties()` Javadoc state that
`ALTER TABLE ... SET/UNSET TBLPROPERTIES` only affect `properties()`, and that
a display entry shows through once a colliding stored key is unset? An `UNSET`
step in that test would pin it. Alternatively, rendering the display entries
separately (the other option in
https://github.com/apache/spark/pull/59036#discussion_r4118738209) would avoid
the shared namespace.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableInfo.java:
##########
@@ -59,19 +62,40 @@ public Map<String, String> properties() {
return properties;
}
+ /**
+ * Returns the additional display-only properties described by {@link
Table#displayProperties()}.
+ *
+ * @since 4.4.0
+ */
+ public Map<String, String> displayProperties() {
Review Comment:
**`TableInfo` is also the input of `TableCatalog.createTable(Identifier,
TableInfo)`, `createTableLike` and `StagingTableCatalog.stage*`, but this new
field has no documented meaning on that path.**
- Spark never sets it: `CreateTableExec`, `ReplaceTableExec`,
`WriteToDataSourceV2Exec` (CTAS/RTAS), `CreateTableLikeExec` and the pipelines
`DatasetManager` build `TableInfo` without it.
- The default `TableCatalog.createTable(ident, tableInfo)`
(`TableCatalog.java` L359-362) and the `StagingTableCatalog.stage*` defaults
flatten `TableInfo` to `(columns, partitions, properties)` and drop it.
`V2SessionCatalog` and `JDBCTableCatalog` use that default, and
`DelegatingCatalogExtension` doesn't override it, so it never reaches the
delegate.
- The test catalogs in this PR persist it at create time
(`BasicInMemoryTableCatalog.createTable` and
`InMemoryRowLevelOperationTableCatalog.createTable`), and
`InMemoryRelationCatalog` stores the whole `TableInfo`.
So a connector modeled on the test catalogs persists "display-only" entries
at create time. Code that copies a metadata-only table with
`catB.createTable(ident, delegatingTable.getTableInfo())` freezes the source
catalog's transient labels into `catB` on some catalogs and silently drops them
on others. Could the Javadoc here and on `withDisplayProperties` say that Spark
doesn't set it for create, replace, stage or `createTableLike`, and that
catalogs may ignore it there?
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/DelegatingTable.java:
##########
@@ -74,6 +74,11 @@ public Map<String, String> properties() {
return Collections.unmodifiableMap(info.properties());
}
+ @Override
+ public Map<String, String> displayProperties() {
Review Comment:
**The metadata-only path this enables
(`TableInfo.Builder.withDisplayProperties` + `DelegatingTable`) is only covered
by unit tests.** `CatalogSuite` and `CatalogV2UtilSuite` call
`displayProperties()` and `tablePropertiesForDisplay` directly, and every
SQL-level test uses `InMemoryBaseTable.setDisplayProperties`. No test runs
`DESCRIBE TABLE EXTENDED`, `SHOW TBLPROPERTIES` or `SHOW TABLE EXTENDED`
against a catalog that returns a `DelegatingTable`.
That path is special-cased elsewhere: `RelationResolution` L464-468 treats
`DelegatingTable` as an "interpret via v1" sentinel for reads, and
`V1Table.toCatalogTable(catalog, ident, DelegatingTable)` reads only
`info.properties`. If the metadata commands were ever routed the same way,
display entries would disappear for metadata-only catalogs without any failing
test. Could we add one end-to-end test? `InMemoryRelationCatalog` is already
registered in v2 `ShowTablesSuite`, and a catalog that builds the `TableInfo`
in `loadTable`, like `TestingDataSourceTableCatalog` in
`DataSourceV2MetadataTableSuite`, would also work.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/ShowTblPropertiesSuite.scala:
##########
@@ -17,10 +17,35 @@
package org.apache.spark.sql.execution.command.v2
+import scala.jdk.CollectionConverters._
+
+import org.apache.spark.sql.Row
+import org.apache.spark.sql.connector.catalog.TableCatalog
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") {
Review Comment:
**Display-only entries are a new input to three redacted outputs, but no
test checks that they are redacted.** Today `DescribeTableExec` L206,
`ShowTablePropertiesExec` L35 and `ShowTablesExtendedExec` L102 redact the
merged map, which is right. However, the new fixtures only use `catalog-label`,
`persisted` and reserved keys, none of which matches `spark.redaction.regex` or
`spark.sql.redaction.options.regex`. The existing redaction test (`SHOW
TBLPROPERTIES BASIC` in `ShowTblPropertiesSuiteBase`) only covers a stored
`password` property.
If a later change redacted `properties()` before merging, a connector's
display entry such as `password` or `jdbc_url` would be printed in clear by
`DESCRIBE TABLE EXTENDED`, `SHOW TBLPROPERTIES`, `SHOW TABLE EXTENDED` and
`getTableProperties`, and no test would fail. Could this test add a
display-only `password` entry and expect `*********(redacted)`?
##########
sql/core/src/test/scala/org/apache/spark/sql/internal/CatalogSuite.scala:
##########
@@ -1201,6 +1201,32 @@ class CatalogSuite extends SharedSparkSession with
AnalysisTest with BeforeAndAf
spark.catalog.dropTable(t)
}
+ test("catalog API: getTableProperties includes V2 display-only properties") {
+ val catalogName = "display_catalog"
Review Comment:
nit: the suite's `before` block already registers `testcat` as the same
`InMemoryCatalog` class for every test, and the other V2 tests in this suite
use it. Could this test use `testcat` and drop the `withSQLConf` wrapper?
##########
sql/api/src/main/scala/org/apache/spark/sql/catalog/Catalog.scala:
##########
@@ -846,6 +846,10 @@ abstract class Catalog {
/**
* Returns all table properties as a map (same as `SHOW TBLPROPERTIES`).
*
+ * For V2 tables, this also includes the table's display-only properties.
These entries may be
Review Comment:
**The Spark Connect Scala client doesn't get this note.**
`org.apache.spark.sql.connect.Catalog.getTableProperties`
(`sql/connect/common/src/main/scala/org/apache/spark/sql/connect/Catalog.scala`
L853-860) has its own Scaladoc, "Returns all table properties as a map (same as
`SHOW TBLPROPERTIES`).", which replaces this one. The classic `Catalog`
inherits this doc, and PySpark Connect reuses the classic docstring
(`python/pyspark/sql/connect/catalog.py` L381), so only the Connect Scala copy
is stale, although Connect returns the same display-only entries. Could we add
the same paragraph there, or drop that override's doc so it inherits this one?
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/InMemoryTableCatalog.scala:
##########
@@ -168,8 +168,11 @@ class BasicInMemoryTableCatalog extends TableCatalog {
}
override def createTable(ident: Identifier, tableInfo: TableInfo): Table = {
- createTable(ident, tableInfo.columns(), tableInfo.partitions(),
tableInfo.properties(),
+ val table = createTable(
+ ident, tableInfo.columns(), tableInfo.partitions(),
tableInfo.properties(),
Distributions.unspecified(), Array.empty, None, None,
tableInfo.constraints())
+
table.asInstanceOf[InMemoryBaseTable].setDisplayProperties(tableInfo.displayProperties())
Review Comment:
**Only two in-memory catalogs honor `TableInfo.displayProperties()` at
create time, and this one needs a cast.** This path and
`InMemoryRowLevelOperationTableCatalog.createTable` set it, but the other
`createTable(ident, TableInfo)` and `stage*` overrides ignore it. That includes
`InMemoryPartitionTableCatalog` (L69-71), which is the v2 command suites'
`test_catalog`, and
`StagingInMemoryTableCatalog.stageCreate/stageReplace/stageCreateOrReplace`,
which is the CTAS/RTAS path of `atomic_test_catalog`.
`NullTableIdInMemoryTableCatalog.createTable` (L39-47) calls
`super.createTable`, which now sets the map, and then replaces the table with
`new InMemoryTable(..., id = null)` without `copyTableStateFrom`, unlike its
`NullColumnId`, `NullTableIdAndNullColumnId` and `MixedColumnId` siblings.
So a `TableInfo` built with `withDisplayProperties(...)` keeps its entries
on `non_part_test_catalog` and `row_level_op_catalog`, but loses them on
`test_catalog`, on the `atomic_test_catalog` stage path and on
`NullTableIdInMemoryTableCatalog`. Since Spark never sets this field at create
time and only the catalyst `CatalogSuite` grid test uses this path, could that
test set the entries on the live table returned by `createTable`, as the
sql/core tests do? Then this change, the one in
`InMemoryRowLevelOperationTableCatalog.createTable` and the cast could go.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/Table.java:
##########
@@ -95,6 +95,24 @@ 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
Review Comment:
**For the session catalog, `SHOW TABLE EXTENDED` doesn't include these
entries even when the catalog returns a V2 table.** `ResolveSessionCatalog`
L397-406 rewrites every `ShowTablesExtended(ResolvedV1Database(db), ...)` to
the v1 `ShowTablesCommand`, while `DESCRIBE TABLE EXTENDED` and `SHOW
TBLPROPERTIES` fall back to v1 only for a `V1Table`. So for a
`CatalogExtension` that returns its own `Table` with `displayProperties()` =
`{label=x}`, `DESCRIBE TABLE EXTENDED`, `SHOW TBLPROPERTIES` and
`getTableProperties` show `label`, but `SHOW TABLE EXTENDED IN spark_catalog.db
LIKE 't'` doesn't.
The PR description explains this, but it isn't in the API docs that
connector authors read. Could we add something like "Commands that run through
V1 implementations, such as `SHOW TABLE EXTENDED` on the session catalog, do
not include these entries."?
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/InMemoryBaseTable.scala:
##########
@@ -85,25 +87,25 @@ abstract class InMemoryBaseTable(
tableColumns = newColumns
}
+ override def displayProperties(): util.Map[String, String] = {
+ Option(tableDisplayProperties).getOrElse(super.displayProperties())
+ }
+
+ def setDisplayProperties(displayProperties: util.Map[String, String]): Unit
= {
+ tableDisplayProperties = new util.HashMap[String,
String](displayProperties)
+ }
+
override def version(): String = tableVersion.toString
def setVersion(version: String): Unit = {
tableVersion = version.toInt
}
/**
- * Copies version and validated version from another table.
- *
- * Some test catalogs (e.g. [[NullColumnIdInMemoryTableCatalog]],
- * [[NullTableIdAndNullColumnIdInMemoryTableCatalog]]) create a new table
object
- * that overrides specific behavior (such as nulling out column IDs). The new
- * object's version counter starts at 0. Without this call, the version
counter
- * resets every time the catalog creates such a replacement table, breaking
the
- * monotonic-version assumption that downstream consumers rely on (e.g.
- * [[InMemoryTable]].copy, validated-version propagation, and the
join-refresh
- * tests in [[DSv2IncrementallyConstructedQueryTests]]).
+ * Copies version, validated version and display-only metadata into a
replacement table.
Review Comment:
**The rename replaced the explanation of why the version must be copied with
a one-line summary.** The previous Scaladoc said that replacement tables start
with a version counter of 0, and that without this call the counter resets and
breaks the monotonic-version assumption that `InMemoryTable.copy`,
validated-version propagation and the join-refresh tests in
`DSv2IncrementallyConstructedQueryTests` rely on. That is still true (the
version-aware `equals`/`hashCode` below uses it), and it isn't documented
anywhere else in the code, so the next person who adds a catalog that rebuilds
tables may skip this call. Also, 5 of the 11 callers are `copy()` snapshots
rather than replacement tables. Could we keep the old paragraph and just add
the display-only metadata to the first sentence?
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/ShowTblPropertiesSuite.scala:
##########
@@ -17,10 +17,35 @@
package org.apache.spark.sql.execution.command.v2
+import scala.jdk.CollectionConverters._
+
+import org.apache.spark.sql.Row
+import org.apache.spark.sql.connector.catalog.TableCatalog
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')")
+ loadTable(catalog, "ns", "table").setDisplayProperties(Map(
+ "catalog-label" -> "catalog-value",
+ "persisted" -> "display-value",
+ TableCatalog.PROP_EXTERNAL -> "true",
+ TableCatalog.PROP_LOCATION -> "file:/display-location").asJava)
+
+ checkAnswer(sql(s"SHOW TBLPROPERTIES $tbl"),
+ Seq(Row("catalog-label", "catalog-value"), Row("persisted", "stored")))
+ checkAnswer(sql(s"SHOW TBLPROPERTIES $tbl ('persisted')"),
Row("persisted", "stored"))
+ checkAnswer(sql(s"SHOW TBLPROPERTIES $tbl ('catalog-label')"),
+ Row("catalog-label", "catalog-value"))
+
+ sql(s"ALTER TABLE $tbl SET TBLPROPERTIES ('persisted' = 'updated')")
Review Comment:
**This holds for `InMemoryTableCatalog`, but the session-catalog test
catalog still drops display state on `ALTER TABLE`.**
`InMemoryTableSessionCatalog.alterTable`
(`DataSourceV2DataFrameSessionCatalogSuite.scala` L160-181) rebuilds the table
as `new InMemoryTable(table.name, schema, table.partitioning, properties)`
without copying the display properties. That catalog is how the tests model a
session-catalog extension that returns V2 tables (`DatasourceV2SQLBase`
registers it as `spark_catalog`), which is the case the PR description says
`DESCRIBE TABLE EXTENDED` and `SHOW TBLPROPERTIES` support. There, after
`setDisplayProperties` on the live table and `ALTER TABLE t SET TBLPROPERTIES
('k' = 'v')`, `SHOW TBLPROPERTIES t` no longer lists the display entry.
No current test hits this, and that rebuild already reset the version before
this PR. But since
https://github.com/apache/spark/pull/59036#discussion_r4139217065 says table
reconstruction during `ALTER TABLE` preserves the entries, could it also call
`setDisplayProperties(table.displayProperties())`?
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/InMemoryTableCatalog.scala:
##########
@@ -276,6 +279,7 @@ class BasicInMemoryTableCatalog extends TableCatalog {
s"Unsupported InMemoryBaseTable subclass: ${other.getClass.getName}")
}
newTable.setVersion(currentVersion)
+ newTable.setDisplayProperties(table.displayProperties())
Review Comment:
nit: this copies the table state by hand instead of calling
`copyTableStateFrom`. Now that the helper is the generic "copy table state"
hook, the most common `ALTER TABLE` path (`test_catalog`,
`InMemoryTableCatalog`, `InMemoryCatalog`) still keeps its own list:
`setVersion` + `setDisplayProperties`, and `validatedVersion` only for
`AddConstraint`. `InMemoryRowLevelOperationTableCatalog.alterTable` carries
`validatedVersion` over through the helper instead. If a field is later added
only to `copyTableStateFrom`, this path resets it, and the catalogs that wrap
`super.alterTable` (`NullColumnIdInMemoryTableCatalog`,
`MixedColumnIdTableCatalog`, `TypeChangeResetsColIdTableCatalog`, ...) copy
from the already-reset table. Could this call
`newTable.copyTableStateFrom(table)` and keep the `AddConstraint` loop, or at
least carry a comment to keep the two in sync?
##########
python/pyspark/sql/catalog.py:
##########
@@ -383,6 +383,12 @@ def getTableProperties(self, tableName: str) -> Dict[str,
str]:
.. versionadded:: 4.2.0
+ Notes
Review Comment:
nit: this `Notes` section comes before `Parameters`. The other `Notes`
sections in this file (e.g. `listViews`, `functionExists` and `listColumns`)
come after `Returns` and before `Examples`, which is also the numpydoc order.
The rendered API docs reorder it, but `help(spark.catalog.getTableProperties)`
and IDEs show the raw order. Could it move below `Returns`?
--
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]
