Repository navigation
feat(delta): browse, preview, verify, generate and validate on Unity Catalog; generated mappings key and join lowercase tables (Iceberg too) - #1902
Conversation
There was a problem hiding this comment.
@bplatz this all looks good and holds up under my review. The only headline note I'd make is that I think I like your pattern here in verify_delta_unity_table more than the one I'd originally authored in Iceberg's verify_storage_access, namely this kind of "report and don't throw" pattern. Though if we do update/align the preexisting Iceberg-verify pattern, it will have consequences for fluree/solo's consumption of that API, just FYI
A Databricks catalog goes from "know your table names and hand-write R2RML" to browse → preview → verify → generate → delta map of the unedited file, and it gets there by extending the shared machinery rather than forking it: the same emitter and emit_options, a format-neutral cross_check_live that Iceberg now sits on too, and two general engine improvements (case-insensitive conventions, declared FKs that outrank inference) instead of Delta special cases. Routes mirror Iceberg's one for one behind the same admin gate, and the mocks match Databricks' documented tables shapes. Nothing blocking.
The notes are a narrow name-canonicalisation gap (mixed-case catalog or table names break a catalog-wide browse and drop declared FKs — reproduced with a scratch test), one Unity client per described table in generate, a question on whether verify should share Iceberg's contract (plus a cheap data-file probe), and a heads-up that lowercase Iceberg schemas now generate differently.
Adherence to repo commitments:
- Patterns/abstractions: ✔ extends
emit_r2rml,emit_options, the validate cross-check (nowcross_check_live),open_placedand the Unity client from #1901; invents no parallel generator or validator. - Performance (speed first, memory second): ✔ generation/validation-time only, no engine or query-path change — no performance-degradation risk.
- Testing: ✔ wiremock listing/description tests, pure API adapter tests, emitter tests for lowercase and declared keys, HTTP refusal/gate tests, CLI tests; CI ran
nextest --workspace --all-featureson5368f6c7a(13164 passed, new tests by name); four mutations caught. - Conventions: ✔ thorough multi-line bodies, clippy/fmt green, docs updated (Delta guide, CLI and endpoint references, Databricks walk-through);
⚠️ the Iceberg-visible generator change isn't in the title/body.
Verified locally at branch HEAD: nextest -p fluree-db-delta -p fluree-db-r2rml (202 passed) and the delta/unity/iceberg-validate/iceberg-generate/format-gating slice of fluree-db-api/fluree-db-server/fluree-db-cli with delta on (80 passed); four mutations (declared-FK skip, repeat-token guard, unmappable_columns, declared FKs in emit_schema) each caught; CI test/clippy/fmt green on 5368f6c7a.
Approving so you can merge when ready, but maybe worth folding in the name-canonicalisation fix and settling the verify question first.
| // when the schema is asked for by name. | ||
| let wanted: Vec<String> = schemas | ||
| .iter() | ||
| .map(|full| full.strip_prefix(&format!("{catalog}.")).unwrap_or(full)) |
There was a problem hiding this comment.
Optional (correctness, narrow input class) — Two places use the request's spelling of a name where Unity's canonical one is needed.
Here, a catalog-wide browse recovers each schema's short name by stripping "{catalog}." from Unity's full_name case-sensitively, falling back to the whole full_name. And UnityClient::describe returns the name as requested (unity.rs:266), not the full_name Unity sends back — which TableRecord already deserializes. Unity stores identifiers lowercase and matches them case-insensitively, so a user who types a name with capitals gets canonical lowercase names back.
What that does, reproduced with a scratch test (a mock answering catalog_name=Main with Unity's lowercase names): fluree delta browse --unity-catalog Main asks for schema_name=main.sales and schema_name=main.information_schema — the system-schema filter misses as well — and the first 404 aborts the whole listing with Unity Catalog, schema 'Main.main.sales': … (404). And generate MAIN.SALES.ORDERS MAIN.SALES.HEADERS silently drops the declared FK: Unity's parent_table is main.sales.headers, which is never in the emitter's joinable set, so it lands as "is not among the mapped tables" with no join. (That Unity answers a mixed-case catalog_name case-insensitively is my assumption from how it treats table lookups; I haven't run it against a workspace.)
A cheap fix: take the schema's own name from each row (Unity's schema objects carry name and catalog_name beside full_name) instead of stripping, and have describe prefer record.full_name when present — keying keyed_overrides off the described names too, so an override spelled as requested still finds its table. This is minor and non-blocking — but if you agree it's right, I'd rather see it folded in now than lost in the backlog.
| let tables: Vec<TableDescription> = futures::stream::iter(req.tables.clone()) | ||
| .map(|name| { | ||
| let unity = unity.clone(); | ||
| async move { fluree_db_delta::describe_unity_table(&unity, &name).await } |
There was a problem hiding this comment.
Optional (efficiency) — Generate builds a fresh UnityClient, and so does an OAuth token exchange, per described table.
This builds a fresh UnityClient per described table (describe_unity_table → UnityClient::new, catalog.rs:152), so with a service principal every table in a generate request does its own OAuth token exchange — up to eight concurrently — where browse already shares one Arc<UnityClient> across its fan-out (catalog.rs:94, :130).
Not hot-path, and I doubt Databricks' token endpoint minds a handful, but a 40-table generate is 40 token requests for one principal. Maybe a describe_unity_tables(&unity, &names) in fluree-db-delta that builds one client and fans out the same way browse does? Minor — if you agree, I'd rather see it in this PR than lost in the backlog. Commenting here because catalog.rs:152 is the other end of it.
| error: None, | ||
| }, | ||
| Err(DeltaError::Config(e)) => return Err(crate::ApiError::Config(e)), | ||
| Err(e) => DeltaTableAccess { |
There was a problem hiding this comment.
Question (API parity; a product call) — verify is the one step where Delta and Iceberg answer the same question differently.
This is more of a question than a suggestion. verify is the one step where Delta and Iceberg answer the same question differently. Iceberg's verify_storage_access (iceberg_catalog.rs:1182-1253) throws — 400 for a catalog failure, 403 StorageAccessDenied / CatalogCredentialsNotVended for storage — and proves both prefixes a query needs, including a HEAD on one data file. This answers 200 readable:false for anything but a config error, and reads only the log (snapshot + file_count, which never touches a data file).
I think report-don't-throw is the nicer contract, and the PR body calls it out as deliberate. But a client written against Iceberg's verify (per its doc comment, the probe behind Solo's onboarding "Test" button) that treats a 200 as a pass will read an unreadable Delta table as a pass unless it learns readable. Two edges fall out too: a table whose data files sit outside the prefix Unity vends for (I think a shallow clone, whose log names the source table's files by absolute path, is one) would verify readable and fail its first query; and a table Unity places on gs:// comes back as a 400 rather than readable:false, because validate_location's refusal is a DeltaError::Config (:310).
So maybe two things. The cheap one I'd fold in now: HEAD the first file file_count sees, the way Iceberg's probe does, and classify a catalog-placed unsupported location as readable:false. The other is a decision rather than a code change — which shape should both source types converge on — and since it touches Iceberg's public contract and Solo's UI, that's the case where a separate PR makes sense; I'd just like us to pick one on purpose.
| } | ||
|
|
||
| /// `strip_prefix`, ignoring ASCII case: catalogs differ in how they fold names. | ||
| pub fn strip_prefix_ignore_case<'a>(s: &'a str, prefix: &str) -> Option<&'a str> { |
There was a problem hiding this comment.
Question (release notes) — The case-insensitive conventions also change Iceberg output.
The case-insensitive conventions are a nice generalisation, and the golden mapping staying byte-identical is the right proof for uppercase catalogs.
They also change Iceberg output, though: a lowercase Iceberg table (Spark- or Trino-written — customer with customer_id) now gets a subject key and inferred FK joins where it used to get a synthesized key and literals. That reads as an improvement to me, but Solo persists the generated StructuredR2rmlMapping per dataset, so regenerating an existing lowercase Iceberg dataset will produce a different mapping. Since release notes come from PR titles, maybe worth a line in the body (or a title that mentions the generator) so it isn't a surprise on the Solo side?
There was a problem hiding this comment.
Addressed: the title and body now call out the change to generated mappings for lowercase tables, Iceberg included.
| } | ||
|
|
||
| /// [`cross_check_mapping`] over any table format's columns. | ||
| pub(crate) fn cross_check_live( |
There was a problem hiding this comment.
Praise — The Iceberg cross-check became format-neutral instead of being copied for Delta.
Turning the Iceberg cross-check into a format-neutral cross_check_live with the two Iceberg-specific wordings passed in, rather than copying it for Delta, is exactly the altitude I'd hope for — Iceberg's entry point and all 13 of its tests are untouched (re-ran them), and generate runs the same emitter with the same emit_options.
Disabling unmappable_columns turns a_mapping_is_checked_against_the_columns_a_query_will_find red.
| // whose local collides with an earlier one is disambiguated (not merged). | ||
| let mut emitted_join_locals: HashSet<String> = HashSet::new(); | ||
|
|
||
| // Declared keys first: nothing is inferred for a column one covers, joined |
There was a problem hiding this comment.
Praise — The declared-FK rules are carefully thought through.
The declared-FK rules are carefully thought through: declared outranks inferred (including inside the subject key), a declared column is never re-inferred, and a composite key, a parent outside the mapping, or a parent with no subject stays literal and says which.
I also checked the one case that worried me — a declared FK whose parent column isn't the parent's subject-template column (e.g. after a --subject-key override) — and it's still correct, because build_ref_shortcut only takes the template shortcut when the template columns are the FK's parent columns and otherwise scans the parent. Removing the declared-column skip turns two of the new tests red.
| } | ||
|
|
||
| /// Every page of a listing. `path` ends ready for another query parameter. | ||
| async fn pages(&self, about: Subject<'_>, path: &str) -> Result<Vec<serde_json::Value>> { |
There was a problem hiding this comment.
Praise — Every page is followed, with a guard against a repeating token.
Following every page with a guard against a repeating token, and testing that guard with a timeout, is the kind of defensive detail catalogs reward. Dropping the guard makes a_page_token_that_repeats_ends_the_listing time out.
| "/iceberg/r2rml/validate", | ||
| post(iceberg::iceberg_r2rml_validate), | ||
| ); | ||
| // The same for a Delta source on Unity Catalog. |
There was a problem hiding this comment.
Praise — Same route names as Iceberg's, behind the same admin gate.
Same route names as Iceberg's, in the same admin-protected reads block, with one flattened UnityConnectionRequest shared by delta/map and the new bodies so the SSRF guard and the env-var allowlist can't drift apart — and the_catalog_endpoints_are_the_administrators pins the gate.
5368f6c to
a055fdb
Compare
A Delta source on Unity Catalog has so far needed its table names known in advance and its mapping written by hand. The Unity client can now say what a catalog holds, without reading any table's files: - a listing reaches as far as the config's `catalog` and `schema` do: the metastore's catalogs, one catalog's schemas and their tables, or one schema's tables. Every page is followed. A catalog-wide listing leaves out `information_schema`, which is still listed when asked for by name. - each listed table carries its type and format, the reason this reader cannot read it (a view, another format), and a row filter if it has one. Whether a governed table can be read stays Unity's decision. - a description adds the columns in table order, the type a scan yields for each (from the column's Delta schema field, since Unity's separate precision and scale read zero), nullability, comments, masks, and the declared primary and foreign keys. The "not a Delta table" wording is now one predicate shared by placement, listing and description. A refused listing names its catalog or schema and carries Unity's own reason.
… foreign keys The generator's engine takes a plain table model, so it serves any tabular catalog. Two things tied it to the catalogs it was first fed: - its naming conventions (`_KEY` / `_ID` suffixes, `DIM_` / `FACT_` markers, the `<stem>_KEY` name fallback) matched uppercase only. They now ignore ASCII case, so a catalog that folds names to lowercase gets the same subject keys, join predicates and diagnostics. Output for uppercase names is unchanged; the golden mapping is byte-identical. - it could only infer foreign keys, by name and type. A table may now carry the keys its catalog declares. A declared single-column key is joined as declared, including where the names would not match, where they suggest a different parent, and where the column sits inside the subject key. Nothing is inferred for a column a declared key covers. A key that spans several columns, names a table outside the mapping, or names a parent with no subject stays literal and says why. Iceberg passes no declared keys, so its mappings are as before.
…e on Unity Catalog None of these registers a source. - preview: a table's columns as Unity records them, with the datatype a generated mapping would give each, whether a mapping can address it, masks, and the declared keys. - verify: reads the table's log with the credentials Unity issues, the steps a query's first touch takes. A table that cannot be read is a report carrying the catalog's or the store's reason; a request that cannot be made is an error. - generate: runs the shared mapping generator over Unity's description of each table. Tables are named `catalog.schema.table`, which `delta map` places through Unity with no defaults. Declared primary keys become subjects and declared foreign keys joins; a view or a table in another format is refused by name. Overrides are given by table name as the request spells it. - validate: compiles a mapping and checks it against each table's own log, placed as a `delta map` request places it, so it serves sources by path too. A mapped column of a type the reader cannot carry is an error, since it fails every query on its table. The mapping cross-check now runs over a format-neutral column, with the two places its wording depended on Iceberg passed in; Iceberg's entry point and its tests are unchanged. Registration and validate open a placed table through one function.
Five read-only endpoints lead up to `delta/map`, beside Iceberg's: POST /delta/catalog/browse catalogs, schemas or tables POST /delta/catalog/preview one table's columns and declared keys POST /delta/catalog/verify read the table with Unity's credentials POST /delta/r2rml/generate a mapping from Unity's table records POST /delta/r2rml/validate a mapping against the tables it names They take the Unity connection fields `delta/map` takes, now one struct with one builder, so the outbound-URL guard and the listed-variable rule for secrets apply alike. `validate` takes a `delta/map` body whose `name` is optional, so it checks exactly what a map of that body would read, for sources by path as well. All five sit behind the admin gate with the other reads that carry a secret and call out. The two concurrent listings hold owned values so their futures are `Send` inside a handler.
The read-only commands that lead up to `fluree delta map` on Unity Catalog: fluree delta browse catalogs, schemas or tables fluree delta preview <table> columns and declared keys fluree delta verify <table> read it with Unity's credentials fluree delta generate <table>... an R2RML mapping, to a file or stdout fluree delta validate --r2rml <file> a mapping against its tables Each runs on a named remote, on the local server when there is one, or in process, and asks the same question either way: one request body per command, under the endpoints' field names. `generate` writes the mapping to standard output or `-o`, and what it had to decide to standard error; `--subject-key` and `--class-name` override a table's key and class. `verify` and `validate` exit non-zero on a table that cannot be read or a mapping with an error, so a script can gate on them. `--json` prints the endpoint's answer as is. `delta map`'s options are regrouped into the connection, storage and source sets the new commands share; its flags and behaviour are as before.
…y are about Run over Unity Catalog, the generator said a subject key came "from Iceberg identifier_field_ids", called a `variant` column "a nested struct/list/map", and closed two notes with internal planning shorthand. The declared key's source is now an emit option, which Delta sets to the primary key Unity Catalog declares and Iceberg leaves as it was. A column that is passed over is described by its own type. The notes on uniqueness say that it cannot be verified from metadata.
…ify, generate, validate The Delta guide gains the path from a catalog to a registered source, the CLI and endpoint references the five read-only operations, and the Databricks walk-through a discovery step before mapping. The catalogs limitation now says what remains: tables by path have no catalog to list, and a generated mapping joins on single-column foreign keys.
a055fdb to
dbc2641
Compare
# Conflicts: # fluree-db-delta/src/unity.rs
… one client
Unity folds names to lowercase and matches them without regard to case,
but two places used the request's spelling:
- A catalog-wide browse recovered each schema's name by stripping
"{catalog}." from Unity's full_name case-sensitively. With
`--unity-catalog Main` it asked for schema `main.sales`, got a 404, and
the whole listing aborted (the information_schema filter missed too).
Each schema is now asked for by the catalog and schema names Unity
returns.
- `describe` returned the name as requested, so `generate
MAIN.SALES.ORDERS …` never matched a declared foreign key's parent
(`main.sales.headers`) and silently dropped the join. A table is now
known by Unity's full_name. Per-table overrides, given as requested,
are keyed by the name each table was described as.
Generate built a UnityClient, and so did an OAuth token exchange, per
described table. `describe_unity_tables` describes them all over one
client, as browse already did.
…l is the table's - `verify` read only the table's log, so a table whose data files can't be read (removed, or outside the prefix its credentials cover) reported readable. It now also stats the first data file through the table's own store, as Iceberg's verify does, and reports it as `probed_data_file`. - A table Unity Catalog places where the reader can't follow (the local filesystem, or Google Cloud Storage) was refused as a config error. That made `verify` a 400 and failed the whole `delta map`. It is now that table's refusal: `verify` reports `readable: false` with the reason, and mapping registers the source with a named table warning, as for a view or a table with an access rule.
Iceberg's `POST /iceberg/catalog/verify` now answers as Delta's does. A
table that cannot be read is a report, not an error:
`200 { "readable": false, "error": "…" }`. That covers a catalog that
won't load the table, vends no credentials while they are required, or
storage refusing a manifest or data-file read. These were a 400, or a 403
with err:catalog/CredentialsNotVended / err:storage/AccessDenied. Only a
request that cannot be made still errors: Direct catalog mode, an
unusable connection, or a catalog that returns no inline metadata.
StorageAccessReport gains `readable` and `error`. `credential_source`,
`metadata_location` and `data_files_listed` become nullable, for a probe
that failed before learning them. Breaking for clients that treated any
200 as a pass (Solo's onboarding "Test" button), which change with it.
Documented under the endpoint in docs/api/endpoints.md.
…types Delta's and Iceberg's verify already share a contract (200 with `readable`/`error`). Their fields now share names too, so one client reads both: - Iceberg's `data_files_listed` is renamed `data_file_count`, Delta's name. Iceberg's report is already breaking in this PR, so the rename costs clients nothing extra. - Delta gains Iceberg's `probed_data_file_bytes` (the probed file's size from its HEAD), and `data_probe_skipped` / `skip_reason` for a version with no data files. Fields that describe one format only stay as they are: Delta's `full_name`, `version` and `location`; Iceberg's `credential_source` and `metadata_location`. docs/api/endpoints.md lists the shared names.
Both catalog verify endpoints now point at a single "Verify response" reference in docs/api/endpoints.md, meant as the definition clients build against. It has: - the status contract: what is a 200 report, what is a 400 and with which @type, the admin 401, and that a table-level refusal is never a 403; - one field table with type, when each field is null, which source type returns it, and what it means; - readable and unreadable examples for each source.
#1901 is merged; this PR is against
main.Changes Iceberg users will see
_KEY/_ID,DIM_/FACT_, the<stem>_KEYfallback) now ignore case. A lowercase Iceberg table (Spark- or Trino-written, e.g.customerwithcustomer_id) now gets a subject key and inferred foreign-key joins where it used to get a synthesized key and literals. Output for uppercase names is unchanged (the golden mapping is byte-identical). Regenerating a mapping for an existing lowercase Iceberg dataset therefore produces a different mapping.POST /iceberg/catalog/verifyreports an unreadable table instead of failing (breaking for clients). A table the catalog won't load, credentials not vended, or storage refusing a manifest or data-file read now answers200 { "readable": false, "error": "…" }, as Delta's verify does. These were a 400, or a 403 witherr:catalog/CredentialsNotVended/err:storage/AccessDenied. The report gainsreadableanderror, anddata_files_listedis renameddata_file_count.credential_source,metadata_locationanddata_file_countbecome nullable. Delta's and Iceberg's verify reports now sharereadable,error,data_file_count,probed_data_file,probed_data_file_bytes,data_probe_skippedandskip_reason. Direct mode, an unusable connection and a catalog without inline metadata still error. Solo's onboarding "Test" button changes with this.Why
A Delta source on Unity Catalog has needed its table names known in advance and its R2RML written by hand. Iceberg sources already have a read-only onboarding API (browse, preview, verify, generate, validate). This adds the same five steps for Delta on Unity, as HTTP endpoints and as CLI commands. None of them registers anything.
fluree delta browsePOST /delta/catalog/browsefluree delta preview <table>POST /delta/catalog/previewfluree delta verify <table>POST /delta/catalog/verifyfluree delta generate <table>…POST /delta/r2rml/generatefluree delta validate --r2rml <file>POST /delta/r2rml/validateWhat changed, by commit
fluree-db-delta: list and describe. The Unity client lists catalogs, schemas and tables (every page followed) and describes a table: columns in table order, the type a scan yields for each, nullability, comments, masks, and declared primary and foreign keys. Column types come from Unity'stype_json(the column as a Delta schema field), parsed by Kernel, because Unity's separate precision and scale fields read zero. The "not a Delta table" wording is now one predicate shared by placement, listing and description. A catalog-wide listing leaves outinformation_schema; it is still listed when named.fluree-db-r2rml: two changes to the shared generator engine._KEY/_ID,DIM_/FACT_, the<stem>_KEYfallback) ignore ASCII case, so a catalog that folds names to lowercase gets the same treatment. Output for uppercase names is unchanged; the golden mapping is byte-identical.fluree-db-api:delta_catalog.rs. The adapter from Unity's description to the engine's input, plus preview, verify and validate. The mapping cross-check now runs over a format-neutral column (cross_check_live), with the two Iceberg-specific wordings passed in; Iceberg's entry point and its 13 tests are untouched. Registration and validate open a placed table through one function (open_placed). Validate reads each table's own log, so it serves sources by path too.fluree-db-server: five routes, in the admin-protected reads block beside Iceberg's. The Unity connection fields are one struct flattened intodelta/mapand the new bodies, so the outbound-URL guard and the listed-variable rule for_envsecrets apply alike.fluree-db-cli: five commands. Each runs on a named remote, the local server, or in process, from one request body per command.verifyandvalidateexit non-zero on a bad result.delta map's flags are regrouped into shared argument sets; its flags and behaviour are as before.variantcolumn "a nested struct/list/map". The key's source is now an emit option, and a skipped column is described by its own type. This rewords two messages Iceberg users see as well.Design points worth a look
access_rule, not as unreadable;verifyis what settles it, with Unity's own reason.200withreadable: false; only a request that cannot be made is an error.catalog.schema.table, so the mapping registers with no catalog or schema defaults.Sendinside an axum handler.Verification
fluree-db-delta,fluree-db-r2rml,fluree-db-api --features deltagraph sources,fluree-db-server --features deltalib +grp_query,fluree-db-clilib). The CLI also builds with--no-default-features --features iceberg.delta/maprefuses (internal hosts, an unlisted secret variable, no auth), that verify and validate guard their S3 endpoint, that validate registers nothing, and that all five sit behind the admin gate.delta mapof the generated file unedited, and a SPARQL query across the generatedrr:parentTriplesMapjoin returning the inserted rows. Browse, verify, generate and validate were also run live over HTTP through a server, with the token named by an allowlisted variable.Not verified
verifyon Azure rests on feat(delta): Databricks tables by name through Unity Catalog #1901's SAS path.fluree-db-deltawith a plain client, and the API's pure parts separately.Limitations
customersdoes not findcustomer_id). Such a table gets a synthesized key and a diagnostic;--subject-key/per_table_overridessettles it.