ayushbilala commented on PR #59235:
URL: https://github.com/apache/spark/pull/59235#issuecomment-6121510392
> ## Review summary
> The routing, listing, and error behavior of the new command match the
tabular `SHOW VIEWS` paths for both the session catalog and `ViewCatalog`
catalogs. I found no correctness problems in the JSON output itself. The
retained findings are small.
>
> * The scaladoc on `databaseInSessionCatalog` says the empty-namespace
branch cannot be reached, but `SHOW VIEWS IN spark_catalog AS JSON` reaches it.
> * In the shared base, the scoping filter in `AS JSON lists user-created
views` keeps every entry, so it does not exclude leftover temp views as its
comment says.
> * The v1 current-namespace test runs against `default`, so it would pass
even if the current namespace were ignored.
> * No test lists a multi-part `ViewCatalog` namespace.
>
> There is also an advisory question below about naming the per-view key
`viewName` rather than `name`, as `SHOW TABLES AS JSON` does.
>
> ## Cross-cutting observations
> * Namespace handling is where the new AS JSON coverage is thinnest. The
catalog-only empty-namespace error, the per-entry namespace assertions,
current-namespace resolution, and multi-part namespace encoding each lack a
working regression signal. The command itself handles all four correctly today.
> Separation: Each gap sits in a different test or comment and needs its
own small edit. No shared fixture or production owner would close them
together, so they stay independent P3 repairs and the pattern changes no
severity or grouping.
>
> ## Findings
> **4 total: 0 P0, 0 P1, 0 P2, 4 P3.**
>
> ### Nit (P3)
> * **Scaladoc wrongly calls the empty-namespace branch unreachable** —
`sql/core/src/main/scala/org/apache/spark/sql/execution/command/ShowViewsJsonCommand.scala:104`
— see inline.
> * **Scoping filter in `AS JSON lists user-created views` filters nothing**
—
`sql/core/src/test/scala/org/apache/spark/sql/execution/command/ShowViewsSuiteBase.scala:100`
— see inline.
> * **Current-namespace test cannot detect a regression** —
`sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/ShowViewsSuite.scala:69`
— see inline.
> * **Multi-part namespace encoding has no test** —
`sql/core/src/main/scala/org/apache/spark/sql/execution/command/ShowViewsJsonCommand.scala:90`
— see inline.
>
> ## Decision challenges
> ### Should the per-view key be `name`, as in SHOW TABLES AS JSON?
> The PR says AS JSON follows `SHOW TABLES AS JSON`, but the per-entry name
key differs. `SHOW TABLES AS JSON` emits
`{"name":...,"namespace":[...],"isTemporary":...}`; it renamed its tabular
`tableName` column to `name`. This command emits `viewName`, keeping the
tabular column name. The `namespace` and `isTemporary` keys already match, so a
client that reads both outputs has to special-case only the name key, and once
this format ships the key is hard to change. Was `viewName` a deliberate
choice, or should it be `name` to match `SHOW TABLES AS JSON`?
>
> _Generated by Omnigent on Databricks._
Thanks for the thorough pass. Fixed all four findings.
### Should the per-view key be `name`, as in SHOW TABLES AS JSON?
This was deliberate, not an oversight: the tabular SHOW VIEWS command's own
output column is already named viewName (see the ShowViews logical plan's
output schema), so I kept that name in the JSON form to match the existing
text-mode contract 1:1, rather than following SHOW TABLES AS JSON's precedent
of renaming its tabular tableName column to the more generic name. I weighed
"matches this command's own text output" against "matches the sibling JSON
command's vocabulary" and picked the former, since AS JSON is meant to be a
structured mirror of the existing command rather than a new independent schema.
That said, I don't feel strongly pinned to it -- if you'd rather have a
uniform name key across every AS JSON command for client-side consistency, I'm
glad to make that change now, before it ships and becomes a compatibility
commitment. Let me know which way you'd like it and I'll update the PR.
--
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]
