stevenzwu commented on code in PR #16144:
URL: https://github.com/apache/iceberg/pull/16144#discussion_r3204616715
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2267,6 +2267,27 @@ components:
type: string
nullable: false
+ CatalogObjectIdentifier:
+ description:
+ Reference to a catalog object (table, view, or namespace) as an
+ ordered list of hierarchical levels.
+ The object kind is determined by context (e.g. the endpoint or a
+ companion CatalogObjectType discriminator), not by the identifier
+ structure alone.
+ type: array
+ items:
+ type: string
+ example: [ "accounting", "tax", "paid" ]
+
+ CatalogObjectType:
+ type: string
+ description: |
+ The type of a catalog object.
+ enum:
+ - table
+ - view
+ - namespace
Review Comment:
To clarify Dan's framing, here are the two response shapes he sketched:
**Shape A** — flat array of typed records, each carrying its own
discriminator:
```json
[
{ "identifier": [...], "type": "table", "metadata": {...} },
{ "identifier": [...], "type": "view", "metadata": {...} }
]
```
**Shape B** — separate bucket per kind:
```json
{
"tables": [ { "identifier": [...], "metadata": {...} }, ... ],
"views": [ ... ],
"namespaces": [ ... ]
}
```
Dan's critique: Shape A's `type` is a closed enum, so adding a new value
(e.g. `function`) breaks generated parsers on old clients.
The Shape B counter has its own forward-compat hole — and a worse one. An
old client that doesn't know the `materialized_views` key never iterates that
field, so the identifier just *vanishes* from the resolved set; the client
can't even distinguish "didn't resolve" from "resolved to a kind I don't
recognize." A parse error at least tells you something is wrong.
The design doc actually already proposes a hybrid that handles the top-level
concern: per-category typed arrays (`relations: [...]`, future `functions:
[...]`) bucket the major categories Shape-B style, while inside each bucket the
result is a flat record that carries an `object-type` discriminator Shape-A
style:
```json
{
"relations": [
{
"identifier": ["analytics", "daily_sales_view"],
"status": "loaded",
"result": { "object-type": "view", "view": { /* LoadViewResult */ } }
}
]
}
```
Adding a new top-level category (`functions`) is safe: generators silently
ignore unknown top-level fields. The forward-compat risk only lives on
`result.object-type` — that's where adding `materialized-view` later could
break old codegen if it's a closed enum.
**Proposal:** spec `result.object-type` (and `CatalogObjectType` itself) as
`type: string` with documented values, not a closed `enum`.
```yaml
ResolveResult:
type: object
required: [object-type]
properties:
object-type:
type: string
description: |
Object kind. Currently one of: table, view, materialized-view.
Clients should fall through to a default handler for unrecognized
values.
table:
$ref: '#/components/schemas/LoadTableResult'
view:
$ref: '#/components/schemas/LoadViewResult'
```
What this buys:
- **No codegen breakage.** `datamodel-codegen` emits `object_type: str`, not
`Literal[...]`. Pydantic, Jackson with default config, Go's `encoding/json`,
etc. all accept arbitrary strings.
- **No silent data loss.** Every resolved identifier still has a row with
`identifier`, `status`, and `result`. A hand-written client switches on known
`object-type` values and falls through unknown ones to a "skip / report
unsupported" branch with full visibility.
- **Schema still documents the valid set.** The description carries the
enumeration; humans and IDE tooltips see it. We can validate server-side
conformance in our own CI without imposing closed-enum behavior on every
generated client.
- **Filter input keeps the closed enum.** `CatalogObjectType` as `enum` on
request bodies (e.g. the events filter) is fine — the client picks values from
its own schema; the server tolerates older filter sets.
Prior art for this pattern:
- **Iceberg REST already uses it internally.** The most-evolved
discriminator fields in this spec are declared as plain `type: string`, not
closed enums, with valid values listed via `discriminator.mapping`:
- `MetadataUpdate.action` (`BaseUpdate`) — the set has grown over time
(`add-encryption-key`, `remove-encryption-key`, `remove-schemas`,
`remove-partition-specs`, `enable-row-lineage`, `set-partition-statistics`,
`remove-partition-statistics`, …) without breaking clients with stale schemas.
- `TableRequirement.type` — same pattern.
- `ViewRequirement.type` — same pattern.
Closed `enum` in this spec is reserved for stable sets (`FileFormat`,
`SortDirection`, `NullOrder`, `SnapshotRefType`) that aren't expected to grow
casually. `CatalogObjectType` is clearly in the first group —
`materialized-view` and `function` are already foreseen.
--
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]
