dongjoon-hyun commented on code in PR #59036:
URL: https://github.com/apache/spark/pull/59036#discussion_r4331344893
##########
docs/sql-ref-syntax-aux-show-tblproperties.md:
##########
@@ -24,6 +24,11 @@ license: |
This statement returns the value of a table property given an optional value
for
a property key. If no key is specified then all the properties are returned.
+For V2 tables, the output may also include display-only properties supplied by
the catalog.
+`ALTER TABLE ... UNSET TBLPROPERTIES` does not remove these entries, and `SHOW
CREATE TABLE`
Review Comment:
nit: this sentence and "unsetting a stored non-reserved property exposes the
display-only value" on L30 read as guarantees from Spark. However, the API
contract in `Table.displayProperties()` only says that catalogs *should* apply
`SET/UNSET TBLPROPERTIES` to `properties()` and keep display-only entries
unchanged, and it now also allows computing them on demand. For example, a
catalog that derives a display label from a stored property, or one that
removes the key from both maps in `alterTable`, would no longer list the entry
after `UNSET`. This is the same distinction that R3-2 made for the Javadoc.
Could we say what Spark does and leave the rest to the catalog, e.g. "Spark
itself does not remove these entries on `ALTER TABLE ... UNSET TBLPROPERTIES`;
whether they remain depends on the catalog."? The same applies to
`sql-ref-syntax-ddl-alter-table.md` L282-283.
##########
docs/sql-ref-syntax-aux-show-tblproperties.md:
##########
@@ -24,6 +24,11 @@ license: |
This statement returns the value of a table property given an optional value
for
a property key. If no key is specified then all the properties are returned.
+For V2 tables, the output may also include display-only properties supplied by
the catalog.
Review Comment:
nit: within `docs/sql-ref*.md`, "V2 tables" appears only here and in
`sql-ref-syntax-ddl-alter-table.md` L281, so SQL users can't tell which tables
it covers. For example, parquet or Hive tables in `spark_catalog` are not
covered. Could we link the existing Data Source V2 page, e.g. `For tables in
[Data Source V2](sql-v2-data-sources.html) catalogs, ...`?
##########
docs/sql-ref-syntax-aux-show-tblproperties.md:
##########
@@ -24,6 +24,11 @@ license: |
This statement returns the value of a table property given an optional value
for
a property key. If no key is specified then all the properties are returned.
+For V2 tables, the output may also include display-only properties supplied by
the catalog.
+`ALTER TABLE ... UNSET TBLPROPERTIES` does not remove these entries, and `SHOW
CREATE TABLE`
+does not include them. Stored properties take precedence over display-only
entries with the
+same key; unsetting a stored non-reserved property exposes the display-only
value if present.
Review Comment:
nit: "non-reserved" isn't defined anywhere in the SQL reference, and the
**Note** at the end of this page lists the V1 exclusions (`spark.sql*`,
`EXTERNAL`, `comment`, Hive statistics), not the V2 reserved keys. In practice,
`comment` is the only reserved key that `UNSET TBLPROPERTIES` accepts, because
`AstBuilder.cleanTableProperties` keeps it and rejects or drops the other
reserved keys. So SQL users can't tell that `UNSET TBLPROPERTIES ('comment')`
doesn't expose a display-only `comment`. Could we name the key instead, e.g.
"unsetting a stored property other than `comment` exposes ..."? The same
applies to `sql-ref-syntax-ddl-alter-table.md` L283.
--
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]
