Feat/load entities from stac - #13
Conversation
Entity datasets could only be opened by root CID, so reaching GHCND meant
knowing a CID the catalog already held. The catalog marks which kind an item
is with `dclimate:layout`, but nothing read it.
Adds `client.loadEntities({ request: { collection, dataset } })`, the entity
counterpart to `loadDataset`, reusing the same `resolveDatasetDetails` lookup.
Separate method rather than a layout branch inside `loadDataset`: the two
return different types with different query surfaces -- `EntityDataset` has no
`point()` and its `nearest()` is async and can find nothing -- so folding them
together would widen `loadDataset`'s return to a union and make every existing
gridded caller narrow before calling a Zarr method.
The layout guard only rejects an item that positively declares itself
something else, so items published before the convention still open. Metadata
carries `commitId`/`streamId` through, so a caller can re-resolve the exact
snapshot a query ran against rather than whatever is newest later.
`columnKey` now defaults to upper-casing, which is what this catalog's
datasets publish; without it `elements("TMAX")` is unknown on a dataset every
document describes that way. `entities.load({ cid })` is unchanged and stays
the way to pin an exact snapshot.
Verified against the live catalog: noaa_ghcnd resolves and opens in ~830ms,
findNearestEntity in ~224ms, and a 10-day TMAX series returns 10 rows.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: Comment |
![da-code-reviewer[bot]](https://avatars.githubusercontent.com/u/78453614?s=60&v=4){kind=small}
| // that says nothing about the actual mistake. `layout` is absent on items | ||
| // published before the convention, so this only rejects an item that | ||
| // positively declares itself something else. | ||
| if (resolved.layout && resolved.layout !== ENTITY_DATASET_LAYOUT) { |
There was a problem hiding this comment.
MEDIUM
Missing dclimate:layout is accepted as an entity dataset, even though the new STAC metadata documentation says legacy gridded items lack this property. Calling loadEntities for such an item passes a Zarr CID into the tabular reader and produces a misleading manifest failure. Require resolved.layout === ENTITY_DATASET_LAYOUT before opening it.
The guard accepted a missing `dclimate:layout` as an entity dataset. That is backwards: entity support postdates the field, so an item without it is a gridded one from before the convention -- there is no legacy entity item to accommodate. The permissive branch protected an empty set while admitting exactly the Zarr items the guard exists to catch, producing the misleading manifest failure it was written to prevent. All 64 items in the live catalogue carry the field (60 zarr, 4 tabular), so nothing published today relies on the absent case either way. Reported by automated review.
`listAvailableDatasetsFromStacServer` fetched `/collections` once and used whatever came back. The endpoint paginates with a default page size of 10 and the catalogue now publishes 14, so the request returned a well-formed but short list -- numberMatched 14, numberReturned 10 -- and the four collections past the first page arrived with no title or organization, since that endpoint is their only source. Item search still found them, so they appeared in the catalogue looking complete apart from the missing fields. That is what the list-datasets-parity test was reporting: prism_prism is the 11th collection, so the IPFS walker had a title for it and the STAC path did not. The failure was real, not a flaky live-data disagreement. Follows rel="next" rather than sending a larger limit: a limit only moves the cliff to whenever the catalogue outgrows it, silently, again. Reuses the page cap and repeat-detection the item search already uses, and refuses a next link that leaves the configured origin. Fixes the last failing test; the suite is now green.
| // this catalog serves is published upper case, so it is the default here | ||
| // rather than a rule each caller has to know; an override stays available | ||
| // for a profile that does otherwise. | ||
| columnKey: request.columnKey ?? defaultEntityColumnKey, |
There was a problem hiding this comment.
MEDIUM
loadEntities uppercases columns for every catalog dataset, but NDBC preserves case (for example SwH, with distinct MM/mm columns). Loading noaa_ndbc without an override therefore renames or collapses published columns. Derive columnKey from the resolved dataset profile, or apply this default only to GHCND and retain identity elsewhere.
| // out of the configured server. | ||
| url = next ? new URL(next, url).toString() : undefined; | ||
| if (url && new URL(url).origin !== new URL(resolvedServerUrl).origin) { | ||
| url = undefined; |
There was a problem hiding this comment.
MEDIUM
Discarding a cross-origin rel="next" returns a successful but incomplete catalog. Even if cross-origin pagination is intentionally forbidden, throw an error here so callers cannot mistake partial collection metadata for a complete result.
| "" | ||
| )}/collections`; | ||
|
|
||
| for (let page = 0; page < MAX_STAC_SEARCH_PAGES; page++) { |
There was a problem hiding this comment.
LOW
The loop returns successfully after MAX_STAC_SEARCH_PAGES even when the final response supplies another next link. Preserve the bound, but throw when url remains set after loop exhaustion to avoid silently truncating growing catalogs.
`loadEntities` defaulted `columnKey` to upper-casing. That was a guess at each dataset's publishing profile dressed up as a catalogue-wide convention, and it is only right by coincidence: GHCND does publish upper case, and NDBC, SCAN and SNOTEL happen to already be upper case. NDBC's `.spec` feed publishes `SwH`, `SwP` and `STEEPNESS`, so the day that dataset is catalogued the default would silently respell its columns -- the exact renaming the ETL's own profile comments say must not happen. The justification was wrong too. `columnKey` renames columns; it does not gate access to them. Without one every column is still readable under the schema's own field names, which are what the dataset stores and so are never wrong -- `rows()` returns all of them either way. So the default bought a cosmetic match with the docs and paid for it with silent mis-naming. Nor is the profile derivable here: tabular deliberately stores the schema's names rather than the writer's rendering, precisely so a reader is not bound to one profile's casing (roots before 0.5.0 did the latter, and a cold reader's `entityColumns` returned names its own `query` then rejected), and the STAC item does not carry the mapping. Publishing it into STAC would fix this properly; until then it is the caller's to pass, as it already is for `entities.load`. Reported by automated review.
| "" | ||
| )}/collections`; | ||
|
|
||
| for (let page = 0; page < MAX_STAC_SEARCH_PAGES; page++) { |
There was a problem hiding this comment.
LOW
The loop returns normally after MAX_STAC_SEARCH_PAGES even when url still references another page, silently producing an incomplete catalog. Detect a remaining next-page URL after the loop and throw an explicit truncation error.
| /** | ||
| * Override how schema fields map to published column names. | ||
| * | ||
| * Defaults to upper-casing, which is what every dataset this catalog serves |
There was a problem hiding this comment.
LOW
This public API documentation says columnKey defaults to upper-casing, but loadEntities deliberately supplies no default and retains schema field names. Correct the comment so callers do not rely on uppercase column names that will not exist.
| // out of the configured server. | ||
| url = next ? new URL(next, url).toString() : undefined; | ||
| if (url && new URL(url).origin !== new URL(resolvedServerUrl).origin) { | ||
| url = undefined; |
There was a problem hiding this comment.
MEDIUM
Clearing an off-origin rel="next" makes it indistinguishable from normal pagination completion, so the method returns a silently truncated catalog. If such links are intentionally disallowed, throw a truncation error here instead of returning partial collection metadata.
1 participant
No description provided.