dongjoon-hyun commented on PR #58092:
URL: https://github.com/apache/spark/pull/58092#issuecomment-5345510368
A few observations, mostly around scope and consistency.
**1. Text and JSON output would disagree for the same table.**
The `AS JSON` path (`DescribeRelationJsonCommand`) renders the detailed
table info via `CatalogTable.toJsonLinkedHashMap`, which reads
`partitionColumnNames` directly rather than `partitionSchema`, so it still
emits `Partition Columns` for a corrupt table. The text path, on the other
hand, excludes `Partition Columns` from `# Detailed Table Information`
(`excludedTableInfo` in `tables.scala`), so once `# Partition Information` is
omitted the text output contains no trace of partitioning at all -- the table
looks unpartitioned. Whichever way this is resolved, the two paths should agree.
**2. Using `Try` as control flow here.**
It does work -- `scala.util.Try` catches `NonFatal`, and `AssertionError` is
neither a `VirtualMachineError` nor a `LinkageError`, so it qualifies -- but
"`Try` swallows an `Error`" is subtle enough to be easy to misread, and it will
also silently swallow any future failure mode added to `partitionSchema`. Since
the condition is predictable, an explicit check seems clearer and drops the
`scala.util.Try` import:
```scala
val partitionFields =
table.schema.takeRight(table.partitionColumnNames.length)
if (partitionFields.map(_.name) == table.partitionColumnNames) { ... }
```
**3. Partial coverage of the same assertion.**
The assertion in `CatalogTable.partitionSchema` still fires on other read
paths a user would reach for while diagnosing or repairing such a table:
- `DESC ... PARTITION` (`tables.scala`, `describeDetailedPartitionInfo`)
- `SHOW TABLE EXTENDED ... PARTITION`
- `SHOW PARTITIONS` (`ShowPartitionsHelper.listV1PartitionNames`)
- `SHOW CREATE TABLE`
`SHOW CREATE TABLE` in particular is about as common as `DESCRIBE` in a
repair workflow. Not asking to fix them all here, but it would help to state in
the PR description which ones are intentionally out of scope. More
fundamentally, an error-class-based exception instead of `AssertionError` would
match Spark conventions better -- probably a separate PR.
**4. Tests.**
- The new test is in `class DescribeTableSuite` (V1 In-Memory) rather than
`trait DescribeTableSuiteBase`, so it does not run against the Hive external
catalog -- which is where corrupt metadata realistically originates. Worth
checking whether it can move to the trait (assuming the Hive catalog accepts
such a `CatalogTable`).
- No coverage for non-`EXTENDED` `DESCRIBE TABLE` or for the `AS JSON` path;
the latter matters given point 1.
- Nit: the test is inserted at the top of the class, ahead of the existing
tests. Placing it near the related partitioned-table tests would fit the file's
organization better.
On the plus side, the change is nicely scoped, the `Try` wraps only the
`partitionSchema` call and not the `describeSchema` body, and CI is green.
--
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]
