dongjoon-hyun commented on PR #58092:
URL: https://github.com/apache/spark/pull/58092#issuecomment-5348696891
Thank you for working on this. The motivation is sound:
`DescribeTableCommand.describePartitionInfo` calls
`CatalogTable.partitionSchema`, whose `assert` (`interface.scala:475`) makes
`DESCRIBE TABLE` itself fail on a corrupt table, which is exactly when users
need to inspect the metadata. However, I think the current patch is broader
than the actual bug, and it introduces a regression on the JSON path.
## 1. The JSON path was not broken, and this PR regresses it
In `DescribeRelationJsonCommand`, the only `partitionSchema` call site is
line 319, inside `describePartitionInfoJson` — the `DESC ... PARTITION AS JSON`
path that this PR explicitly declares out of scope. `partition_columns` itself
comes from `CatalogTable.toJsonLinkedHashMap` (`interface.scala:653`), which
just emits `partitionColumnNames` and never touches the assertion.
So `DESCRIBE TABLE EXTENDED ... AS JSON` already worked fine for corrupt
partition metadata. This PR now drops `partition_columns` for those tables and
replaces it with `invalid_partition_information`.
More concretely, consider a legacy table with an empty stored schema.
`DescribeRelationJsonCommand.run` already handles that case:
```scala
val schema = if (metadata.schema.isEmpty) {
// In older versions of Spark,
// the table schema can be empty and should be inferred at runtime.
sparkSession.table(metadata.identifier).schema
} else { metadata.schema }
describeColsJson(schema, jsonMap)
```
but the new `describeInvalidPartitionInfoJson(metadata, jsonMap)` and the
`describeFormattedTableInfoJson` filter both read `metadata.schema` (empty)
rather than the resolved `schema`. `lastSchemaColumns` is then `Nil !=
partitionColumnNames`, so `isValid` is `false` and a perfectly healthy legacy
table loses `partition_columns` and gains a bogus
`invalid_partition_information`. That is a new user-facing regression, not a
fix.
The text path (`tables.scala:686`) has the same blind spot, but there it
previously threw `AssertionError`, so it is only a misleading-message issue
rather than a regression.
Suggestion: pass the already-resolved schema into `describePartitionInfo` /
`describeInvalidPartitionInfoJson` and base `isValid` on it. And since the JSON
change fixes no bug, consider splitting it out — or at least keeping
`partition_columns` and adding a flag alongside it, which is safer for JSON
consumers than removing a documented field.
## 2. Design: a new output section plus a free-form "Recommendation" sentence
`DESCRIBE` rows are `(col_name, data_type, comment)` tuples, and this puts
an English sentence into the `data_type` column:
```scala
Row("Recommendation", "Repair the catalog metadata so the declared partition
columns match the last columns in the table schema.", "")
```
That is the kind of text that belongs in an error condition, not in a result
set. Two smaller alternatives worth considering:
1. In `describePartitionInfo`, resolve partition columns by name instead of
positionally (`takeRight`). Most real corruption cases are ordering mismatches
and would then render correctly with no new section at all.
2. Skip the section entirely when the metadata is inconsistent and let `DESC
EXTENDED`'s existing `Partition Columns` row carry the information. For the
strict paths (`DESC ... PARTITION`, `SHOW PARTITIONS`, `SHOW CREATE TABLE`),
replacing the `assert` with a structured error (e.g. the existing
`INTERNAL_ERROR_METADATA_CATALOG` family) can be a separate follow-up as you
noted.
If the current approach is kept, please extract the recommendation string
into a single constant — it is currently duplicated as a literal in
`tables.scala`, `DescribeRelationJsonCommand.scala`, and the test.
## 3. `PartitionMetadata` helper
- It is inserted between the `DescribeCommandBase` trait and
`DescribeTableCommand`'s scaladoc, splitting the two command definitions.
Placing it next to `DescribeCommandBase` or at the bottom of the file would
read better.
- It does not need to be a `case class`; `equals`/`hashCode`/`unapply` are
unused.
- `DescribeRelationJsonCommand` constructs `PartitionMetadata(table)` twice
(in `describeFormattedTableInfoJson` and `describeInvalidPartitionInfoJson`).
Compute it once and pass it down.
- The display helpers (`declaredPartitionColumnsDisplay`, `formatColumns`)
are only used by the text path, so presentation logic is mixed into a shared
helper.
- In `describeInvalidPartitionInfoJson`, the
`declaredPartitionColumns.nonEmpty &&` guard is redundant: when it is empty,
`isValid` is already `true`.
## 4. Minor
- `isValid` is an exact string comparison. A Hive catalog that lower-cases
partition column names could flag a healthy table as invalid. Same semantics as
the existing `assert`, so not a regression, but a `conf.resolver`-based
comparison would be more accurate.
- Docs: the JSON example shows `partition_columns` and
`invalid_partition_information` side by side although they are mutually
exclusive. Also, because of `addKeyValueToMap` ordering,
`invalid_partition_information` actually lands after `schema` near the end of
the object, not where the example places it.
- `docs/sql-ref-syntax-aux-describe-table.md:141`: the trailing-whitespace
removal is unrelated to this change.
- Tests: the empty-stored-schema case from (1) is not covered; worth adding.
- The PR description says the regression test was "expanded", but it is a
newly added test. It also notes the test could not be run locally due to DNS
failures — please confirm `build/sbt 'sql/testOnly *v1.DescribeTableSuite'`
passes before merging.
CI on `f4f2eb2` is still in progress (Linters, Precompile, Maven build,
Docs), with no failures so far.
## Summary
The core direction — not letting `DESCRIBE TABLE` die on corrupt metadata —
is right. As written, though, it (a) regresses the JSON path that was never
broken, and (b) adds a fair amount of new user-facing output surface for a
corrupt-metadata edge case. I would fix (1) and reconsider the JSON change and
the "Recommendation" row.
--
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]
