dongjoon-hyun commented on code in PR #59036:
URL: https://github.com/apache/spark/pull/59036#discussion_r4300312848
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/Table.java:
##########
@@ -95,6 +95,34 @@ 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()}.
+ * After a stored property is removed, Spark displays the entry for that key
if it remains in
Review Comment:
**This sentence doesn't hold for `comment`, the one ignored key that
`SET/UNSET TBLPROPERTIES` accept.** `AstBuilder.cleanTableProperties` keeps
`comment` (`AstBuilder.scala` L6058-6059: "we don't make it a reserved table
property"), so `ALTER TABLE t UNSET TBLPROPERTIES ('comment')` reaches
`alterTable` as `TableChange.removeProperty("comment")`. However,
`tablePropertiesForDisplay` and the reserved-row loops still treat `comment` as
one of `TABLE_RESERVED_PROPERTIES`. Take a table created with `COMMENT 'c'`
whose `displayProperties()` contains `comment=d`. After that `UNSET`:
- `DESCRIBE TABLE EXTENDED` has no `Comment` row and no `comment` entry in
`Table Properties`.
- `SHOW TBLPROPERTIES t ('comment')` returns "does not have property:
comment".
So the stored property was removed and `comment` remains in this map, but
nothing is displayed. The two examples on L103-104 (`external`, `location`) are
keys that the parser rejects in `TBLPROPERTIES`, so they don't hint at this
exception, and the wording I suggested in R3-2 had the same gap. Could we name
`{@link TableCatalog#PROP_COMMENT}` among the ignored keys and scope this
sentence to non-reserved keys, e.g. "After a stored non-reserved property is
removed, ..."?
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/Table.java:
##########
@@ -95,6 +95,34 @@ 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()}.
+ * After a stored property is removed, Spark displays the entry for that key
if it remains in
+ * this map.
+ *
+ * <p>Spark passes {@code ALTER TABLE ... SET/UNSET TBLPROPERTIES} to
+ * {@link TableCatalog#alterTable} without consulting this map. Catalogs
should apply these
+ * changes to {@code properties()} and keep display-only entries unchanged.
+ *
+ * <p>For now, {@code SHOW TABLE EXTENDED} omits these entries for the
built-in session catalog
+ * and its {@link CatalogExtension}s.
+ *
+ * <p>These entries may be transient and are excluded from {@code SHOW
CREATE TABLE}. The
Review Comment:
**`CREATE TABLE ... LIKE` hands the source table, including these entries,
to the target catalog.** `DataSourceV2Strategy` L294 passes the resolved source
`Table` as is, and `CreateTableLikeExec` L71 calls
`createTableLike(targetIdent, tableInfo, sourceTable)`. The `createTableLike`
Javadoc says that connectors "may read format-specific properties or other
custom state from this object to clone additional metadata"
(`TableCatalog.java` L382-383, see also L372-373). With this PR, that object
also exposes `displayProperties()`, and a `DelegatingTable` also exposes
`getTableInfo().displayProperties()`. Nothing tells the connector not to clone
them, so a catalog that does would persist the source catalog's transient
labels on the new table, which is what this PR keeps out of `SHOW CREATE
TABLE`. Unlike R2-2 and R3-3, which were about non-Spark callers filling
`TableInfo`, here Spark itself makes the call. Could this sentence also cover
it, e.g. "These entries may be transient. Spark e
xcludes them from `SHOW CREATE TABLE`, and `TableCatalog#createTableLike`
implementations should not copy them from the source table."?
`InMemoryTableCatalog.createTableLike` (L324) already copies only
`sourceTable.properties()`.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableInfo.java:
##########
@@ -59,19 +62,46 @@ 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
+ * calling {@code TableCatalog.createTable},
+ * {@code TableCatalog.createTableLike}, {@code
StagingTableCatalog.stageCreate},
+ * {@code StagingTableCatalog.stageReplace}, and {@code
StagingTableCatalog.stageCreateOrReplace}.
+ *
+ * @since 4.4.0
+ */
+ public Map<String, String> displayProperties() {
+ return displayProperties;
+ }
+
public Transform[] partitions() {
return partitions;
}
public Constraint[] constraints() { return constraints; }
public static class Builder extends RelationBuilder<Builder> {
+ protected Map<String, String> displayProperties = new HashMap<>();
protected Transform[] partitions = new Transform[0];
protected Constraint[] constraints = new Constraint[0];
@Override
protected Builder self() { return this; }
+ /**
+ * Sets additional display-only properties.
+ *
+ * @param displayProperties a non-null map with non-null keys and values
+ * @see Table#displayProperties()
Review Comment:
nit: after the R3-3 change, the setter no longer has the create/stage note,
but its only cross-reference is still `@see Table#displayProperties()`, and
`Table#displayProperties()` never mentions `TableInfo`. So
`withDisplayProperties` has no link to the note that now lives only at L68-71;
R3-3 suggested `{@link TableInfo#displayProperties()}` here. Separately, the
five method names at L69-71 are `{@code ...}` spans, which doclint doesn't
resolve, and four of them have three overloads each (e.g.
`TableCatalog.createTable` at `TableCatalog.java` L320, L335 and L359).
Elsewhere in this package they are linked with full signatures, e.g. `{@link
TableCatalog#createTable(Identifier, TableInfo)}` in `StagingTableCatalog.java`
L44, so `{@link ...(Identifier, TableInfo)}` here would make them checked and
clickable. Optionally, the class summary at L28 ("its columns, properties,
partitioning and constraints") could mention the display-only properties too.
##########
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:
nit: some assertions in these three helper tests can't fail.
- The first test (L40-47) calls `table.displayProperties()` on the mock
itself. Mockito's default answer returns a new empty `HashMap` there without
running `Table.displayProperties()` or any exec, so the test is the same as
`tablePropertiesForDisplay(util.Map.of(...), new util.HashMap(), new SQLConf)`
and stays green even if the default method or an exec regresses.
- In the second test, the `DelegatingTable` only passes back read-only views
of copies of its two maps, and L62 repeats the assertion at `CatalogSuite` L91.
- L77-78 read `util.Map.of` maps, which throw
`UnsupportedOperationException` on any write, so a helper that redacted in
place would fail at L72 first. These two asserts can never fail, although the
R3-6 reply counts them as the "unchanged input maps" check.
Could we call the helper on plain maps in all three tests, and either drop
L62 and L77-78 or pass mutable `new util.HashMap(...)` copies if the inputs
should be checked?
##########
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:
nit: `CREATE EXTERNAL TABLE ... LOCATION` should work on this V2 catalog, so
the fixture doesn't need to write `external` into the stored map by hand. For a
non-session catalog, the parser keeps the `EXTERNAL` flag, the LOCATION
requirement exists only in the V1 `SessionCatalog.createTable`, and
`CreateTableExec` stores `external=true` through
`CatalogV2Util.convertTableProperties`. That is the same path that already
stores `external=true` for `spark.catalog.createTable` with a path on
`testcat`, and the reserved-key error points users to it ("please use CREATE
EXTERNAL TABLE"). Could `createTableWithDisplayProperties` use `CREATE ${if
(external) "EXTERNAL " else ""}TABLE ...` and drop L99-101? Then
`setDisplayProperties` only sets display entries, like
`InMemoryBaseTable.setDisplayProperties`, and the caveat in its Scaladoc (L96)
is no longer needed. The conflicting display `external` value can still come
from the argument.
##########
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:
nit: until display properties are set, this returns the interface default
`Collections.emptyMap()`, which rejects `put`. After `setDisplayProperties` or
any `copyTableStateFrom` call (every `copy()`, `alterTable` and `pinTable`
path), it returns the internal `HashMap` itself, so
`t.displayProperties().put(...)` silently changes the table's state. No current
test mutates it, so this is latent, but `DelegatingTable` returns a read-only
view, which the new catalyst `CatalogSuite` test pins. Returning
`Collections.unmodifiableMap(tableDisplayProperties)` here would make both
states read-only and leave `setDisplayProperties` as the only way to change
them.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableInfo.java:
##########
@@ -59,19 +62,46 @@ 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
+ * calling {@code TableCatalog.createTable},
+ * {@code TableCatalog.createTableLike}, {@code
StagingTableCatalog.stageCreate},
+ * {@code StagingTableCatalog.stageReplace}, and {@code
StagingTableCatalog.stageCreateOrReplace}.
+ *
+ * @since 4.4.0
+ */
+ public Map<String, String> displayProperties() {
+ return displayProperties;
+ }
+
public Transform[] partitions() {
return partitions;
}
public Constraint[] constraints() { return constraints; }
public static class Builder extends RelationBuilder<Builder> {
+ protected Map<String, String> displayProperties = new HashMap<>();
protected Transform[] partitions = new Transform[0];
protected Constraint[] constraints = new Constraint[0];
@Override
protected Builder self() { return this; }
+ /**
+ * Sets additional display-only properties.
+ *
+ * @param displayProperties a non-null map with non-null keys and values
+ * @see Table#displayProperties()
+ * @since 4.4.0
+ */
+ public Builder withDisplayProperties(Map<String, String>
displayProperties) {
Review Comment:
**Spark reads these entries only in display commands, but this builder makes
a catalog compute them on every load.** The only readers are
`DescribeTableExec` L207, `ShowTablePropertiesExec` L36 and
`ShowTablesExtendedExec` L103. This setter copies the map when the `TableInfo`
is built (L101), and `loadTable`/`loadRelation` don't know which command is
running. So a catalog that follows the new `TestingDataSourceTableCatalog`
fixture computes the entries for every query that resolves the table, although
`RelationResolution` L467 turns a `DelegatingTable` into a `CatalogTable`
through `V1Table.toCatalogTable`, which reads only `info.properties`
(`V1Table.scala` L123). That costs nothing when the labels come with the table
metadata. A catalog that fetches them from a separate service, such as a
tagging API, pays one extra call per query, and nothing says that it can defer
them by overriding `displayProperties()`, e.g. in a `DelegatingTable` subclass.
Could the `Table#displayProperties
()` Javadoc, or this setter, say something like "Spark reads these entries
only to display table metadata, so implementations may compute them on demand."?
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/Table.java:
##########
@@ -95,6 +95,34 @@ 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()}.
+ * After a stored property is removed, Spark displays the entry for that key
if it remains in
+ * this map.
+ *
+ * <p>Spark passes {@code ALTER TABLE ... SET/UNSET TBLPROPERTIES} to
+ * {@link TableCatalog#alterTable} without consulting this map. Catalogs
should apply these
Review Comment:
**The SQL reference pages don't mention these entries, so SQL users only see
the effect.** This paragraph explains the `SET/UNSET` behavior to connector
authors, but `docs/sql-ref-syntax-ddl-alter-table.md` L279 still says that
`ALTER TABLE UNSET` "is used to drop the table property", and
`docs/sql-ref-syntax-aux-show-tblproperties.md` L24-25 says that all the
properties are returned. For a V2 table with display-only entries:
- `ALTER TABLE t UNSET TBLPROPERTIES ('catalog-label')` succeeds, but
`catalog-label` is still listed.
- Unsetting a stored key that collides with a display-only key switches the
listed value to the display-only value instead of removing the key, as the new
`ShowTblPropertiesSuite` test pins (L47-61).
Neither page becomes wrong, but the explanation now exists only in this
Javadoc and in the PR description. Could we add a short note to the `SHOW
TBLPROPERTIES` page, and optionally to the `UNSET` section of the `ALTER TABLE`
page, e.g. "For V2 tables, the output may also include display-only properties
supplied by the catalog. `ALTER TABLE ... UNSET TBLPROPERTIES` does not remove
them, and `SHOW CREATE TABLE` does not include them."?
##########
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:
nit: no test pins the new Javadoc sentence that `SHOW TABLE EXTENDED` omits
these entries for the session catalog and its `CatalogExtension`s (`Table.java`
L113-114), but this test already has the setup. `InMemoryTableSessionCatalog`
is a `DelegatingCatalogExtension` registered as `spark_catalog`, and its
`createTable` also creates the V1 table, so `ResolveSessionCatalog` rewrites
`SHOW TABLE EXTENDED` to the V1 `ShowTablesCommand`, which reads only the V1
`CatalogTable`. Could we add one line after this check, e.g.
`assert(!sql(s"SHOW TABLE EXTENDED LIKE
'$t'").head().getString(3).contains("catalog-label"))`? It passes today
(`head()` fails if the table is not listed), and it would fail once this
command goes through `ShowTablesExtendedExec` for extensions, which is exactly
when the Javadoc sentence needs an update.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/Table.java:
##########
@@ -95,6 +95,34 @@ 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:
**Could this paragraph also say that these entries are redacted like stored
properties?** `tablePropertiesForDisplay` applies `conf.redactOptions` to the
merged map (`CatalogV2Util` L687), which uses both
`spark.sql.redaction.options.regex` (default `(?i)url`) and
`spark.redaction.regex`, and `Utils.redact` replaces a value when either the
key or the value matches. With the default configs, a catalog that returns
`docs_url -> https://wiki/t` and `team -> credentials-platform` gets
`*********(redacted)` for both entries in every output, including
`getTableProperties`. Redacting them is the right behavior, but only the
Scaladoc of the `private[sql]` helper mentions it, so a connector author can't
tell from the API doc why an ordinary label is masked. A sentence such as
"Spark redacts these entries in the same way as stored properties." would be
enough.
##########
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:
nit: this test's name says that it excludes reserved keys, but the
single-key branch of `ShowTablePropertiesExec` (L38-40) is never queried with
one, and the "KEY NOT FOUND" test in `ShowTblPropertiesSuiteBase` only uses a
non-existing key. The fixture puts every reserved key into the display map
(`location -> display-location`, `comment -> display-comment`, ...), and this
table has no stored `LOCATION` or `COMMENT`. Today `SHOW TBLPROPERTIES t
('location')` returns "does not have property: location", because the lookup
goes through the filtered merged map. If that branch were later changed to read
`properties().get(p)` and then `displayProperties().get(p)`, e.g. to skip the
merge for a stored key, it would return `display-location`, and the stored
`owner` and `provider` would also come out of the single-key path, while every
assertion here still passes. Could we add a check that `SHOW TBLPROPERTIES $tbl
('location')` returns "does not have property: location"?
##########
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:
nit: this is a top-level class rather than a suite, so this `assert` is
`Predef.assert`. Listing any other namespace, e.g. `SHOW TABLES IN
table_catalog.default`, now fails with `java.lang.AssertionError: assertion
failed` without the namespace, and `QueryExecution.runCommand` reports it as
`INTERNAL_ERROR` ("Executed command failed. You hit a bug in Spark ..."). The
previous `RuntimeException("shouldn't be called")` surfaced as is. Could this
match the namespace and keep the old exception otherwise, e.g. `namespace match
{ case Array("display_properties") => Array(Identifier.of(namespace,
"test_display_properties")); case _ => throw new RuntimeException("shouldn't be
called") }`? Also, the class Scaladoc (L143-147) still describes only the V1
read-path fixtures; it could mention that
`display_properties.test_display_properties` backs the V2 metadata-command test
and is the only table that `listTables` returns.
##########
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:
Minor, latent: the fixture writes into whatever `loadTable` returns. On
`test_catalog` that is the live table, because `copyOnLoad` defaults to false,
so the current tests are fine. However, `row_level_op_catalog`, registered in
the same `sparkConf` (L50-51), returns `rlot.copy()` on every load, and the
copy shares `properties` with the live table
(`InMemoryRowLevelOperationTable.copy`) but gets its own display map
(`InMemoryBaseTable.setDisplayProperties`). With that catalog, `external =
true` would reach the live table while every display entry is lost on the next
load, so the "excludes display-only properties" checks would pass without
checking anything. Could the fixture take the table from
`BasicInMemoryTableCatalog.liveTable`, as `RowLevelOperationSuiteBase.table`
does for the same reason?
##########
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:
nit: this is the only one of the four V2 display-property tests that still
creates its table by hand. `createTableWithDisplayProperties("ns", "table")`
creates the same table with `persisted = stored` and the same display fixture.
Its extra `COMMENT 'table comment'` doesn't change any assertion here, because
`comment` is a reserved key that `tablePropertiesForDisplay` drops, and
`getCreateTableString` still contains no `persisted`. Could L29-31 call the
helper, so that the shared table definition lives in one place?
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/CatalogV2UtilSuite.scala:
##########
@@ -17,6 +17,8 @@
package org.apache.spark.sql.connector.catalog
+import java.util
Review Comment:
nit: the rest of this file writes JDK types fully qualified (e.g. L81
`java.util.Set[String]` and L161 `java.util.Map.of`), but the new tests add
`import java.util` for `util.Map.of` and `util.HashMap` (L42-69). Could they
use `java.util.Map.of` and `new java.util.HashMap` and drop the import? The
same applies to `DataSourceV2DataFrameSessionCatalogSuite` L20 and L63, where
the only `util.Map.of` sits next to the existing `java.util.Map[String,
String]` at L151.
--
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]
