chore: clarify sort and partition semantics in table schemas - #106
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| pub last_sync: Option<Option<String>>, | ||
| /// The table's partition keys, in the order they were declared when the table was created. Empty when the table is not partitioned. A table's storage layout is fixed when the table is created and cannot be changed afterwards, so this is how to confirm a table really was created with the layout that was asked for. The field is always present: an empty array means \"no partitioning declared\", which is not the same as a response that omits the field entirely. Reported for tables in a hotdata-managed database, which are the only ones whose layout is declared here. A table discovered from an external connection always reports an empty array — its layout belongs to the upstream system, so an empty array there means \"not known from here\", not \"confirmed unpartitioned\". | ||
| #[serde(rename = "partition_by")] | ||
| pub partition_by: Vec<models::TablePartitionKey>, |
There was a problem hiding this comment.
nit: partition_by and sorted_by land as required, non-default fields on a response model, so any information_schema response that omits them fails to deserialize as a whole — the error surfaces as a parse failure on the entire page, not a missing field on one table. During a rolling deploy (or against a server that predates the spec change), information_schema_read and any downstream caller break rather than degrade.
This repo already treats that as a case worth deviating from the generator on — see the 0.10.0 CHANGELOG entry: "Pagination metadata on ListDatabasesResponse ... is now nullable so the client tolerates responses from a server that predates these fields (rolling deploy / version skew)." Note the request-side counterparts in AddManagedTableDecl are Option<Vec<..>>, so only the response side is strict.
Adding #[serde(default)] to both fields would keep the ergonomic non-Option type while tolerating an absent field (deserializing to an empty Vec, which the doc comment already defines as "none declared"). Best fixed upstream in the OpenAPI spec so it survives regeneration. (not blocking)
|
|
||
| ### Changed | ||
|
|
||
| - chore: clarify sort and partition semantics in table schemas |
There was a problem hiding this comment.
nit: this entry reads as documentation-only ("clarify ... semantics"), but the diff also adds two required fields to TableInfo and changes TableInfo::new from 4 to 6 parameters — a source-breaking change for anyone constructing it. Other breaking entries in this file carry a **Breaking:** prefix; worth matching that here so the release notes reflect the constructor change. (not blocking)
There was a problem hiding this comment.
Generated-code change; docs and model fields are consistent with the spec update. Two non-blocking inline notes (serde tolerance for the new required response fields, and CHANGELOG framing of the TableInfo::new signature change).
Note: integration tests and scenario-parity were still queued when this review ran, so I have no result for them either way.
Auto-generated from the updated HotData OpenAPI spec.
Source: https://github.com/hotdata-dev/www.hotdata.dev/pull/309