Conversation
`ObjectStore::list` sends no `max-keys`, so a server applies its own default of 1000 per page. A recursive walk of a large bucket becomes a long chain of strictly sequential requests, and the listing is pure wait rather than work. Measured against a SeaweedFS bucket of 2 853 217 objects: the walk cost 2854 requests, and the same walk with max-keys=65535 cost 44. `BigPageList` wraps a `PaginatedListStore` and re-expresses `list` over `list_paginated` with an explicit page size. The stream stays lazy, so a caller that stops early still stops paying. Every other method delegates. AWS caps max-keys at 1000 and ignores a larger value, so this is inert there. Also: `s3_data_lake_deprecated` read `BEACON_S3_DATASETS`, the new name, which left the back-compat path dead. A 1.x deployment carrying only `BEACON_S3_DATA_LAKE=true` fell back to the local datasets directory and reported an empty store.
A page chain is strictly sequential: each page needs the continuation token of the one before it, so a walk costs one round trip per page however fast the server is. `list` now descends `list_with_delimiter` three levels to find shards, then walks each shard as its own page chain, 16 at a time. Discovery costs one request per directory seen and finished in under a second. Objects at the intermediate levels are emitted too, so nothing is missed. A prefix with no sub-directories falls back to the sequential walk, as does a store that cannot answer a delimiter listing. Measured against a SeaweedFS bucket of 2 853 217 objects, warm and back to back. Every run returned the full count: sequential, no max-keys (before) 79.9s 2854 requests sequential, max-keys=5000 64.3s 571 requests 2-level shards, 16 ways 33.8s 3-level shards, 16 ways 19.4s 3-level shards, 32 ways 17.7s DEFAULT_MAX_KEYS is 5000, not the 65535 SeaweedFS will parse. Larger pages measured worse and they broke: at 65535 with 8 to 16 shards in flight, SeaweedFS failed whole responses with a body error and the walk silently returned 2 288 498 of 2 853 217 objects. Shards interleave, so results are no longer incidentally sorted. `ObjectStore::list` does not guarantee order.
`list_datasets` matched `if let Ok(entry)` over the listing stream and dropped everything else. A transient object-store failure part-way through a walk therefore produced a short dataset list that looked complete: a timeout on object 2 000 000 of 2 850 000 reported success and lost the rest without a word. The error now propagates, naming how far the walk got. The page-size note blamed SeaweedFS for failing large responses. It is not a server limit. `ClientOptions` gives a request 30 seconds by default; one page of 65535 keys off a cold filer took 6.5s on its own, and with 16 shards in flight some requests crossed 30s. Re-run warm, the same walk returned all 2 853 217 objects with no errors, at both the default timeout and a raised one. 5000 stays the default because it holds whatever the filer's cache is doing; `AWS_TIMEOUT` is the knob to raise first for anyone who wants larger pages.
The datasets page enumerated the whole store to draw a folder. It asked for 100 000 rows, grouped them into folders in the browser, and asked `/api/total-datasets` for a count that also walked everything. Both ran on mount and again after every upload. Nothing about a folder view needs the subtree, and the gap is not a constant factor. Against a SeaweedFS bucket of 2 853 217 objects the recursive walk took 79.9s and one delimiter request took 14 ms. `GET /api/browse-datasets?prefix=` returns one level: sub-folder names and the datasets sitting directly in it. `ListingFactory::browse_datasets` does the delimiter listing and classifies only what is at that level; `Runtime::browse_datasets` exposes it without going through SQL, since there is no pattern to plan and no rows to page. The page now browses per folder. Search is the one view that genuinely needs every path, so it is the one view that pays for them, and only once something has been typed. The total badge counts the current folder rather than walking the store. A directory-shaped dataset (Zarr, Atlas) keeps its marker inside its directory, so at this level it reads as a folder; descending shows the marker. Classifying it in place would cost one request per sub-folder, which is the recursive cost this removes. Folder item counts are gone for the same reason. Authorization matches `list_datasets`: authenticated, no per-path grant filtering. `GRANT ... ON PATH` still governs reading a dataset. Moving both listings onto granted paths is a separate change and has to move them together.
Neither listing consulted grants. `scan_targets` resolves a scan to a table or to its file paths by downcasting the provider, and returns no targets for anything it does not recognise -- "unintrospectable sources are exempt". `list_datasets` is a table function returning a `MemTable`, so it matched nothing and no grant was ever checked. `browse_datasets` does not go through a plan at all, so it inherited the same hole. `GRANT ... ON PATH` still gated reading the bytes, because a `read_*` resolves to a `FastObjectTable` and that does downcast. What leaked was the shape of the store: every path, format, size and mtime. With `BEACON_AUTH_ANONYMOUS_ENABLED` defaulting to true, to anyone who could reach the API. Both listings now filter. A dataset survives when `Select` is allowed on its path -- the same evaluator the read path uses, so a listing cannot name a file the caller would then be refused. A folder survives when some grant could match a path inside it: `RoleProvider::prefix_is_reachable` walks the pattern segment by segment, so a grant on `argo/floats/**` keeps `argo` navigable while leaving `argo/gliders` hidden. Deny is asymmetric on purpose. Hiding a directory takes a deny covering the whole subtree; a deny over part of it leaves the directory visible and refuses the individual files, or granting `data/**` and denying `data/*.csv` would strand the parquet beside it. Gated on `BEACON_AUTH_ENFORCE`, as reads are, and never applied to the super-user. A deployment that has not opted into grants sees what it saw before.
Read authorization asks whether a caller may read one path, per scan, and
refuses the statement when the answer is no. A listing asks the same
question about every candidate and about the directories above them, and
answers by dropping rows. Same rule, different consequence -- so it was
written twice, once on `Runtime` and once at the call site.
`PathVisibility` is that rule. Two states rather than a context plus a
flag: a caller not subject to grants at all, because enforcement is off
or they are the super-user, is a different thing from one whose grants
happen to allow everything, and a holder should not have to remember
which it has.
It carries no storage types, so it serves an object listing, a directory
browse and a planner target alike. An object listing passes
`meta.location.as_ref()`.
Replaces `Runtime::{listing_is_filtered, may_read_path,
prefix_is_reachable}` with one `path_visibility` accessor.
`TableFunctionImpl::call` is a synchronous trait method, and DataFusion 53 has no async form of it. `list_datasets` did the whole walk inside it, so it reached the object store through `block_in_place` and `block_on` and held a worker thread for the duration. It also ran during logical planning, so a statement paid for the listing before anything decided to read it. `TableProvider::scan` is async. `call` now reads its arguments and returns a `DatasetsTable` that has touched nothing; the walk happens in `scan`. The runtime handle and the `Weak<SessionContext>` existed only to drive `block_on`, so both are gone. `browse_datasets([prefix])` joins it as a second table function over the same provider, with a delimiter instead of a glob. `/api/browse-datasets` now runs `SELECT * FROM browse_datasets(...)`, so both listings reach the store the same way the engine reaches everything else. Folders are not rows, so that half still comes from the runtime. `scan` takes DataFusion's `limit` push-down and bounds the rows with it. It does not yet bound the walk: a glob listing classifies objects into datasets in one pass and the two do not correspond one to one, so stopping early needs the classifier to work in chunks. The four existing `list_datasets` tests pass unchanged, which is what says the refactor kept its contract.
Both listings now reach the store through one provider and two table functions. `list_datasets` globs and descends. `browse_datasets` stops at one directory level and reports the sub-directories as rows, flagged by a new `is_directory` column, so a single query describes a level. `/api/browse-datasets` runs `SELECT * FROM browse_datasets(...)` and only splits the rows. Two names rather than one function with a depth argument. A depth argument changes what the *first* argument means, glob or directory, and an argument that retypes another argument is a bad seam. It cannot be inferred from the glob either: DataFusion matches listing globs with the default `MatchOptions`, where `require_literal_separator` is false, so `sub/*` already matches `sub/deep/c.csv`. Reading it as one level would silently change what existing patterns return. Grant filtering is removed from both listings, restoring what `list_datasets` did before: any caller who reaches the API sees every path. That is a known gap, deferred deliberately. `PathVisibility` and `prefix_is_reachable` stay, tested and unused, because the filter belongs inside the provider rather than at the transport, and that needs the caller on the session at scan time. `Runtime::browse_folders` and the `file_formats` field it needed are gone with it.
#415 fixed the same `BEACON_S3_DATA_LAKE` variable this branch did, and fixed it better: the one-line change plus a deprecation warning and a test. Its version is kept whole. The conflict was only the comment this branch had added beside it, which the warning and the test now say more usefully.
This branch is the listing performance work and the browse feature. The grant filtering was a second subject that grew out of a question asked while reviewing it, and it does not belong in the same change. `beacon-auth` returns to exactly what `main` holds: `PathVisibility`, `prefix_is_reachable` and their tests are gone, along with `Runtime::path_visibility`. Listings do not consult grants, which is what they did before any of this. That gap is real and unclosed. Narrowing a listing to the caller belongs inside the provider rather than at the transport, because a caller who can reach SQL can otherwise read the unfiltered function directly, and reaching the provider needs the caller on the session at scan time. It wants its own change.
Both listing functions lived in `file_formats/list_datasets.rs`, which made `browse_datasets` read as churn in a file that had no reason to change, and put a cross-format concern under `file_formats`. `beacon-functions/src/listing/` now holds the whole subject: `provider` for the table both functions return, `list_datasets` and `browse_datasets` for the two argument readers, `args` for the literal parsing they share. Also fixes where the sharded listing is applied. `BigPageList` only wrapped stores built by `object_store_registry`, which serves ad-hoc `s3://` URLs. The datasets store is not one of those: beacon-server builds it and `register_object_stores` registers it verbatim, so it kept the one-request-per-1000-objects walk and the sharding never reached the data anyone actually queries. It is wrapped in `build_datasets_store` now, beside the builder that makes it.
Zarr v3 gives every group *and every array* a `zarr.json`, so a marker
alone does not say which it is. Discovery told them apart by comparing
markers to each other: collect every one, then keep those with no
ancestor directory that also holds one. Correct, but it needs the whole
listing in hand before it can classify any of it, which is the one thing
that stops a listing from streaming.
The directory already carries the answer. A store is a `*.zarr` directory
holding `zarr.json` beside its arrays:
gridded-example.zarr/zarr.json the store
gridded-example.zarr/lat/zarr.json an array inside it
`is_zarr_store_root` reads that off one object, so discovery can classify
a listing as it arrives.
This does require the suffix. A store in a directory named anything else
is no longer discovered, where the ancestor rule found it. Every fixture
in the tree already follows the convention, and `read_zarr` on an
explicit path is unaffected: `top_level_zarr_meta_v3` still serves the
paths where the caller has named the store, and a listing scoped to one
store is small enough to compare.
Atlas has the same shape and the same fix available. It is excluded from
the workspace, so it is left alone.
Discovery named a store by its directory; an explicit `read_zarr` path still named it by comparing markers to each other. Two rules meant a directory could be a store for one caller and not the other, which is a worse answer than either rule alone. `is_zarr_store_root` is now the only one. `infer_schema` and the scan that partitions file groups both use it, and the ancestor walk `top_level_zarr_meta_v3` is gone along with the five tests that pinned its behaviour. The error a path that is not a store now gets says what one looks like, because the rule is stricter than it was and "no zarr.json found" would not explain a directory that plainly has one.
A listing collected every object, classified them all, built one record batch and handed it to a `MemTable`. Nothing reached the caller until the walk finished, and a listing of 2 853 217 objects held about a gigabyte of `ObjectMeta` to do it. Every format decides per object now, so nothing has to be held. `classify_object` asks a format about one object; the default asks `discover_datasets` about a listing of one, so no format crate changes. The size and timestamp come off the object there, which is what the separate enrichment pass over the whole listing used to do. `stream_datasets` walks and classifies as pages arrive. It takes a resolved store and URL rather than a session, so the stream is `'static` and can be rebuilt: `resolve_listing` does the half that needs a session, during `scan`, and the plan does the rest per execute. `DatasetsExec` turns that into batches of 8192 rows. First rows arrive in the time of the first page instead of the whole walk, memory is bounded by one batch, and `LIMIT` now stops the walk rather than the output -- the source is lazy, so the node stopping drops the pages behind it. `EXPLAIN` names what it will list. A browse is one request whose rows are known once it answers, so `scan` awaits it and the node replays them.
`stream_datasets` sat in `listing_factory` and never touched the factory. It took a store, a URL and the file formats, and returned classified datasets -- so a module whose job is resolving paths and walking a store had to know what a Zarr marker is. Walking a store and deciding what a file *is* are different jobs, and only the second one needs formats. `stream_objects` does the first and returns `ObjectMeta`. The provider does the second, in the crate where the formats already live, with `filter_map` over the stream. `classify` also states a rule that was implicit in the old loop: the first format to claim an object wins, in registration order, so a file two formats both read produces one row rather than two. `beacon-datafusion-ext::listing_factory` no longer imports `FileFormatFactoryExt` for the streaming path, and a raw object stream is now available to anything else that wants one -- the file-statistics pass hand-rolls `store.list(prefix)` today and could use this instead.
The factory had grown `resolve_listing`, a free `stream_objects` that never touched it, and `browse_datasets` -- three entry points that each resolved a path their own way, and one of which had to know what a Zarr marker is. `ListingFactory::listing` resolves once and returns an `ObjectListing`: the store, and the URL selecting objects in it. Resolution is the only half that needs a session, so the handle holds none and can be read as many times as a caller wants. `stream()` walks it, `level()` reads one directory. Both inherit the same path rules by construction -- the configured store, a schemed path, a local directory, the glob -- rather than repeating them. `browse_datasets` is gone from the factory with `BrowseResult`. `level()` returns objects, and the provider classifies them exactly as it does the streamed ones, so there is one rule for what a file is instead of two paths to it. The file-statistics pass now resolves through the factory too. It called `store.list(prefix)` directly with its own prefix handling, so a scan prefix meant something subtly different there than in a query, and a glob meant nothing at all.
**A store the caller names keeps the ancestor rule.** Unifying the Zarr
rules was wrong, and `cf_time_nan` and `statistics` say why: both write a
store into a temp directory and read it by path. That directory does not
end in `.zarr`, so the strict rule refused a store the caller had just
pointed at.
The two questions are not the same one. Discovery asks whether a marker
is a store root among many stores, and has to answer from one object to
stream. A read asks which marker is the root of the one store it was
given, and has a listing small enough to compare. `infer_schema`, the
scan that partitions file groups, and the schema cache go back to
`top_level_zarr_meta_v3`. `discover_datasets` keeps `is_zarr_store_root`.
**A URL naming one file is not a listing.** `client_endpoints_http`
caught it: `list_datasets('obs/a.parquet')` returned nothing. A store
lists a prefix at segment boundaries, so listing `obs/a.parquet` looks
for a directory of that name. `ObjectListing::stream` now asks for the
object with `head` when the URL is not a collection, and falls back to
listing on NotFound -- which is what `ListingTableUrl::list_prefixed_files`
does, and what the streaming path dropped.
Codecov called out the patch, correctly: `BigPageList` and `ObjectListing` were new and untested. `InMemory` does not implement `PaginatedListStore`, so a sharded walk had no fixture and I had leaned on an external probe against a real bucket -- which tested a separate implementation of the same idea, not this one. A fake paginated store fixes that, and the first test it ran found a real bug: **a sharded walk returned every object twice.** `discover_shards` added each level's own objects to the ones gathered above, including the level that found no children -- and that level is the one the shards are taken from, so its walk returned them again. It now keeps a level's objects only when it descends past that level. The probe never saw this. It walked the same way but was its own code, so it agreed with the design and not with the implementation. Fifteen tests: the walk is whole, repeats nothing, matches the sequential walk, sizes its pages, clamps a zero page or concurrency, and passes a delimiter listing through; a listing streams a subtree, narrows by glob, yields a single named file, is empty for a missing path, reads one level, and reads twice. One of them corrects me rather than the code. `*.csv` matches `sub/b.csv`, because DataFusion matches listing globs with `require_literal_separator` false. The test now says so, since that fact is why one directory level needs its own function.
…-merge-2f7ba1 # Conflicts: # beacon-clients/beacon-web/src/pages/datasets.tsx # beacon-db/beacon-functions/src/lib.rs
`PaginatedListStore::list_paginated` adds no trailing delimiter, unlike `ObjectStore::list`. A shard of `a/one` therefore also returned `a/one.txt` from the level above and everything under a sibling `a/one2/`, so both came back twice. The test store split on directories and hid it. Also: `browse_datasets()` with no arguments resolved an empty path, which `std::path::absolute` refuses; `browse_datasets` was missing from the Python table-function list; and the Zarr tests for `top_level_zarr_meta_v3` went with a function that is still in use. Document both listing functions and the new endpoint.
…the contract test
…-merge-2f7ba1 # Conflicts: # CHANGELOG.md
Discovery resolves the scan prefix through the listing factory now, so the service's own copy of the URL went unread. Also make the listing Row public: it is in the signature of a public plan node.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Beacon lists the full datasets store for each browse. A recursive walk sends one sequential request per 1000 objects. A bucket with 2853217 objects needs 2854 requests.
The listing also ran inside a synchronous table function. It held a worker thread during query planning.
Change
Split the walk into shards. Read three directory levels first. Walk each shard in parallel, 16 at a time. Set the page size to 5000 keys.
Stream the listing. A plan node emits rows as pages arrive, so
LIMITstops the walk.Add
browse_datasets. It reads one directory level. It shares the provider withlist_datasets. A newis_directorycolumn marks sub-directory rows.GET /api/browse-datasetsruns SQL.Detect a Zarr store from its own path: a
*.zarrdirectory that holdszarr.json.Result
Measurements use a real SeaweedFS bucket. Each run returns all objects.
Notes
Shards interleave. Results no longer arrive in sorted order.
read_zarrnow needs the.zarrsuffix.Listings do not filter by grant rules, as before.
The crawler follows in #417.