diff --git a/README.md b/README.md index e535b6a..c7c591d 100644 --- a/README.md +++ b/README.md @@ -189,7 +189,7 @@ examples. Install the extra this needs once: `pip install "EEADataLakehouse[note import eea_datalakehouse.notebook # registers %catalog/%ingest — no %load_ext needed %catalog data_copy("draft.raw_2026", "bwd.reference.water_temperature") -%catalog set_tags(".water_temperature", ["reviewed"]) +%catalog set_tags("bwd.reference.water_temperature", ["reviewed"]) %ingest ingest(folder="./bw_2026", target_catalog_path="bwd.reference", data_format="parquet", table_name="water_temperature") @@ -201,38 +201,56 @@ parameters, description — rather than a raw Python signature, since its audien custodian, not necessarily a developer. `%ingest help` does the same for `IngestSession`'s (fewer) methods — handy when you don't remember an exact parameter name mid-notebook. +A freshly created session already has a context — it starts at `"catalog"` (the catalog +root), not empty — so a relative path works even before the first `use()` call. +`use(None)`/`use("")` reset it back to `"catalog"` the same way; `set_context(None)` (the +lower-level primitive `use` wraps, not normally called directly) is the one way left to +clear it to no context at all. + `%%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. `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: +path on each line. `use` is the *only* way context ever changes — no other verb touches it as +a side effect of running, even one whose own `path` fully resolves to something that could +sensibly become the new context (`create_folder`, notably, used to leave its own path as the +new context; it no longer does — every verb only *reads* context, never writes it). `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)`/`use("")` skip that check and reset context to `"catalog"` instead. +Every other verb's own `path`/`source_path`/ +`target_path` — including `data_copy`/`data_move`/`create_view`'s two, each resolved +independently against that same context — 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. That's what makes pairing a relative `source_path` with a genuinely unrelated absolute +`target_path` in the same `data_copy`/`data_move`/`create_view` call unambiguous without +needing a dot on the absolute side: a real absolute path here always starts with `catalog`. +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; `"."`/`"./"` alone (no name after either) mean the context itself. `get_context()` +shows what the context currently is — always exactly what `use`/`set_context` last stored — +and `use(path)` prints the same thing itself (`context set to `), since there's nothing +to commit — `use` never queues anything, so `%catalog`'s usual auto-commit would otherwise +have nothing to show: ```python %%catalog use("bwd.reference") set_tags("water_temperature", ["reviewed"]) # bare, no dot — same as ".water_temperature" -create_folder(".2027") +create_folder(".2027") # creates bwd.reference.2027 for real, but leaves context untouched -%catalog use("bwd.reference.2027") # use() always takes the whole, absolute path +%catalog get_context() # -> 'bwd.reference' — still what use() set, not create_folder +%catalog use("bwd.reference.2027") # prints: context set to 'bwd.reference.2027' %catalog get_context() # -> 'bwd.reference.2027' +%catalog list(".") # '.' alone -> the context itself: same as list("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 +%catalog data_copy(".water_temperature", "catalog.other_root.archive.water_temperature_2027") +# ^ starts with 'catalog' — absolute, not appended either +%catalog data_copy(".water_temperature", "archive") +# ^ bare, no dot — relative too, appends to the context ``` `get_wiki(path)`, `get_tags(path)`, `list(path)` (every table/view under `path`, at any depth) diff --git a/debugger/test_catalog_magic.ipynb b/debugger/test_catalog_magic.ipynb index f028312..f1bc94f 100644 --- a/debugger/test_catalog_magic.ipynb +++ b/debugger/test_catalog_magic.ipynb @@ -23,9 +23,20 @@ }, { "cell_type": "code", - "execution_count": null, + "execution_count": 1, "metadata": {}, - "outputs": [], + "outputs": [ + { + "name": "stdout", + "output_type": "stream", + "text": [ + "env file /home/esparza/EEADatahub/EEALakeHouse.python/debugger/.env\n", + "DREMIO_BASE_URL https://dremio.eea.europa.eu:9047/\n", + "DREMIO_USERNAME esparza@discomap.eea.europa.eu\n", + "DREMIO_TOKEN set\n" + ] + } + ], "source": [ "import os\n", "from pathlib import Path\n", @@ -67,7 +78,7 @@ }, { "cell_type": "code", - "execution_count": 1, + "execution_count": 2, "id": "c5e8b5d6", "metadata": {}, "outputs": [], @@ -88,11 +99,118 @@ }, { "cell_type": "code", - "execution_count": null, + "execution_count": 3, "id": "c311fdb0", "metadata": {}, - "outputs": [], - "source": "%catalog help()" + "outputs": [ + { + "name": "stdout", + "output_type": "stream", + "text": [ + "%catalog methods — usage: %catalog data_copy(\"a.b\", \"c.d\") | %catalog set_tags(\"a.b\", [\"reviewed\"]) (each call runs immediately)\n", + "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. Only use() ever changes the context — no other call does, even one that fully resolved an absolute path.\n" + ] + }, + { + "data": { + "text/html": [ + "
CommandParametersDescription
usepathSet 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_contextShow the current path (None if nothing has been set yet).
data_copysource_path, target_path, overwrite=False, create_target_folder=FalseCopy source_path to target_path. overwrite=False (default) raises CatalogOperationError if target_path already exists; overwrite=True replaces it and is not undoable. create_target_folder=True creates every missing folder level of target_path — except the space/source itself, which must already exist (raises CatalogOperationError otherwise).
data_movesource_path, target_path, overwrite=False, create_target_folder=FalseMove source_path to target_path, always as a TABLE (use create_view for a view). overwrite=False (default) raises CatalogOperationError if target_path already exists; overwrite=True replaces it and is not undoable. create_target_folder=True creates every missing folder level of target_path — except the space/source itself, which must already exist (raises CatalogOperationError otherwise).
listpath=''Show every table/view under path, at any depth, as full dot-separated paths. path may be omitted to list the current context itself — raises CatalogSessionError if none is set. Raises CatalogOperationError if path doesn't exist.
schemapathShow path's column schema and row count (never fetches the actual rows). path must be a table or view — raises CatalogOperationError otherwise, or if path doesn't exist at all.
delete_tablepathDelete the table at path. Not undoable. Raises CatalogOperationError if path doesn't exist (unlike a bare DROP TABLE, this doesn't quietly no-op on a typo).
create_viewsource_path, target_path, overwrite=False, create_target_folder=FalseCreate a VIEW at target_path over source_path, leaving source_path untouched. overwrite=False (default) raises CatalogOperationError if target_path already exists; overwrite=True replaces it and is not undoable. create_target_folder=True creates every missing folder level of target_path — except the space/source itself, which must already exist (raises CatalogOperationError otherwise).
delete_viewpathDelete the view at path. Not undoable. Raises CatalogOperationError if path doesn't exist (unlike a bare DROP VIEW, this doesn't quietly no-op on a typo).
set_wikipath, text, tags=NoneSet a path's wiki text. tags: a list of {tag_name, tag_value, tag_title} dicts, e.g. [{"tag_name": "owner", "tag_value": "bw-team", "tag_title": "Owner"}]. Ignored if path is a table/view (they have set_tags/get_tags for that instead).
delete_wikipathDelete a path's wiki text.
get_wikipathShow a path's wiki text. Raises CatalogOperationError if the path doesn't exist, or exists but has no wiki at all.
set_tagspath, tagsReplace a path's tag set. tags: a list of strings, e.g. ["reviewed"]. Tables/views only — raises CatalogOperationError otherwise.
delete_tagspath, tagsRemove tags from a path's tag set. tags: a list of strings, e.g. ["reviewed"]. Tables/views only — raises CatalogOperationError otherwise.
get_tagspathShow the tags on a path — tables/views only. Raises CatalogOperationError if the path doesn't exist or isn't a table/view.
create_folderpath, create_parents=FalseCreate a folder. create_parents=True creates every missing level of the full path; create_parents=False (default) raises CatalogOperationError if the parent folder doesn't already exist.
delete_folderpath, cascade=FalseDelete a folder. Not undoable. cascade=True also deletes everything at path and below; cascade=False (default) raises CatalogOperationError if the folder isn't empty. A path that's already gone is not an error.
" + ], + "text/plain": [ + "" + ] + }, + "metadata": {}, + "output_type": "display_data" + } + ], + "source": [ + "%catalog help()" + ] + }, + { + "cell_type": "code", + "execution_count": 5, + "id": "4eb4c501", + "metadata": {}, + "outputs": [ + { + "data": { + "text/plain": [ + "'catalog.water_management_resources.bathing_water.bwd.draft'" + ] + }, + "execution_count": 5, + "metadata": {}, + "output_type": "execute_result" + } + ], + "source": [ + "%catalog use('catalog.water_management_resources.bathing_water.bwd.draft')" + ] + }, + { + "cell_type": "code", + "execution_count": 7, + "id": "f975f358", + "metadata": {}, + "outputs": [ + { + "data": { + "text/plain": [ + "'catalog.water_management_resources.bathing_water.bwd.draft'" + ] + }, + "execution_count": 7, + "metadata": {}, + "output_type": "execute_result" + } + ], + "source": [ + "%catalog get_context()" + ] + }, + { + "cell_type": "code", + "execution_count": 6, + "id": "5c4813fb", + "metadata": {}, + "outputs": [ + { + "data": { + "text/plain": [ + "CommitReport(succeeded=[\"delete folder 'catalog.water_management_resources.bathing_water.bwd.draft.aaaaa'\"])" + ] + }, + "execution_count": 6, + "metadata": {}, + "output_type": "execute_result" + } + ], + "source": [ + "%catalog delete_folder('aaaaa',cascade=True)" + ] + }, + { + "cell_type": "code", + "execution_count": 16, + "id": "338b04ca", + "metadata": {}, + "outputs": [ + { + "data": { + "text/plain": [ + "CommitReport(succeeded=[\"data_copy 'catalog.water_management_resources.bathing_water.bwd.draft.bw_assessment.assessments' -> 'catalog.water_management_resources.bathing_water.bwd.draft.aaaaa.tttt.jkjkjkk'\"])" + ] + }, + "execution_count": 16, + "metadata": {}, + "output_type": "execute_result" + } + ], + "source": [ + "%catalog data_copy('bw_assessment.assessments', 'aaaaa.tttt.jkjkjkk',create_target_folder= True)" + ] }, { "cell_type": "markdown", @@ -107,6 +225,14 @@ "it at the end." ] }, + { + "cell_type": "code", + "execution_count": null, + "id": "844929cf", + "metadata": {}, + "outputs": [], + "source": [] + }, { "cell_type": "code", "execution_count": null, @@ -152,10 +278,22 @@ }, { "cell_type": "code", - "execution_count": null, + "execution_count": 4, "id": "0fca8ccf", "metadata": {}, - "outputs": [], + "outputs": [ + { + "ename": "NameError", + "evalue": "name 'os' is not defined", + "output_type": "error", + "traceback": [ + "\u001b[31m---------------------------------------------------------------------------\u001b[39m", + "\u001b[31mNameError\u001b[39m Traceback (most recent call last)", + "\u001b[36mCell\u001b[39m\u001b[36m \u001b[39m\u001b[32mIn[4]\u001b[39m\u001b[32m, line 4\u001b[39m\n\u001b[32m 1\u001b[39m \u001b[38;5;28;01mfrom\u001b[39;00m eea_datalakehouse.catalog \u001b[38;5;28;01mimport\u001b[39;00m Catalog\n\u001b[32m 2\u001b[39m \n\u001b[32m 3\u001b[39m verify_catalog = Catalog(\n\u001b[32m----> \u001b[39m\u001b[32m4\u001b[39m os.environ[\u001b[33m\"DREMIO_BASE_URL\"\u001b[39m], os.environ[\u001b[33m\"DREMIO_TOKEN\"\u001b[39m],\n\u001b[32m 5\u001b[39m username=os.environ.get(\u001b[33m\"DREMIO_USERNAME\"\u001b[39m),\n\u001b[32m 6\u001b[39m )\n\u001b[32m 7\u001b[39m \u001b[38;5;66;03m# `_catalog_rest.exists` is the same private check `CatalogSession.create_folder`'s own\u001b[39;00m\n", + "\u001b[31mNameError\u001b[39m: name 'os' is not defined" + ] + } + ], "source": [ "from eea_datalakehouse.catalog import Catalog\n", "\n", @@ -206,7 +344,12 @@ "cell_type": "markdown", "id": "24a3cbc3", "metadata": {}, - "source": "## 3. Wiki text — also immediate\n\n`set_wiki` works on any catalog entity, folders included. Commits itself the moment its\ncell runs, exactly like `create_folder` above." + "source": [ + "## 3. Wiki text — also immediate\n", + "\n", + "`set_wiki` works on any catalog entity, folders included. Commits itself the moment its\n", + "cell runs, exactly like `create_folder` above." + ] }, { "cell_type": "code", @@ -218,12 +361,6 @@ "%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, @@ -235,33 +372,28 @@ ] }, { - "cell_type": "code", - "id": "70ce527b", - "source": "%catalog list(TEST_FOLDER) # -> [] (only folders live here — not an error, unlike a typo'd path)", - "metadata": {}, - "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)", + "cell_type": "markdown", + "id": "59854882", "metadata": {}, - "execution_count": null, - "outputs": [] + "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)." + ] }, { "cell_type": "markdown", "metadata": {}, - "source": "## 5. What a failed call looks like\n\n`TEST_FOLDER` still has `.nested` inside it, so deleting it without `cascade=True` should\nfail — `%catalog` prints a short message instead of a traceback (see the design doc's\n\"Exceptions translated at the boundary\")." + "source": [ + "## 5. What a failed call looks like\n", + "\n", + "`TEST_FOLDER` still has `.nested` inside it, so deleting it without `cascade=True` should\n", + "fail — `%catalog` prints a short message instead of a traceback (see the design doc's\n", + "\"Exceptions translated at the boundary\")." + ] }, { "cell_type": "code", @@ -282,6 +414,14 @@ "other calls, has already happened by the time the cell below finishes." ] }, + { + "cell_type": "code", + "execution_count": null, + "id": "9a1afe5c", + "metadata": {}, + "outputs": [], + "source": [] + }, { "cell_type": "code", "execution_count": null, @@ -299,6 +439,45 @@ "source": [ "print(\"folder exists after cleanup:\", verify_catalog._catalog_rest.exists(TEST_FOLDER)) # noqa" ] + }, + { + "cell_type": "code", + "execution_count": 1, + "id": "6d7cde28", + "metadata": {}, + "outputs": [ + { + "name": "stderr", + "output_type": "stream", + "text": [ + "UsageError: Line magic function `%catalog` not found.\n" + ] + } + ], + "source": [ + "%catalog use('catalog.water_management_resources.bathing_water')" + ] + }, + { + "cell_type": "code", + "execution_count": 15, + "id": "a653e15f", + "metadata": {}, + "outputs": [ + { + "data": { + "text/plain": [ + "TableInfo(schema={'country_code': 'CHARACTER VARYING', 'bathing_water_identifier': 'CHARACTER VARYING', 'group_identifier': 'CHARACTER VARYING', 'bathing_water_name': 'CHARACTER VARYING', 'bathing_water_type': 'CHARACTER VARYING', 'geographical_constraint': 'BOOLEAN', 'lon': 'DOUBLE', 'lat': 'DOUBLE', 'bw_profile': 'CHARACTER VARYING', 'season': 'INTEGER', 'quality_raw': 'CHARACTER VARYING', 'quality_code': 'INTEGER', 'quality_label': 'CHARACTER VARYING', 'quality_scheme': 'CHARACTER VARYING', 'monitoring_calendar_code': 'INTEGER', 'monitoring_calendar_label': 'CHARACTER VARYING', 'management_code': 'INTEGER', 'management_label': 'CHARACTER VARYING', 'release': 'CHARACTER VARYING'}, row_count=694918)" + ] + }, + "execution_count": 15, + "metadata": {}, + "output_type": "execute_result" + } + ], + "source": [ + "%catalog schema('bwd.draft.bw_assessment.assessments') # should be empty now, after the delete" + ] } ], "metadata": { @@ -322,4 +501,4 @@ }, "nbformat": 4, "nbformat_minor": 5 -} \ No newline at end of file +} diff --git a/docs/notebook-facade-for-data-scientists.md b/docs/notebook-facade-for-data-scientists.md index ede131e..4b0b238 100644 --- a/docs/notebook-facade-for-data-scientists.md +++ b/docs/notebook-facade-for-data-scientists.md @@ -8,7 +8,7 @@ Idea note, not a plan — captures a discussion, nothing here is agreed or sched commit" and "Two sessions, not one" below; `%catalog`/`%ingest` (`src/eea_datalakehouse/notebook/magics.py`) implement "Two magics, two sessions" and "Loading the magics without typing a magic to do it" over them; -relative-path context (a leading `.`, auto-inferred from usage, plus a Comm +relative-path context (a leading `.`, set explicitly via `use`, plus a Comm channel for an integration to push it invisibly) implements "Pre-filling catalog context" — see `docs/notebooks/catalog_session_example.ipynb` and `docs/notebooks/ingest_session_example.ipynb` for worked examples. Still just @@ -41,27 +41,27 @@ similarly renamed `set_tags`/`delete_tags` (matching `set_wiki`/ `Catalog.setmeta2wiki`/`getmetafromwiki` are still there for a folder's 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` +Relative-path resolution grew, then partly retreated, then grew again. +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` were a deliberate exception at first: since 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, a bare path there stayed absolute always, +context or not — see below for why that exception didn't last). `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()`. +before. `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 @@ -72,10 +72,84 @@ 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. +unambiguous instead. `data_copy`/`data_move`/`create_view` didn't get this +treatment at first — the reasoning above still seemed to hold, that a +`_ROOT_SOURCE` check couldn't tell a deliberately relative bare path +(still meant to be appended there) apart from one that just happened not +to start with `_ROOT_SOURCE` — until it became clear that reasoning was +wrong: since a *genuinely* absolute path in this single-source deployment +always starts with `_ROOT_SOURCE` by definition, the "unrelated absolute +target" case these three verbs exist to support is already exactly the +case `_ROOT_SOURCE` disambiguates. There was no real ambiguity left to protect against — only a papercut, +the same one every other verb already had fixed, still hitting +`data_copy`/`data_move`/`create_view` calls that tried a bare relative +path and got a confusing failure instead (worse than a papercut for +`data_copy`/`data_move`, in fact: a bare single-segment path like +`"raw_2026"` isn't valid as a literal absolute path at all — no schema to +split it on — so the failure surfaced as a raw `ValueError` from deep in +`operations.py`, not even a clean `CatalogOperationError`). Dropped the +`dot_required` parameter entirely (`_resolve_path` no longer needs two +modes) and `data_copy`/`data_move`/`create_view` now resolve exactly like +every other verb. + +Finally, context stopped being a side effect at all: every queueing verb +used to update it from whatever path it just touched (the target's parent +for `data_copy`/`data_move`/`create_view`, the touched path itself for +`create_folder`) — this is what "ordinary use already keeps context +current on its own" meant throughout the rest of this doc. That auto-update +is gone; `use`/`set_context` are now the *only* two ways context ever +changes. Every verb still *reads* context to resolve its own relative +`path` (unchanged, see above), it just never writes it back — a custodian +found a call that plainly wasn't about navigation (`create_folder`, most +concretely) silently moving context out from under them more surprising +than the convenience of not having to call `use` again was worth. `_resolve` +(the internal helper that used to do the resolve-then-update-context pair) +is gone along with it — every verb now calls `_resolve_path` directly. + +`use`'s own output followed from that: since it never queues anything, +`%catalog`'s auto-commit (`_dispatch` in `magics.py`) used to hand back an +empty `CommitReport` — accurate, but useless to look at, and not what a +custodian typing `use(path)` actually wants to see. It now prints +`context set to ` instead whenever a commit had nothing to report, +covering `use`/`set_context` today and any future verb with the same +"chains back to `self`, queues nothing" shape (a plain `print()`, not a +returned value IPython would auto-display, matching every other status +line this module prints — see its own module docstring). `%%catalog`'s +own line loop needed a small fix alongside this: it used to treat a bare +`None` result as "an error was already printed", which broke the moment +`use(None)` (clearing context) started legitimately returning `None` too +— see `_DISPATCH_FAILED` in `magics.py`. + +Two more `_resolve_path` gaps surfaced once `use`'s own live existence +check made it obvious a custodian would reach for `"."`/`"./"` to mean +"the context, unchanged" (the same way `cd .` does): neither was handled +before, so both silently appended themselves as a literal trailing +`.`/`./` onto the context instead — `_resolve_path(".")` returned +`f"{context}."`, not `context`. Both now resolve to the context exactly +as `get_context()` would show it; `"./name"` also now means the same +thing as `".name"` rather than embedding a stray `/` in the resolved +path. The `%catalog help` note describing all of this was rewritten into +an explicit, itemised list (absolute / `.`-or-`./` / `../` / bare-or-dot +relative) rather than one dense paragraph, once it became the obvious +place a custodian would actually go looking for exactly this. + +A freshly built `CatalogSession` no longer starts with an empty context +either: `__init__` now sets it to `_ROOT_SOURCE` ("catalog", the catalog +root) rather than `None`, so a custodian's very first relative call — one +made before ever calling `use()` — already has something to resolve +against instead of raising "no context is set". Every other invariant +above is unchanged (only `use`/`set_context` ever touch context). `use`'s +own `None`/`""` handling changed to match: rather than clearing context +to `None` (a state a fresh session no longer starts in either), it now +resets to `_ROOT_SOURCE` — the same place a custodian already lands on +before ever calling `use()`, so "clear the context" and "go back to +where I started" become the same action. `set_context` — the lower-level +primitive `use` wraps, documented as not something a custodian should +normally reach for directly — kept its old `None` behaviour (clears to no +context at all), since it's still needed as an explicit "unset" for the +JupyterLab-Comm integration path and internal use. `%catalog help`'s note +and `get_context`'s/`use`'s own entries were updated to say so. + See `src/eea_datalakehouse/notebook/magics.py`'s module docstring and `src/eea_datalakehouse/catalog/session.py`'s `use`/`get_context`/ `_resolve_path`/`list` for the current, authoritative behaviour. @@ -342,14 +416,14 @@ they're in, not a per-call decision. This is the same idea "Session context lives in the Python process" above already raised (`a "current" catalog path/context ... so repeated calls can take a relative path`), sharpened with a concrete trigger: a catalog-tree UI in the JupyterLab extension, where -selecting a leaf pre-fills that context automatically, rather than only -updating lazily from whatever path a call last touched. +selecting a leaf pre-fills that context automatically, rather than the +custodian having to call `use` themselves first. This splits into two sides that live in two different repositories. **The receiving side, here — implemented and encapsulated on both routes in:** -`CatalogSession.set_context(path)`/`_resolve` (`catalog/session.py`) resolve the -open question below with an explicit marker (a leading `.`, e.g. +`CatalogSession.set_context(path)`/`_resolve_path` (`catalog/session.py`) resolve +the 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 for setting @@ -357,12 +431,15 @@ 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 - parent for `data_copy`/`data_move`; the touched path itself for a - folder-scoped verb like `create_folder`), so a script that never once - calls `set_context` still gets working relative paths after its first - absolute one. +- **`use`/`set_context` are the only two ways context ever changes.** No + queueing verb updates it as a side effect of running, even one whose own + `path` fully resolved to something that could sensibly become the new + context (an earlier draft of this design had exactly that — every verb + updating context from whatever it just touched, the target's parent for + `data_copy`/`data_move`, the touched path itself for `create_folder` — + but a custodian finding context moved out from under them by a call that + never looked like it should touch it turned out to be a bigger surprise + than the convenience was worth; see `use`'s own docstring). - **The Comm channel below is the *other* caller**, not the custodian either. **The emitting side, in `eeadata/EEALakeHouse` — still not built:** the diff --git a/docs/notebooks/catalog_session_example.ipynb b/docs/notebooks/catalog_session_example.ipynb index cc06d42..39837a5 100644 --- a/docs/notebooks/catalog_session_example.ipynb +++ b/docs/notebooks/catalog_session_example.ipynb @@ -35,7 +35,7 @@ { "cell_type": "markdown", "metadata": {}, - "source": "## Each call runs right away\n\nThere's no queue and no `commit()` to call afterwards — the moment a cell below runs, the\noperation has already happened against Dremio (with the same retry-on-cold-engine and\nrollback-on-failure safety `commit()` always had, applied automatically per call). The\nsame `CatalogSession` is still reused across every `%catalog` cell in this kernel, purely\nto keep the \"current path\" context up to date (see \"Session context lives in the Python\nprocess\" in the design doc) — not to batch anything." + "source": "## Each call runs right away\n\nThere's no queue and no `commit()` to call afterwards — the moment a cell below runs, the\noperation has already happened against Dremio (with the same retry-on-cold-engine and\nrollback-on-failure safety `commit()` always had, applied automatically per call). The\nsame `CatalogSession` is still reused across every `%catalog` cell in this kernel — so\ncontext set via `use()` stays in effect across cells, and idempotency keys keep\nincrementing — not to batch anything." }, { "cell_type": "code", @@ -44,12 +44,6 @@ "outputs": [], "source": "%catalog data_copy(\"bwd.draft.raw_2026\", \"bwd.reference.water_temperature\", overwrite=True)" }, - { - "cell_type": "markdown", - "id": "d1f58114", - "source": "%catalog set_tags(\".water_temperature\", [\"reviewed\", \"2026\"])", - "metadata": {} - }, { "cell_type": "code", "execution_count": null, @@ -66,7 +60,7 @@ { "cell_type": "code", "id": "e3533db0", - "source": "%catalog get_context() # 'bwd.reference' — left there by set_tags above", + "source": "%catalog get_context() # -> 'catalog' — its default; data_copy/set_tags only *read* context, never write it; only use() does", "metadata": {}, "execution_count": null, "outputs": [] @@ -88,13 +82,13 @@ { "cell_type": "markdown", "id": "4b6abba9", - "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.", + "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), and so does `data_copy`/\n`data_move`/`create_view`'s own `source_path`/`target_path` (each resolved independently\nagainst that same context — see below). Neither call above changed the context itself,\nthough — it's still `\"bwd.reference\"`, exactly what `use` set it to on the magic line\nabove. Only `use`/`set_context` ever change context; every other verb only *reads* it to\nresolve its own `path`. `use` itself is the one real exception to the dot-optional\nreading above: its own `path` must always be a whole, absolute path — never resolved\nagainst whatever context already exists, and never a leading `.`/`../` fragment.\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. It's also what makes\n`data_copy`/`data_move`/`create_view` safe to resolve this way at all — pairing a\nrelative `source_path` with a genuinely unrelated absolute `target_path` stays\nunambiguous, since a real absolute path here always starts with `catalog`.\n\n`use(None)`/`use(\"\")` reset the context back to `\"catalog\"` (the root) rather than clearing it entirely — the same place a fresh session already starts. `set_context(None)` is the one way left to clear it to no context at all.\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 — nothing set context there\nautomatically, so a fresh `use()` is the only way to actually move.\n\n`use` prints `context set to ` itself, rather than an empty commit report — there's\nnothing to commit, since `use` never queues anything, so `%catalog` shows the context\ninstead of a blank result.", "metadata": {} }, { "cell_type": "code", "id": "417250b0", - "source": "%catalog use(\"bwd.reference.2027\") # use() always takes the whole, absolute path\n%catalog get_context() # -> 'bwd.reference.2027'", + "source": "%catalog use(\"bwd.reference.2027\") # prints: context set to 'bwd.reference.2027'\n%catalog get_context() # -> 'bwd.reference.2027'\n%catalog list(\".\") # '.' alone -> the context itself: same as list(\"bwd.reference.2027\") -> []", "metadata": {}, "execution_count": null, "outputs": [] @@ -102,13 +96,13 @@ { "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 — 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\").", + "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).", "metadata": {} }, { "cell_type": "code", "id": "28305b98", - "source": "%catalog set_tags(\"../water_temperature\", [\"archived\"]) # up one level, then a sibling — any verb\n%catalog get_context() # -> 'bwd.reference' — set_tags' own tracking already moved it there", + "source": "%catalog set_tags(\"../water_temperature\", [\"archived\"]) # up one level, then a sibling — any verb\n%catalog get_context() # -> 'bwd.reference.2027' — unchanged; set_tags only reads context, never writes it", "metadata": {}, "execution_count": null, "outputs": [] @@ -138,7 +132,7 @@ { "cell_type": "code", "id": "91156d7b", - "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", + "source": "%catalog use(\"bwd.reference\") # prints: context set to 'bwd.reference'\n%catalog list(\"bwd.reference\") # every table/view under bwd.reference\n%catalog list() # same thing — path omitted, lists the context itself\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/client.py b/src/eea_datalakehouse/catalog/client.py index 85f4dbe..a1fffa3 100644 --- a/src/eea_datalakehouse/catalog/client.py +++ b/src/eea_datalakehouse/catalog/client.py @@ -344,7 +344,11 @@ def createfolder( def deletefolder(self, path: str, *, cascade: bool = False, idempotency_key: str) -> None: return operations.deletefolder( - self._catalog_rest, path, cascade=cascade, idempotency_key=idempotency_key + self._executor, + self._catalog_rest, + path, + cascade=cascade, + idempotency_key=idempotency_key, ) def retry_pending(self, idempotency_key: str) -> SqlResult | list[Any]: diff --git a/src/eea_datalakehouse/catalog/operations.py b/src/eea_datalakehouse/catalog/operations.py index 0fc3c7e..12cd6cd 100644 --- a/src/eea_datalakehouse/catalog/operations.py +++ b/src/eea_datalakehouse/catalog/operations.py @@ -953,7 +953,16 @@ def setwikito( appended to `text` before it's sent (see `_render_wiki_metadata`). The rendered text, metadata included, is what a retry remembers and re-sends — `tags` itself is never persisted to retry_state. + + Ignored outright when `path` is a table or view: those already have + Dremio's own native tags/labels (`settagsto`/`gettagsfrom`) for exactly + this — the wiki Meta Data convention exists only because a *folder* + has no such native concept (see `get_tags`' own docstring). Silently + dropped rather than raised, since a caller passing `tags` alongside a + table/view `path` almost certainly meant `settagsto`, not this. """ + if tags and catalog_rest.is_table_or_view(path): + tags = None if tags: _validate_tags(tags) text = f"{text}\n\n{_render_wiki_metadata(tags)}" @@ -1244,6 +1253,7 @@ def create() -> None: def deletefolder( + executor: SqlExecutor, catalog_rest: CatalogRestClient, path: str, *, @@ -1252,15 +1262,35 @@ def deletefolder( ) -> None: """Delete the folder at `path`. - REST-only (see module docstring) — folder deletion has no SQL or - Flight equivalent. Idempotent: a folder that's already gone is not an - error. `cascade=False` (the default) raises `CatalogOperationError` if - the folder still has contents — `rmdir` vs `rm -r`. `cascade=True` - deletes every table/view and subfolder inside first, depth-first, then - the folder itself (see `CatalogRestClient.delete_folder`). + Folder deletion itself has no SQL or Flight equivalent, so the folder + (and any subfolder) always goes through `catalog_rest` directly. + Idempotent: a folder that's already gone is not an error. + `cascade=False` (the default) raises `CatalogOperationError` if the + folder still has contents — `rmdir` vs `rm -r`. `cascade=True` deletes + every table/view and subfolder inside first, depth-first, then the + folder itself (see `CatalogRestClient.delete_folder`) — a dataset child + goes through the same SQL `DROP TABLE`/`DROP VIEW` `deletetable`/ + `deleteview` themselves use (`_entry_kind` tells the two apart via + INFORMATION_SCHEMA), not the REST catalog API's own generic delete, + which this project has never confirmed actually drops a *physical* + table's underlying data rather than just forgetting its catalog entry. """ + step = 0 + + def delete_dataset(dataset_path: str) -> None: + nonlocal step + step += 1 + sub_key = f"{idempotency_key}-cascade-{step}" + kind = _entry_kind(executor, dataset_path, idempotency_key=sub_key) + if kind == "VIEW": + deleteview(executor, dataset_path, idempotency_key=sub_key) + else: + deletetable(executor, dataset_path, idempotency_key=sub_key) + try: - catalog_rest.delete_folder(path, cascade=cascade) + catalog_rest.delete_folder( + path, cascade=cascade, delete_dataset=delete_dataset if cascade else None + ) except EngineStartingError as exc: retry_state.record( idempotency_key, "deletefolder", path, str(exc), params={"path": path, "cascade": cascade} diff --git a/src/eea_datalakehouse/catalog/rest.py b/src/eea_datalakehouse/catalog/rest.py index 16a6fd5..44e5d19 100644 --- a/src/eea_datalakehouse/catalog/rest.py +++ b/src/eea_datalakehouse/catalog/rest.py @@ -1,7 +1,10 @@ """Dremio's own REST Catalog API (v3) — used for folder existence/creation/ deletion and for reading an entity's wiki/tags, none of which have a SQL or Flight equivalent (this is the one place in `catalog` that isn't -transport-agnostic through SqlExecutor). +transport-agnostic through SqlExecutor) — except `delete_folder`'s cascade, +which accepts an optional SQL-backed callback for its dataset children (see +that method, and `operations.deletefolder`, which supplies one) rather than +trusting this API's own generic delete to drop a *physical* table's data. Unverified against a real Dremio deployment: the v3 catalog API's by-path lookup, folder-creation, wiki/tag-collaboration, and folder-deletion @@ -14,6 +17,7 @@ from __future__ import annotations +from collections.abc import Callable from typing import Any import httpx @@ -70,8 +74,21 @@ def _lookup_by_path(self, path: str) -> dict[str, Any] | None: return resp.json() def exists(self, path: str) -> bool: - """Whether `path` (dot-separated) is any entity in the catalog.""" - return self._lookup_by_path(path) is not None + """Whether `path` (dot-separated) is any entity in the catalog. + + Best-effort, same reasoning as `is_folder`/`is_table_or_view`: some + Dremio source types reject a by-path lookup into a nested item that + doesn't exist outright (a 400, not a 404) rather than answering + "not found", so any lookup failure here is treated the same as + "not found", never raised — most visibly, this is what lets + `CatalogSession.create_folder` check whether a not-yet-created + folder is already there without that check itself blowing up the + creation it's only meant to make undoable. + """ + try: + return self._lookup_by_path(path) is not None + except CatalogOperationError: + return False def is_folder(self, path: str) -> bool: """Whether `path` names an existing folder — not a table/view/space/source. @@ -337,17 +354,33 @@ def _delete_entity(self, entity_id: str, *, what: str) -> None: f"could not delete {what} ({resp.status_code}): {resp.text[:300]}" ) - def delete_folder(self, path: str, *, cascade: bool = False) -> None: + def delete_folder( + self, + path: str, + *, + cascade: bool = False, + delete_dataset: Callable[[str], None] | None = None, + ) -> None: """Delete the folder at `path`. Idempotent — a folder that's already gone is not an error, same as `deleteview`'s ``DROP VIEW IF EXISTS``. `cascade=False` (the default) raises `CatalogOperationError` if the folder still has contents — `rmdir` vs `rm -r`. `cascade=True` - deletes every table/view and subfolder inside first, depth-first - (each straight through this same catalog API — Dremio supports - deleting a dataset by id here too, not just via SQL DROP), then the - folder itself. + deletes every table/view and subfolder inside first, depth-first, + then the folder itself. A subfolder child always recurses through + this same catalog API (folder deletion has no SQL equivalent); a + dataset child (table or view) goes through `delete_dataset( + child_path)` if given — `operations.deletefolder` passes one that + runs the same SQL `DROP TABLE`/`DROP VIEW` `deletetable`/ + `deleteview` themselves use, since this project has never confirmed + that deleting a dataset by id through this catalog API actually + drops a *physical* table's underlying data, rather than just + forgetting its catalog entry (the same reason `deletetable`/ + `deleteview` don't use this API for that either). With no + `delete_dataset` — a caller using this client directly, with no SQL + executor available — falls back to that same generic catalog + delete. """ entity = self._lookup_by_path(path) if entity is None: @@ -367,7 +400,9 @@ def delete_folder(self, path: str, *, cascade: bool = False) -> None: if not child_path: continue if self._child_is_folder(child): - self.delete_folder(child_path, cascade=True) + self.delete_folder(child_path, cascade=True, delete_dataset=delete_dataset) + elif delete_dataset is not None: + delete_dataset(child_path) else: self._delete_entity(child["id"], what=f"{child_path!r}") diff --git a/src/eea_datalakehouse/catalog/session.py b/src/eea_datalakehouse/catalog/session.py index 32f8202..c4d69de 100644 --- a/src/eea_datalakehouse/catalog/session.py +++ b/src/eea_datalakehouse/catalog/session.py @@ -149,7 +149,13 @@ def __init__(self, catalog: Catalog) -> None: self._id = uuid.uuid4().hex[:8] self._steps: list[_Step] = [] self._next_step = 1 - self._context: str | None = None + # Starts at the catalog root, not None — a data custodian's very + # first relative path (no use() call yet) still has something to + # resolve against, rather than raising "no context is set" before + # they've ever had a chance to set one deliberately. use(None)/ + # use("") reset back here explicitly; only set_context(None) can + # still clear it to no context at all. + self._context: str | None = _ROOT_SOURCE def __repr__(self) -> str: return f"CatalogSession(pending={len(self._steps)})" @@ -160,27 +166,34 @@ def _key(self, suffix: str = "") -> str: 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 - `.` — 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 - `_resolve` — every queued step updates it from whatever path it - just touched), and an integration (the JupyterLab catalog-tree - extension pushing a selected leaf through a Comm — see - `docs/notebook-facade-for-data-scientists.md`, "Pre-filling catalog - context") is the other caller, seeding it before any path has been - typed yet. + `.`, e.g. `.water_temperature`) resolves against this. `None` clears + it to no context at all (unlike `use(None)`/`use("")`, which reset to + `_ROOT_SOURCE` instead — see `use`'s own docstring for why). `path` + here is always taken literally, even if it itself starts with `.` — + same as `use`, which differs only in that `None`/`""` handling, plus + also making a live existence check (see `use`'s own docstring). + + `use` (via `%catalog use`/`%%catalog use(path)`) is the only + custodian-facing way to set context — no queueing verb touches it + as a side effect of running, deliberately, so context only ever + changes when asked. `set_context` itself is not something a data + custodian should normally call directly: an integration (the + JupyterLab catalog-tree extension pushing a selected leaf through + a Comm — see `docs/notebook-facade-for-data-scientists.md`, + "Pre-filling catalog context") is its other caller, seeding it + before any path has been typed yet. """ self._context = path return self def use(self, path: str | None) -> CatalogSession: """Set the current path for every call after this one — same as - `set_context`, plus a live existence check (below); `None` clears - it exactly like `set_context(None)`. Unlike every other verb's + `set_context`, plus a live existence check (below) and different + `None`/`""` handling: either one resets context to `_ROOT_SOURCE` + (the catalog root — the same starting point a freshly built session + already has), rather than clearing it to no context at all the way + `set_context(None)` still does (see that method's own docstring for + why the two diverge here). 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 `.` @@ -188,25 +201,24 @@ def use(self, path: str | None) -> CatalogSession: 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 - top of a cell, for every call in that cell and every cell after - it, until changed again); ordinary use already keeps context - current on its own, so this is only needed to jump somewhere a - queued step hasn't already touched. + This is the only way to set context — no other verb touches it as + a side effect of running (see `%%catalog use(path)` — the cell + magic sets it once at the top of a cell, for every call in that + cell and every cell after it, until changed again). Unlike everything else in this class, this makes a live call 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. + against later. `None`/`""` skip this check entirely — the catalog + root always exists. """ - if path is None: - self._context = None + if path is None or path == "": + self._context = _ROOT_SOURCE return self try: found = self._catalog._catalog_rest.exists(path) # noqa: SLF001 — see module docstring - except (CatalogOperationError, EngineStartingError) as exc: + except EngineStartingError as exc: raise CatalogSessionError(f"could not check whether {path!r} exists: {exc}") from exc if not found: raise CatalogSessionError(f"{path!r} does not exist in the catalog") @@ -214,19 +226,17 @@ def use(self, path: str | None) -> CatalogSession: return self def get_context(self) -> str | None: - """The current path, always the full resolved path — never a - `.`/`..`-prefixed fragment, even though most verbs' own relative - paths can be — see `set_context`/`_resolve`. `None` if nothing has - been set yet.""" + """The current path — see `use`/`set_context`, the only two ways + it changes after construction. Starts at `_ROOT_SOURCE` (the + catalog root) for a freshly built session, never `None` — `None` + only after an explicit `set_context(None)`. `use(None)`/`use("")` + reset it back to `_ROOT_SOURCE` instead, rather than to `None`.""" return self._context - 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 - path (never a `.`/`..`-prefixed fragment), so a value stored from - here — including into `self._context` — is safe to read back as-is - (see `get_context`). + def _resolve_path(self, path: str) -> str: + """Just the relative -> absolute resolution — never updates the + context itself; only `use`/`set_context` do that. Always returns a + full, already-resolved path (never a `.`/`..`-prefixed fragment). A leading `.` resolves against the current context (`".water_temperature"` -> `f"{context}.water_temperature"`); once a @@ -239,28 +249,35 @@ def _resolve_path(self, path: str, *, dot_required: bool = False) -> str: 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.""" + relative path. Every verb resolves this way, including `data_copy`/ + `data_move`/`create_view`'s two paths (each resolved independently + against the *same* current context) — since a genuinely absolute + path in this deployment always starts with `_ROOT_SOURCE`, pairing + a relative one with an unrelated absolute one in the same call is + already unambiguous without needing a dot on the absolute side too. + Otherwise, a bare path is only ever taken literally as absolute + when no context has been set yet. `"."`/`"./"` alone (no name + after either) resolve to the context itself, exactly as given by + `get_context()` — not the context with a stray trailing `.`/`./` + appended. One or more leading `../` segments (or a bare `..`) + instead walk up that many levels of the context *first* — + `"../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 path in (".", "./"): + if self._context is None: + raise CatalogSessionError( + f"{path!r} means the current context, but no context is set yet — " + "use an absolute path first, or call set_context()" + ) + return self._context + if path.startswith("./"): + path = f".{path[2:]}" # "./name" means the same thing as ".name" if not path.startswith("."): is_root_absolute = path == _ROOT_SOURCE or path.startswith(f"{_ROOT_SOURCE}.") - if dot_required or self._context is None or is_root_absolute: + if self._context is None or is_root_absolute: return path path = f".{path}" if self._context is None: @@ -295,19 +312,6 @@ def _resolve_parent_path(self, path: str) -> str: remainder = f".{remainder}" return f"{ancestor}{remainder}" - def _resolve(self, path: str) -> str: - """Resolve `path` (see `_resolve_path`), then update the context to - its own parent — so the next short name keeps resolving against - wherever this one just landed, without anyone calling - `set_context` again. Verbs with two paths (`data_copy`/`data_move`/ - `create_view`) resolve both against the *same* starting context via - `_resolve_path` directly instead — only the target should become - the new context, not whatever `source_path`'s own folder happens - to be.""" - resolved = self._resolve_path(path) - self._context = operations._parent_path(resolved) or self._context # noqa: SLF001 - return resolved - # -- queueing verbs --------------------------------------------------- def data_copy( @@ -324,14 +328,11 @@ 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 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, 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 + Either path may be relative — with or without a leading `.` — to the session's + current context — see `set_context`. Does not change the context itself — only + `use` does that.""" + source_path = self._resolve_path(source_path) + target_path = self._resolve_path(target_path) def run(catalog: Catalog, key: str) -> _Undo | None: catalog.datacopy( @@ -366,14 +367,11 @@ def data_move( `target_path` to `source_path`. Same `overwrite=True` limitation as `data_copy`. - 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, 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 + Either path may be relative — with or without a leading `.` — to the session's + current context — see `set_context`. Does not change the context itself — only + `use` does that.""" + source_path = self._resolve_path(source_path) + target_path = self._resolve_path(target_path) def run(catalog: Catalog, key: str) -> _Undo | None: catalog.datamove( @@ -417,14 +415,11 @@ 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 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, 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 + Either path may be relative — with or without a leading `.` — to the session's + current context — see `set_context`. Does not change the context itself — only + `use` does that.""" + source_path = self._resolve_path(source_path) + target_path = self._resolve_path(target_path) def run(catalog: Catalog, key: str) -> _Undo | None: catalog.createview( @@ -450,8 +445,8 @@ def set_tags(self, path: str, tags: list[str]) -> CatalogSession: tags were on `path` immediately before this step ran. `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) + context — see `set_context`. Does not change the context itself — only `use` does that.""" + path = self._resolve_path(path) def run(catalog: Catalog, key: str) -> _Undo: existing = catalog.gettagsfrom(path, idempotency_key=f"{key}-read") @@ -470,8 +465,8 @@ def delete_tags(self, path: str, tags: list[str]) -> CatalogSession: immediately before this step ran. `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) + context — see `set_context`. Does not change the context itself — only `use` does that.""" + path = self._resolve_path(path) def run(catalog: Catalog, key: str) -> _Undo: existing = catalog.gettagsfrom(path, idempotency_key=f"{key}-read") @@ -489,11 +484,16 @@ def set_wiki( self, path: str, text: str, *, tags: list[dict[str, str]] | None = None ) -> CatalogSession: """Queue `setwikito`. Undo restores the previous wiki text verbatim, - or deletes it if `path` had none. + or deletes it if `path` had none. `tags` (a list of `{tag_name, + tag_value, tag_title}` dicts, rendered into a "# Meta Data" section + of the wiki text) is ignored outright when `path` is a table or + view — those already have Dremio's own native tags/labels for + this (`set_tags`/`get_tags`); the wiki Meta Data convention exists + only because a folder has no such native concept. `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) + context — see `set_context`. Does not change the context itself — only `use` does that.""" + path = self._resolve_path(path) def run(catalog: Catalog, key: str) -> _Undo: existing = _read_wiki_or_none(catalog, path, idempotency_key=f"{key}-read") @@ -516,8 +516,8 @@ def delete_wiki(self, path: str) -> CatalogSession: was one — a no-op if `path` had no wiki to begin with. `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) + context — see `set_context`. Does not change the context itself — only `use` does that.""" + path = self._resolve_path(path) def run(catalog: Catalog, key: str) -> _Undo | None: existing = _read_wiki_or_none(catalog, path, idempotency_key=f"{key}-read") @@ -539,11 +539,9 @@ def create_folder(self, path: str, *, create_parents: bool = False) -> CatalogSe that already existed is left alone on rollback too. `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).""" + context — see `set_context`. Does not change the context itself — only + `use` does that.""" path = self._resolve_path(path) - self._context = path def run(catalog: Catalog, key: str) -> _Undo | None: already_there = catalog._catalog_rest.exists(path) # noqa: SLF001 — see module docstring diff --git a/src/eea_datalakehouse/notebook/magics.py b/src/eea_datalakehouse/notebook/magics.py index 72e4244..04c78aa 100644 --- a/src/eea_datalakehouse/notebook/magics.py +++ b/src/eea_datalakehouse/notebook/magics.py @@ -50,16 +50,16 @@ `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:: +`_resolve_path`) is set only by `use(path)` — no other verb changes it as +a side effect of running, even one whose own `path`/`source_path`/ +`target_path` fully resolved to something that could sensibly become the +new context. 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") set_tags(".water_temperature", ["reviewed"]) @@ -132,12 +132,15 @@ "use", "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.", + "verb's path/source_path/target_path. None or '' resets it to 'catalog' (the " + "root). Raises if path doesn't exist in the catalog. See %%catalog to set it " + "once at the top of a cell.", ), ( "get_context", - "Show the current path (None if nothing has been set yet).", + "Show the current path. A new session starts at 'catalog' (the root); " + "use(None)/use('') reset back to it too — None only after an explicit " + "set_context(None).", ), # -- data --------------------------------------------------------------- ( @@ -187,7 +190,8 @@ ( "set_wiki", "Set a path's wiki text. tags: a list of {tag_name, tag_value, tag_title} dicts, " - "e.g. " + _META_TAGS_EXAMPLE + ".", + "e.g. " + _META_TAGS_EXAMPLE + ". Ignored if path is a table/view (they have " + "set_tags/get_tags for that instead).", ), ("delete_wiki", "Delete a path's wiki text."), ( @@ -274,12 +278,20 @@ 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 = ( - "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." + "A new session's context starts at 'catalog' (the root source), not empty — " + "use(None)/use('') reset back to it too; only set_context(None) clears it to no " + "context at all.\n" + "Every path/source_path/target_path (except use's own) is either absolute or " + "relative to the current context, once one is set:\n" + " - starts with 'catalog.' (this deployment's one real root source) -> always " + "absolute, used exactly as given, never appended to the context\n" + " - '.' or './' alone -> the context itself, exactly as get_context() shows it\n" + " - '../' (or a bare '..'), optionally chained ('../../') -> walks up that many " + "levels of the context first, then appends whatever's left, if anything\n" + " - '.name', or a bare 'name' with no leading '.' at all -> both mean relative to " + "the context; the dot is optional sugar, not what makes it relative\n" + "Only use() ever changes the context — no other call does, even one that fully " + "resolved an absolute path." ) @@ -324,6 +336,16 @@ def _print_help_table( display(HTML(_help_table_html(rows))) +_DISPATCH_FAILED = object() +# Sentinel `_dispatch` returns when it printed a friendly error — distinct +# from a legitimate `None` result (e.g. `use(...)` printing its own status +# instead of an empty commit report, or `get_context()` with nothing set +# yet). `%%catalog`'s own loop needs to tell those apart to know whether to +# keep running the rest of the cell; +# every caller converts this back to a plain `None` before it reaches +# IPython, so it's never actually displayed. + + def _dispatch( session: Any, line: str, @@ -346,10 +368,18 @@ def _dispatch( # A queueing verb just chained back to `self` — commit it right # away instead of waiting for a separate commit() call (see the # module docstring: %catalog executes immediately). - result = session.commit(retry=True) + report = session.commit(retry=True) + if report.succeeded: + result = report + else: + # use()/set_context() also chain back to `self`, but queue + # nothing — commit() then has nothing to report, so print + # the context they just set instead of an empty CommitReport. + print(f"context set to {session.get_context()!r}") + result = None except error_type as exc: print(f"{label} error: {exc}") - return None + return _DISPATCH_FAILED return result @@ -436,7 +466,7 @@ def catalog(self, line: str) -> Any: return None if not self._ensure_catalog_session(): return None - return _dispatch( + result = _dispatch( self._catalog_session, line, self.shell.user_ns, @@ -444,6 +474,7 @@ def catalog(self, line: str) -> Any: CatalogSessionError, auto_commit=True, ) + return None if result is _DISPATCH_FAILED else result @cell_magic("catalog") def catalog_cell(self, line: str, cell: str) -> Any: @@ -476,8 +507,8 @@ def catalog_cell(self, line: str, cell: str) -> Any: CatalogSessionError, auto_commit=True, ) - if result is None: - break # a friendly error was already printed by _dispatch + if result is _DISPATCH_FAILED: + return None # a friendly error was already printed by _dispatch return result @line_magic @@ -487,9 +518,10 @@ def ingest(self, line: str) -> Any: return None if self._ingest_session is None: self._ingest_session = IngestSession() - return _dispatch( + result = _dispatch( self._ingest_session, line, self.shell.user_ns, "ingest", IngestSessionError ) + return None if result is _DISPATCH_FAILED else result def load_ipython_extension(ipython: Any) -> None: diff --git a/tests/catalog/conftest.py b/tests/catalog/conftest.py index bbf1f59..06da9af 100644 --- a/tests/catalog/conftest.py +++ b/tests/catalog/conftest.py @@ -1,5 +1,6 @@ from __future__ import annotations +from collections.abc import Callable from typing import Any import pytest @@ -190,7 +191,13 @@ def _children_of(self, path: str) -> list[str]: depth = path.count(".") + 1 return [p for p in self.existing if p.startswith(prefix) and p.count(".") == depth] - def delete_folder(self, path: str, *, cascade: bool = False) -> None: + def delete_folder( + self, + path: str, + *, + cascade: bool = False, + delete_dataset: Callable[[str], None] | None = None, + ) -> None: from eea_datalakehouse.catalog.errors import CatalogOperationError if self._raise_on_delete_folder is not None: @@ -208,7 +215,9 @@ def delete_folder(self, path: str, *, cascade: bool = False) -> None: if cascade: for child in children: if child in self.folders: - self.delete_folder(child, cascade=True) + self.delete_folder(child, cascade=True, delete_dataset=delete_dataset) + elif delete_dataset is not None: + delete_dataset(child) else: self.existing.discard(child) self.deleted.append(child) diff --git a/tests/catalog/test_client.py b/tests/catalog/test_client.py index a71c14f..c82b248 100644 --- a/tests/catalog/test_client.py +++ b/tests/catalog/test_client.py @@ -329,7 +329,7 @@ def test_deletewiki_delegates_to_operations() -> None: def test_setwikito_with_tags_delegates_to_operations() -> None: - fake_rest = FakeCatalogRest(existing={"a.b"}) + fake_rest = FakeCatalogRest(existing={"a.b"}, folders={"a.b"}) catalog = Catalog(BASE_URL, "pat", executor=FakeExecutor(), catalog_rest=fake_rest) tags = [{"tag_name": "owner", "tag_value": "bwd-team", "tag_title": "Owner"}] diff --git a/tests/catalog/test_operations.py b/tests/catalog/test_operations.py index c16701f..7236e3e 100644 --- a/tests/catalog/test_operations.py +++ b/tests/catalog/test_operations.py @@ -852,7 +852,9 @@ def test_setwikito_engine_starting_is_remembered_and_retryable(executor) -> None def test_setwikito_appends_metadata_section_when_tags_given() -> None: - fake_rest = FakeCatalogRest(existing={"a.b"}) + # Folder — tags on a table/view are ignored instead (see the dedicated + # test below), since those already have native tags/labels. + fake_rest = FakeCatalogRest(existing={"a.b"}, folders={"a.b"}) tags = [ {"tag_name": "owner", "tag_value": "bwd-team", "tag_title": "Owner"}, {"tag_name": "status", "tag_value": "published", "tag_title": "Status"}, @@ -880,8 +882,21 @@ def test_setwikito_without_tags_leaves_text_untouched() -> None: assert fake_rest._wikis["a.b"] == "# Docs" -def test_setwikito_raises_when_a_tag_is_missing_a_key() -> None: +def test_setwikito_ignores_tags_on_a_table_or_view() -> None: + # "a.b" is a table/view here (in existing, not in folders) — those + # already have native tags/labels (settagsto/gettagsfrom), so the wiki + # Meta Data convention doesn't apply and tags is silently dropped + # rather than embedded or raised on. fake_rest = FakeCatalogRest(existing={"a.b"}) + tags = [{"tag_name": "owner", "tag_value": "bwd-team"}] # missing tag_title, but never checked + + operations.setwikito(fake_rest, "a.b", "# Docs", tags=tags, idempotency_key="k") + + assert fake_rest._wikis["a.b"] == "# Docs" + + +def test_setwikito_raises_when_a_tag_is_missing_a_key() -> None: + fake_rest = FakeCatalogRest(existing={"a.b"}, folders={"a.b"}) tags = [{"tag_name": "owner", "tag_value": "bwd-team"}] # no tag_title with pytest.raises(CatalogOperationError, match="tag_title"): @@ -893,7 +908,9 @@ def test_setwikito_raises_when_a_tag_is_missing_a_key() -> None: def test_setwikito_retry_resends_the_already_rendered_metadata(executor) -> None: fake_rest = FakeCatalogRest( - existing={"a.b"}, raise_on_set_wiki=EngineStartingError("stalled", idempotency_key="k") + existing={"a.b"}, + folders={"a.b"}, + raise_on_set_wiki=EngineStartingError("stalled", idempotency_key="k"), ) tags = [{"tag_name": "owner", "tag_value": "bwd-team", "tag_title": "Owner"}] @@ -1329,51 +1346,77 @@ def test_createfolder_engine_starting_is_remembered_and_retryable(executor) -> N assert fake_rest.created == ["a.b.c"] -def test_deletefolder_deletes_an_empty_folder() -> None: +def test_deletefolder_deletes_an_empty_folder(executor) -> None: fake_rest = FakeCatalogRest(existing={"a", "a.b"}, folders={"a.b"}) - operations.deletefolder(fake_rest, "a.b", idempotency_key="k") + operations.deletefolder(executor, fake_rest, "a.b", idempotency_key="k") assert fake_rest.deleted == ["a.b"] assert "a.b" not in fake_rest.existing -def test_deletefolder_is_idempotent_when_already_gone() -> None: +def test_deletefolder_is_idempotent_when_already_gone(executor) -> None: fake_rest = FakeCatalogRest(existing={"a"}) - operations.deletefolder(fake_rest, "a.missing", idempotency_key="k") + operations.deletefolder(executor, fake_rest, "a.missing", idempotency_key="k") assert fake_rest.deleted == [] -def test_deletefolder_raises_when_not_empty_and_not_cascading() -> None: +def test_deletefolder_raises_when_not_empty_and_not_cascading(executor) -> None: fake_rest = FakeCatalogRest( existing={"a", "a.b", "a.b.c"}, folders={"a.b", "a.b.c"} ) with pytest.raises(CatalogOperationError, match="not empty"): - operations.deletefolder(fake_rest, "a.b", idempotency_key="k") + operations.deletefolder(executor, fake_rest, "a.b", idempotency_key="k") assert fake_rest.deleted == [] -def test_deletefolder_cascade_deletes_contents_and_subfolders() -> None: +def test_deletefolder_cascade_recurses_into_subfolders() -> None: + # Only one dataset in the whole tree, under a nested subfolder — proves + # both that subfolders recurse (via fake_rest, no SQL) and that the + # dataset at the bottom still gets a real SQL drop (see the two + # single-dataset tests below for TABLE vs VIEW specifically). fake_rest = FakeCatalogRest( - existing={"a", "a.b", "a.b.c", "a.b.view1", "a.b.c.table1"}, + existing={"a", "a.b", "a.b.c", "a.b.c.table1"}, folders={"a.b", "a.b.c"}, ) + executor = FakeExecutor(rows=[{"TABLE_TYPE": "TABLE"}]) - operations.deletefolder(fake_rest, "a.b", cascade=True, idempotency_key="k") + operations.deletefolder(executor, fake_rest, "a.b", cascade=True, idempotency_key="k") - assert set(fake_rest.deleted) == {"a.b", "a.b.c", "a.b.view1", "a.b.c.table1"} + assert set(fake_rest.deleted) == {"a.b", "a.b.c"} # folders only — dataset went via SQL + assert 'DROP TABLE IF EXISTS "a"."b"."c"."table1"' in executor.statements assert "a.b" not in fake_rest.existing -def test_deletefolder_raises_when_path_is_not_a_folder() -> None: +def test_deletefolder_cascade_drops_a_view_child_via_sql_drop_view() -> None: + fake_rest = FakeCatalogRest(existing={"a", "a.b", "a.b.view1"}, folders={"a.b"}) + executor = FakeExecutor(rows=[{"TABLE_TYPE": "VIEW"}]) + + operations.deletefolder(executor, fake_rest, "a.b", cascade=True, idempotency_key="k") + + assert fake_rest.deleted == ["a.b"] # the view never went through fake_rest's own delete + assert 'DROP VIEW IF EXISTS "a"."b"."view1"' in executor.statements + + +def test_deletefolder_cascade_drops_a_table_child_via_sql_drop_table() -> None: + fake_rest = FakeCatalogRest(existing={"a", "a.b", "a.b.table1"}, folders={"a.b"}) + executor = FakeExecutor(rows=[{"TABLE_TYPE": "TABLE"}]) + + operations.deletefolder(executor, fake_rest, "a.b", cascade=True, idempotency_key="k") + + assert fake_rest.deleted == ["a.b"] # the table never went through fake_rest's own delete + assert 'DROP TABLE IF EXISTS "a"."b"."table1"' in executor.statements + + +def test_deletefolder_raises_when_path_is_not_a_folder(executor) -> None: fake_rest = FakeCatalogRest(existing={"a", "a.b"}) # "a.b" not in folders with pytest.raises(CatalogOperationError, match="not a folder"): - operations.deletefolder(fake_rest, "a.b", idempotency_key="k") + operations.deletefolder(executor, fake_rest, "a.b", idempotency_key="k") def test_deletefolder_engine_starting_is_remembered_and_retryable(executor) -> None: @@ -1384,7 +1427,7 @@ def test_deletefolder_engine_starting_is_remembered_and_retryable(executor) -> N ) with pytest.raises(EngineStartingError): - operations.deletefolder(fake_rest, "a.b", idempotency_key="k") + operations.deletefolder(executor, fake_rest, "a.b", idempotency_key="k") pending = retry_state.get("k") assert pending is not None diff --git a/tests/catalog/test_rest.py b/tests/catalog/test_rest.py index 522e166..90cffa9 100644 --- a/tests/catalog/test_rest.py +++ b/tests/catalog/test_rest.py @@ -29,14 +29,38 @@ def test_exists_false_on_404() -> None: @respx.mock -def test_exists_raises_on_other_errors() -> None: +def test_exists_false_on_other_errors() -> None: + # Best-effort, same as is_folder/is_table_or_view below — a lookup + # failure can't be told apart from "not found", so it's never raised. respx.get(f"{BASE_URL}/api/v3/catalog/by-path/a/b").mock( return_value=httpx.Response(500, text="boom") ) client = CatalogRestClient(BASE_URL, "pat") - with pytest.raises(CatalogOperationError): - client.exists("a.b") + assert client.exists("a.b") is False + + +@respx.mock +def test_exists_false_on_a_broken_by_path_lookup_rather_than_raising() -> None: + # The real error this project hit: a Dremio source type whose by-path + # lookup rejects a nested, not-yet-created item with a 400, not a 404 — + # this is what let CatalogSession.create_folder's own "is it already + # there?" check block folder creation outright (see + # test_session_context.py's create_folder/delete_folder coverage). + respx.get(f"{BASE_URL}/api/v3/catalog/by-path/catalog/deep/nested").mock( + return_value=httpx.Response( + 400, + json={ + "errorMessage": ( + "Can not get internal item from non-filesystem source [catalog] " + "of type [com.dremio.plugins.dremiocatalog.store.DremioCatalogLocalPlugin]" + ) + }, + ) + ) + client = CatalogRestClient(BASE_URL, "pat") + + assert client.exists("catalog.deep.nested") is False @respx.mock diff --git a/tests/catalog/test_session.py b/tests/catalog/test_session.py index ac0ff7d..efdf580 100644 --- a/tests/catalog/test_session.py +++ b/tests/catalog/test_session.py @@ -33,9 +33,20 @@ def _catalog(rest: FakeCatalogRest, executor: FakeExecutor | None = None) -> Cat return Catalog(BASE_URL, "pat", executor=executor, flight_executor=executor, catalog_rest=rest) +def _session(rest: FakeCatalogRest, executor: FakeExecutor | None = None) -> CatalogSession: + """A `CatalogSession` with context cleared. This file's fixture paths + are short absolute stand-ins (e.g. `"bwd.table1"`) that predate + `CatalogSession`'s own default context (the catalog root, per + `_ROOT_SOURCE`) — clearing it keeps them absolute rather than + silently getting `"catalog."` prepended.""" + session = CatalogSession(_catalog(rest, executor)) + session.set_context(None) + return session + + def test_nothing_runs_until_commit() -> None: rest = FakeCatalogRest(existing={"a", "bwd"}) - session = CatalogSession(_catalog(rest)) + session = _session(rest) session.create_folder("bwd.new") @@ -45,11 +56,9 @@ def test_nothing_runs_until_commit() -> None: 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 = _session(rest) - # 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"]) + session.create_folder("bwd.newfolder").set_tags("bwd.table1", ["reviewed"]) report = session.commit() assert report.succeeded == [ @@ -64,7 +73,7 @@ def test_commit_runs_queued_steps_in_order_and_clears_the_queue() -> None: def test_commit_rolls_back_a_cleanly_reversible_batch_on_failure() -> None: rest = FakeCatalogRest(existing={"a", "bwd", "bwd.table1"}) executor = FakeExecutor(rows=[{"TABLE_NAME": "table1", "TABLE_TYPE": "TABLE"}]) - session = CatalogSession(_catalog(rest, executor)) + session = _session(rest, executor) # step 1 succeeds and is reversible; step 2 fails because the target # already exists and overwrite defaults to False. @@ -98,10 +107,9 @@ def test_data_move_rollback_moves_the_table_back() -> None: [{"TABLE_NAME": "table1"}], ] ) - session = CatalogSession(_catalog(rest, executor)) + session = _session(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,10 +131,9 @@ def test_create_view_rollback_drops_the_view() -> None: rows_sequence=[[{"TABLE_NAME": "table1"}], []], # source-exists, target-empty rows=[{"TABLE_TYPE": "VIEW"}], # answers the undo's kind-detection lookup ) - session = CatalogSession(_catalog(rest, executor)) + session = _session(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: @@ -142,7 +149,7 @@ def test_create_view_rollback_drops_the_view() -> None: def test_commit_reports_what_it_could_not_undo() -> None: rest = FakeCatalogRest(existing={"a", "bwd", "bwd.table1"}) executor = FakeExecutor(rows=[{"TABLE_NAME": "table1", "TABLE_TYPE": "TABLE"}]) - session = CatalogSession(_catalog(rest, executor)) + session = _session(rest, executor) session.create_folder("bwd.newfolder") # reversible session.data_copy("bwd.table1", "bwd.table1", overwrite=True) # succeeds, but NOT reversible @@ -172,7 +179,7 @@ def create_folder(self, path: str) -> bool: return super().create_folder(path) rest = _FlakyRest(existing={"a", "bwd"}) - session = CatalogSession(_catalog(rest)) + session = _session(rest) session.create_folder("bwd.new") report = session.commit(retry=True, retry_delay=0) @@ -188,7 +195,7 @@ def create_folder(self, path: str) -> bool: raise EngineStartingError("engine starting", idempotency_key="k") rest = _AlwaysStartingRest(existing={"a", "bwd"}) - session = CatalogSession(_catalog(rest)) + session = _session(rest) session.create_folder("bwd.new") diff --git a/tests/catalog/test_session_context.py b/tests/catalog/test_session_context.py index e272bf9..f88da6d 100644 --- a/tests/catalog/test_session_context.py +++ b/tests/catalog/test_session_context.py @@ -1,9 +1,10 @@ """CatalogSession's "current path" context: relative paths (a leading `.`), -auto-inferred from usage, `set_context()` as an explicit seed. +set only via `use()`/`set_context()` — no other verb ever changes it, even +though most resolve their own `path` against it. -See src/eea_datalakehouse/catalog/session.py's `_resolve`/`_resolve_path` -and docs/notebook-facade-for-data-scientists.md ("Pre-filling catalog -context"). +See src/eea_datalakehouse/catalog/session.py's `use`/`set_context`/ +`_resolve_path` and docs/notebook-facade-for-data-scientists.md +("Pre-filling catalog context"). """ from __future__ import annotations @@ -31,30 +32,52 @@ def _catalog(rest: FakeCatalogRest, executor: FakeExecutor | None = None) -> Cat return Catalog(BASE_URL, "pat", executor=executor, flight_executor=executor, catalog_rest=rest) +def _session(rest: FakeCatalogRest, executor: FakeExecutor | None = None) -> CatalogSession: + """A `CatalogSession` with context cleared. This file's fixture paths + are short absolute stand-ins (e.g. `"bwd.table1"`) that predate + `CatalogSession`'s own default context (the catalog root, per + `_ROOT_SOURCE`) — clearing it keeps them absolute rather than + silently getting `"catalog."` prepended. Tests that exercise the + "no context set yet" error paths, or the new default itself, construct + a plain `CatalogSession(_catalog(rest))` directly instead.""" + session = CatalogSession(_catalog(rest, executor)) + session.set_context(None) + return session + + +def test_new_session_defaults_context_to_the_catalog_root() -> None: + # A freshly built session already has something to resolve a relative + # path against — the catalog root — rather than None; use()/ + # set_context(None) are still the only way to clear it explicitly. + rest = FakeCatalogRest(existing={"a"}) + session = CatalogSession(_catalog(rest)) + + assert session.get_context() == "catalog" + + def test_relative_path_without_any_context_raises() -> None: rest = FakeCatalogRest(existing={"a", "bwd", "bwd.table1"}) - session = CatalogSession(_catalog(rest)) + session = _session(rest) with pytest.raises(CatalogSessionError, match="no context is set"): session.set_tags(".table1", ["reviewed"]) -def test_touching_a_leaf_infers_context_for_a_later_relative_call() -> None: +def test_ordinary_verbs_never_change_or_infer_the_context() -> None: + # Only use()/set_context() may change context — set_tags below fully + # resolves an absolute path, but that never becomes the new context + # (unlike the old "ordinary use keeps context current" behaviour), so + # a later relative call still has nothing to resolve against. rest = FakeCatalogRest( existing={"a", "bwd", "bwd.reference.water_temperature", "bwd.reference.stations"} ) - session = CatalogSession(_catalog(rest)) + session = _session(rest) - session.set_tags("bwd.reference.water_temperature", ["reviewed"]) # -> bwd.reference - session.set_tags(".stations", ["reviewed"]) # resolves to bwd.reference.stations + session.set_tags("bwd.reference.water_temperature", ["reviewed"]) - report = session.commit() - - assert report.succeeded == [ - "set tags ['reviewed'] on 'bwd.reference.water_temperature'", - "set tags ['reviewed'] on 'bwd.reference.stations'", - ] - assert rest.get_tags("bwd.reference.stations") == ["reviewed"] + assert session.get_context() is None + with pytest.raises(CatalogSessionError, match="no context is set"): + session.set_tags(".stations", ["reviewed"]) def test_bare_path_starting_with_root_source_stays_absolute_even_with_context_set() -> None: @@ -148,49 +171,191 @@ def test_use_always_takes_a_bare_path_literally() -> None: assert session.get_context() == "bwd.other" -def test_use_none_clears_context_like_set_context() -> None: +def test_use_none_or_empty_resets_context_to_the_catalog_root() -> None: + # Unlike set_context(None) (still "no context at all"), use(None)/ + # use("") reset to _ROOT_SOURCE — the same starting point a freshly + # built session already has (see test_new_session_defaults_context_to_the_catalog_root). rest = FakeCatalogRest(existing={"a", "bwd", "bwd.reference", "bwd.other"}) session = CatalogSession(_catalog(rest)) session.use("bwd.reference") session.use(None) - assert session.get_context() is None + assert session.get_context() == "catalog" session.use("bwd.other") - assert session.get_context() == "bwd.other" + session.use("") + assert session.get_context() == "catalog" -def test_get_context_reflects_state_set_by_other_verbs() -> None: - rest = FakeCatalogRest(existing={"a", "bwd", "bwd.reference.water_temperature"}) + +def test_set_context_none_still_clears_context_entirely() -> None: + # set_context (the lower-level primitive, not normally called by a data + # custodian directly) keeps the old "no context at all" behaviour — + # only use()'s own None/"" handling changed. + rest = FakeCatalogRest(existing={"a", "bwd", "bwd.reference"}) session = CatalogSession(_catalog(rest)) + session.use("bwd.reference") + session.set_context(None) + assert session.get_context() is None - session.set_tags("bwd.reference.water_temperature", ["reviewed"]) # auto-updates context - assert session.get_context() == "bwd.reference" +def test_get_context_is_unaffected_by_other_verbs() -> None: + rest = FakeCatalogRest(existing={"a", "bwd", "bwd.reference.water_temperature"}) + session = _session(rest) + assert session.get_context() is None -def test_create_folder_context_becomes_the_folder_itself_not_its_parent() -> None: - rest = FakeCatalogRest(existing={"a", "bwd", "bwd.reference.water_temperature"}) + session.set_tags("bwd.reference.water_temperature", ["reviewed"]) + + assert session.get_context() is None + + +def test_create_folder_does_not_change_the_context() -> None: + # The exact case reported: create_folder used to leave its own path as + # the new context (a deliberate design choice at the time), which then + # surprised a custodian who only ever wanted that to come from use(). + rest = FakeCatalogRest(existing={"a", "bwd"}) + session = _session(rest) + + session.create_folder("bwd.reference.2027", create_parents=True) + + assert session.get_context() is None + + session.use("bwd") + session.create_folder(".reference") # relative to the context use() set + + assert session.get_context() == "bwd" # still what use() set — create_folder didn't touch it + + +def test_create_folder_and_delete_folder_resolve_a_bare_relative_path() -> None: + # Same dot-optional resolution as every other single-path verb: a bare + # path with no leading '.' still appends to the context once one exists. + rest = FakeCatalogRest(existing={"a", "bwd", "bwd.reference"}) session = CatalogSession(_catalog(rest)) - session.create_folder("bwd.reference.2027", create_parents=True) # context -> that folder - session.set_context("bwd.reference") # re-seed for the rest of this test - session.set_tags(".water_temperature", ["reviewed"]) + session.use("bwd.reference") + session.create_folder("2027") # bare, no dot — still bwd.reference.2027 + session.delete_folder("2027") report = session.commit() assert report.succeeded == [ "create folder 'bwd.reference.2027'", - "set tags ['reviewed'] on 'bwd.reference.water_temperature'", + "delete folder 'bwd.reference.2027'", ] -def test_data_copy_resolves_both_paths_against_the_same_starting_context() -> None: +def test_create_folder_and_delete_folder_keep_a_root_source_path_absolute() -> 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 (see test_bare_path_starting_with_root_source_ + # stays_absolute_even_with_context_set for the general rule). + rest = FakeCatalogRest(existing={"a", "bwd", "bwd.reference", "catalog", "catalog.other_root"}) + session = CatalogSession(_catalog(rest)) + + session.use("bwd.reference") + session.create_folder("catalog.other_root.new_folder") + session.delete_folder("catalog.other_root.new_folder") + + report = session.commit() + + assert report.succeeded == [ + "create folder 'catalog.other_root.new_folder'", + "delete folder 'catalog.other_root.new_folder'", + ] + + +def test_queued_write_verbs_resolve_relative_and_root_source_absolute_paths() -> None: + # set_wiki/delete_wiki/set_tags/delete_tags/delete_view/delete_table all + # share the exact same _resolve_path call as every other verb — a bare + # relative name appends to context, a catalog.-prefixed path stays + # absolute even with a context set. + rest = FakeCatalogRest( + existing={ + "a", + "bwd", + "bwd.reference", + "bwd.reference.table1", + "bwd.reference.view1", + "catalog", + "catalog.other_root", + "catalog.other_root.table1", + "catalog.other_root.view1", + } + ) + session = CatalogSession(_catalog(rest)) + + session.use("bwd.reference") + session.set_wiki("table1", "hello") # relative + session.set_wiki("catalog.other_root.table1", "hello, absolute") # absolute + session.delete_wiki("table1") + session.delete_wiki("catalog.other_root.table1") + session.set_tags("table1", ["x"]) + session.set_tags("catalog.other_root.table1", ["x-abs"]) + session.delete_tags("table1", ["x"]) + session.delete_tags("catalog.other_root.table1", ["x-abs"]) + session.delete_view("view1") + session.delete_view("catalog.other_root.view1") + session.delete_table("table1") + session.delete_table("catalog.other_root.table1") + + report = session.commit() + + assert report.succeeded == [ + "set wiki on 'bwd.reference.table1'", + "set wiki on 'catalog.other_root.table1'", + "delete wiki on 'bwd.reference.table1'", + "delete wiki on 'catalog.other_root.table1'", + "set tags ['x'] on 'bwd.reference.table1'", + "set tags ['x-abs'] on 'catalog.other_root.table1'", + "delete tags ['x'] from 'bwd.reference.table1'", + "delete tags ['x-abs'] from 'catalog.other_root.table1'", + "delete view 'bwd.reference.view1'", + "delete view 'catalog.other_root.view1'", + "delete table 'bwd.reference.table1'", + "delete table 'catalog.other_root.table1'", + ] + + +def test_read_only_verbs_resolve_relative_and_root_source_absolute_paths() -> None: + # get_wiki/get_tags aren't queued, but resolve their own path the same + # dot-optional, root-source-aware way as every write verb. rest = FakeCatalogRest( - existing={"a", "bwd", "bwd.draft.raw_2026", "bwd.reference", "bwd.reference.stations"} + existing={ + "a", + "bwd", + "bwd.reference", + "bwd.reference.table1", + "catalog", + "catalog.other_root", + "catalog.other_root.table1", + }, + wikis={"bwd.reference.table1": "hi", "catalog.other_root.table1": "hi, absolute"}, + tags={"bwd.reference.table1": ["x"], "catalog.other_root.table1": ["x-abs"]}, + ) + session = CatalogSession(_catalog(rest)) + + session.use("bwd.reference") + + assert session.get_wiki("table1") == "hi" # relative + assert session.get_wiki("catalog.other_root.table1") == "hi, absolute" # absolute + assert session.get_tags("table1") == ["x"] # relative + assert session.get_tags("catalog.other_root.table1") == ["x-abs"] # absolute + + +def test_data_copy_resolves_both_paths_against_the_current_context() -> None: + rest = FakeCatalogRest( + existing={ + "a", + "bwd", + "bwd.draft.raw_2026", + "catalog", + "catalog.other_root", + "catalog.other_root.water_temperature", + } ) executor = FakeExecutor( rows_sequence=[ @@ -201,24 +366,23 @@ def test_data_copy_resolves_both_paths_against_the_same_starting_context() -> No session = CatalogSession(_catalog(rest, executor)) session.set_context("bwd.draft") - session.data_copy(".raw_2026", "bwd.reference.water_temperature") # target NOT under bwd.draft - session.set_tags(".stations", ["reviewed"]) # resolves against the target's parent + session.data_copy(".raw_2026", "catalog.other_root.water_temperature") # NOT under bwd.draft report = session.commit() assert report.succeeded == [ - "data_copy 'bwd.draft.raw_2026' -> 'bwd.reference.water_temperature'", - "set tags ['reviewed'] on 'bwd.reference.stations'", + "data_copy 'bwd.draft.raw_2026' -> 'catalog.other_root.water_temperature'", ] + assert session.get_context() == "bwd.draft" # data_copy never changes it -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"}) +def test_data_copy_resolves_a_bare_relative_path_for_both_paths() -> None: + # data_copy/data_move/create_view are no longer an exception to the + # dot-optional rule — a bare path resolves relative to context here + # too, same as every other verb, since a genuinely absolute path in + # this deployment always starts with _ROOT_SOURCE and so is never + # ambiguous with a deliberately relative one in the same call. + rest = FakeCatalogRest(existing={"a", "bwd", "bwd.reference", "bwd.reference.raw_2026"}) executor = FakeExecutor( rows_sequence=[ [{"TABLE_NAME": "raw_2026"}], # source exists @@ -227,19 +391,26 @@ def test_data_copy_bare_target_path_stays_absolute_even_with_context_set() -> No ) session = CatalogSession(_catalog(rest, executor)) - session.set_context("bwd.draft") - session.data_copy(".raw_2026", "other.reference.water_temperature") # bare, unrelated root + session.use("bwd.reference") + session.data_copy("raw_2026", "archive") # bare, no dot — both resolve under bwd.reference report = session.commit() assert report.succeeded == [ - "data_copy 'bwd.draft.raw_2026' -> 'other.reference.water_temperature'", + "data_copy 'bwd.reference.raw_2026' -> 'bwd.reference.archive'", ] -def test_data_move_resolves_both_paths_against_the_same_starting_context() -> None: +def test_data_move_resolves_both_paths_against_the_current_context() -> None: rest = FakeCatalogRest( - existing={"a", "bwd", "bwd.draft.raw_2026", "bwd.reference", "bwd.reference.stations"} + existing={ + "a", + "bwd", + "bwd.draft.raw_2026", + "catalog", + "catalog.other_root", + "catalog.other_root.water_temperature", + } ) executor = FakeExecutor( rows_sequence=[ @@ -251,20 +422,47 @@ def test_data_move_resolves_both_paths_against_the_same_starting_context() -> No session = CatalogSession(_catalog(rest, executor)) session.set_context("bwd.draft") - session.data_move(".raw_2026", "bwd.reference.water_temperature") # target NOT under bwd.draft - session.set_tags(".stations", ["reviewed"]) # resolves against the target's parent + session.data_move(".raw_2026", "catalog.other_root.water_temperature") # NOT under bwd.draft + + report = session.commit() + + assert report.succeeded == [ + "data_move 'bwd.draft.raw_2026' -> 'catalog.other_root.water_temperature'", + ] + assert session.get_context() == "bwd.draft" # data_move never changes it + + +def test_data_move_resolves_a_bare_relative_path_for_both_paths() -> None: + rest = FakeCatalogRest(existing={"a", "bwd", "bwd.reference", "bwd.reference.raw_2026"}) + executor = FakeExecutor( + rows_sequence=[ + [{"TABLE_NAME": "raw_2026"}], # source exists + [], # target does not exist yet + [{"TABLE_NAME": "archive"}], # verify target exists after the move + ] + ) + session = CatalogSession(_catalog(rest, executor)) + + session.use("bwd.reference") + session.data_move("raw_2026", "archive") # bare, no dot — both resolve under bwd.reference report = session.commit() assert report.succeeded == [ - "data_move 'bwd.draft.raw_2026' -> 'bwd.reference.water_temperature'", - "set tags ['reviewed'] on 'bwd.reference.stations'", + "data_move 'bwd.reference.raw_2026' -> 'bwd.reference.archive'", ] -def test_create_view_resolves_both_paths_against_the_same_starting_context() -> None: +def test_create_view_resolves_both_paths_against_the_current_context() -> None: rest = FakeCatalogRest( - existing={"a", "bwd", "bwd.draft.raw_2026", "bwd.reference", "bwd.reference.stations"} + existing={ + "a", + "bwd", + "bwd.draft.raw_2026", + "catalog", + "catalog.other_root", + "catalog.other_root.water_temperature", + } ) executor = FakeExecutor( rows_sequence=[ @@ -275,40 +473,101 @@ def test_create_view_resolves_both_paths_against_the_same_starting_context() -> session = CatalogSession(_catalog(rest, executor)) session.set_context("bwd.draft") - session.create_view(".raw_2026", "bwd.reference.water_temperature") # target NOT bwd.draft - session.set_tags(".stations", ["reviewed"]) # resolves against the target's parent + session.create_view(".raw_2026", "catalog.other_root.water_temperature") # target NOT bwd.draft + + report = session.commit() + + assert report.succeeded == [ + "create view 'bwd.draft.raw_2026' -> 'catalog.other_root.water_temperature'", + ] + assert session.get_context() == "bwd.draft" # create_view never changes it + + +def test_create_view_resolves_a_bare_relative_path_for_both_paths() -> None: + rest = FakeCatalogRest(existing={"a", "bwd", "bwd.reference", "bwd.reference.raw_2026"}) + executor = FakeExecutor( + rows_sequence=[ + [{"TABLE_NAME": "raw_2026"}], # source exists + [], # target does not exist yet + ] + ) + session = CatalogSession(_catalog(rest, executor)) + + session.use("bwd.reference") + session.create_view("raw_2026", "archive") # bare, no dot — both resolve under bwd.reference report = session.commit() assert report.succeeded == [ - "create view 'bwd.draft.raw_2026' -> 'bwd.reference.water_temperature'", - "set tags ['reviewed'] on 'bwd.reference.stations'", + "create view 'bwd.reference.raw_2026' -> 'bwd.reference.archive'", ] +def test_bare_dot_resolves_to_the_context_itself() -> None: + # "." alone used to append a stray trailing "." to the context instead + # of meaning the context itself — get_wiki (a read-only verb, so the + # resolved path is directly observable rather than via a step + # description) proves the fix rather than just _resolve_path directly. + rest = FakeCatalogRest(existing={"a", "bwd", "bwd.reference"}, wikis={"bwd.reference": "hi"}) + session = CatalogSession(_catalog(rest)) + + session.use("bwd.reference") + + assert session.get_wiki(".") == "hi" + assert session.get_wiki("./") == "hi" + + +def test_bare_dot_with_no_context_yet_raises() -> None: + rest = FakeCatalogRest(existing={"a", "bwd"}) + session = _session(rest) + + with pytest.raises(CatalogSessionError, match="no context is set"): + session.get_wiki(".") + + +def test_dot_slash_name_means_the_same_as_dot_name() -> None: + rest = FakeCatalogRest( + existing={"a", "bwd", "bwd.reference", "bwd.reference.water_temperature"}, + wikis={"bwd.reference.water_temperature": "hi"}, + ) + session = CatalogSession(_catalog(rest)) + + session.use("bwd.reference") + + assert session.get_wiki("./water_temperature") == "hi" + + 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. + # It never changes context though, so the resolution is checked via + # the committed step's own description, not get_context(). rest = FakeCatalogRest(existing={"a", "bwd", "bwd.reference", "bwd.reference.2027"}) session = CatalogSession(_catalog(rest)) session.use("bwd.reference.2027") session.create_folder("..") - assert session.get_context() == "bwd.reference" + report = session.commit() + + assert report.succeeded == ["create folder 'bwd.reference'"] + assert session.get_context() == "bwd.reference.2027" # unchanged — still what use() set def test_dotdot_slash_name_resolves_to_a_sibling_of_the_context() -> None: rest = FakeCatalogRest( - existing={"a", "bwd", "bwd.reference.2027.stations", "bwd.reference.2027.water_temp"} + existing={"a", "bwd", "bwd.reference.2027", "bwd.reference.2027.stations"} ) session = CatalogSession(_catalog(rest)) session.use("bwd.reference.2027.stations") session.create_folder("../water_temp") # sibling of the current context - assert session.get_context() == "bwd.reference.2027.water_temp" + report = session.commit() + + assert report.succeeded == ["create folder 'bwd.reference.2027.water_temp'"] + assert session.get_context() == "bwd.reference.2027.stations" # unchanged def test_chained_dotdot_walks_up_multiple_levels() -> None: @@ -319,13 +578,15 @@ def test_chained_dotdot_walks_up_multiple_levels() -> None: session.use("bwd.reference.2027.stations") session.create_folder("../../../archive") # up three levels, then into "archive" + session.create_folder("../..") # up two levels, no name after it — still off the same context - assert session.get_context() == "bwd.archive" - - session.set_context("bwd.reference.2027.stations") # re-seed for the second half - session.create_folder("../..") # up two levels, no name after it + report = session.commit() - assert session.get_context() == "bwd.reference" + assert report.succeeded == [ + "create folder 'bwd.archive'", + "create folder 'bwd.reference'", + ] + assert session.get_context() == "bwd.reference.2027.stations" # unchanged throughout def test_dotdot_past_the_top_of_the_context_raises() -> None: @@ -340,7 +601,7 @@ def test_dotdot_past_the_top_of_the_context_raises() -> None: def test_dotdot_with_no_context_yet_raises() -> None: rest = FakeCatalogRest(existing={"a", "bwd"}) - session = CatalogSession(_catalog(rest)) + session = _session(rest) with pytest.raises(CatalogSessionError, match="no context is set"): session.create_folder("../reference") @@ -366,18 +627,7 @@ def test_dotdot_works_for_ordinary_verbs() -> None: report = session.commit() assert report.succeeded == ["set tags ['reviewed'] on 'bwd.reference.water_temperature'"] - - -def test_get_context_after_dotdot_is_the_full_resolved_path() -> None: - rest = FakeCatalogRest(existing={"a", "bwd", "bwd.reference", "bwd.reference.2027"}) - session = CatalogSession(_catalog(rest)) - - session.use("bwd.reference.2027") - session.create_folder("..") - - context = session.get_context() - assert context == "bwd.reference" - assert not context.startswith(".") # never a raw relative fragment + assert session.get_context() == "bwd.reference.2027" # set_tags never changes it def test_use_raises_when_the_resolved_path_does_not_exist() -> None: diff --git a/tests/catalog/test_session_queries.py b/tests/catalog/test_session_queries.py index 37eeaad..32fbe24 100644 --- a/tests/catalog/test_session_queries.py +++ b/tests/catalog/test_session_queries.py @@ -39,12 +39,23 @@ def _catalog(rest: FakeCatalogRest, executor: FakeExecutor | None = None) -> Cat return Catalog(BASE_URL, "pat", executor=executor, flight_executor=executor, catalog_rest=rest) +def _session(rest: FakeCatalogRest, executor: FakeExecutor | None = None) -> CatalogSession: + """A `CatalogSession` with context cleared. This file's fixture paths + are short absolute stand-ins (e.g. `"bwd.table1"`) that predate + `CatalogSession`'s own default context (the catalog root, per + `_ROOT_SOURCE`) — clearing it keeps them absolute rather than + silently getting `"catalog."` prepended.""" + session = CatalogSession(_catalog(rest, executor)) + session.set_context(None) + return session + + # -- get_wiki ----------------------------------------------------------------- def test_get_wiki_returns_the_wiki_text() -> None: rest = FakeCatalogRest(existing={"a", "bwd", "bwd.table1"}, wikis={"bwd.table1": "hello"}) - session = CatalogSession(_catalog(rest)) + session = _session(rest) assert session.get_wiki("bwd.table1") == "hello" @@ -54,7 +65,7 @@ def test_get_wiki_resolves_a_relative_path() -> None: existing={"a", "bwd", "bwd.reference", "bwd.reference.table1"}, wikis={"bwd.reference.table1": "hi"}, ) - session = CatalogSession(_catalog(rest)) + session = _session(rest) session.use("bwd.reference") assert session.get_wiki(".table1") == "hi" @@ -62,7 +73,7 @@ def test_get_wiki_resolves_a_relative_path() -> None: def test_get_wiki_raises_when_path_does_not_exist() -> None: rest = FakeCatalogRest(existing={"a", "bwd"}) - session = CatalogSession(_catalog(rest)) + session = _session(rest) with pytest.raises(CatalogOperationError, match="does not exist"): session.get_wiki("bwd.missing") @@ -70,7 +81,7 @@ def test_get_wiki_raises_when_path_does_not_exist() -> None: def test_get_wiki_does_not_queue_anything() -> None: rest = FakeCatalogRest(existing={"a", "bwd", "bwd.table1"}, wikis={"bwd.table1": "hi"}) - session = CatalogSession(_catalog(rest)) + session = _session(rest) session.get_wiki("bwd.table1") @@ -82,14 +93,14 @@ def test_get_wiki_does_not_queue_anything() -> None: def test_get_tags_returns_the_tags() -> None: rest = FakeCatalogRest(existing={"a", "bwd", "bwd.table1"}, tags={"bwd.table1": ["reviewed"]}) - session = CatalogSession(_catalog(rest)) + session = _session(rest) assert session.get_tags("bwd.table1") == ["reviewed"] def test_get_tags_raises_when_path_does_not_exist() -> None: rest = FakeCatalogRest(existing={"a", "bwd"}) - session = CatalogSession(_catalog(rest)) + session = _session(rest) with pytest.raises(CatalogOperationError, match="does not exist"): session.get_tags("bwd.missing") @@ -97,7 +108,7 @@ def test_get_tags_raises_when_path_does_not_exist() -> None: def test_get_tags_raises_on_a_folder() -> None: rest = FakeCatalogRest(existing={"a", "bwd", "bwd.folder1"}, folders={"bwd.folder1"}) - session = CatalogSession(_catalog(rest)) + session = _session(rest) with pytest.raises(CatalogOperationError, match="only works on tables/views"): session.get_tags("bwd.folder1") @@ -111,7 +122,7 @@ def test_list_returns_full_paths() -> None: executor = FakeExecutor( rows=[{"TABLE_SCHEMA": "bwd.reference", "TABLE_NAME": "water_temperature"}] ) - session = CatalogSession(_catalog(rest, executor)) + session = _session(rest, executor) assert session.list("bwd.reference") == ["bwd.reference.water_temperature"] @@ -119,7 +130,7 @@ def test_list_returns_full_paths() -> None: def test_list_resolves_a_relative_path() -> None: rest = FakeCatalogRest(existing={"a", "bwd", "bwd.reference"}) executor = FakeExecutor(rows=[]) - session = CatalogSession(_catalog(rest, executor)) + session = _session(rest, executor) session.use("bwd") session.list(".reference") # must not raise — "bwd.reference" exists @@ -134,7 +145,7 @@ def test_list_resolves_a_bare_relative_path_too() -> None: # one exception — see test_session_context.py). rest = FakeCatalogRest(existing={"a", "bwd", "bwd.reference"}) executor = FakeExecutor(rows=[]) - session = CatalogSession(_catalog(rest, executor)) + session = _session(rest, executor) session.use("bwd") session.list("reference") # bare, no dot — still resolves to "bwd.reference" @@ -147,7 +158,7 @@ def test_list_with_no_path_lists_the_context_itself() -> None: executor = FakeExecutor( rows=[{"TABLE_SCHEMA": "bwd.reference", "TABLE_NAME": "water_temperature"}] ) - session = CatalogSession(_catalog(rest, executor)) + session = _session(rest, executor) session.use("bwd.reference") assert session.list() == ["bwd.reference.water_temperature"] @@ -156,7 +167,7 @@ def test_list_with_no_path_lists_the_context_itself() -> None: 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 = _session(rest, executor) session.use("bwd.reference") session.list("") # explicit empty string — same as omitting path @@ -166,7 +177,7 @@ def test_list_with_empty_path_lists_the_context_itself() -> None: def test_list_with_no_path_and_no_context_raises() -> None: rest = FakeCatalogRest(existing={"a", "bwd"}) - session = CatalogSession(_catalog(rest)) + session = _session(rest) with pytest.raises(CatalogSessionError, match="no context is set"): session.list() @@ -177,7 +188,7 @@ def test_list_absolute_path_still_works_with_no_context() -> None: executor = FakeExecutor( rows=[{"TABLE_SCHEMA": "bwd.reference", "TABLE_NAME": "water_temperature"}] ) - session = CatalogSession(_catalog(rest, executor)) + session = _session(rest, executor) assert session.list("bwd.reference") == ["bwd.reference.water_temperature"] @@ -186,7 +197,7 @@ 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". rest = FakeCatalogRest(existing={"a", "bwd"}) - session = CatalogSession(_catalog(rest)) + session = _session(rest) with pytest.raises(CatalogOperationError, match="does not exist"): session.list("bwd.missing") @@ -200,7 +211,7 @@ def test_schema_returns_table_info() -> None: executor = FakeExecutor( rows_sequence=[[{"COLUMN_NAME": "id", "DATA_TYPE": "INTEGER"}], [{"row_count": 5}]] ) - session = CatalogSession(_catalog(rest, executor)) + session = _session(rest, executor) assert session.schema("bwd.table1") == TableInfo(schema={"id": "INTEGER"}, row_count=5) @@ -208,7 +219,7 @@ def test_schema_returns_table_info() -> None: def test_schema_raises_on_a_folder() -> None: # The explicit requirement: schema() only works on a table or view. rest = FakeCatalogRest(existing={"a", "bwd", "bwd.folder1"}, folders={"bwd.folder1"}) - session = CatalogSession(_catalog(rest)) + session = _session(rest) with pytest.raises(CatalogOperationError, match="only works on tables/views"): session.schema("bwd.folder1") @@ -216,7 +227,7 @@ def test_schema_raises_on_a_folder() -> None: def test_schema_raises_when_path_does_not_exist() -> None: rest = FakeCatalogRest(existing={"a", "bwd"}) - session = CatalogSession(_catalog(rest)) + session = _session(rest) with pytest.raises(CatalogOperationError, match="does not exist"): session.schema("bwd.missing") @@ -228,7 +239,7 @@ def test_schema_raises_when_path_does_not_exist() -> None: def test_delete_view_drops_the_view() -> None: rest = FakeCatalogRest(existing={"a", "bwd", "bwd.view1"}) executor = FakeExecutor() - session = CatalogSession(_catalog(rest, executor)) + session = _session(rest, executor) session.delete_view("bwd.view1") report = session.commit() @@ -240,7 +251,7 @@ def test_delete_view_drops_the_view() -> None: def test_delete_view_resolves_a_relative_path() -> None: rest = FakeCatalogRest(existing={"a", "bwd", "bwd.reference", "bwd.reference.view1"}) executor = FakeExecutor() - session = CatalogSession(_catalog(rest, executor)) + session = _session(rest, executor) session.use("bwd.reference") session.delete_view(".view1") @@ -253,7 +264,7 @@ def test_delete_view_raises_when_path_does_not_exist() -> None: # Unlike the raw Catalog.deleteview (DROP VIEW IF EXISTS — a no-op on a # missing path), CatalogSession.delete_view requires it to be real. rest = FakeCatalogRest(existing={"a", "bwd"}) - session = CatalogSession(_catalog(rest)) + session = _session(rest) session.delete_view("bwd.missing") with pytest.raises(CatalogCommitError, match="does not exist"): @@ -262,7 +273,7 @@ def test_delete_view_raises_when_path_does_not_exist() -> None: def test_delete_view_is_never_reversible() -> None: rest = FakeCatalogRest(existing={"a", "bwd", "bwd.view1"}) - session = CatalogSession(_catalog(rest)) + session = _session(rest) session.delete_view("bwd.view1") # succeeds, but not reversible session.set_tags("bwd.missing", ["x"]) # fails: path does not exist @@ -282,7 +293,7 @@ def test_delete_view_is_never_reversible() -> None: def test_delete_table_drops_the_table() -> None: rest = FakeCatalogRest(existing={"a", "bwd", "bwd.table1"}) executor = FakeExecutor() - session = CatalogSession(_catalog(rest, executor)) + session = _session(rest, executor) session.delete_table("bwd.table1") report = session.commit() @@ -293,7 +304,7 @@ def test_delete_table_drops_the_table() -> None: def test_delete_table_raises_when_path_does_not_exist() -> None: rest = FakeCatalogRest(existing={"a", "bwd"}) - session = CatalogSession(_catalog(rest)) + session = _session(rest) session.delete_table("bwd.missing") with pytest.raises(CatalogCommitError, match="does not exist"): @@ -302,7 +313,7 @@ def test_delete_table_raises_when_path_does_not_exist() -> None: def test_delete_table_is_never_reversible() -> None: rest = FakeCatalogRest(existing={"a", "bwd", "bwd.table1"}) - session = CatalogSession(_catalog(rest)) + session = _session(rest) session.delete_table("bwd.table1") # succeeds, but not reversible session.set_tags("bwd.missing", ["x"]) # fails: path does not exist diff --git a/tests/notebook/test_magics.py b/tests/notebook/test_magics.py index bb5490a..6e4dbb3 100644 --- a/tests/notebook/test_magics.py +++ b/tests/notebook/test_magics.py @@ -17,31 +17,53 @@ from eea_datalakehouse.notebook.magics import EEALakehouseMagics +class _FakeCommitReport: + """Mirrors the real `CommitReport`'s one field `_dispatch` inspects.""" + + def __init__(self, succeeded: list[str]) -> None: + self.succeeded = succeeded + + class _FakeCatalogSession: """Stands in for a real `CatalogSession` — records calls, never touches - a real Catalog.""" + a real Catalog. `use()` doesn't queue anything (matching the real + class), so `commit()`'s report only ever reflects `data_copy`/ + `set_tags`/... calls made since the last commit.""" def __init__(self) -> None: self.calls: list[str] = [] self.committed = False self.commit_kwargs: dict[str, Any] | None = None + self._pending: list[str] = [] + self._context: str | None = None def data_copy(self, *args: Any, **kwargs: Any) -> _FakeCatalogSession: - self.calls.append(f"data_copy{args!r}{kwargs!r}") + call = f"data_copy{args!r}{kwargs!r}" + self.calls.append(call) + self._pending.append(call) return self - def use(self, path: str) -> _FakeCatalogSession: + def use(self, path: str | None) -> _FakeCatalogSession: self.calls.append(f"use({path!r})") + # Matches the real CatalogSession: None/"" reset to the catalog + # root rather than clearing to no context at all. + self._context = "catalog" if path is None or path == "" else path return self + def get_context(self) -> str | None: + return self._context + def set_tags(self, *args: Any, **kwargs: Any) -> _FakeCatalogSession: - self.calls.append(f"set_tags{args!r}{kwargs!r}") + call = f"set_tags{args!r}{kwargs!r}" + self.calls.append(call) + self._pending.append(call) return self - def commit(self, **kwargs: Any) -> str: + def commit(self, **kwargs: Any) -> _FakeCommitReport: self.committed = True self.commit_kwargs = kwargs - return "committed" + succeeded, self._pending = self._pending, [] + return _FakeCommitReport(succeeded) def raise_commit_error(self) -> None: raise CatalogCommitError( @@ -87,6 +109,33 @@ def test_catalog_magic_executes_and_commits_immediately( assert fake.commit_kwargs == {"retry": True} +def test_use_prints_the_resulting_context_not_an_empty_commit_report( + ip: Any, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + # use() chains back to `self` like every queueing verb, but queues + # nothing — commit() then has nothing to report, so the context it + # just set is printed instead of an uninformative empty CommitReport. + fake = _FakeCatalogSession() + monkeypatch.setattr(magics_module, "_build_catalog_session", lambda: fake) + + result = ip.run_line_magic("catalog", 'use("bwd.reference")') + + assert result is None + assert capsys.readouterr().out == "context set to 'bwd.reference'\n" + assert fake.committed is True # still committed — a harmless no-op, nothing was queued + + +def test_use_none_prints_context_set_to_the_catalog_root( + ip: Any, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + fake = _FakeCatalogSession() + monkeypatch.setattr(magics_module, "_build_catalog_session", lambda: fake) + + ip.run_line_magic("catalog", "use(None)") + + assert capsys.readouterr().out == "context set to 'catalog'\n" + + def test_catalog_magic_builds_the_session_once_and_reuses_it( ip: Any, monkeypatch: pytest.MonkeyPatch ) -> None: @@ -180,6 +229,21 @@ def test_catalog_cell_magic_stops_at_the_first_failing_line( assert fake.calls == [] # the second line never ran +def test_catalog_cell_magic_does_not_stop_after_use_none( + ip: Any, monkeypatch: pytest.MonkeyPatch +) -> None: + # use(None) legitimately dispatches to a `None` result (it prints its + # own status instead of an empty commit report) — the cell loop must + # not mistake that for "an error was printed" and bail out before + # running the rest of the cell. + fake = _FakeCatalogSession() + monkeypatch.setattr(magics_module, "_build_catalog_session", lambda: fake) + + ip.run_cell_magic("catalog", "use(None)", 'set_tags("a.b", ["reviewed"])') + + assert fake.calls == ["use(None)", "set_tags('a.b', ['reviewed']){}"] + + def test_catalog_cell_magic_missing_credentials_prints_a_friendly_message( ip: Any, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] ) -> None: @@ -230,15 +294,16 @@ def test_catalog_help_lists_methods_without_needing_credentials( out = capsys.readouterr().out assert "%catalog methods" in out assert "DREMIO_BASE_URL" not in out # never tried to build a session - # 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 + # General note about absolute/relative paths — printed once, not per-row, + # since it applies across every path/source_path/target_path. + assert "is either absolute or" 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 + assert "the context itself, exactly as get_context() shows it" in out # '.'/'./' alone + # 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 "starts with 'catalog.'" in out + assert "the dot is optional sugar, not what makes it relative" in out assert len(displayed) == 1 table_html = displayed[0].data