diff --git a/README.md b/README.md index 184c514..e535b6a 100644 --- a/README.md +++ b/README.md @@ -203,25 +203,36 @@ custodian, not necessarily a developer. `%ingest help` does the same for `Ingest `%%catalog` (the cell-magic form) sets the context once, with `use(path)` on its magic line, then runs every other line of the cell in order under that context, without repeating the full -path on each line. A leading `.` resolves any `path`/`source_path`/`target_path` against the -context (so `data_copy`/`set_tags`/`create_folder`/... all understand it); `use` alone also accepts a bare -path with no dot, once a context exists — `"2027"` and `".2027"` narrow it the same way there. -One or more leading `../` (or a bare `..`) instead walks up that many levels of the context -first, everywhere a relative path is understood, not just in `use` — `"../water_temperature"` is -a sibling of the context, `"../../water_temperature"` a level further up. `use` also makes a live -check that the resolved path actually exists in the catalog, raising if it doesn't, rather than -silently pointing context somewhere later calls would fail against anyway; `use(None)` clears the -context. `get_context()` shows what it currently is — always the full resolved path: +path on each line. `use`'s own `path` must always be a whole, absolute path — unlike every +other verb, it's never resolved against whatever context already exists, and never accepts a +leading `.`/`../`. It does make a live check that `path` actually exists in the catalog first, +raising if it doesn't, rather than silently pointing context somewhere later calls would fail +against anyway; `use(None)` clears the context. Every other verb's own `path`/`source_path`/ +`target_path` still resolves against the current context once one is set — with or without a +leading `.` (`"2027"` and `".2027"` mean the same thing) — unless it already starts with +`catalog` (this deployment's one real root source), in which case it's always taken literally +as absolute rather than appended to the context, dot or not. `data_copy`/`data_move`/ +`create_view`'s two paths are a further exception: they resolve `source_path`/`target_path` +independently against that same context, so routinely pair a relative one with a genuinely +unrelated absolute one, and still require the dot to mean relative (a bare path there is +always absolute, context or not, `catalog`-prefixed or not). One or more leading `../` (or a +bare `..`) instead walks up that many levels of the context first, everywhere a relative path +is understood except inside `use` itself — `"../water_temperature"` is a sibling of the +context, `"../../water_temperature"` a level further up. `get_context()` shows what the +context currently is — always the full resolved path: ```python %%catalog use("bwd.reference") -set_tags(".water_temperature", ["reviewed"]) -create_folder(".2027") # create_folder still needs the dot — only use() makes it optional - -%catalog use("bwd.reference") # back to a path that already exists -%catalog use("2027") # bare, no dot — same as use(".2027"); already created above -%catalog get_context() # -> 'bwd.reference.2027' -%catalog set_tags("../water_temperature", ["archived"]) # ../ works for any verb, not just use() +set_tags("water_temperature", ["reviewed"]) # bare, no dot — same as ".water_temperature" +create_folder(".2027") + +%catalog use("bwd.reference.2027") # use() always takes the whole, absolute path +%catalog get_context() # -> 'bwd.reference.2027' +%catalog set_tags("../water_temperature", ["archived"]) # ../ works for any verb except use() +%catalog get_tags("catalog.other_root.assessments") # starts with 'catalog' — absolute, not appended +%catalog data_copy(".water_temperature", "other_root.archive.water_temperature_2027") +# ^ bare, but data_copy/data_move/create_view always +# take a bare target_path/source_path literally ``` `get_wiki(path)`, `get_tags(path)`, `list(path)` (every table/view under `path`, at any depth) @@ -230,12 +241,15 @@ to commit or undo. `delete_view(path)`/`delete_table(path)` queue like every oth unlike the idempotent `DROP ... IF EXISTS` `Catalog.deleteview`/`deletetable` wrap — require `path` to already exist, and (like `delete_folder`) can never be undone. Every one of these raises `CatalogOperationError` if `path` doesn't exist; `schema` also requires it to be a table -or view, not a folder: +or view, not a folder. `list`'s `path` may also be omitted (or `""`) to list the current +context itself — raises `CatalogSessionError` if none is set yet: ```python %catalog get_wiki("bwd.reference.water_temperature") %catalog get_tags("bwd.reference.water_temperature") %catalog list("bwd.reference") # -> ['bwd.reference.water_temperature', ...] +%catalog use("bwd.reference") +%catalog list() # same result — path omitted, lists the context itself %catalog schema("bwd.reference.water_temperature") # -> TableInfo(schema={...}, row_count=...) %catalog delete_view("bwd.reference.old_view") ``` diff --git a/debugger/test_catalog_magic.ipynb b/debugger/test_catalog_magic.ipynb index e7a0b98..f028312 100644 --- a/debugger/test_catalog_magic.ipynb +++ b/debugger/test_catalog_magic.ipynb @@ -218,6 +218,12 @@ "%catalog set_wiki(TEST_FOLDER, \"# Catalog magic smoke test\\n\\nCreated by the %catalog debugger notebook.\")" ] }, + { + "cell_type": "markdown", + "id": "851a3985", + "source": "## 4. Read-only queries answer immediately too\n\n`get_wiki`/`get_tags`/`list`/`schema` are read-only `CatalogSession` methods — no `%catalog\ncommit` involved, same as every other call in this notebook. `get_wiki` reads back the wiki\njust set above; `list` finds no tables/views here since `TEST_FOLDER` only holds folders —\nan empty list, not an error (it would only raise if `TEST_FOLDER` itself didn't exist).\n`list`'s own `path` can also be omitted entirely, to list whatever the current context is —\nthe next cell does exactly that, having just pointed context at `TEST_FOLDER` via `use`. The\ncell after that passes `TEST_ROOT` — a `catalog.`-prefixed path — while context is still\n`TEST_FOLDER`, to prove it's taken literally as absolute rather than appended to it (it\nwould otherwise become `TEST_FOLDER.catalog....`, which doesn't exist, and raise).", + "metadata": {} + }, { "cell_type": "code", "execution_count": null, @@ -229,17 +235,28 @@ ] }, { - "cell_type": "markdown", - "id": "59854882", + "cell_type": "code", + "id": "70ce527b", + "source": "%catalog list(TEST_FOLDER) # -> [] (only folders live here — not an error, unlike a typo'd path)", "metadata": {}, - "source": [ - "## 4. Read-only queries answer immediately too\n", - "\n", - "`get_wiki`/`get_tags`/`list`/`schema` are read-only `CatalogSession` methods — no `%catalog\n", - "commit` involved, same as every other call in this notebook. `get_wiki` reads back the wiki\n", - "just set above; `list` finds no tables/views here since `TEST_FOLDER` only holds folders —\n", - "an empty list, not an error (it would only raise if `TEST_FOLDER` itself didn't exist)." - ] + "execution_count": null, + "outputs": [] + }, + { + "cell_type": "code", + "id": "66eb277b", + "source": "%catalog use(TEST_FOLDER)\n%catalog list() # path omitted — lists the current context (TEST_FOLDER) itself; same [] result", + "metadata": {}, + "execution_count": null, + "outputs": [] + }, + { + "cell_type": "code", + "id": "60e1494f", + "source": "%catalog list(TEST_ROOT) # starts with 'catalog' — always absolute, never appended to\n # the current context (still TEST_FOLDER, per the cell above)", + "metadata": {}, + "execution_count": null, + "outputs": [] }, { "cell_type": "markdown", diff --git a/docs/notebook-facade-for-data-scientists.md b/docs/notebook-facade-for-data-scientists.md index 489a219..ede131e 100644 --- a/docs/notebook-facade-for-data-scientists.md +++ b/docs/notebook-facade-for-data-scientists.md @@ -23,14 +23,11 @@ behaviour described below is unchanged; there's just no longer a separate still queues/commits as described throughout this doc. Context has also grown past what's described below: `use(path)` (also via `%%catalog use(path)`, the cell-magic form) sets it deliberately — making a live -check that the resolved path exists in the catalog first, unlike -everything else here, and always storing the full resolved path, never a -raw `.`/`..`-prefixed fragment — `get_context()` reads it back, and a -leading `../` (or a bare `..`) on any relative path, not just inside -`use`, walks up that many levels of the context first. `copy`/`move` were -later renamed `datacopy`/`datamove`, matching `Catalog`'s own names instead -of inventing friendlier ones, then renamed again to `data_copy`/`data_move` -for consistency with the rest of the facade's underscored verbs (`Catalog`'s +check that `path` exists in the catalog first, unlike everything else +here — and `get_context()` reads it back. `copy`/`move` were later renamed +`datacopy`/`datamove`, matching `Catalog`'s own names instead of inventing +friendlier ones, then renamed again to `data_copy`/`data_move` for +consistency with the rest of the facade's underscored verbs (`Catalog`'s own `datacopy`/`datamove` are unchanged — only the `CatalogSession`/ `%catalog` wrapper got the underscore); six more verbs were added — read-only `get_wiki`, `get_tags`, `list`, `schema` (answered immediately, like @@ -42,10 +39,46 @@ for consistency with `delete_folder`/`delete_wiki`). `tag`/`untag` were similarly renamed `set_tags`/`delete_tags` (matching `set_wiki`/ `delete_wiki`'s pattern), and `set_meta` was removed — the raw `Catalog.setmeta2wiki`/`getmetafromwiki` are still there for a folder's -wiki Meta Data section, just not wrapped by `CatalogSession` any more. See -`src/eea_datalakehouse/notebook/magics.py`'s module docstring and +wiki Meta Data section, just not wrapped by `CatalogSession` any more. + +Relative-path resolution grew, then partly retreated. It first grew past +a leading `.`: a leading `../` (or a bare `..`) on any relative path +walked up that many levels of the context first, and — since requiring a +dot everywhere turned out to be a real papercut in practice (a custodian's +first instinct after `use(...)` was to type a bare short name regardless +of which verb came next) — every single-path verb started accepting a +bare path with no leading `.` at all too, once a context existed, the same +as `use` already did (`data_copy`/`data_move`/`create_view` are the +deliberate exception: they resolve `source_path`/`target_path` +independently against the *same* starting context and routinely pair a +relative one with a genuinely unrelated absolute one in the same call, so +a bare path there still always means absolute, context or not). `use` +itself then reverted the other way: it now only ever accepts a whole, +absolute `path` — never resolved against whatever context already exists, +and never a leading `.`/`../` fragment — so pointing context somewhere +always means saying exactly where, with the same live existence check as +before. Every other verb's own relative-path behaviour (dot-optional, +`../`-aware) is unchanged. `list`'s own `path` became optional on top of +that — omitted (or `""`), it lists the current context itself, raising +`CatalogSessionError` if none is set, rather than making a custodian who's +already `use()`d somewhere repeat that same path right back to `list()`. + +The dot-optional rule then grew one more exception of its own: a bare path +that already starts with `catalog` (`_ROOT_SOURCE` in `session.py` — this +deployment's one real top-level source) is always taken literally as +absolute, context or not, rather than getting appended to whatever +context happens to be set. Before this, passing a full `catalog....` path +alongside an already-set context — mixing an absolute path with ordinary +relative use in the same session — silently produced a nonsense +double-nested path (`f"{context}.catalog...."`) unless a custodian +remembered to clear context first; `_ROOT_SOURCE` makes that case +unambiguous instead. `data_copy`/`data_move`/`create_view` don't get this +treatment — a `_ROOT_SOURCE` check can't tell a deliberately relative bare +path (still meant to be appended there) apart from one that just happens +not to start with `_ROOT_SOURCE`, so they keep requiring the dot outright. +See `src/eea_datalakehouse/notebook/magics.py`'s module docstring and `src/eea_datalakehouse/catalog/session.py`'s `use`/`get_context`/ -`_resolve_path` for the current, authoritative behaviour. +`_resolve_path`/`list` for the current, authoritative behaviour. ## The facade's surface, end to end @@ -319,8 +352,10 @@ This splits into two sides that live in two different repositories. open question below with an explicit marker (a leading `.`, e.g. `session.set_tags(".water_temperature", ...)`), and — critically — `set_context` itself is not something a data custodian is expected to call directly -(`use`, added later, is the custodian-facing entry point built on the same -mechanism — see "What it exposes" above): +(`use`, added later, is the custodian-facing entry point for setting +context deliberately — see "What it exposes" above — though it takes only +a whole, absolute path, plus a live existence check `set_context` itself +doesn't make): - **Ordinary use already keeps it current on its own.** Every queueing verb updates the context from whatever path it just touched (the target's diff --git a/docs/notebooks/catalog_session_example.ipynb b/docs/notebooks/catalog_session_example.ipynb index 27e5d54..cc06d42 100644 --- a/docs/notebooks/catalog_session_example.ipynb +++ b/docs/notebooks/catalog_session_example.ipynb @@ -21,7 +21,7 @@ { "cell_type": "markdown", "id": "0f5e1ae8", - "source": "## Quick reference\n\n`%catalog help` (or `%catalog help()`) prints every command as a plain table — name,\nparameters, description — rather than a raw Python signature, since a data custodian\nreading it may not be fluent in Python type-hint syntax. Handy when you don't remember\nan exact parameter name mid-notebook. It works even before `DREMIO_BASE_URL`/\n`DREMIO_TOKEN` are set, since it's answered before a session is built.", + "source": "## Quick reference\n\n`%catalog help` (or `%catalog help()`) renders every command as an HTML table — name,\nparameters, description — rather than a raw Python signature, since a data custodian\nreading it may not be fluent in Python type-hint syntax. Handy when you don't remember\nan exact parameter name mid-notebook. It works even before `DREMIO_BASE_URL`/\n`DREMIO_TOKEN` are set, since it's answered before a session is built.", "metadata": {} }, { @@ -80,7 +80,7 @@ { "cell_type": "code", "id": "7e896ad6", - "source": "%%catalog use(\"bwd.reference\")\nset_tags(\".water_temperature\", [\"archived\"])\ncreate_folder(\".2027\")", + "source": "%%catalog use(\"bwd.reference\")\nset_tags(\"water_temperature\", [\"archived\"])\ncreate_folder(\"2027\")", "metadata": {}, "execution_count": null, "outputs": [] @@ -88,13 +88,13 @@ { "cell_type": "markdown", "id": "4b6abba9", - "source": "### `use`'s two differences from every other verb\n\n`create_folder` above still needed the leading `.` on `.2027` — every verb's own\n`path`/`source_path`/`target_path` does, so an absolute path can always be passed even\nwith a context already set. `use` alone accepts a bare path too, once a context\nexists — `use(\"2027\")` and `use(\".2027\")` mean the same thing there. `use(None)` clears\nthe context entirely, the same as `set_context(None)`.\n\n`use` also makes a live check: the resolved path must already exist in the catalog, or\nit raises instead of quietly pointing context somewhere later calls would fail against\nanyway. That's why the cell below re-enters `\"bwd.reference.2027\"`, created above,\nrather than a path nothing has created yet.", + "source": "### `use`'s live existence check\n\n`create_folder`/`set_tags` above dropped the leading `.` on `2027`/`water_temperature`\nentirely — once a context exists, every single-path verb's own `path` accepts a bare\nname this way (`\"2027\"` and `\".2027\"` mean the same thing). There are two exceptions:\n\n- `use` itself: its own `path` must always be a whole, absolute path — never resolved\n against whatever context already exists, and never a leading `.`/`../` fragment.\n- `data_copy`/`data_move`/`create_view`: they resolve `source_path`/`target_path`\n independently against that same starting context, so routinely pair a relative one\n with a genuinely unrelated absolute one — a bare path there still always means\n absolute, context or not.\n\nA bare path starting with `catalog` (this deployment's one real root source, e.g.\n`\"catalog.other_root.assessments\"`) is also always taken literally as absolute rather\nthan appended to the context — but that's not an exception to the dot-optional rule\nabove, just a further refinement of what \"bare\" means: without it, passing a full\n`catalog....` path alongside an already-set context would silently produce a nonsense\ndouble-nested path instead of raising or doing the obvious thing.\n\n`use(None)` clears the context entirely, the same as `set_context(None)`.\n\n`use` also makes a live check none of the others do: `path` must already exist in the\ncatalog, or it raises instead of quietly pointing context somewhere later calls would fail\nagainst anyway. That's why the cell below re-enters the full `\"bwd.reference.2027\"`,\ncreated above, rather than a path nothing has created yet.", "metadata": {} }, { "cell_type": "code", "id": "417250b0", - "source": "%catalog use(\"bwd.reference\") # back to a path that already exists\n%catalog use(\"2027\") # bare, no dot — same as use(\".2027\"); already created above\n%catalog get_context() # -> 'bwd.reference.2027'", + "source": "%catalog use(\"bwd.reference.2027\") # use() always takes the whole, absolute path\n%catalog get_context() # -> 'bwd.reference.2027'", "metadata": {}, "execution_count": null, "outputs": [] @@ -102,7 +102,7 @@ { "cell_type": "markdown", "id": "9be5a25f", - "source": "### Going up: `../`\n\nOne or more leading `../` (or a bare `..`) walks up that many levels of the context\nfirst, then resolves whatever's left against the result — and unlike the dot-optional\nshortcut above, this works for *every* relative path, not just `use`'s.", + "source": "### Going up: `../`\n\nOne or more leading `../` (or a bare `..`) walks up that many levels of the context\nfirst, then resolves whatever's left against the result — this works for every relative\npath except `use`'s own (always a whole, absolute path — see above), including\n`data_copy`/`data_move`/`create_view`'s own two paths (the one place among the rest a\nbare path with no dot at all still means \"take it literally as absolute\").", "metadata": {} }, { @@ -116,7 +116,7 @@ { "cell_type": "markdown", "id": "6cbe67f0", - "source": "## Read-only queries: `get_wiki`, `get_tags`, `list`, `schema`\n\nThese answer immediately too, like `get_context()` above — nothing to commit or undo.\nAll four raise `CatalogOperationError` if `path` doesn't exist; `schema` also requires a\ntable or view, not a folder.", + "source": "## Read-only queries: `get_wiki`, `get_tags`, `list`, `schema`\n\nThese answer immediately too, like `get_context()` above — nothing to commit or undo.\nAll four raise `CatalogOperationError` if `path` doesn't exist; `schema` also requires a\ntable or view, not a folder. `list`'s `path` may also be omitted (or `\"\"`) to list the\ncurrent context itself — raises `CatalogSessionError` if none is set yet.", "metadata": {} }, { @@ -138,7 +138,7 @@ { "cell_type": "code", "id": "91156d7b", - "source": "%catalog list(\"bwd.reference\") # every table/view under bwd.reference\n%catalog schema(\"bwd.reference.water_temperature\") # column types + row count, no rows fetched", + "source": "%catalog list(\"bwd.reference\") # every table/view under bwd.reference\n%catalog list() # same thing — path omitted, context is 'bwd.reference'\n%catalog schema(\"bwd.reference.water_temperature\") # column types + row count, no rows fetched", "metadata": {}, "execution_count": null, "outputs": [] diff --git a/src/eea_datalakehouse/catalog/session.py b/src/eea_datalakehouse/catalog/session.py index 6a15da4..32f8202 100644 --- a/src/eea_datalakehouse/catalog/session.py +++ b/src/eea_datalakehouse/catalog/session.py @@ -103,6 +103,13 @@ def _read_wiki_or_none(catalog: Catalog, path: str, *, idempotency_key: str) -> return None +_ROOT_SOURCE = "catalog" +# This deployment's one real top-level source/space (see e.g. the "catalog." prefix +# on every real path in debugger/test_catalog_magic.ipynb's TEST_ROOT) — a bare path +# starting with it is therefore unambiguously absolute, context or not; see +# `_resolve_path`. + + def _drop_entry(catalog: Catalog, path: str, *, idempotency_key: str) -> None: """Drop whatever entity (table or view) now sits at `path`. @@ -155,10 +162,8 @@ def set_context(self, path: str | None) -> CatalogSession: """Set the "current" catalog path — a later relative path (a leading `.`, e.g. `.water_temperature`) resolves against this. `None` clears it. `path` here is always taken literally, even if it itself starts with - `.` — see `use` for a version that resolves a leading `.` against - whatever context already exists, which is what a data custodian - deliberately narrowing the context (e.g. `%%catalog use(".2027")`) - actually wants. + `.` — same as `use`, which differs only in also making a live + existence check (see `use`'s own docstring). Not something a data custodian should normally call directly: ordinary use already keeps this up to date on its own (see @@ -174,20 +179,14 @@ def set_context(self, path: str | None) -> CatalogSession: def use(self, path: str | None) -> CatalogSession: """Set the current path for every call after this one — same as - `set_context`, but `path` may also be relative rather than only a - whole path, and `None` clears it exactly like `set_context(None)`. - A relative `path` may lead with a `.` (`".2027"`) or not - (`"2027"`) — both resolve against whatever context already exists - the same way; the dot is optional sugar here, unlike everywhere - else in this class (every other verb's `path`/`source_path`/ - `target_path` still requires it to mean relative, so an absolute - path can always be passed even with a context already set — see - `_resolve_path`). Once a context exists, `use` therefore has no way - to jump straight to an unrelated absolute path without a leading - dot; clear it first (`use(None)`) if that's what's needed. `path` - may also lead with one or more `../` (or be a bare `..`) to walk - up that many levels of the context first — `use("../stations")` - moves to a sibling of the context, `use("..")` to its parent. + `set_context`, plus a live existence check (below); `None` clears + it exactly like `set_context(None)`. Unlike every other verb's + `path`/`source_path`/`target_path`, `path` here is always taken + literally as a whole, absolute path — never resolved against + whatever context already exists, and never accepts a leading `.` + or `../` fragment (pass `get_context()`'s own return value, or + build the full path yourself, to move somewhere relative to where + you already are). This is the one custodian-facing way to set context deliberately (see `%%catalog use(path)` — the cell magic sets it once at the @@ -197,37 +196,31 @@ def use(self, path: str | None) -> CatalogSession: queued step hasn't already touched. Unlike everything else in this class, this makes a live call - against Dremio: the resolved path must already exist as some - catalog entity (folder, table, or view) — raises - `CatalogSessionError` otherwise, so context never silently points - somewhere real work would fail against later. Always stores the - fully resolved path — never a `.`/`..`-prefixed fragment — so - `get_context()` reads back exactly what `use` just checked. + against Dremio: `path` must already exist as some catalog entity + (folder, table, or view) — raises `CatalogSessionError` otherwise, + so context never silently points somewhere real work would fail + against later. """ if path is None: self._context = None return self - if self._context is not None and not path.startswith("."): - path = f".{path}" - resolved = self._resolve_path(path) try: - found = self._catalog._catalog_rest.exists(resolved) # noqa: SLF001 — see module docstring + found = self._catalog._catalog_rest.exists(path) # noqa: SLF001 — see module docstring except (CatalogOperationError, EngineStartingError) as exc: - raise CatalogSessionError( - f"could not check whether {resolved!r} exists: {exc}" - ) from exc + raise CatalogSessionError(f"could not check whether {path!r} exists: {exc}") from exc if not found: - raise CatalogSessionError(f"{resolved!r} does not exist in the catalog") - self._context = resolved + raise CatalogSessionError(f"{path!r} does not exist in the catalog") + self._context = path return self def get_context(self) -> str | None: """The current path, always the full resolved path — never a - `.`/`..`-prefixed fragment, even right after a relative `use` — - see `use`/`set_context`. `None` if nothing has been set yet.""" + `.`/`..`-prefixed fragment, even though most verbs' own relative + paths can be — see `set_context`/`_resolve`. `None` if nothing has + been set yet.""" return self._context - def _resolve_path(self, path: str) -> str: + def _resolve_path(self, path: str, *, dot_required: bool = False) -> str: """Just the relative -> absolute resolution — does NOT update the context; see `_resolve` for the verbs where the touched path also becomes the new context. Always returns a full, already-resolved @@ -236,16 +229,40 @@ def _resolve_path(self, path: str) -> str: (see `get_context`). A leading `.` resolves against the current context - (`".water_temperature"` -> `f"{context}.water_temperature"`). One - or more leading `../` segments (or a bare `..`) instead walk up - that many levels of the context *first* — `"../stations"` is a + (`".water_temperature"` -> `f"{context}.water_temperature"`); once a + context exists, a bare path with no leading `.` at all resolves the + same way too (`"water_temperature"` -> `f"{context}.water_temperature"`, + same as `".water_temperature"`) — the dot is optional sugar there, + not a marker that distinguishes relative from absolute. A bare path + starting with `_ROOT_SOURCE` (e.g. `"catalog.other_root.table"`) is + the one unambiguous exception: real absolute paths in this + deployment always start there, so it's taken literally as absolute + even with a context already set, rather than getting appended to + it — no need to clear context first just to pass one alongside a + relative path. `dot_required=True` goes further: a bare path is + then *always* taken literally as absolute, context or not, + regardless of whether it starts with `_ROOT_SOURCE`. + `data_copy`/`data_move`/`create_view` pass `dot_required=True` for + both of their paths — they resolve `source_path`/`target_path` + independently against the *same* starting context (see `_resolve`), + and a `_ROOT_SOURCE` check alone can't tell a deliberately relative + bare path (which they still want appended) apart from one that + merely doesn't happen to start with `_ROOT_SOURCE` but was meant + literally anyway. Everywhere else, a bare path is only ever taken + literally as absolute when no context has been set yet, or it + starts with `_ROOT_SOURCE`. One or more leading `../` segments (or + a bare `..`) instead walk up that many levels of the context + *first*, regardless of `dot_required` — `"../stations"` is a sibling of the context, `"../../stations"` a level further up, and so on; `".."` alone (no name after it) resolves to the context's parent itself.""" if path == ".." or path.startswith("../"): return self._resolve_parent_path(path) if not path.startswith("."): - return path + is_root_absolute = path == _ROOT_SOURCE or path.startswith(f"{_ROOT_SOURCE}.") + if dot_required or self._context is None or is_root_absolute: + return path + path = f".{path}" if self._context is None: raise CatalogSessionError( f"{path!r} is relative (starts with '.') but no context is set yet — " @@ -307,11 +324,13 @@ def data_copy( `target_path` is gone the moment this step runs; there is nothing to restore, so this step cannot be undone (see `CatalogCommitError`). - Either path may be relative (a leading `.`) to the session's current - context — see `set_context`; `target_path` becomes the new context + Either path may be relative to the session's current context — see `set_context` — + but here (unlike every other verb) a leading `.` is required to mean relative; + a bare path is always taken literally as absolute, context or not (see + `_resolve_path`'s `dot_required`). `target_path` becomes the new context afterwards.""" - source_path = self._resolve_path(source_path) - target_path = self._resolve_path(target_path) + source_path = self._resolve_path(source_path, dot_required=True) + target_path = self._resolve_path(target_path, dot_required=True) self._context = operations._parent_path(target_path) or self._context # noqa: SLF001 def run(catalog: Catalog, key: str) -> _Undo | None: @@ -347,11 +366,13 @@ def data_move( `target_path` to `source_path`. Same `overwrite=True` limitation as `data_copy`. - Either path may be relative (a leading `.`) to the session's current - context — see `set_context`; `target_path` becomes the new context + Either path may be relative to the session's current context — see `set_context` — + but here (unlike every other verb) a leading `.` is required to mean relative; + a bare path is always taken literally as absolute, context or not (see + `_resolve_path`'s `dot_required`). `target_path` becomes the new context afterwards.""" - source_path = self._resolve_path(source_path) - target_path = self._resolve_path(target_path) + source_path = self._resolve_path(source_path, dot_required=True) + target_path = self._resolve_path(target_path, dot_required=True) self._context = operations._parent_path(target_path) or self._context # noqa: SLF001 def run(catalog: Catalog, key: str) -> _Undo | None: @@ -396,11 +417,13 @@ def create_view( the moment this step runs; there is nothing to restore, so this step cannot be undone (see `CatalogCommitError`). - Either path may be relative (a leading `.`) to the session's current - context — see `set_context`; `target_path` becomes the new context + Either path may be relative to the session's current context — see `set_context` — + but here (unlike every other verb) a leading `.` is required to mean relative; + a bare path is always taken literally as absolute, context or not (see + `_resolve_path`'s `dot_required`). `target_path` becomes the new context afterwards.""" - source_path = self._resolve_path(source_path) - target_path = self._resolve_path(target_path) + source_path = self._resolve_path(source_path, dot_required=True) + target_path = self._resolve_path(target_path, dot_required=True) self._context = operations._parent_path(target_path) or self._context # noqa: SLF001 def run(catalog: Catalog, key: str) -> _Undo | None: @@ -426,7 +449,7 @@ def set_tags(self, path: str, tags: list[str]) -> CatalogSession: """Queue `settagsto` (replaces the tag set). Undo restores whatever tags were on `path` immediately before this step ran. - `path` may be relative (a leading `.`) to the session's current + `path` may be relative — with or without a leading `.` — to the session's current context — see `set_context`; it becomes the new context afterwards.""" path = self._resolve(path) @@ -446,7 +469,7 @@ def delete_tags(self, path: str, tags: list[str]) -> CatalogSession: """Queue `deletetags`. Undo restores the full tag set `path` had immediately before this step ran. - `path` may be relative (a leading `.`) to the session's current + `path` may be relative — with or without a leading `.` — to the session's current context — see `set_context`; it becomes the new context afterwards.""" path = self._resolve(path) @@ -468,7 +491,7 @@ def set_wiki( """Queue `setwikito`. Undo restores the previous wiki text verbatim, or deletes it if `path` had none. - `path` may be relative (a leading `.`) to the session's current + `path` may be relative — with or without a leading `.` — to the session's current context — see `set_context`; it becomes the new context afterwards.""" path = self._resolve(path) @@ -492,7 +515,7 @@ def delete_wiki(self, path: str) -> CatalogSession: """Queue `deletewiki`. Undo restores the previous wiki text, if there was one — a no-op if `path` had no wiki to begin with. - `path` may be relative (a leading `.`) to the session's current + `path` may be relative — with or without a leading `.` — to the session's current context — see `set_context`; it becomes the new context afterwards.""" path = self._resolve(path) @@ -515,7 +538,7 @@ def create_folder(self, path: str, *, create_parents: bool = False) -> CatalogSe actually created it; `createfolder` is idempotent, so a `path` that already existed is left alone on rollback too. - `path` may be relative (a leading `.`) to the session's current + `path` may be relative — with or without a leading `.` — to the session's current context — see `set_context` — and becomes the new context *itself* afterwards (a folder's contents, not its parent, is where a following short name most likely points).""" @@ -541,7 +564,7 @@ def delete_folder(self, path: str, *, cascade: bool = False) -> CatalogSession: contents cannot be recreated, so this always leaves the commit unable to claim a clean rollback if a later step fails. - `path` may be relative (a leading `.`) to the session's current + `path` may be relative — with or without a leading `.` — to the session's current context — see `set_context`. Does not change the context itself — there's nothing meaningful to navigate into once it's deleted.""" path = self._resolve_path(path) @@ -561,7 +584,7 @@ def delete_view(self, path: str) -> CatalogSession: `CatalogOperationError` if `path` doesn't exist at all, so a typo fails clearly instead of silently doing nothing. - `path` may be relative (a leading `.`) to the session's current + `path` may be relative — with or without a leading `.` — to the session's current context — see `set_context`. Does not change the context itself — there's nothing meaningful to navigate into once it's deleted.""" path = self._resolve_path(path) @@ -583,7 +606,7 @@ def delete_table(self, path: str) -> CatalogSession: `CatalogOperationError` if `path` doesn't exist at all, so a typo fails clearly instead of silently doing nothing. - `path` may be relative (a leading `.`) to the session's current + `path` may be relative — with or without a leading `.` — to the session's current context — see `set_context`. Does not change the context itself — there's nothing meaningful to navigate into once it's deleted.""" path = self._resolve_path(path) @@ -605,7 +628,7 @@ def get_wiki(self, path: str) -> str: `CatalogOperationError` if `path` doesn't exist, or exists but has no wiki at all. - `path` may be relative (a leading `.`) to the session's current + `path` may be relative — with or without a leading `.` — to the session's current context — see `set_context`.""" path = self._resolve_path(path) return self._catalog.getwikifrom(path, idempotency_key=self._key()) @@ -617,12 +640,12 @@ def get_tags(self, path: str) -> list[str]: isn't a table/view (folders have Dremio's own wiki Meta Data section for this instead — see `Catalog.setmeta2wiki`). - `path` may be relative (a leading `.`) to the session's current + `path` may be relative — with or without a leading `.` — to the session's current context — see `set_context`.""" path = self._resolve_path(path) return self._catalog.gettagsfrom(path, idempotency_key=self._key()) - def list(self, path: str) -> list[str]: + def list(self, path: str = "") -> list[str]: """Full paths of every table and view under `path`, at any depth (see `gettablesfrom`). Answered immediately — not queued, since there's nothing to commit or undo. Raises `CatalogOperationError` @@ -630,9 +653,19 @@ def list(self, path: str) -> list[str]: (an empty result and "nothing there" look the same to it), so this checks first rather than returning `[]` for a typo'd path. - `path` may be relative (a leading `.`) to the session's current - context — see `set_context`.""" - path = self._resolve_path(path) + `path` may be a whole, absolute path, or relative — with or + without a leading `.` — to the session's current context (see + `set_context`). Omitted (or `""`), it lists the content of the + current context itself — raises `CatalogSessionError` if no + context is set yet.""" + if not path: + if self._context is None: + raise CatalogSessionError( + "path was omitted but no context is set yet — pass a path, or call use() first" + ) + path = self._context + else: + path = self._resolve_path(path) if not self._catalog._catalog_rest.exists(path): # noqa: SLF001 — see module docstring raise CatalogOperationError(f"{path!r} does not exist") return self._catalog.gettablesfrom(path, idempotency_key=self._key()) @@ -645,7 +678,7 @@ def schema(self, path: str) -> TableInfo: otherwise (a folder has no schema of its own), or if `path` doesn't exist at all. - `path` may be relative (a leading `.`) to the session's current + `path` may be relative — with or without a leading `.` — to the session's current context — see `set_context`.""" path = self._resolve_path(path) operations._require_table_or_view( # noqa: SLF001 — see module docstring diff --git a/src/eea_datalakehouse/notebook/magics.py b/src/eea_datalakehouse/notebook/magics.py index 5f24015..72e4244 100644 --- a/src/eea_datalakehouse/notebook/magics.py +++ b/src/eea_datalakehouse/notebook/magics.py @@ -48,18 +48,21 @@ construction already use — this module never asks for or stores a token itself. -`CatalogSession`'s "current path" context (a leading `.` on a path resolves -against it — see that class' `set_context`/`_resolve`) is already kept -up to date automatically just from ordinary `%catalog` use, so no data -custodian ever needs to set it themselves for that. `use(path)` sets it -deliberately instead — unlike `set_context`, `path` itself may be relative -too, resolved against whatever context already exists. Most often reached -through `%%catalog`, the cell-magic form: it runs `use(...)` on its magic -line, then every other line of the cell in order, so the whole cell shares -one context without repeating a path on each line:: +`CatalogSession`'s "current path" context (a path resolves against it once +it's set, with or without a leading `.` — see that class' `set_context`/ +`_resolve_path`) is already kept up to date automatically just from +ordinary `%catalog` use, so no data custodian ever needs to set it +themselves for that. `use(path)` sets it deliberately instead — unlike +every other verb's own `path`/`source_path`/`target_path`, `use`'s `path` +is always a whole, absolute path (never resolved against whatever context +already exists), and it makes a live check that it actually exists in the +catalog first. Most often reached through `%%catalog`, the cell-magic +form: it runs `use(...)` on its magic line, then every other line of the +cell in order, so the whole cell shares one context without repeating a +path on each line:: %%catalog use("bwd.reference") - tag(".water_temperature", ["reviewed"]) + set_tags(".water_temperature", ["reviewed"]) create_folder(".2027") This module also registers a Jupyter Comm target (see @@ -127,11 +130,10 @@ _CATALOG_HELP = [ ( "use", - "Set the current path for every call after this one; path may be a whole path, " - "or relative to the current context (with or without a leading '.') once one " - "exists — '../name' or a bare '..' walks up one level first, chainable " - "('../../name'). None clears it. Raises if the resolved path doesn't exist in " - "the catalog. See %%catalog to set it once at the top of a cell.", + "Set the current path for every call after this one; path must be a whole, " + "absolute path — never relative to the current context, unlike every other " + "verb's path/source_path/target_path. None clears it. Raises if path doesn't " + "exist in the catalog. See %%catalog to set it once at the top of a cell.", ), ( "get_context", @@ -154,7 +156,9 @@ ( "list", "Show every table/view under path, at any depth, as full dot-separated paths. " - "Raises CatalogOperationError if path doesn't exist.", + "path may be omitted to list the current context itself — raises " + "CatalogSessionError if none is set. Raises CatalogOperationError if path " + "doesn't exist.", ), ( "schema", @@ -270,9 +274,12 @@ def _plain_params(func: Any) -> str: # since it applies across every path/source_path/target_path. %ingest help # has no equivalent note. _CATALOG_HELP_NOTE = ( - "A path/source_path/target_path starting with '.' resolves against the current " - "context (see use); one or more leading '../' (or a bare '..') walks up that many " - "levels first. Ordinary use already keeps context current on its own." + "Every path/source_path/target_path (except use's own) resolves against the current " + "context once one is set — with or without a leading '.' — unless it already starts " + "with 'catalog' (this deployment's one real root source), which is always taken " + "literally as absolute instead of being appended to the context. One or more leading " + "'../' (or a bare '..') walks up that many levels first. Ordinary use already keeps " + "context current on its own." ) diff --git a/tests/catalog/test_session.py b/tests/catalog/test_session.py index 91c0649..ac0ff7d 100644 --- a/tests/catalog/test_session.py +++ b/tests/catalog/test_session.py @@ -47,7 +47,9 @@ def test_commit_runs_queued_steps_in_order_and_clears_the_queue() -> None: rest = FakeCatalogRest(existing={"a", "bwd", "bwd.table1"}) session = CatalogSession(_catalog(rest)) - session.create_folder("bwd.newfolder").set_tags("bwd.table1", ["reviewed"]) + # set_context(None) keeps 'bwd.table1' below literal — create_folder's own + # context-tracking would otherwise treat a following bare path as relative. + session.create_folder("bwd.newfolder").set_context(None).set_tags("bwd.table1", ["reviewed"]) report = session.commit() assert report.succeeded == [ @@ -99,6 +101,7 @@ def test_data_move_rollback_moves_the_table_back() -> None: session = CatalogSession(_catalog(rest, executor)) session.data_move("bwd.table1", "bwd.table2") # reversible + session.set_context(None) # keep 'bwd.missing' below literal, not relative to data_move session.set_tags("bwd.missing", ["x"]) # fails: path does not exist with pytest.raises(CatalogCommitError) as exc_info: @@ -123,6 +126,7 @@ def test_create_view_rollback_drops_the_view() -> None: session = CatalogSession(_catalog(rest, executor)) session.create_view("bwd.table1", "bwd.view1") # reversible — source is untouched either way + session.set_context(None) # keep 'bwd.missing' below literal, not relative to create_view session.set_tags("bwd.missing", ["x"]) # fails: path does not exist with pytest.raises(CatalogCommitError) as exc_info: diff --git a/tests/catalog/test_session_context.py b/tests/catalog/test_session_context.py index 146ed5a..e272bf9 100644 --- a/tests/catalog/test_session_context.py +++ b/tests/catalog/test_session_context.py @@ -57,25 +57,47 @@ def test_touching_a_leaf_infers_context_for_a_later_relative_call() -> None: assert rest.get_tags("bwd.reference.stations") == ["reviewed"] -def test_set_context_seeds_it_explicitly_before_any_path_is_used() -> None: - rest = FakeCatalogRest(existing={"a", "bwd", "bwd.reference.water_temperature"}) +def test_bare_path_starting_with_root_source_stays_absolute_even_with_context_set() -> None: + # "catalog" is this deployment's one real top-level source (_ROOT_SOURCE) — a + # bare path starting with it is unambiguous, so it's never appended to an + # existing context the way an ordinary bare name ("stations" above) is. + rest = FakeCatalogRest( + existing={ + "a", + "bwd", + "bwd.reference", + "catalog", + "catalog.other_root", + "catalog.other_root.table1", + } + ) session = CatalogSession(_catalog(rest)) session.set_context("bwd.reference") - session.set_tags(".water_temperature", ["reviewed"]) + session.set_tags("catalog.other_root.table1", ["reviewed"]) report = session.commit() - assert report.succeeded == ["set tags ['reviewed'] on 'bwd.reference.water_temperature'"] + assert report.succeeded == ["set tags ['reviewed'] on 'catalog.other_root.table1'"] -def test_use_with_a_whole_path_sets_context_like_set_context() -> None: - rest = FakeCatalogRest( - existing={"a", "bwd", "bwd.reference", "bwd.reference.water_temperature"} - ) +def test_bare_path_equal_to_root_source_stays_absolute_even_with_context_set() -> None: + rest = FakeCatalogRest(existing={"a", "bwd", "bwd.reference", "catalog"}) session = CatalogSession(_catalog(rest)) - session.use("bwd.reference") + session.set_context("bwd.reference") + session.set_tags("catalog", ["reviewed"]) + + report = session.commit() + + assert report.succeeded == ["set tags ['reviewed'] on 'catalog'"] + + +def test_set_context_seeds_it_explicitly_before_any_path_is_used() -> None: + rest = FakeCatalogRest(existing={"a", "bwd", "bwd.reference.water_temperature"}) + session = CatalogSession(_catalog(rest)) + + session.set_context("bwd.reference") session.set_tags(".water_temperature", ["reviewed"]) report = session.commit() @@ -83,69 +105,48 @@ def test_use_with_a_whole_path_sets_context_like_set_context() -> None: assert report.succeeded == ["set tags ['reviewed'] on 'bwd.reference.water_temperature'"] -def test_use_with_a_relative_path_resolves_against_the_existing_context() -> None: +def test_use_with_a_whole_path_sets_context_like_set_context() -> None: rest = FakeCatalogRest( - existing={ - "a", - "bwd", - "bwd.reference", - "bwd.reference.2027", - "bwd.reference.2027.water_temperature", - } + existing={"a", "bwd", "bwd.reference", "bwd.reference.water_temperature"} ) session = CatalogSession(_catalog(rest)) session.use("bwd.reference") - session.use(".2027") # narrows bwd.reference -> bwd.reference.2027 session.set_tags(".water_temperature", ["reviewed"]) report = session.commit() - assert report.succeeded == ["set tags ['reviewed'] on 'bwd.reference.2027.water_temperature'"] - - -def test_use_with_a_relative_path_and_no_context_yet_raises() -> None: - rest = FakeCatalogRest(existing={"a", "bwd"}) - session = CatalogSession(_catalog(rest)) - - with pytest.raises(CatalogSessionError, match="no context is set"): - session.use(".reference") + assert report.succeeded == ["set tags ['reviewed'] on 'bwd.reference.water_temperature'"] -def test_use_bare_path_resolves_relative_once_context_exists() -> None: - # No leading '.' needed once inside a context — "2027" and ".2027" mean - # the same thing here (unlike every other verb's path arguments, which - # always require the dot to mean relative). +def test_use_never_resolves_a_leading_dot_relatively() -> None: + # use() takes path literally as a whole, absolute path — unlike every + # other verb, a leading '.' means nothing special here; it's just part + # of a literal string that (in this case) doesn't exist in the catalog. rest = FakeCatalogRest( - existing={ - "a", - "bwd", - "bwd.reference", - "bwd.reference.2027", - "bwd.reference.2027.water_temperature", - } + existing={"a", "bwd", "bwd.reference", "bwd.reference.2027"}, ) session = CatalogSession(_catalog(rest)) session.use("bwd.reference") - session.use("2027") # bare, no dot — still narrows to bwd.reference.2027 - session.set_tags(".water_temperature", ["reviewed"]) - - report = session.commit() + with pytest.raises(CatalogSessionError, match="does not exist in the catalog"): + session.use(".2027") # NOT resolved against 'bwd.reference' — taken literally - assert report.succeeded == ["set tags ['reviewed'] on 'bwd.reference.2027.water_temperature'"] + assert session.get_context() == "bwd.reference" # unchanged after the failed check -def test_use_bare_path_with_no_context_yet_is_absolute() -> None: - # The very first use() has nothing to be relative to, so a bare path is - # unambiguous: it must be the whole path. - rest = FakeCatalogRest(existing={"a", "bwd"}) +def test_use_always_takes_a_bare_path_literally() -> None: + # Whether or not a context already exists, a bare path passed to use() + # is always the whole, absolute path — never relative to it. + rest = FakeCatalogRest(existing={"a", "bwd", "bwd.reference", "bwd.other"}) session = CatalogSession(_catalog(rest)) session.use("bwd") - assert session.get_context() == "bwd" + session.use("bwd.other") # NOT relative to 'bwd' — still a literal, whole path + assert session.get_context() == "bwd.other" + def test_use_none_clears_context_like_set_context() -> None: rest = FakeCatalogRest(existing={"a", "bwd", "bwd.reference", "bwd.other"}) @@ -155,8 +156,7 @@ def test_use_none_clears_context_like_set_context() -> None: session.use(None) assert session.get_context() is None - # Cleared, so a bare path is absolute again rather than relative to - # whatever used to be there. + session.use("bwd.other") assert session.get_context() == "bwd.other" @@ -212,6 +212,31 @@ def test_data_copy_resolves_both_paths_against_the_same_starting_context() -> No ] +def test_data_copy_bare_target_path_stays_absolute_even_with_context_set() -> None: + # data_copy/data_move/create_view are the deliberate exception to the + # dot-optional rule: they resolve source_path/target_path independently + # against the same starting context, so a bare path must stay absolute + # even with a context set — otherwise pairing a relative source with a + # genuinely unrelated absolute target would be inexpressible. + rest = FakeCatalogRest(existing={"a", "bwd", "bwd.draft.raw_2026", "other.reference"}) + executor = FakeExecutor( + rows_sequence=[ + [{"TABLE_NAME": "raw_2026"}], # source exists + [], # target does not exist yet + ] + ) + session = CatalogSession(_catalog(rest, executor)) + + session.set_context("bwd.draft") + session.data_copy(".raw_2026", "other.reference.water_temperature") # bare, unrelated root + + report = session.commit() + + assert report.succeeded == [ + "data_copy 'bwd.draft.raw_2026' -> 'other.reference.water_temperature'", + ] + + def test_data_move_resolves_both_paths_against_the_same_starting_context() -> None: rest = FakeCatalogRest( existing={"a", "bwd", "bwd.draft.raw_2026", "bwd.reference", "bwd.reference.stations"} @@ -262,11 +287,14 @@ def test_create_view_resolves_both_paths_against_the_same_starting_context() -> def test_bare_dotdot_resolves_to_the_parent_of_the_context() -> None: + # use() no longer accepts '..' at all (see test_use_never_resolves_a_ + # leading_dot_relatively) — create_folder exercises the same + # _resolve_path/_resolve_parent_path machinery every other verb shares. rest = FakeCatalogRest(existing={"a", "bwd", "bwd.reference", "bwd.reference.2027"}) session = CatalogSession(_catalog(rest)) session.use("bwd.reference.2027") - session.use("..") + session.create_folder("..") assert session.get_context() == "bwd.reference" @@ -278,7 +306,7 @@ def test_dotdot_slash_name_resolves_to_a_sibling_of_the_context() -> None: session = CatalogSession(_catalog(rest)) session.use("bwd.reference.2027.stations") - session.use("../water_temp") # sibling of the current context + session.create_folder("../water_temp") # sibling of the current context assert session.get_context() == "bwd.reference.2027.water_temp" @@ -290,15 +318,12 @@ def test_chained_dotdot_walks_up_multiple_levels() -> None: session = CatalogSession(_catalog(rest)) session.use("bwd.reference.2027.stations") - session.use("../../../archive") # up three levels, then into "archive" + session.create_folder("../../../archive") # up three levels, then into "archive" assert session.get_context() == "bwd.archive" - # A bare absolute path is only absolute again once the context is - # cleared — see use()'s own docstring on this tradeoff. - session.use(None) - session.use("bwd.reference.2027.stations") - session.use("../..") # up two levels, no name after it + session.set_context("bwd.reference.2027.stations") # re-seed for the second half + session.create_folder("../..") # up two levels, no name after it assert session.get_context() == "bwd.reference" @@ -310,7 +335,7 @@ def test_dotdot_past_the_top_of_the_context_raises() -> None: session.use("bwd") with pytest.raises(CatalogSessionError, match="goes above the top"): - session.use("..") + session.create_folder("..") def test_dotdot_with_no_context_yet_raises() -> None: @@ -318,12 +343,12 @@ def test_dotdot_with_no_context_yet_raises() -> None: session = CatalogSession(_catalog(rest)) with pytest.raises(CatalogSessionError, match="no context is set"): - session.use("../reference") + session.create_folder("../reference") -def test_dotdot_works_for_verbs_other_than_use_too() -> None: - # ../ is a general path-resolution feature (_resolve_path), not - # something special-cased inside use() alone. +def test_dotdot_works_for_ordinary_verbs() -> None: + # ../ is a general path-resolution feature (_resolve_path), available + # to every verb except use(), which only takes whole, absolute paths. rest = FakeCatalogRest( existing={ "a", @@ -348,7 +373,7 @@ def test_get_context_after_dotdot_is_the_full_resolved_path() -> None: session = CatalogSession(_catalog(rest)) session.use("bwd.reference.2027") - session.use("..") + session.create_folder("..") context = session.get_context() assert context == "bwd.reference" @@ -369,7 +394,7 @@ def test_use_leaves_context_unchanged_after_a_failed_check() -> None: session.use("bwd.reference") with pytest.raises(CatalogSessionError, match="does not exist"): - session.use(".nonexistent") + session.use("bwd.nonexistent") assert session.get_context() == "bwd.reference" # still the last good value diff --git a/tests/catalog/test_session_queries.py b/tests/catalog/test_session_queries.py index f062898..37eeaad 100644 --- a/tests/catalog/test_session_queries.py +++ b/tests/catalog/test_session_queries.py @@ -18,7 +18,11 @@ from eea_datalakehouse.catalog.client import Catalog from eea_datalakehouse.catalog.errors import CatalogOperationError from eea_datalakehouse.catalog.operations import TableInfo -from eea_datalakehouse.catalog.session import CatalogCommitError, CatalogSession +from eea_datalakehouse.catalog.session import ( + CatalogCommitError, + CatalogSession, + CatalogSessionError, +) from .conftest import FakeCatalogRest, FakeExecutor @@ -123,6 +127,61 @@ def test_list_resolves_a_relative_path() -> None: assert executor.statements # the gettablesfrom query actually ran +def test_list_resolves_a_bare_relative_path_too() -> None: + # list() (and every other single-path verb) treats a bare path with no + # leading '.' as relative once a context exists — the dot is optional + # sugar there, not the marker for relative vs absolute (use() is the + # one exception — see test_session_context.py). + rest = FakeCatalogRest(existing={"a", "bwd", "bwd.reference"}) + executor = FakeExecutor(rows=[]) + session = CatalogSession(_catalog(rest, executor)) + session.use("bwd") + + session.list("reference") # bare, no dot — still resolves to "bwd.reference" + + assert executor.statements # the gettablesfrom query actually ran + + +def test_list_with_no_path_lists_the_context_itself() -> None: + rest = FakeCatalogRest(existing={"a", "bwd", "bwd.reference"}) + executor = FakeExecutor( + rows=[{"TABLE_SCHEMA": "bwd.reference", "TABLE_NAME": "water_temperature"}] + ) + session = CatalogSession(_catalog(rest, executor)) + session.use("bwd.reference") + + assert session.list() == ["bwd.reference.water_temperature"] + + +def test_list_with_empty_path_lists_the_context_itself() -> None: + rest = FakeCatalogRest(existing={"a", "bwd", "bwd.reference"}) + executor = FakeExecutor(rows=[]) + session = CatalogSession(_catalog(rest, executor)) + session.use("bwd.reference") + + session.list("") # explicit empty string — same as omitting path + + assert executor.statements # the gettablesfrom query actually ran + + +def test_list_with_no_path_and_no_context_raises() -> None: + rest = FakeCatalogRest(existing={"a", "bwd"}) + session = CatalogSession(_catalog(rest)) + + with pytest.raises(CatalogSessionError, match="no context is set"): + session.list() + + +def test_list_absolute_path_still_works_with_no_context() -> None: + rest = FakeCatalogRest(existing={"a", "bwd", "bwd.reference"}) + executor = FakeExecutor( + rows=[{"TABLE_SCHEMA": "bwd.reference", "TABLE_NAME": "water_temperature"}] + ) + session = CatalogSession(_catalog(rest, executor)) + + assert session.list("bwd.reference") == ["bwd.reference.water_temperature"] + + def test_list_raises_when_path_does_not_exist() -> None: # Unlike the raw gettablesfrom (which would just return []), list() # checks first rather than treating a typo as "nothing found". diff --git a/tests/notebook/test_magics.py b/tests/notebook/test_magics.py index ccb4fee..bb5490a 100644 --- a/tests/notebook/test_magics.py +++ b/tests/notebook/test_magics.py @@ -233,8 +233,12 @@ def test_catalog_help_lists_methods_without_needing_credentials( # General note about leading-'.'/'../' relative paths — printed once, # not per-row, since it applies across every path/source_path/target_path. assert "resolves against the current context" in out - assert "see use" in out + assert "except use's own" in out # use is the one path that's always literal/absolute assert "walks up that many" in out # ../ support, mentioned generally + # A path already starting with 'catalog' (the one real root source) is never + # appended to an existing context, even without a leading dot. + assert "starts with 'catalog'" in out + assert "taken literally as absolute" in out assert len(displayed) == 1 table_html = displayed[0].data @@ -268,10 +272,10 @@ def test_catalog_help_lists_methods_without_needing_credentials( # set_tags/delete_tags call out the specific error they can raise. assert "Tables/views only — raises CatalogOperationError otherwise." in table_html # use/get_context are listed; set_context is deliberately not (use covers it). - assert "with or without a leading" in table_html # use's dot-optional relative paths - assert "walks up one level" in table_html # use's ../ support + assert "must be a whole" in table_html # use takes path literally, never relative + assert "never relative to the current context" in table_html # use's live existence check (apostrophe in "doesn't" comes back escaped). - assert "Raises if the resolved path" in table_html + assert "Raises if path" in table_html assert "exist in the catalog" in table_html assert "get_context" in table_html assert "Show the current path" in table_html @@ -295,6 +299,11 @@ def test_catalog_help_lists_methods_without_needing_credentials( assert "deleteview" not in table_html and "deletetable" not in table_html # renamed assert ">list<" in table_html and ">schema<" in table_html assert "must be a table or view" in table_html # schema's type requirement + # list's path is now optional — defaults to listing the current context + # (the empty-string default's quotes come back html-escaped as '). + assert "path=''" in table_html + assert "path may be omitted to list the current context itself" in table_html + assert "raises CatalogSessionError if none is set" in table_html # list/delete_view/delete_table each spell out the existence error the # same way (apostrophe in "doesn't" comes back html-escaped, hence # stopping before it).