szehon-ho commented on PR #58092:
URL: https://github.com/apache/spark/pull/58092#issuecomment-5349410560
Thanks @dongjoon-hyun for the detailed review — the scope concern was right,
and the patch is now considerably smaller. Pushed as f12d690.
**1. JSON regression.** Reverted entirely. `DescribeRelationJsonCommand` and
`DescribeTableSuiteBase` are no longer touched, so `partition_columns` keeps
coming from `CatalogTable.toJsonLinkedHashMap` for corrupt tables exactly as
before. The test now asserts that explicitly rather than asserting the field
disappears.
On the empty-stored-schema case: the check is now based on the schema that
is actually described, as you suggested. I tried to add the test for it and
found the scenario is not reachable for a v1 file-based table —
`DataSource.resolveRelation` calls `catalogTable.get.partitionSchema` itself
(`DataSource.scala:438`) while inferring the schema, so `DESCRIBE TABLE` on a
partitioned table with an empty stored schema fails inside schema inference,
before the partition section is reached:
```
java.lang.AssertionError: assertion failed: Corrupted table metadata
detected for table
`spark_catalog`.`ns`.`table`. ... Table schema columns: [] Declared
partition columns: [part].
at
org.apache.spark.sql.catalyst.catalog.CatalogTable.partitionSchema(interface.scala:484)
at
org.apache.spark.sql.execution.datasources.DataSource.resolveRelation(DataSource.scala:438)
at
...FindDataSourceTable...readDataSourceTable(DataSourceStrategy.scala:275)
```
So there is no false `# Invalid Partition Information` report to guard
against there, and no test I could write that passes. I left the
resolved-schema plumbing in since it is the correct basis for the comparison,
and listed this path in the PR description as out of scope alongside the other
strict callers.
**2. `Recommendation` row.** Removed — no free-form prose in the result set.
The section now carries only the two column lists, and the repair guidance plus
the list of commands that still require a valid partition schema moved to
`docs/sql-ref-syntax-aux-describe-table.md`. That also removes the duplicated
literal.
**3. `PartitionMetadata` helper.** Removed entirely; the comparison is
inlined in `describePartitionInfo`, so the placement, the unnecessary `case
class`, the double construction, the mixed-in presentation helpers, and the
redundant `nonEmpty` guard are all gone.
**4. Minor.**
- Now compares with `conf.resolver` instead of exact string equality.
- Docs: the JSON example is untouched again since the JSON path is
unchanged. The note sits in the `### Description` section rather than under the
`AS JSON` parameter, since it describes text output.
- The unrelated trailing-whitespace change at line 141 is reverted.
- PR description now says the test was added, not expanded.
- Test placement: it sits next to `DESCRIBE TABLE EXTENDED of a partitioned
table`.
**Test run.** `build/sbt 'sql/testOnly *v1.DescribeTableSuite'` now passes
locally — 70 succeeded, 0 failed (each test runs for both the V1 and V2 command
versions).
On moving the fixture to `DescribeTableSuiteBase`: I confirmed it cannot run
against the Hive external catalog. `HiveExternalCatalog.createDataSourceTable`
calls `table.partitionSchema` when building the metastore table
(`HiveExternalCatalog.scala:342`), so the malformed `CatalogTable` is rejected
at creation time, before it can be described. It stays in the V1 in-memory
suite for that reason.
--
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]
