feat: local-filesystem Iceberg tables (catalog-less Direct mode) - #1611
Conversation
Direct { table_location } now accepts file:// URIs and bare absolute paths
alongside s3://, reading Iceberg tables straight from local disk — no REST
catalog, no object store, no AWS credential-chain resolution. Primary use
case: locally-written test/dev datasets (pyiceberg or Spark against a local
warehouse), with the full snapshot machinery — incremental windows, pinned
reads, materialization — working unchanged over local tables.
- FileIcebergStorage: read/read_range/file_size/list over tokio::fs,
resolving file:///abs, file:/abs, and bare absolute paths. Object-store
URIs are rejected with an error naming the copied-table cause; location
remapping for tables copied from S3 is a deliberate follow-up.
- IcebergStorageBackend enum (S3 | File) threads one concrete storage type
through the scan surface, session caches, and the lazy-storage path;
dispatch on location scheme happens in the Direct arms and
build_preview_storage. SendIcebergStorage's trait definition is no longer
aws-gated (only the S3 impl is).
- version-hint.text fallback: when the hint is absent (pyiceberg and most
non-Hadoop writers never produce it), Direct mode lists metadata/ and
takes the highest-versioned *.metadata.json — both v{N} and
{NNNNN}-{uuid} naming conventions. S3 behavior unchanged: listing is not
attempted there and the original hint error is kept.
- Config: Direct table_location validation accepts local paths; table
identifiers derive from local paths the same way as S3 ones.
- Docs: local-filesystem Direct mode in docs/graph-sources/iceberg.md.
…for the local scan path A copied or moved Iceberg table carries its ORIGINAL root in every manifest file reference (Iceberg metadata uses absolute URIs), so a naive local read of a relocated table tries to fetch from the old location. Both ends of the fix are already known — the metadata's own 'location' field (the old root, possibly s3://) and the configured table_location (where the table sits now) — so the remap is INFERRED, with zero configuration: copy the table directory, point table_location at it, done. - FileIcebergStorage::with_remap(from, to): prefix rewrite on a path-segment boundary, applied before path resolution; only ever rewrites toward the operator-configured location, never toward anything derived from the (untrusted) metadata. - remap_local_storage in the provider: constructed after metadata parse in both scan paths (prepare_iceberg_scan and the scan_table loader); no-op for REST, S3, and locally-written tables. - Committed fixture tests/fixtures/iceberg/silver/people (44 KB, real pyiceberg two-snapshot table) and the end-to-end test now runs UNCONDITIONALLY in CI: the fixture's metadata references the /tmp path it was written under, so every CI run also proves the remap. The env-var override remains for ad-hoc local tables. - Docs: the copied-table paragraph in Direct-mode local filesystem docs now describes the inferred remap instead of a rejection.
aaj3f
left a comment
aaj3f
left a comment
There was a problem hiding this comment.
This is a really nice piece of work, @bplatz, and the second commit is what makes it: inferring the relocation from the metadata's own location versus the configured table_location means the committed fixture is the remap test, because it still carries the /tmp paths pyiceberg wrote it under. That's the rare case where the test fixture and the feature prove each other. I checked it rather than assuming — I made remap_local_storage return its input unchanged and the end-to-end test went red on both scan paths (query and materialize), so the coverage has real teeth. Nice too that SendIcebergStorage's trait definition losing its aws gate doesn't break anything: cargo check -p fluree-db-iceberg --no-default-features still compiles.
The one thing I'd like a decision on before this merges is the config-validation relaxation, and it's a design call rather than a defect. Accepting any absolute table_location is right for the CLI and local dev, but solo forwards a caller-supplied table_location from POST /v1/fluree/iceberg/map straight into IcebergCreateConfig::new_direct (fluree-lambda-query/src/handler.rs:3130) and does no scheme check of its own — because it never had to, this crate did it. On the next pin bump that goes away, and resolve_direct_table_location's warehouse-root miss error enumerates the directories it found back to the caller. To be accurate about severity: that's directory and file-existence disclosure, not arbitrary file read, and it needs graph-source-create permission. But it lands in a different repo, silently, on a version bump. Gating local locations behind a cargo feature or a FLUREE_ICEBERG_LOCAL_ROOTS allowlist would close it here for every embedder at the cost of one flag in the dev workflow. I'm also fine with "embedders validate their own input" as the answer — I'd just want it written down in docs/graph-sources/iceberg.md so nobody rediscovers it the hard way, and I'll file the solo-side issue either way.
Smaller things, none of which should hold this up: pick_latest_metadata_file picks the highest filename version, which is silently-stale rather than loud if a writer ever produces a lower-numbered newer file — probably spec-forbidden, worth a comment either way. And three places still describe the test as env-gated after commit 2 made it unconditional (Cargo.toml:158, r2rml_materialize.rs:193, the repo.ttl fact).
Adherence to repo commitments:
- Patterns/abstractions: ✔ Implements the two existing storage traits rather than inventing a third;
IcebergStorageBackendas a concrete enum keeps the whole scan surface non-generic;list_filesadded as a trait method defaulting to an error so only the backend that can list, does. - Performance (speed first, memory second): ✔ No query-engine, indexer, or binary-index path touched. The new path removes a credential-chain resolution per scan; the enum adds one match per storage call. No performance-degradation risk.
- Testing: ✔ Real committed pyiceberg fixture, unconditional in CI, driving query → incremental window → pinned read → typed
SnapshotNotFound; eight new unit tests, all verified present by name; and the e2e test goes red under mutation of the remap.
Verified locally at branch HEAD (ff3bb3a3d): cargo test -p fluree-db-iceberg --features aws --lib → 277/0 with all eight new tests confirmed by name; it_iceberg_local_fs → 1 passed; the same test red with remap_local_storage neutered (both scan paths, after clearing $TMPDIR/fluree_binary_cache), worktree restored clean; cargo check -p fluree-db-iceberg --no-default-features compiles; cargo fmt --all -- --check and clippy on both changed packages clean.
Approving so you can merge when ready — I'd just like the local-path gating question answered one way or the other first, and note that this conflicts with #1613 in prepare_iceberg_scan's Direct arm, so whichever of you goes second gets the rebase
| /// Whether `location` addresses the local filesystem: a `file://` URI or a | ||
| /// bare absolute path. The dispatch predicate used when choosing a storage | ||
| /// backend for a `Direct` table location. | ||
| pub fn is_local_location(location: &str) -> bool { |
There was a problem hiding this comment.
Blocking-ish, but it's a design call rather than a defect. Relaxing Direct-location validation to accept any absolute path turns table_location into a local-filesystem read primitive for whatever embeds this crate, and we have an embedder that hands it caller-supplied input. (Same call, via the two validation gates at fluree-db-iceberg/src/config.rs:194 and fluree-db-api/src/graph_source/config.rs:974.)
The mechanism: is_local_location is location.starts_with("file://") || location.starts_with('/'), and resolve (:99-129) turns whatever it is handed into a PathBuf with no containment check against a configured root. That is entirely reasonable for the CLI and for local dev, which is the use case the PR is written for. The part I want to name is what happens downstream — in solo, POST /v1/fluree/iceberg/map passes the request's table_location straight through to IcebergCreateConfig::new_direct (fluree-lambda-query/src/handler.rs:3130) and does no scheme checking of its own, because until now it didn't have to: this crate rejected anything that wasn't s3://. On the next pin bump that check quietly goes away.
The concrete scenario, which needs no relocation trickery: create a Direct graph source with table_location: "/var/task" and an rr:tableName that doesn't match the leaf directory. resolve_direct_table_location (r2rml.rs:2047) treats it as a warehouse root, LISTs it, fails to match, and the error it raises enumerates every directory it found — "no directory under the warehouse root matches table 'x' … Found N directories: [<names>]" — which surfaces to the caller as an InvalidQuery. Point it at /tmp, /opt, $HOME and you have a directory-listing oracle over the process's filesystem. Reading actual contents is much harder (you'd need a valid Iceberg metadata JSON sitting there), so I want to be accurate about severity: this is directory and file-existence disclosure, not arbitrary file read.
I don't think the fix belongs downstream, because every embedder would have to independently rediscover that the validation changed. A cheap version here would be to gate local locations behind an explicit opt-in — a local-filesystem-tables cargo feature, or an env-supplied allowlist of permitted local roots that resolve also confines paths to — which costs the local/dev workflow one flag and closes it for everyone. Something like:
pub fn is_local_location(location: &str) -> bool {
if std::env::var_os("FLUREE_ICEBERG_LOCAL_ROOTS").is_none() {
return false;
}
location.starts_with("file://") || location.starts_with('/')
}I'm genuinely open to "no, embedders own their own input validation and we'll fix it in solo" — that's a defensible answer. I'd just rather we decide it deliberately than inherit it on a pin bump. If we go that way, it's worth a line in docs/graph-sources/iceberg.md next to the local-filesystem section saying so explicitly, and I'll open the corresponding solo issue.
| /// and bare absolute paths. Object-store URIs are rejected by name — the | ||
| /// usual cause is a table copied from S3 whose manifests still reference | ||
| /// the original bucket. | ||
| fn resolve(path: &str) -> Result<PathBuf> { |
There was a problem hiding this comment.
Optional, and related to the above. resolve doesn't canonicalize, and apply_remap (:69) appends the un-inspected remainder after the prefix swap, so a manifest reference of s3://bucket/wh/table/../../../etc/passwd under a configured remap resolves to <configured-root>/../../../etc/passwd. Against S3 that reference would just 404; locally it escapes the root. Manifest content is only as trustworthy as whoever handed you the table directory, so this is mostly the same trust boundary as the item above — but a path.canonicalize()?.starts_with(root) check in resolve is a few lines and closes both at once. Happy to punt to a follow-up if you'd rather do it in one pass with the gating decision.
| /// `{NNNNN}-{uuid}.metadata.json` (pyiceberg / REST-commit style). The highest | ||
| /// parsed version wins; a tie breaks lexicographically so the choice is | ||
| /// deterministic. | ||
| fn pick_latest_metadata_file(files: &[String]) -> Option<&str> { |
There was a problem hiding this comment.
Optional, more of a question. pick_latest_metadata_file takes the highest leading-integer filename version, which is the same heuristic pyiceberg's static-table reader uses, so I think it's the right default. The case I can't talk myself out of is a table whose newest metadata file has a lower number than a surviving older one — a REPLACE TABLE, or a writer that resets its counter after a metadata rewrite. There we'd silently serve a stale snapshot rather than error, and silent-stale is the failure mode I'd least want here. It may be that Iceberg's spec forbids that and I'm inventing a hazard; if so a one-line comment saying "monotonic by spec" would settle it for the next reader. If it doesn't forbid it, metadata.json's own last-updated-ms would be the tiebreak.
| name = "it_import_low_fd" | ||
| path = "tests/it_import_low_fd.rs" | ||
|
|
||
| # Standalone, env-gated: end-to-end over a REAL local Iceberg table |
There was a problem hiding this comment.
Nit. Three places still describe the test the way commit 1 left it, before commit 2 committed the fixture and made it unconditional: this comment (Standalone, env-gated … Skips cleanly when unset), fluree-db-api/src/graph_source/r2rml_materialize.rs:193 ("what remains is a committed local Iceberg table fixture (#1608)" — it's in this PR), and the .fluree-memory/repo.ttl fact, which says "Env-gated test it_iceberg_local_fs (FLUREE_LOCAL_ICEBERG_TABLE)". This one is the one that'll mislead someone, since Cargo.toml is where you look to find out whether a test runs.
| ] . | ||
| "#; | ||
|
|
||
| fn table_location() -> String { |
There was a problem hiding this comment.
Optional, hygiene. The test shares $TMPDIR/fluree_binary_cache (ledger_manager.rs:733) with every other run on the machine, and that cache survives between runs. It cost me a confusing half-cycle during review: with the remap mutated off, the query assertion still passed against a warm cache and only the materialize scan failed, which reads as "the query path doesn't need the remap" — it does, and clearing the cache proved it. CI is fresh each time so this isn't a correctness issue, but a per-test cache_dir (or a LedgerCacheConfig pointing at a tempdir) would make local runs mean what they look like they mean.
| /// helper serves both the Send and non-Send clients. A storage backend without | ||
| /// listing support (S3) surfaces the ORIGINAL version-hint error, so S3 | ||
| /// Direct-mode behavior is unchanged. | ||
| async fn fallback_metadata_location( |
There was a problem hiding this comment.
Praise. Passing the not-yet-awaited listing future into fallback_metadata_location so one helper serves both the Send and non-Send clients is a nice way out of that duplication, and having a backend with no listing support fall back to re-raising the original hint error is exactly the right call — it's what makes "S3 behavior unchanged" true rather than aspirational, and I checked that S3IcebergStorage takes the trait default here.
| if rest.is_empty() { | ||
| return std::borrow::Cow::Owned(to.clone()); | ||
| } | ||
| if let Some(rest) = rest.strip_prefix('/') { |
There was a problem hiding this comment.
Praise. The mid-segment boundary check so /wh/table doesn't swallow /wh/table2 is the bug I went looking for first, and it's already handled with a test naming that exact case.
Direct `table_location` gained `file://` and bare-absolute-path support, which relaxed a check that embedders were relying on: services that forward caller-supplied locations never had to validate the scheme themselves, because this crate rejected everything that was not s3://. Removing that on a version bump would be silent, and combined with the warehouse-root listing it turns `table_location` into a directory-enumeration oracle — the miss error names every directory it found. Local locations are now fail-closed behind FLUREE_ICEBERG_LOCAL_ROOTS, a colon-separated allowlist of absolute directories. Unset, a local location is refused when the graph source is CREATED, with an error naming the switch. An env switch rather than a cargo feature because `iceberg` is a default feature of both server and CLI: a compile-time flag would be on in exactly the builds that need it off, and could not be enabled for a legitimate local deployment without a rebuild. The same roots close the traversal hole: `resolve` did no normalization and `apply_remap` appended the remainder uninspected, so a manifest reference of `.../table/../../../etc/passwd` resolved outside the table root. Every path is now normalized and confined, checked both lexically and against its canonical form so a symlink out of a root does not escape either. Roots keep both forms so macOS `/var` -> `/private/var` still matches. Policy lives in one module and all five decision sites route through it — the three dispatch predicates and, critically, BOTH config validation gates, which duplicated the scheme test inline and would otherwise have accepted a location the dispatcher then refused. The storage captures its roots at construction, so the per-read path touches no global state. Also settable as `iceberg_local_roots` in config.toml/config.jsonld, and documented in the Iceberg graph-source and configuration guides. Corrects three places that still described it_iceberg_local_fs as env-gated after it became an unconditional CI test.
…nt cap `repo_memory_blocks_are_well_formed` has failed on main since #1611: fact-01kzemcff3b773aq7yckej9dhk is 996 chars against the 750-char cap, so every branch's CI is red on a lint that is not about the branch. It was carrying two separable things. The original id keeps the mechanism and the two-places-validation gotcha; a new fact takes the fail-closed FLUREE_ICEBERG_LOCAL_ROOTS guard and the test coverage that pins it, with its own tags and artifact refs so it is recallable on a security question rather than only on an Iceberg one. Both are well under the cap, and no content was dropped. Unrelated to this branch's storage work, fixed here to unblock CI.
2 participants
Summary
Direct-mode graph sources now read Iceberg tables straight from the local filesystem —
table_locationacceptsfile://URIs and bare absolute paths alongsides3://. No REST catalog, no object store, no AWS credential-chain resolution. The primary use case is local test/dev datasets (write with pyiceberg or Spark against a local warehouse, point a graph source at the directory), with the full snapshot machinery — incremental windows, pinned reads, materialization — working unchanged over local tables, and a large per-query latency win from removing the catalog/object-store round-trips.Two commits:
FileIcebergStorage(the 3-method storage trait overtokio::fs); anIcebergStorageBackendenum (S3 | File) threads one concrete storage type through the scan surface, session caches, and lazy-storage path, dispatched per table location;version-hint.textfallback — when the hint is absent (pyiceberg and most non-Hadoop writers never produce it), Direct mode listsmetadata/and takes the highest-versioned*.metadata.json(bothv{N}and{NNNNN}-{uuid}conventions; S3 behavior unchanged).SendIcebergStorage's trait definition is no longer aws-gated (only the S3 impl is).location→ configuredtable_location— so copy the table directory, point at it needs zero configuration, including tables copied down from S3.Testing
tests/fixtures/iceberg/silver/people: a committed, real pyiceberg-written two-snapshot table (44 KB), regenerable viascripts/local/write_local_iceberg_table.py.it_iceberg_local_fsruns unconditionally in CI and drives the whole stack end-to-end: graph-source creation, R2RML query (5 rows through metadata → Avro manifests → Parquet decode), incremental window (exactly the second append's 2 rows), snapshot-pinned read (3 rows as of snapshot 1, pin honored in the watermark), typedSnapshotNotFoundon an unknown pin — and, because the fixture's metadata carries the/tmppaths it was written under, every run also proves the relocation remap.check --all-features --all-targets, clippy-D warnings, 277 iceberg + 925 api lib tests green.Closes #1608 — this delivers what that issue asked for with a real-table fixture instead of an in-memory writer: the storage seam (
IcebergStorageBackend), offline CI coverage ofplan_scan/plan_incremental/ the listing fallback / pruning / the pinned-read paths, and it ships as a user feature rather than test scaffolding. (Adversarial fixtures for the MoR guard — delete-file-bearing snapshots — remain possible follow-up work, noted there.)