From c1149d5c138d59e70c219880430162b2e9389694 Mon Sep 17 00:00:00 2001 From: blikij Date: Fri, 16 Oct 2026 19:45:53 +0200 Subject: [PATCH 01/19] feat(dds_ingestion): FolderIngest.diagnose() (PY-7) Checks, in plain language and without starting anything, whether a transfer would work: DDS reachable (GET /health); the credential accepted (GET /ingest/limits, or the transfer list on a server predating it); DDS's own S3 write test (POST /storage-selftest); and a dry run of this exact folder, which covers the privileges on the target, the target folder, collisions and the table definition. A check that cannot run is reported as skipped with the reason; an older server's dry-run session is cancelled. Returns a Diagnosis (one DiagnosisStep per check) and prints each when show_progress is on. IngestClient gains health(), limits() and storage_selftest(). Additive; 97 tests pass (+5); mypy has no new errors. Co-Authored-By: Claude Opus 5.5 --- src/eea_datalakehouse/dds_ingestion/client.py | 25 +++ src/eea_datalakehouse/dds_ingestion/folder.py | 84 +++++++++- tests/dds_ingestion/test_diagnose.py | 147 ++++++++++++++++++ 3 files changed, 255 insertions(+), 1 deletion(-) create mode 100644 tests/dds_ingestion/test_diagnose.py diff --git a/src/eea_datalakehouse/dds_ingestion/client.py b/src/eea_datalakehouse/dds_ingestion/client.py index 1d84b91..9f8ba89 100644 --- a/src/eea_datalakehouse/dds_ingestion/client.py +++ b/src/eea_datalakehouse/dds_ingestion/client.py @@ -353,6 +353,31 @@ def get_status(self, session_id: str) -> StatusResult: self._raise_for_status(resp, f"GET /api/v1/ingest/{session_id}") return StatusResult.from_json(resp.json()) + def health(self) -> None: + """Raise unless DDS answers its liveness probe (``GET /health``, no login).""" + resp = self._http.get(f"{self._base_url}/health") + self._raise_for_status(resp, "GET /health") + + def limits(self, runner: str = "notebook") -> dict[str, Any]: + """The ingest size limits for this kind of client (``GET /api/v1/ingest/limits``, + contract v0.8). Needs a login, so it also proves the credential works.""" + resp = self._http.get( + f"{self._base_url}/api/v1/ingest/limits", + params={"runner": runner}, + headers=self._auth_headers, + ) + self._raise_for_status(resp, "GET /api/v1/ingest/limits") + result: dict[str, Any] = resp.json() + return result + + def storage_selftest(self) -> dict[str, Any]: + """Re-run DDS's S3 write test and return its verdict (``POST /storage-selftest``, + no login). ``storage_write_test.ok`` says whether ingest can write.""" + resp = self._http.post(f"{self._base_url}/storage-selftest") + self._raise_for_status(resp, "POST /storage-selftest") + result: dict[str, Any] = resp.json() + return result + def list_sessions(self, state: str | None = None) -> list[StatusResult]: """Your ingest sessions (``GET /api/v1/ingest``). diff --git a/src/eea_datalakehouse/dds_ingestion/folder.py b/src/eea_datalakehouse/dds_ingestion/folder.py index 9da0e49..0f8855f 100644 --- a/src/eea_datalakehouse/dds_ingestion/folder.py +++ b/src/eea_datalakehouse/dds_ingestion/folder.py @@ -26,7 +26,7 @@ import httpx import pyarrow as pa -from .client import IngestApiError, IngestClient +from .client import IngestApiError, IngestClient, UnsupportedByServerError from .credentials import DremioCreds, load_base_url, load_creds from .formats import FORMATS, compare_columns, local_columns from .models import ( @@ -59,6 +59,27 @@ class IngestStateError(RuntimeError): """ +@dataclass(frozen=True, slots=True) +class DiagnosisStep: + """One check of :meth:`FolderIngest.diagnose`; ``ok`` is ``None`` when skipped.""" + + name: str + ok: bool | None + detail: str + + +@dataclass(slots=True) +class Diagnosis: + """What :meth:`FolderIngest.diagnose` found, one step per check, in order.""" + + steps: list[DiagnosisStep] = field(default_factory=list) + + @property + def ok(self) -> bool: + """Whether every check that ran passed.""" + return all(step.ok is not False for step in self.steps) + + @dataclass(slots=True) class IngestOutcome: """Summary returned by :meth:`FolderIngest.run` and :meth:`FolderIngest.retry`. @@ -401,6 +422,67 @@ def _check_parquet_columns(self, rel_paths: list[str], given: list[Mapping[str, err.retryable = False raise err + def diagnose(self) -> Diagnosis: + """Check, in plain language, whether this transfer would work — without + starting it (PY-7). Nothing is uploaded and no session is created. + + In order: DDS reachable; the credential accepted; DDS's own S3 write test; + and a dry run of this exact folder, which covers the privileges on the + target, the target folder, collisions and the table definition. A check + that cannot run (DDS unreachable, a server predating it) is reported as + skipped, with the reason, instead of failing the ones after it. + """ + result = Diagnosis() + + def add(name: str, ok: bool | None, detail: str) -> None: + result.steps.append(DiagnosisStep(name, ok, detail)) + if self.show_progress: + mark = {True: "✓", False: "✗", None: "–"}[ok] + print(f"{mark} {name}: {detail}") + + try: + self._client.health() + except (httpx.HTTPError, IngestApiError) as exc: + add("DDS reachable", False, str(exc)) + for name in ("Credential accepted", "Storage write test", "Dry run of this folder"): + add(name, None, "skipped: DDS is not reachable") + return result + add("DDS reachable", True, "the service answers") + + try: + self._client.limits("notebook") + add("Credential accepted", True, "DDS accepts your Dremio credential") + except IngestApiError as exc: + if exc.status_code == 404: # a server predating GET /ingest/limits + try: + self._client.list_sessions() + add("Credential accepted", True, "DDS accepts your Dremio credential") + except IngestApiError as inner: + add("Credential accepted", False, inner.message) + else: + add("Credential accepted", False, exc.message) + + try: + verdict = self._client.storage_selftest().get("storage_write_test") or {} + if verdict.get("ok"): + add("Storage write test", True, "DDS can write to its S3 storage") + elif verdict: + failures = "; ".join(str(f) for f in verdict.get("failures") or []) or "failed" + add("Storage write test", False, failures) + else: + add("Storage write test", None, "skipped: no verdict reported yet") + except IngestApiError as exc: + add("Storage write test", None, f"skipped: {exc.message}") + + try: + dry = self.dry_run() + add("Dry run of this folder", True, dry.plan.describe()) + except UnsupportedByServerError: + add("Dry run of this folder", None, "skipped: this DDS server has no dry run") + except (IngestApiError, FileNotFoundError, ValueError) as exc: + add("Dry run of this folder", False, str(getattr(exc, "message", exc))) + return result + def _report_plan(self, plan: BeginPlan) -> None: """Show what commit will do: printed when ``show_progress`` (the same switch as the progress bar), always logged; the parts of the table diff --git a/tests/dds_ingestion/test_diagnose.py b/tests/dds_ingestion/test_diagnose.py new file mode 100644 index 0000000..1d20fa9 --- /dev/null +++ b/tests/dds_ingestion/test_diagnose.py @@ -0,0 +1,147 @@ +"""PY-7: FolderIngest.diagnose(), through the real IngestClient over mocked HTTP.""" + +from __future__ import annotations + +from pathlib import Path + +import httpx +import respx +from eea_datalakehouse.dds_ingestion.client import IngestClient +from eea_datalakehouse.dds_ingestion.credentials import DremioCreds +from eea_datalakehouse.dds_ingestion.folder import Diagnosis, FolderIngest + +from .conftest import BASE_URL + +PLAN = { + "placement": "staged", + "table_path": "bio/uploads/t", + "storage_path": None, + "action": "create", + "definition_source": "parquet_footer", +} +DRY_RUN = { + "session_id": None, + "status": "dry_run", + "s3": {"bucket": "b", "key_prefix": "p/", "uploads": []}, + "plan": PLAN, +} + + +def _diagnose(folder: Path, creds: DremioCreds) -> Diagnosis: + with IngestClient(BASE_URL, creds) as client: + job = FolderIngest( + folder, "bio/uploads", data_format="parquet", show_progress=False, client=client + ) + return job.diagnose() + + +def _summary(result: Diagnosis) -> list[tuple[str, bool | None]]: + return [(step.name, step.ok) for step in result.steps] + + +@respx.mock +def test_everything_green(data_folder: Path, creds: DremioCreds) -> None: + respx.get(f"{BASE_URL}/health").mock(return_value=httpx.Response(200, json={"status": "ok"})) + respx.get(f"{BASE_URL}/api/v1/ingest/limits").mock( + return_value=httpx.Response(200, json={"max_bytes": 1}) + ) + respx.post(f"{BASE_URL}/storage-selftest").mock( + return_value=httpx.Response(200, json={"storage_write_test": {"ok": True}}) + ) + respx.post(f"{BASE_URL}/api/v1/ingest/begin").mock( + return_value=httpx.Response(200, json=DRY_RUN) + ) + + result = _diagnose(data_folder, creds) + + assert result.ok + assert _summary(result) == [ + ("DDS reachable", True), + ("Credential accepted", True), + ("Storage write test", True), + ("Dry run of this folder", True), + ] + assert result.steps[3].detail == "Importing as a new editable table at bio/uploads/t." + + +@respx.mock +def test_an_unreachable_dds_skips_the_rest(data_folder: Path, creds: DremioCreds) -> None: + respx.get(f"{BASE_URL}/health").mock(side_effect=httpx.ConnectError("refused")) + result = _diagnose(data_folder, creds) + assert not result.ok + assert _summary(result) == [ + ("DDS reachable", False), + ("Credential accepted", None), + ("Storage write test", None), + ("Dry run of this folder", None), + ] + + +@respx.mock +def test_a_rejected_credential(data_folder: Path, creds: DremioCreds) -> None: + respx.get(f"{BASE_URL}/health").mock(return_value=httpx.Response(200)) + respx.get(f"{BASE_URL}/api/v1/ingest/limits").mock( + return_value=httpx.Response(401, json={"error": "unauthorized", "message": "bad PAT"}) + ) + respx.post(f"{BASE_URL}/storage-selftest").mock( + return_value=httpx.Response(200, json={"storage_write_test": {"ok": True}}) + ) + respx.post(f"{BASE_URL}/api/v1/ingest/begin").mock( + return_value=httpx.Response(401, json={"error": "unauthorized", "message": "bad PAT"}) + ) + result = _diagnose(data_folder, creds) + assert dict(_summary(result))["Credential accepted"] is False + assert result.steps[1].detail == "bad PAT" + + +@respx.mock +def test_an_older_server(data_folder: Path, creds: DremioCreds) -> None: + respx.get(f"{BASE_URL}/health").mock(return_value=httpx.Response(200)) + respx.get(f"{BASE_URL}/api/v1/ingest/limits").mock(return_value=httpx.Response(404)) + respx.get(f"{BASE_URL}/api/v1/ingest").mock(return_value=httpx.Response(200, json=[])) + respx.post(f"{BASE_URL}/storage-selftest").mock(return_value=httpx.Response(404)) + respx.post(f"{BASE_URL}/api/v1/ingest/begin").mock( + return_value=httpx.Response( + 200, + json={ + "session_id": "s1", + "status": "uploading", + "s3": {"bucket": "b", "key_prefix": "p/"}, + }, + ) + ) + cancel = respx.delete(f"{BASE_URL}/api/v1/ingest/s1").mock(return_value=httpx.Response(204)) + + result = _diagnose(data_folder, creds) + + assert result.ok # nothing failed; two checks could not run + assert _summary(result) == [ + ("DDS reachable", True), + ("Credential accepted", True), + ("Storage write test", None), + ("Dry run of this folder", None), + ] + assert cancel.called # the session the old server opened was not left behind + + +@respx.mock +def test_no_write_privilege_on_the_target(data_folder: Path, creds: DremioCreds) -> None: + respx.get(f"{BASE_URL}/health").mock(return_value=httpx.Response(200)) + respx.get(f"{BASE_URL}/api/v1/ingest/limits").mock(return_value=httpx.Response(200, json={})) + respx.post(f"{BASE_URL}/storage-selftest").mock( + return_value=httpx.Response( + 200, + json={"storage_write_test": {"ok": False, "failures": ["catalog/upload: HTTP 501"]}}, + ) + ) + respx.post(f"{BASE_URL}/api/v1/ingest/begin").mock( + return_value=httpx.Response( + 403, json={"error": "forbidden", "message": "no WRITE privilege on bio/uploads"} + ) + ) + result = _diagnose(data_folder, creds) + assert not result.ok + steps = {step.name: step for step in result.steps} + assert steps["Storage write test"].ok is False + assert "HTTP 501" in steps["Storage write test"].detail + assert steps["Dry run of this folder"].detail == "no WRITE privilege on bio/uploads" From 60cad5466ff0edc3a1813ade348f42ffa68ea593 Mon Sep 17 00:00:00 2001 From: blikij Date: Mon, 19 Oct 2026 11:21:43 +0200 Subject: [PATCH 02/19] fix(dds_ingestion): restore the upload_mode handling the main merge dropped 101b09c, merging eea_bliki into main, dropped part of 774691a: BeginResult.from_json raised NameError on every begin, and FolderIngest(upload_mode="proxy") reported a proxy upload while it uploaded to S3. Restore 774691a as written: BeginResult.upload_mode is None when the server sends none and is read from the top level or from inside s3, and _upload_one(target, session_id, proxy) sends a proxied file through DDS. PY-1's network-error fallback stays, as an addition: only when both FolderIngest and the server say "auto" (_falls_back_to_proxy). PY-1's own test now expects None for an absent hint, as 774691a does. Co-Authored-By: Claude Opus 5.5 --- src/eea_datalakehouse/dds_ingestion/folder.py | 50 ++++++++++++------- src/eea_datalakehouse/dds_ingestion/models.py | 13 ++--- .../dds_ingestion/test_folder_upload_modes.py | 4 +- 3 files changed, 42 insertions(+), 25 deletions(-) diff --git a/src/eea_datalakehouse/dds_ingestion/folder.py b/src/eea_datalakehouse/dds_ingestion/folder.py index 0f8855f..89678de 100644 --- a/src/eea_datalakehouse/dds_ingestion/folder.py +++ b/src/eea_datalakehouse/dds_ingestion/folder.py @@ -41,6 +41,7 @@ Intent, StatusResult, TableDefinition, + UploadMode, UploadTarget, definition_payload, ) @@ -304,6 +305,12 @@ def _uses_proxy(self, begin: BeginResult) -> bool: return begin.upload_mode == "proxy" return self.upload_mode == "proxy" + def _falls_back_to_proxy(self, begin: BeginResult) -> bool: + """Whether a presigned upload that fails at the network layer is sent + through DDS instead: only with ``upload_mode="auto"`` (follow the + server) and when ``begin`` says ``upload_mode: "auto"``.""" + return self.upload_mode == "auto" and begin.upload_mode == "auto" + def begin(self) -> BeginResult: """Open the transfer and get the presigned upload targets (step 1 of 3). @@ -690,6 +697,7 @@ def _upload_all( ) -> tuple[dict[str, str], list[dict[str, object]], int, int]: targets = list(begin.s3.uploads) proxy = self._uses_proxy(begin) + fallback = self._falls_back_to_proxy(begin) if proxy: logger.info("uploading through DDS (proxy), not to the presigned S3 targets") done = self._already_done(begin.session_id) @@ -710,7 +718,8 @@ def _upload_all( return etags, multipart_etags, 0, skipped with ThreadPoolExecutor(max_workers=self.parallelism) as pool: futures = { - pool.submit(self._upload_one, t, begin): t for t in pending + pool.submit(self._upload_one, t, begin.session_id, proxy, fallback): t + for t in pending } try: for future in as_completed(futures): @@ -741,32 +750,39 @@ def _upload_all( return etags, multipart_etags, len(pending), skipped def _upload_one( - self, target: UploadTarget, begin: BeginResult + self, + target: UploadTarget, + session_id: str, + proxy: bool = False, + fallback: bool = False, ) -> tuple[str | None, list[dict[str, object]] | None]: """Upload one file; return ``(single_etag, multipart_parts)``. - At most one element is non-``None``: a single-shot upload yields the ETag - (or ``None`` if S3 omitted it); a multipart upload yields the per-part - ETag list; a file sent through DDS (``/stage``) yields neither. - - Honours the server's ``upload_mode`` (contract upload-mode rule, the same - one the JupyterLab extension implements): ``proxy`` always goes through - DDS; ``direct`` uses the presigned target only; ``auto`` uses it and, if - it fails at the **network** layer (unreachable endpoint, DNS, TLS, - timeout), sends the file through DDS instead. An HTTP error from S3 is - not retried through DDS: that is a signing or permission problem the - proxy would not fix. + At most one element is non-``None``: a single-shot upload yields the + ETag (or ``None`` if S3 omitted it); a multipart upload yields the + per-part ETag list and the single ETag is ``None``. A proxied upload + (`proxy`) sends the whole file through DDS — DDS writes it to S3 with + its own connection, so this process never contacts the storage — and + yields neither: there are no S3 ETags to hand to commit. + + With `fallback` (see `_falls_back_to_proxy`), a presigned upload that + fails at the **network** layer (unreachable endpoint, DNS, TLS, + timeout) is sent through DDS instead — the contract upload-mode rule + the JupyterLab extension implements. An HTTP error from S3 is not + retried through DDS: that is a signing or permission problem the proxy + would not fix. """ data = (self.folder / target.rel_path).read_bytes() - if begin.upload_mode == "proxy": - return self._stage(begin.session_id, target, data) + if proxy: + self._client.stage(session_id, target.rel_path, data) + return None, None try: if target.is_multipart: parts = self._client.upload_file_multipart(target, data) return None, [dict(p) for p in parts] return self._client.upload_file(target, data), None except httpx.TransportError as exc: - if begin.upload_mode != "auto": + if not fallback: raise logger.warning( "presigned upload of %s failed at the network layer (%s); " @@ -774,7 +790,7 @@ def _upload_one( target.rel_path, exc, ) - return self._stage(begin.session_id, target, data) + return self._stage(session_id, target, data) def _stage( self, session_id: str, target: UploadTarget, data: bytes diff --git a/src/eea_datalakehouse/dds_ingestion/models.py b/src/eea_datalakehouse/dds_ingestion/models.py index 7cc5904..43765b0 100644 --- a/src/eea_datalakehouse/dds_ingestion/models.py +++ b/src/eea_datalakehouse/dds_ingestion/models.py @@ -215,11 +215,10 @@ class BeginResult: status: str s3: S3Plan collision: Any = None - # How the server wants the bytes sent (DDS_INGEST_UPLOAD_MODE): ``direct`` — - # presigned only; ``proxy`` — through ``POST /ingest/stage``; ``auto`` — - # presigned, then the proxy on a network failure. Absent on older servers, - # which never offered a choice: ``direct``. - upload_mode: str = "direct" + # The values a server sends (DDS_INGEST_UPLOAD_MODE): ``direct`` — presigned + # only; ``proxy`` — through ``POST /ingest/stage``; ``auto`` — presigned, + # then the proxy on a network failure (see `FolderIngest._upload_one`). + upload_mode: str | None = None # What commit will do (DI-12.2). ``None`` from a server predating it — which # is also how a client knows that server ignored a definition sent on begin. plan: BeginPlan | None = None @@ -227,12 +226,14 @@ class BeginResult: @classmethod def from_json(cls, data: dict[str, Any]) -> BeginResult: plan = data.get("plan") + s3 = data["s3"] + mode = data.get("upload_mode") or (s3.get("upload_mode") if isinstance(s3, dict) else None) return cls( session_id=data["session_id"], status=data["status"], s3=S3Plan.from_json(s3), collision=data.get("collision"), - upload_mode=str(data.get("upload_mode") or "direct"), + upload_mode=str(mode) if mode else None, plan=BeginPlan.from_json(plan) if isinstance(plan, Mapping) else None, ) diff --git a/tests/dds_ingestion/test_folder_upload_modes.py b/tests/dds_ingestion/test_folder_upload_modes.py index 077aeeb..cc51fe3 100644 --- a/tests/dds_ingestion/test_folder_upload_modes.py +++ b/tests/dds_ingestion/test_folder_upload_modes.py @@ -72,9 +72,9 @@ def _network_error() -> httpx.ConnectError: # --- BeginResult.upload_mode ---------------------------------------------------- -def test_upload_mode_is_read_from_begin_and_defaults_to_direct() -> None: +def test_upload_mode_is_read_from_begin_and_is_none_when_absent() -> None: base = {"session_id": "s", "status": "uploading", "s3": {"bucket": "b", "key_prefix": "p/"}} - assert BeginResult.from_json(base).upload_mode == "direct" + assert BeginResult.from_json(base).upload_mode is None assert BeginResult.from_json({**base, "upload_mode": "auto"}).upload_mode == "auto" From e4a5816adf71f0ed3a482facf246abf70195fec9 Mon Sep 17 00:00:00 2001 From: blikij Date: Mon, 19 Oct 2026 11:21:44 +0200 Subject: [PATCH 03/19] feat(catalog): copy or move a whole folder (foldertree) folderscan maps every sub-folder, table and view under a source to where it would land; foldercopy reproduces the subtree, compares COUNT(*) on both sides for each table and recreates each view from its own SQL with references into the subtree remapped (remap_view_sql); foldermove deletes the source only once the copy verified. New primitives under them: getentriesfrom, entryexists, getviewsql, createviewas, getfoldersfrom and CatalogRestClient.children / list_folders. Merged with main's catalog session work: deletetable is main's, so the copy of it written here is dropped. Co-Authored-By: Claude Opus 5.5 --- README.md | 33 + docs/dev-notes.md | 20 + docs/python-client-guide.md | 158 +++ src/eea_datalakehouse/catalog/__init__.py | 45 +- src/eea_datalakehouse/catalog/client.py | 133 ++- src/eea_datalakehouse/catalog/foldertree.py | 1110 +++++++++++++++++++ src/eea_datalakehouse/catalog/operations.py | 257 +++++ src/eea_datalakehouse/catalog/rest.py | 68 ++ tests/catalog/conftest.py | 17 + tests/catalog/test_foldertree.py | 688 ++++++++++++ tests/catalog/test_operations.py | 123 ++ tests/catalog/test_rest.py | 98 ++ 12 files changed, 2747 insertions(+), 3 deletions(-) create mode 100644 src/eea_datalakehouse/catalog/foldertree.py create mode 100644 tests/catalog/test_foldertree.py diff --git a/README.md b/README.md index 818b7b6..94f26c3 100644 --- a/README.md +++ b/README.md @@ -139,7 +139,11 @@ exact same arguments. - `draft2version(draft_path, version_path)` / `publishversion(consumer_view_path, version_path)` — **not implemented yet** (both raise `NotImplementedError`); promoting a draft table into a permanent version and repointing a consumer-facing view at it. +- `createviewas(view_path, sql, overwrite=False, create_target_folder=True)` — `table2view`'s + general form: creates a view from any query body you hand it, not just `SELECT * FROM `. + The SQL is sent through unchanged. - `deleteview(view_path)` — `DROP VIEW IF EXISTS`. +- `deletetable(table_path)` — `DROP TABLE IF EXISTS`; `deleteview`'s counterpart. **Copying/moving data** — always over Arrow Flight SQL, never REST, regardless of `EEA_CATALOG_TRANSPORT` (needs `username=` on `Catalog(...)` for the Flight auth handshake): @@ -156,11 +160,39 @@ exact same arguments. `target_path` actually exists in the catalog before declaring success, rather than trusting a silent CREATE/DROP. +**A whole folder at once** (`catalog/foldertree.py` — orchestration over the operations above, +not one statement each): +- `folderscan(source_path, target_path)` — read-only: every sub-folder, table and view under + `source_path`, mapped to where it would land under `target_path`, with each view's SQL already + rewritten and the views in dependency order. Returns a `FolderTree`. +- `foldercopy(source_path, target_path, dry_run=False, overwrite=False, skip_existing=True, + verify_views=True, copy_metadata=True, tree=None)` — reproduces the subtree and **proves it**: + folders first (empty ones included), then tables via `datacopy` with `COUNT(*)` compared on + both sides, then views recreated from their own SQL with every reference into the moved + subtree remapped to the new location, so the copy is self-contained. Returns a + `FolderTransferReport`; a per-entry failure is reported, not raised. +- `foldermove(source_path, target_path, ..., delete_source_root=True)` — `foldercopy`, and then + removes the source **only if `report.verified`**: views in reverse dependency order, then + tables, then folders deepest-first. Anything less than a clean copy leaves the source exactly + where it is. +- `remap_view_sql(sql, source_path=..., target_path=..., entries=..., context=...)` — the view + rewriter on its own, if you want to see what it would do to one definition. + **Discovery** - `gettablesfrom(schema_path)` — every table/view under `schema_path`, recursing into subfolders; returns full dot-separated paths. +- `getentriesfrom(schema_path)` — the same sweep, but each path carries whether it is a + `"TABLE"` or a `"VIEW"` (`CatalogEntry`), so a folder-level operation knows what each entry is + without a lookup per path. - `gettableitemsfrom(table_path)` — one table/view's schema (`{column: DATA_TYPE}`) and row count, without fetching any actual rows. +- `entryexists(path)` — whether `path` is a table or view Dremio knows about. Fetches no rows. +- `getviewsql(view_path)` — the SQL a view is defined by, as a `ViewDefinition(sql, context)`. + Read from Dremio's catalog entity where it answers (which also carries the `sqlContext` the + SQL was written against), falling back to `INFORMATION_SCHEMA."VIEWS"`, which has the text but + no context. Raises rather than guessing if neither answers. +- `getfoldersfrom(path, recursive=True)` — the folders under `path`, shallowest first. REST-only: + folders are not in `INFORMATION_SCHEMA`, so an *empty* folder can only be found this way. **Wiki & tags** (Dremio's catalog collaboration API — REST-only, no SQL/Flight equivalent): - `getwikifrom(path)` / `setwikito(path, text, tags=None)` / `deletewiki(path)` — read, @@ -379,6 +411,7 @@ context itself — raises `CatalogSessionError` if none is set yet: | `sdi/iso.py` | reads UUID, title, edition, dates and series children from ISO 19115-3 | | `catalog/client.py` | `Catalog` — two connections (REST + Flight), every operation as a method | | `catalog/operations.py` | the operations themselves (table2view, datacopy, createfolder, ...), as functions taking an executor and/or a `CatalogRestClient` | +| `catalog/foldertree.py` | `folderscan`/`foldercopy`/`foldermove` — a whole folder at a time, with the view-SQL rewriter (`remap_view_sql`) that keeps a copied subtree self-contained | | `catalog/sql.py` | `SqlExecutor` protocol + REST/Flight implementations, `resolve_executor()` (transport env var) | | `catalog/rest.py` | `CatalogRestClient` — Dremio's native `/api/v3/catalog` for folders, wiki, and tags (none of which have a SQL/Flight equivalent) | | `catalog/retry_state.py` | persisted memory of stalled attempts, for `retry_pending()` | diff --git a/docs/dev-notes.md b/docs/dev-notes.md index 04f01d5..d3bd917 100644 --- a/docs/dev-notes.md +++ b/docs/dev-notes.md @@ -450,6 +450,26 @@ rule, destructive operations should echo the fully resolved path before acting. #### 8. Proposed: `datacopy` should accept a folder and copy everything in it +> **Status: done** — implemented as `catalog/foldertree.py` +> (`folderscan` / `foldercopy` / `foldermove`), taking §8a's option 1: separate +> verbs named for the folder, not an overloaded `datacopy`. What §8 left for the +> team to decide was decided as follows, and each is documented where it lives: +> +> | Question | What was built | +> |---|---| +> | Views | Recreated **as views**, with every reference into the moved subtree rewritten to the new location (`remap_view_sql`), so the copy is self-contained. Never materialised into tables. `getentriesfrom` adds `TABLE_TYPE` as suggested, as a sibling of `gettablesfrom` rather than a change to its return type. | +> | Depth | `cp -r`, stated: the whole subtree, empty folders included (found over REST, since `INFORMATION_SCHEMA` cannot see them). | +> | Partial failure | Per-entry, not per-operation: each entry gets its own derived idempotency key (`"::"`), a failure is reported rather than raised, and `skip_existing=True` makes a re-run skip what already landed with a matching row count. | +> | `overwrite=True` at folder scale | Per colliding entry, never "replace the target folder". | +> | Parallelism | Not done, deliberately — the Flight-client concern in §8 is unresolved, and the module docstring says so. | +> | Return type | `FolderTransferReport`, per entry, in the spirit of `IngestOutcome`. | +> | `is_folder` best-effort | A failed REST folder listing degrades to rebuilding the tree from the entry paths, with a note on the report saying empty folders will not be reproduced — rather than failing or silently losing them. | +> | Should `datamove` get the same treatment? | Yes, and its partial-failure story is answered by never deleting anything until the copy has verified: `foldermove` is `foldercopy` + delete, gated on `report.verified`. | +> +> `datacopy`/`datamove` are unchanged, so nothing released is broken. §9 (the +> naming sweep) is still open — the new verbs follow the existing +> lowercase-run-together style rather than pre-empting that decision. + **Position to put to the team** (proposed here, not yet discussed with them): a custodian should be able to point `datacopy` at a dataset folder and have every table inside it copied, rather than making one call per table. diff --git a/docs/python-client-guide.md b/docs/python-client-guide.md index 53e08ef..f7ca4e9 100644 --- a/docs/python-client-guide.md +++ b/docs/python-client-guide.md @@ -37,6 +37,7 @@ behaviour behind those snippets. - [The setup cell](#the-setup-cell) - [Ingest recipes](#ingest-recipes) - [Catalog recipes](#catalog-recipes) + - [Move a whole folder](#move-a-whole-folder-somewhere-else) - [End to end, in one cell](#end-to-end-in-one-cell) - [Part 1 — `dds_ingestion`](#part-1--dds_ingestion) - [The transfer model](#the-transfer-model) @@ -55,6 +56,7 @@ behaviour behind those snippets. - [`Catalog` and its two transports](#catalog-and-its-two-transports) - [`idempotency_key` and the retry contract](#idempotency_key-and-the-retry-contract) - [Operation reference](#operation-reference) + - [Whole folders](#whole-folders--catalogfoldertree) - [Executors (`catalog.sql`)](#executors-catalogsql) - [`CatalogRestClient` (`catalog.rest`)](#catalogrestclient-catalogrest) - [`retry_state`](#retry_state) @@ -141,6 +143,9 @@ fewest lines that actually work. | turn an ingested table into a view | `catalog.table2view(view, source, idempotency_key=KEY)` | | …for every table in a folder | `for p in catalog.gettablesfrom(SRC, …): catalog.table2view(…)` | | copy / move a table | `catalog.datacopy(a, b, …)` · `catalog.datamove(a, b, …)` | +| **move a whole folder** — subfolders, tables and views | `foldermove(catalog, src, dst, idempotency_key=KEY)` | +| …see what that would do, first | `foldermove(…, dry_run=True)` → `print(report.summary())` | +| copy a folder without removing the source | `foldercopy(catalog, src, dst, idempotency_key=KEY)` | | list everything under a folder | `catalog.gettablesfrom("bwd", idempotency_key=KEY)` | | schema + row count, no rows fetched | `catalog.gettableitemsfrom(path, idempotency_key=KEY)` | | create / delete a folder | `catalog.createfolder(p, …)` · `catalog.deletefolder(p, …)` | @@ -445,6 +450,61 @@ the target is verified to exist before it reports success: catalog.datamove("bwd.draft.bw", "bwd.versions.v2026", idempotency_key=KEY) ``` +### Move a whole folder somewhere else + +Sub-folders, tables and views together — the views rewritten so the copy points +at its own tables rather than back at the originals. Look before you leap: + +```python +from eea_datalakehouse.catalog import foldermove + +report = foldermove(catalog, "bwd.draft", "bwd.archive.2025", + idempotency_key=KEY, dry_run=True) +print(report.summary()) # every folder, table (with its row count) and rewritten view +``` + +Nothing was written. When the plan reads right, drop `dry_run`: + +```python +report = foldermove(catalog, "bwd.draft", "bwd.archive.2025", + idempotency_key=KEY, on_progress=print) +print(report.summary()) +report.verified, report.source_deleted +``` + +What it actually does, in this order: + +1. **Folders** first, shallowest first — including empty ones, which + `INFORMATION_SCHEMA` cannot see and only the REST catalog knows about. +2. **Tables** with `datacopy`, then `COUNT(*)` **on both sides**. A table whose + counts disagree fails the whole move. +3. **Views** recreated from their own SQL, with every reference into the moved + subtree remapped to the new location, in dependency order (a view over + another view is created after it). They are counted on both sides too. +4. **Only then** the source is taken apart — views, tables, folders + deepest-first, then the source folder itself. + +**If anything fails, the source is left exactly where it is** and +`report.verified` is `False`. Fix what the report names and run it again: +tables that already landed with a matching count are skipped, so a re-run is +cheap and safe. To copy without ever removing the source, call `foldercopy` +with the same arguments. + +Worth knowing: + +- **`overwrite=False` by default.** A target table that already exists with a + *different* row count fails that entry rather than being silently accepted. + Pass `overwrite=True` to replace it. +- **`report.summary()` is the thing to read** — per entry: what happened, both + row counts, and any note about how a view's SQL was rewritten. +- **The view rewrite is textual**, not a parse of Dremio's SQL dialect. It + never touches string literals or comments, and only rewrites chains it can + resolve. Read the rewritten definitions in a dry run before applying a + subtree of complicated views. +- **Dremio wiki and tags travel with each entry** (`copy_metadata=True`), + best-effort — a wiki that will not copy is a note, not a failure. The **DDS** + `meta.json` sidecar is a different system and is *not* carried. + ### Delete a view `DROP VIEW IF EXISTS` — safe to re-run, and it fails loudly rather than dropping @@ -1138,10 +1198,20 @@ created. Nothing in the library fans an operation out over a folder's contents to convert a whole folder, loop over `gettablesfrom`: [recipe](#turn-every-table-in-a-folder-into-views-in-another-folder). +**`createviewas(view_path, sql, *, overwrite=False, create_target_folder=True)`** +— `table2view`'s general form: a view over any query body you hand it, not just +`SELECT * FROM `. `sql` is the body only (no `CREATE VIEW` prefix) and is +sent through unchanged — quoting and correctness are yours, since the text +usually comes back out of Dremio itself. `overwrite=True` drops whatever is +actually at `view_path` first, detecting whether it is a table or a view. + **`deleteview(view_path)`** — `DROP VIEW IF EXISTS`. Idempotent. If `view_path` is actually a table, this fails loudly rather than dropping the wrong kind of entry. +**`deletetable(table_path)`** — `DROP TABLE IF EXISTS`; `deleteview`'s +counterpart, failing just as loudly on the wrong kind of entry. + **`draft2version(draft_path, version_path)`** and **`publishversion(consumer_view_path, version_path)`** — **not implemented**; both raise `NotImplementedError`. See [Not implemented yet](#not-implemented-yet). @@ -1182,6 +1252,65 @@ Shared behaviour: for a no-result-rows DDL statement are unverified here — so it checks `target_path` actually exists in the catalog before declaring success. +### Whole folders — `catalog.foldertree` + +`folderscan` / `foldercopy` / `foldermove` (also methods on `Catalog`). These +are orchestration over the operations above rather than one statement each, so +they live in their own module — see its docstring for the assumptions they rest +on, and the [recipe](#move-a-whole-folder-somewhere-else) for how to drive them. + +**`folderscan(catalog, source_path, target_path) -> FolderTree`** — read-only. +Every sub-folder, table and view under `source_path`, mapped to where it would +land, with each view's SQL already rewritten and the views in dependency order. +Raises if the two paths are the same, or if either is inside the other. + +```python +@dataclass(frozen=True, slots=True) +class TreeEntry: + source_path: str + target_path: str + kind: Literal["FOLDER", "TABLE", "VIEW"] + view_sql: str | None # views: as Dremio stores it + view_context: tuple[str, ...] # views: its sqlContext, if known + remapped_sql: str | None # views: what will actually be created + depends_on: tuple[str, ...] # views: what it selects from, in this subtree + notes: tuple[str, ...] +``` + +**`foldercopy(catalog, source_path, target_path, *, dry_run=False, +overwrite=False, skip_existing=True, verify_views=True, copy_metadata=True, +qualify_external=True, tree=None, on_progress=None) -> FolderTransferReport`** +— reproduces the subtree and proves it. Folders first (empty ones included), +then tables via `datacopy` with `COUNT(*)` compared on both sides, then views +recreated from their own SQL with references into the subtree remapped. A +per-entry failure is recorded, not raised — the sweep finishes so you see the +whole picture. + +`skip_existing=True` is what makes a re-run cheap: a target table already there +**with the source's row count** is reported `skipped`. One with a *different* +count fails that entry rather than being silently accepted. + +**`foldermove(catalog, source_path, target_path, *, delete_source_root=True, +…)`** — `foldercopy`, then removal of the source **only if `report.verified`**: +views in reverse dependency order, then tables, then folders deepest-first. +`deletefolder(cascade=True)` is deliberately not used — a folder that will not +delete because something unexpected is inside it is a fact worth surfacing. + +**`FolderTransferReport`** — `folders` / `tables` / `views` are lists of +`EntryResult(entry, status, source_rows, target_rows, sql, error, notes)`, where +`status` is `planned` / `created` / `copied` / `skipped` / `failed`. +`report.verified` is the green light, `report.failures` is what to fix, +`report.deleted` and `report.delete_errors` say what came off the source, and +`report.summary()` prints the lot. + +**`remap_view_sql(sql, *, source_path, target_path, entries, context=(), +external_exists=None) -> RemapResult`** — the view rewriter on its own. Textual, +never a parse of Dremio's dialect: it skips string literals and comments, and +rewrites only identifier chains it can resolve — against a known entry, against +the source root, or against the view's own `sqlContext`. Its docstring lists the +four rules in order, and the one corner it can get wrong. + + ### Discovery **`gettablesfrom(schema_path) -> list[str]`** — every table and view under @@ -1191,6 +1320,35 @@ since SQL `LIKE`'s `%` matches across dots), with `_`/`%` in real folder names escaped so an underscore is not read as a wildcard. Full paths rather than bare names because two subfolders can each hold a table of the same name. +**`getentriesfrom(schema_path) -> list[CatalogEntry]`** — the same sweep, with +`TABLE_TYPE` selected as well, so each path carries whether it is a `"TABLE"` or +a `"VIEW"`. Sorted by path. This is what a folder-level operation needs: copying +a view with `CREATE TABLE … AS SELECT` silently materialises it into a table. + +**`entryexists(path) -> bool`** — whether `path` is a table or view Dremio knows +about, via `INFORMATION_SCHEMA` (so REST or Flight both answer). Fetches no rows, +so it costs nothing on a large table. Unlike `CatalogRestClient.exists`, it does +not see folders, spaces or sources. + +**`getviewsql(view_path) -> ViewDefinition`** — the SQL a view is defined by. + +```python +@dataclass(frozen=True, slots=True) +class ViewDefinition: + sql: str # the query body, as Dremio stores it + context: tuple[str, ...] # the sqlContext it was written against; () = unknown +``` + +Two sources, better one first: Dremio's own catalog entity (which carries the +`sqlContext` as well as the text), then `INFORMATION_SCHEMA."VIEWS"` (text only, +so `context` comes back empty — meaning *unknown*, not *none*). Raises if +neither answers, rather than letting a caller replace a view with a guess. + +**`getfoldersfrom(path, *, recursive=True) -> list[str]`** — the folders under +`path`, shallowest first (the order they have to be created in on the other +side). REST-only: folders are not in `INFORMATION_SCHEMA`, so an **empty** folder +is invisible to `gettablesfrom`/`getentriesfrom` and can only be found this way. + **`gettableitemsfrom(table_path) -> TableInfo`** — one table or view's schema and row count, **without fetching any rows**. diff --git a/src/eea_datalakehouse/catalog/__init__.py b/src/eea_datalakehouse/catalog/__init__.py index 3f41a1e..eb44604 100644 --- a/src/eea_datalakehouse/catalog/__init__.py +++ b/src/eea_datalakehouse/catalog/__init__.py @@ -5,12 +5,19 @@ catalog.draft2version("bwd.draft.bw", "bwd.versions.v1", idempotency_key="bwd-v2025_1") table2view, draft2version, publishversion, datacopy, datamove, deleteview, -gettablesfrom, gettableitemsfrom, getwikifrom, gettagsfrom, setwikito, +deletetable, createviewas, gettablesfrom, getentriesfrom, gettableitemsfrom, +entryexists, getviewsql, getfoldersfrom, getwikifrom, gettagsfrom, setwikito, deletewiki, setmeta2wiki, getmetafromwiki, settagsto, deletetags, createfolder, deletefolder — over REST (the default) or Arrow Flight (opt in via EEA_CATALOG_TRANSPORT=flight in .env), with retry-later error handling for a Dremio engine that's still starting up. +A whole folder at a time — sub-folders, tables and views together — lives in +`foldertree`: `folderscan` plans it, `foldercopy` reproduces it and proves +the copy by row count, `foldermove` does that and then removes the source, +but only once the copy verified. Views are rewritten to point at the copies +rather than back at the originals (`remap_view_sql`). + The module-level functions in `operations` (table2view(executor, ...) etc.) are what `Catalog`'s methods delegate to — call them directly if you'd rather manage the executor yourself. @@ -20,22 +27,41 @@ from .client import Catalog from .errors import CatalogAuthError, CatalogOperationError, EngineStartingError +from .foldertree import ( + EntryResult, + FolderTransferReport, + FolderTree, + RemapResult, + TreeEntry, + foldercopy, + foldermove, + folderscan, + remap_view_sql, +) from .operations import ( + CatalogEntry, TableInfo, VersionResult, + ViewDefinition, createfolder, createversion, + createviewas, datacopy, datamove, deletefolder, + deletetable, deletetags, deleteview, deletewiki, draft2version, + entryexists, + getentriesfrom, + getfoldersfrom, getmetafromwiki, gettableitemsfrom, gettablesfrom, gettagsfrom, + getviewsql, getwikifrom, publishversion, retry_pending, @@ -68,6 +94,7 @@ "Catalog", "CatalogAuthError", "CatalogCommitError", + "CatalogEntry", "CatalogListing", "CatalogOperationError", "CatalogRestClient", @@ -75,27 +102,43 @@ "CatalogSessionError", "CommitReport", "EngineStartingError", + "EntryResult", "FlightSqlExecutor", + "FolderTransferReport", + "FolderTree", + "RemapResult", "RestSqlExecutor", "SqlExecutor", "SqlResult", "TableInfo", + "TreeEntry", "VersionResult", + "ViewDefinition", "createfolder", "createversion", + "createviewas", "datacopy", "datamove", "deletefolder", + "deletetable", "deletetags", "deleteview", "deletewiki", "draft2version", + "entryexists", + "foldercopy", + "foldermove", + "folderscan", + "getentriesfrom", + "getfoldersfrom", "gettableitemsfrom", "gettablesfrom", "gettagsfrom", + "getviewsql", "getwikifrom", "publishversion", "getmetafromwiki", + "remap_view_sql", "resolve_executor", "retry_pending", "setmeta2wiki", diff --git a/src/eea_datalakehouse/catalog/client.py b/src/eea_datalakehouse/catalog/client.py index 3a009f0..6fb6fcd 100644 --- a/src/eea_datalakehouse/catalog/client.py +++ b/src/eea_datalakehouse/catalog/client.py @@ -34,8 +34,9 @@ import weakref from typing import Any, Literal -from . import operations, retry_state -from .operations import TableInfo, VersionResult +from . import foldertree, operations, retry_state +from .foldertree import FolderTransferReport, FolderTree, ProgressCallback +from .operations import CatalogEntry, TableInfo, VersionResult, ViewDefinition from .rest import CatalogRestClient from .sql import ( DEFAULT_TIMEOUT, @@ -306,6 +307,48 @@ def gettableitemsfrom(self, table_path: str, *, idempotency_key: str) -> TableIn self._executor, table_path, idempotency_key=idempotency_key ) + def getentriesfrom(self, schema_path: str, *, idempotency_key: str) -> list[CatalogEntry]: + return operations.getentriesfrom( + self._executor, schema_path, idempotency_key=idempotency_key + ) + + def entryexists(self, path: str, *, idempotency_key: str) -> bool: + return operations.entryexists(self._executor, path, idempotency_key=idempotency_key) + + def getviewsql(self, view_path: str, *, idempotency_key: str) -> ViewDefinition: + return operations.getviewsql( + self._executor, + view_path, + catalog_rest=self._catalog_rest, + idempotency_key=idempotency_key, + ) + + def createviewas( + self, + view_path: str, + sql: str, + *, + overwrite: bool = False, + create_target_folder: bool = True, + idempotency_key: str, + ) -> SqlResult: + return operations.createviewas( + self._executor, + view_path, + sql, + overwrite=overwrite, + create_target_folder=create_target_folder, + catalog_rest=self._catalog_rest, + idempotency_key=idempotency_key, + ) + + def getfoldersfrom( + self, path: str, *, recursive: bool = True, idempotency_key: str + ) -> list[str]: + return operations.getfoldersfrom( + self._catalog_rest, path, recursive=recursive, idempotency_key=idempotency_key + ) + def getwikifrom(self, path: str, *, idempotency_key: str) -> str: return operations.getwikifrom(self._catalog_rest, path, idempotency_key=idempotency_key) @@ -377,6 +420,92 @@ def deletefolder(self, path: str, *, cascade: bool = False, idempotency_key: str idempotency_key=idempotency_key, ) + # --- whole folders: scan / copy / move ------------------------------- + # These are orchestration over the operations above rather than one SQL + # statement each, so they live in `foldertree` and are exposed here for + # a caller who already has a Catalog. See that module's docstring for + # what a move actually does, and in what order. + + def folderscan( + self, + source_path: str, + target_path: str, + *, + idempotency_key: str, + qualify_external: bool = True, + on_progress: ProgressCallback | None = None, + ) -> FolderTree: + return foldertree.folderscan( + self, + source_path, + target_path, + idempotency_key=idempotency_key, + qualify_external=qualify_external, + on_progress=on_progress, + ) + + def foldercopy( + self, + source_path: str, + target_path: str, + *, + idempotency_key: str, + dry_run: bool = False, + overwrite: bool = False, + skip_existing: bool = True, + verify_views: bool = True, + copy_metadata: bool = True, + qualify_external: bool = True, + tree: FolderTree | None = None, + on_progress: ProgressCallback | None = None, + ) -> FolderTransferReport: + return foldertree.foldercopy( + self, + source_path, + target_path, + idempotency_key=idempotency_key, + dry_run=dry_run, + overwrite=overwrite, + skip_existing=skip_existing, + verify_views=verify_views, + copy_metadata=copy_metadata, + qualify_external=qualify_external, + tree=tree, + on_progress=on_progress, + ) + + def foldermove( + self, + source_path: str, + target_path: str, + *, + idempotency_key: str, + dry_run: bool = False, + overwrite: bool = False, + skip_existing: bool = True, + verify_views: bool = True, + copy_metadata: bool = True, + qualify_external: bool = True, + delete_source_root: bool = True, + tree: FolderTree | None = None, + on_progress: ProgressCallback | None = None, + ) -> FolderTransferReport: + return foldertree.foldermove( + self, + source_path, + target_path, + idempotency_key=idempotency_key, + dry_run=dry_run, + overwrite=overwrite, + skip_existing=skip_existing, + verify_views=verify_views, + copy_metadata=copy_metadata, + qualify_external=qualify_external, + delete_source_root=delete_source_root, + tree=tree, + on_progress=on_progress, + ) + def retry_pending(self, idempotency_key: str) -> SqlResult | list[Any]: pending = retry_state.get(idempotency_key) if pending is None: diff --git a/src/eea_datalakehouse/catalog/foldertree.py b/src/eea_datalakehouse/catalog/foldertree.py new file mode 100644 index 0000000..68233c0 --- /dev/null +++ b/src/eea_datalakehouse/catalog/foldertree.py @@ -0,0 +1,1110 @@ +"""Move or copy a whole folder — sub-folders, tables and views — from one +place in the Dremio catalog to another. + + from eea_datalakehouse.catalog import Catalog, foldermove + + with Catalog(DREMIO_URL, DREMIO_TOKEN, username=DREMIO_USERNAME) as catalog: + report = foldermove(catalog, "bwd.draft", "bwd.archive.2025", idempotency_key=KEY) + print(report.summary()) + +`foldercopy` reproduces the source subtree under the target and proves the +copy landed. `foldermove` is `foldercopy` plus removal of the source — and it +removes **nothing** unless the copy verified clean (`report.verified`). That +order — copy, verify, then delete — is the whole point: a folder move across a +Community-Edition Dremio has no transaction behind it, so the only safe +sequencing is to leave the source standing until the target is proven. + +What each kind of entry needs is different, and that is why this is more than +a loop over `datacopy`: + +* **Folders** are reproduced first, shallowest first, so nothing lands in a + folder that does not exist yet. Empty ones are included — they are invisible + to `INFORMATION_SCHEMA` and only Dremio's REST catalog knows about them. +* **Tables** are copied with `datacopy` (``CREATE TABLE … AS SELECT``, over + Arrow Flight) and then **counted on both sides**: ``COUNT(*)`` on the source + and on the target, which have to agree. A table whose counts disagree fails + the whole move. +* **Views** are *not* copied as data — a ``CREATE TABLE … AS SELECT`` over a + view silently materialises it into a table, which is a different kind of + thing from the original. Each view is recreated from its own SQL with every + reference into the moved subtree **remapped to the new location** (see + `remap_view_sql`), so the copied folder is self-contained and still points at + its own tables rather than back at the ones about to be deleted. Views are + created in dependency order, so a view selecting from another view in the + same subtree is created after it. + +Assumptions worth checking against your actual Dremio deployment before +relying on this — in the same spirit as `operations`' own list: + +* **A view's SQL is read from Dremio's catalog entity where possible** + (`getviewsql`), because that carries the ``sqlContext`` the SQL was written + against as well as the text. Where that lookup is rejected — some source + types answer a nested by-path lookup with a 400, see + `CatalogRestClient.is_folder` — it falls back to + ``INFORMATION_SCHEMA."VIEWS"``, which has the text but no context. With no + context, an unqualified reference inside a view cannot be resolved and is + left exactly as it was; `remap_view_sql` says so in its notes rather than + guessing. +* **The rewrite is textual**, not a parse of Dremio's SQL dialect. It skips + string literals and comments, and only rewrites identifier chains it can + resolve to something it knows about (see `remap_view_sql` for the exact + rules and the one corner it can get wrong). A dry run prints every rewritten + definition — read them before turning `dry_run` off on a subtree of + complicated views. +* **Nothing here is parallel.** `datacopy` is Flight-only and + `FlightSqlExecutor` holds one client with a lazily-cached auth header whose + initialisation is unguarded; this repo has not verified concurrent statement + execution on one Flight client. N tables copy one after another. +* **Re-running is expected to be cheap.** With the default + `skip_existing=True`, a target table that is already there and already has + the source's row count is left alone and reported as ``skipped`` — so a run + interrupted halfway (a cold engine, a dropped kernel) is resumed by simply + running it again, rather than by unpicking what already made it. +* **Deletion is explicit, never a cascade.** The source is taken apart in the + order it was built: views (reverse dependency order), then tables, then + folders deepest-first. `deletefolder`'s own `cascade=True` is deliberately + not used — a folder that will not delete because something unexpected is + still inside it is a fact worth surfacing, not something to bulldoze. +""" + +from __future__ import annotations + +import re +from collections.abc import Callable, Iterator +from dataclasses import dataclass, field +from typing import TYPE_CHECKING, Literal + +from .errors import CatalogOperationError +from .operations import _quote_path + +if TYPE_CHECKING: # pragma: no cover - typing only + from .client import Catalog + +EntryKind = Literal["FOLDER", "TABLE", "VIEW"] +EntryStatus = Literal["planned", "created", "copied", "skipped", "failed"] + +ProgressCallback = Callable[[str], None] + +# Identifier chain segment: a double-quoted identifier (with "" as an escaped +# quote inside it) or a bare one. Digits can't start a bare identifier, which +# is what keeps `1.5` from being read as a two-segment chain. +_SQL_IDENT = re.compile(r'"(?:[^"]|"")*"|[A-Za-z_][A-Za-z0-9_$]*') + +# Keywords a dataset reference can follow. Only used to decide whether an +# unresolved chain is worth one existence check to see if it was written +# relative to a context this move is dropping — never to decide a rewrite that +# the chain itself already justifies. +_REFERENCE_KEYWORDS = frozenset({"FROM", "JOIN", "INTO", "TABLE", "UPDATE"}) + +# Enough of Dremio's SQL vocabulary to keep the "unqualified reference" +# warning below from firing on every keyword in an ordinary SELECT. It only +# gates a note; nothing is ever rewritten on the strength of this list. +_SQL_WORDS = frozenset({ + "ALL", "AND", "AS", "ASC", "AVG", "BETWEEN", "BY", "CASE", "CAST", "COALESCE", "COUNT", + "CROSS", "CURRENT_DATE", "CURRENT_TIMESTAMP", "DESC", "DISTINCT", "ELSE", "END", "ESCAPE", + "EXISTS", "EXTRACT", "FALSE", "FROM", "FULL", "GROUP", "HAVING", "IN", "INNER", "INTERVAL", + "INTO", "IS", "JOIN", "LEFT", "LIKE", "LIMIT", "MAX", "MIN", "NATURAL", "NOT", "NULL", + "NULLIF", "OFFSET", "ON", "OR", "ORDER", "OUTER", "OVER", "PARTITION", "RIGHT", "ROW", + "ROWS", "SELECT", "SET", "SUBSTRING", "SUM", "TABLE", "THEN", "TRIM", "TRUE", "UNION", + "UPDATE", "USING", "VALUES", "WHEN", "WHERE", "WITH", +}) + + +@dataclass(frozen=True, slots=True) +class TreeEntry: + """One thing to reproduce at the target: a folder, a table or a view.""" + + source_path: str + target_path: str + kind: EntryKind + #: Views only — the SQL Dremio holds for the view, exactly as stored. + view_sql: str | None = None + #: Views only — the ``sqlContext`` that SQL was written against, if known. + view_context: tuple[str, ...] = () + #: Views only — the rewritten SQL that will be used at the target. + remapped_sql: str | None = None + #: Views only — source paths inside this subtree the view selects from. + depends_on: tuple[str, ...] = () + #: Anything the caller should read before trusting this entry. + notes: tuple[str, ...] = () + + @property + def name(self) -> str: + return self.source_path.rsplit(".", 1)[-1] + + +@dataclass(frozen=True, slots=True) +class FolderTree: + """What `folderscan` found under a source folder, already mapped to targets. + + `views` is in **dependency order**: a view that selects from another view + in this same subtree comes after it, so creating them in list order works. + `folders` is shallowest first, for the same reason. + """ + + source_path: str + target_path: str + folders: tuple[TreeEntry, ...] = () + tables: tuple[TreeEntry, ...] = () + views: tuple[TreeEntry, ...] = () + notes: tuple[str, ...] = () + + @property + def entries(self) -> tuple[TreeEntry, ...]: + """Every entry, in the order a copy has to create them.""" + return self.folders + self.tables + self.views + + def __len__(self) -> int: + return len(self.entries) + + +@dataclass(slots=True) +class EntryResult: + """What actually happened to one `TreeEntry`.""" + + entry: TreeEntry + status: EntryStatus = "planned" + source_rows: int | None = None + target_rows: int | None = None + sql: str | None = None + error: str | None = None + notes: list[str] = field(default_factory=list) + + @property + def counts_match(self) -> bool: + """Both sides counted, and they agree. False while either is unknown.""" + return ( + self.source_rows is not None + and self.target_rows is not None + and self.source_rows == self.target_rows + ) + + +@dataclass(slots=True) +class FolderTransferReport: + """The outcome of a `foldercopy` / `foldermove`, entry by entry. + + `verified` is the green light: every folder reproduced, every table copied + *and* counted equal on both sides, every view created, and nothing failed. + `foldermove` deletes the source only when it is true. + """ + + source_path: str + target_path: str + dry_run: bool = False + folders: list[EntryResult] = field(default_factory=list) + tables: list[EntryResult] = field(default_factory=list) + views: list[EntryResult] = field(default_factory=list) + deleted: list[str] = field(default_factory=list) + delete_errors: list[tuple[str, str]] = field(default_factory=list) + notes: list[str] = field(default_factory=list) + source_deleted: bool = False + + @property + def results(self) -> list[EntryResult]: + return self.folders + self.tables + self.views + + @property + def failures(self) -> list[EntryResult]: + return [result for result in self.results if result.status == "failed"] + + @property + def verified(self) -> bool: + """Whether the target is a proven-good copy of the source. + + A dry run is never verified — nothing was written, so there is nothing + to have proven. Tables must have matching counts on both sides; views + must have been created or deliberately skipped, and have matching + counts too wherever they were checked (`verify_views`). + """ + if self.dry_run or not self.results: + return False + if self.failures: + return False + if any(result.status == "planned" for result in self.results): + return False + for result in self.tables: + if not result.counts_match: + return False + for result in self.views: + if result.source_rows is not None and not result.counts_match: + return False + return True + + def summary(self) -> str: + """A readable report — what moved, what it counted, what went wrong.""" + head = "PLAN" if self.dry_run else "RESULT" + lines: list[str] = [f"{head}: {self.source_path} -> {self.target_path}"] + for label, results in ( + ("folders", self.folders), + ("tables", self.tables), + ("views", self.views), + ): + if not results: + continue + lines.append(f"\n{label} ({len(results)}):") + for result in results: + mark = {"failed": "x", "planned": "-", "skipped": "="}.get(result.status, "+") + counts = "" + if result.source_rows is not None or result.target_rows is not None: + src = "?" if result.source_rows is None else f"{result.source_rows:,}" + tgt = "?" if result.target_rows is None else f"{result.target_rows:,}" + # "->" while a side is still unknown (a plan, say) — only a + # count that was actually taken on both sides can disagree. + both_known = result.source_rows is not None and result.target_rows is not None + agree = "==" if result.counts_match else ("!=" if both_known else "->") + counts = f" [{src} {agree} {tgt} rows]" + lines.append( + f" {mark} {result.entry.source_path} -> {result.entry.target_path}" + f" ({result.status}){counts}" + ) + lines.extend(f" note: {note}" for note in result.notes) + if result.error: + lines.append(f" error: {result.error}") + if self.deleted: + lines.append(f"\nremoved from the source ({len(self.deleted)}):") + lines.extend(f" - {path}" for path in self.deleted) + if self.delete_errors: + lines.append("\ncould not remove:") + lines.extend(f" x {path}: {error}" for path, error in self.delete_errors) + if self.notes: + lines.append("\nnotes:") + lines.extend(f" {note}" for note in self.notes) + tail = f"\nverified: {self.verified}" + if self.dry_run: + tail += " (dry run — nothing was written)" + else: + tail += f" source removed: {self.source_deleted}" + lines.append(tail) + return "\n".join(lines) + + +# -------------------------------------------------------------------------- +# paths +# -------------------------------------------------------------------------- + + +def _segments(path: str) -> list[str]: + return [segment for segment in path.split(".") if segment] + + +def _lower(segments: list[str] | tuple[str, ...]) -> list[str]: + return [segment.lower() for segment in segments] + + +def _remap_path(path: str, source_path: str, target_path: str) -> str: + """`path`, with its `source_path` prefix swapped for `target_path`.""" + remainder = _segments(path)[len(_segments(source_path)) :] + return ".".join(_segments(target_path) + remainder) + + +def _is_inside(path: str, ancestor: str) -> bool: + """Whether `path` is `ancestor` itself, or something under it (case-insensitively).""" + path_segments = _lower(_segments(path)) + ancestor_segments = _lower(_segments(ancestor)) + return path_segments[: len(ancestor_segments)] == ancestor_segments + + +def _unquote_ident(raw: str) -> str: + if raw.startswith('"') and raw.endswith('"') and len(raw) >= 2: + return raw[1:-1].replace('""', '"') + return raw + + +# -------------------------------------------------------------------------- +# view SQL rewriting +# -------------------------------------------------------------------------- + + +@dataclass(frozen=True, slots=True) +class _Token: + """One piece of a SQL string: passthrough text, or an identifier chain.""" + + kind: Literal["text", "chain"] + raw: str + segments: tuple[str, ...] = () + + +def _iter_sql_chains(sql: str) -> Iterator[_Token]: + """Walk `sql`, yielding a `_Token` per piece: text, or an identifier chain. + + String literals (``'…'``, with ``''`` as an escaped quote), line comments + (``-- …``) and block comments (``/* … */``) are passed through as text and + never looked inside — the one hard rule that keeps a path-shaped string + from being rewritten as if it were a reference. + + A *chain* is one or more identifiers joined by dots with no whitespace + between them: ``a.b``, ``"my space"."tbl"``, ``t.col``. Whitespace around + a dot ends the chain, which is a deliberate simplification — Dremio's own + stored view SQL does not write references that way. + """ + index, length = 0, len(sql) + buffer: list[str] = [] + while index < length: + char = sql[index] + if char == "'": + cursor = index + 1 + while cursor < length: + if sql[cursor] == "'": + if cursor + 1 < length and sql[cursor + 1] == "'": + cursor += 2 + continue + cursor += 1 + break + cursor += 1 + buffer.append(sql[index:cursor]) + index = cursor + continue + if sql.startswith("--", index): + cursor = sql.find("\n", index) + cursor = length if cursor == -1 else cursor + buffer.append(sql[index:cursor]) + index = cursor + continue + if sql.startswith("/*", index): + cursor = sql.find("*/", index + 2) + cursor = length if cursor == -1 else cursor + 2 + buffer.append(sql[index:cursor]) + index = cursor + continue + match = _SQL_IDENT.match(sql, index) + if match is None: + buffer.append(char) + index += 1 + continue + + segments: list[str] = [] + cursor = index + while True: + match = _SQL_IDENT.match(sql, cursor) + if match is None: + break + segments.append(_unquote_ident(match.group(0))) + cursor = match.end() + if cursor < length and sql[cursor] == "." and _SQL_IDENT.match(sql, cursor + 1): + cursor += 1 + continue + break + if buffer: + yield _Token("text", "".join(buffer)) + buffer = [] + yield _Token("chain", sql[index:cursor], tuple(segments)) + index = cursor + if buffer: + yield _Token("text", "".join(buffer)) + + +@dataclass(frozen=True, slots=True) +class RemapResult: + """`remap_view_sql`'s answer: the new SQL, and what it had to touch.""" + + sql: str + #: Source paths inside the moved subtree this SQL selects from. + references: tuple[str, ...] = () + #: Anything the caller should read before trusting the rewrite. + notes: tuple[str, ...] = () + + +def remap_view_sql( + sql: str, + *, + source_path: str, + target_path: str, + entries: dict[str, str], + context: tuple[str, ...] = (), + external_exists: Callable[[str], bool] | None = None, +) -> RemapResult: + """Rewrite a view's SQL so it points at the copied subtree, not the original. + + `entries` maps a **lower-cased** source path to its target path — every + table and view being moved. `context` is the view's own ``sqlContext``. + + Each identifier chain outside a string literal or comment is resolved, in + this order, and the first rule that answers wins: + + 1. **A known entry.** The longest prefix of the chain that is one of + `entries` becomes that entry's target path — so ``bwd.draft.t.col`` + rewrites the ``bwd.draft.t`` part and leaves ``.col`` alone. + 2. **Anything else under the source root.** A chain starting with + `source_path` has that prefix swapped for `target_path`. This is the + safety net for something the scan did not enumerate (a table created + between the scan and the rewrite). + 3. **Context-relative, into the subtree.** With a `context`, the chain is + retried as ``context + chain``; if that resolves under rule 1 or 2, the + chain is replaced with the **fully qualified** target path. This is + what keeps a view that says ``FROM sales`` working: after the move it + no longer has that context, so the reference is spelled out. + 4. **Context-relative, out of the subtree.** A chain that follows + ``FROM``/``JOIN``/``INTO``/``TABLE``/``UPDATE`` and still has not + resolved is checked once against `external_exists` as + ``context + chain``; if it really is there, it is spelled out in full + so it survives losing the context. Skipped entirely without an + `external_exists`. + + Anything else is left exactly as written. + + **The corner it can get wrong.** Rule 2 is prefix matching, so if + `source_path` is a single segment (a whole space) and the SQL aliases a + table with that same name, ``alias.column`` looks like a path into the + source. Moving a whole space is not the normal case, and moving a folder + (two segments or more) cannot hit it — but it is why a dry run prints + every rewritten definition. + """ + source_segments = _segments(source_path) + target_segments = _segments(target_path) + source_lower = _lower(source_segments) + context_segments = list(context) + + references: set[str] = set() + notes: list[str] = [] + probed: dict[str, bool] = {} + + def resolve_known( + segments: list[str], *, allow_root_prefix: bool = True + ) -> tuple[int, list[str]] | None: + """(segments consumed, replacement) under rules 1 and 2, or None. + + `allow_root_prefix=False` drops rule 2, leaving only an exact match + against a known entry. Rule 3 needs that: prepending a context that is + itself inside the source tree makes *every* chain — every keyword + included — start with the source root, so the fallback would rewrite + `SELECT` as eagerly as a real reference. The enumeration is complete + for the subtree, so an exact match is all a context-relative + reference into it ever needs. + """ + for count in range(len(segments), 0, -1): + key = ".".join(_lower(segments[:count])) + if key in entries: + references.add(key) + return count, _segments(entries[key]) + if not allow_root_prefix: + return None + lowered = _lower(segments) + if len(lowered) >= len(source_lower) and lowered[: len(source_lower)] == source_lower: + return len(segments), target_segments + segments[len(source_segments) :] + return None + + out: list[str] = [] + previous_keyword = "" + for token in _iter_sql_chains(sql): + if token.kind == "text": + out.append(token.raw) + stripped = token.raw.strip() + if stripped and stripped[-1] == ",": + previous_keyword = "," + continue + + segments = list(token.segments) + replacement: list[str] | None = None + consumed = len(segments) + + resolved = resolve_known(segments) + if resolved is not None: + consumed, replacement = resolved + elif context_segments: + relative = resolve_known(context_segments + segments, allow_root_prefix=False) + if relative is not None: + # The match covered the context as well, so only what is left + # over of the chain itself counts as consumed — `sales.region` + # against a context that resolves `sales` keeps `.region`. + consumed = max(relative[0] - len(context_segments), 1) + replacement = relative[1] + elif external_exists is not None and previous_keyword in _REFERENCE_KEYWORDS: + candidate = ".".join(context_segments + segments) + if candidate not in probed: + probed[candidate] = external_exists(candidate) + if probed[candidate]: + consumed, replacement = len(segments), context_segments + segments + notes.append( + f"{'.'.join(segments)!r} was relative to the view's context — " + f"written out in full as {candidate!r}" + ) + + if replacement is None: + out.append(token.raw) + else: + out.append(_quote_path(".".join(replacement))) + # Whatever of the chain the rule did not consume (a column, say) + # is appended back unchanged. + if consumed < len(segments): + out.append("." + ".".join(f'"{segment}"' for segment in segments[consumed:])) + + previous_keyword = segments[0].upper() if len(segments) == 1 else "" + + if not context_segments: + unqualified = [ + token + for token in _iter_sql_chains(sql) + if token.kind == "chain" + and len(token.segments) == 1 + and token.segments[0].upper() not in _SQL_WORDS + ] + if unqualified: + notes.append( + "the view's sqlContext is unknown, so any unqualified reference in its " + "SQL was left as written — check it resolves from the new location" + ) + + return RemapResult(sql="".join(out), references=tuple(sorted(references)), notes=tuple(notes)) + + +# -------------------------------------------------------------------------- +# scanning +# -------------------------------------------------------------------------- + + +def _key(idempotency_key: str, operation: str, path: str) -> str: + """One idempotency key per entry, derived from the caller's own. + + A folder move is N operations, and `retry_state` remembers one attempt per + key — so sharing a single key across them would leave only the last stall + remembered. Deriving ``"::"`` keeps every step + separately resumable with `catalog.retry_pending(...)`, and keeps a re-run + of the same job landing on the same keys. + """ + return f"{idempotency_key}:{operation}:{path}" + + +def _emit(on_progress: ProgressCallback | None, message: str) -> None: + if on_progress is not None: + on_progress(message) + + +def _order_views(views: list[TreeEntry]) -> list[TreeEntry]: + """Views sorted so a view comes after every view in this subtree it selects + from. A dependency cycle (which Dremio would not have let you create) is + left in scan order rather than raising.""" + by_path = {entry.source_path.lower(): entry for entry in views} + state: dict[str, str] = {} + ordered: list[TreeEntry] = [] + + def visit(entry: TreeEntry) -> None: + key = entry.source_path.lower() + if state.get(key) in ("done", "visiting"): + return + state[key] = "visiting" + for dependency in entry.depends_on: + other = by_path.get(dependency.lower()) + if other is not None and other.source_path != entry.source_path: + visit(other) + state[key] = "done" + ordered.append(entry) + + for entry in views: + visit(entry) + return ordered + + +def folderscan( + catalog: Catalog, + source_path: str, + target_path: str, + *, + idempotency_key: str, + qualify_external: bool = True, + on_progress: ProgressCallback | None = None, +) -> FolderTree: + """Everything under `source_path`, mapped to where it would land under + `target_path`. Reads only — nothing is created, changed or deleted. + + Views come back with their SQL already rewritten (`TreeEntry.remapped_sql`) + and in dependency order, so the plan a dry run prints is exactly what an + apply would run. + + Raises `CatalogOperationError` if the two paths are the same, if either is + inside the other (a move into your own subtree has no fixed point), or if + `source_path` cannot be read at all. + """ + if not _segments(source_path) or not _segments(target_path): + raise CatalogOperationError("both a source and a target path are required") + if _lower(_segments(source_path)) == _lower(_segments(target_path)): + raise CatalogOperationError(f"source and target are the same folder: {source_path!r}") + if _is_inside(target_path, source_path): + raise CatalogOperationError( + f"target {target_path!r} is inside source {source_path!r} — " + "a folder cannot be moved into itself" + ) + if _is_inside(source_path, target_path): + raise CatalogOperationError( + f"source {source_path!r} is inside target {target_path!r} — " + "that would copy a folder onto its own parent" + ) + + notes: list[str] = [] + + _emit(on_progress, f"scanning {source_path} …") + entries = catalog.getentriesfrom( + source_path, idempotency_key=_key(idempotency_key, "getentriesfrom", source_path) + ) + + folder_paths: list[str] = [] + try: + folder_paths = catalog.getfoldersfrom( + source_path, idempotency_key=_key(idempotency_key, "getfoldersfrom", source_path) + ) + except CatalogOperationError as exc: + if not entries: + raise CatalogOperationError(f"could not read {source_path!r}: {exc}") from exc + notes.append( + f"folders could not be listed over the REST catalog ({exc}) — the tree was " + "rebuilt from the paths of the tables and views found, so any empty folder " + "under the source will not be reproduced" + ) + + # Every folder an entry sits in has to exist at the target whether or not + # the REST listing found it, so derive those too and merge. + derived: set[str] = set() + source_depth = len(_segments(source_path)) + for entry in entries: + segments = _segments(entry.path) + for depth in range(source_depth + 1, len(segments)): + derived.add(".".join(segments[:depth])) + all_folders = sorted(set(folder_paths) | derived, key=lambda path: (path.count("."), path)) + + folders = tuple( + TreeEntry( + source_path=path, + target_path=_remap_path(path, source_path, target_path), + kind="FOLDER", + ) + for path in all_folders + ) + tables = tuple( + TreeEntry( + source_path=entry.path, + target_path=_remap_path(entry.path, source_path, target_path), + kind="TABLE", + ) + for entry in entries + if entry.kind == "TABLE" + ) + + # The whole map has to be known before any view can be rewritten — a view + # may select from a table listed after it. + path_map = {entry.source_path.lower(): entry.target_path for entry in (*tables, *folders)} + canonical = {entry.source_path.lower(): entry.source_path for entry in (*tables, *folders)} + view_entries = [entry for entry in entries if entry.kind == "VIEW"] + for entry in view_entries: + path_map[entry.path.lower()] = _remap_path(entry.path, source_path, target_path) + canonical[entry.path.lower()] = entry.path + + def external_exists(path: str) -> bool: + return catalog.entryexists(path, idempotency_key=_key(idempotency_key, "entryexists", path)) + + views: list[TreeEntry] = [] + for entry in view_entries: + entry_target = _remap_path(entry.path, source_path, target_path) + entry_notes: list[str] = [] + view_sql: str | None = None + view_context: tuple[str, ...] = () + remapped: str | None = None + references: tuple[str, ...] = () + try: + definition = catalog.getviewsql( + entry.path, idempotency_key=_key(idempotency_key, "getviewsql", entry.path) + ) + view_sql, view_context = definition.sql, definition.context + result = remap_view_sql( + view_sql, + source_path=source_path, + target_path=target_path, + entries=path_map, + context=view_context, + external_exists=external_exists if qualify_external else None, + ) + remapped = result.sql + references = tuple(canonical.get(ref, ref) for ref in result.references) + entry_notes.extend(result.notes) + except CatalogOperationError as exc: + entry_notes.append(f"could not read this view's definition: {exc}") + views.append( + TreeEntry( + source_path=entry.path, + target_path=entry_target, + kind="VIEW", + view_sql=view_sql, + view_context=view_context, + remapped_sql=remapped, + depends_on=references, + notes=tuple(entry_notes), + ) + ) + + tree = FolderTree( + source_path=source_path, + target_path=target_path, + folders=folders, + tables=tables, + views=tuple(_order_views(views)), + notes=tuple(notes), + ) + _emit( + on_progress, + f"found {len(tree.folders)} folder(s), {len(tree.tables)} table(s), " + f"{len(tree.views)} view(s) under {source_path}", + ) + return tree + + +# -------------------------------------------------------------------------- +# copying +# -------------------------------------------------------------------------- + + +def _row_count(catalog: Catalog, path: str, idempotency_key: str) -> int | None: + """``COUNT(*)`` for `path`, or None if it is not there at all.""" + try: + return catalog.gettableitemsfrom( + path, idempotency_key=_key(idempotency_key, "gettableitemsfrom", path) + ).row_count + except CatalogOperationError: + return None + + +def _carry_metadata(catalog: Catalog, entry: TreeEntry, idempotency_key: str) -> list[str]: + """Copy the wiki (and, for tables/views, Dremio's tags) across. + + Best-effort by design: metadata is descriptive, and losing a wiki is not a + reason to fail a move that otherwise verified. Whatever went wrong comes + back as notes on the entry instead. + + Not covered: the DDS ``meta.json`` sidecar the JupyterLab extension shows + under *⚙ Domain*. That lives in the Document Service, not in Dremio, and + is a different system's copy to make. + """ + problems: list[str] = [] + try: + wiki = catalog.getwikifrom( + entry.source_path, + idempotency_key=_key(idempotency_key, "getwikifrom", entry.source_path), + ) + except CatalogOperationError: + wiki = "" # no wiki, or nothing readable — normal, nothing to carry + if wiki: + try: + catalog.setwikito( + entry.target_path, + wiki, + idempotency_key=_key(idempotency_key, "setwikito", entry.target_path), + ) + except CatalogOperationError as exc: + problems.append(f"wiki not carried over: {exc}") + + if entry.kind == "FOLDER": + return problems # Dremio's tags exist only on tables and views + try: + tags = catalog.gettagsfrom( + entry.source_path, + idempotency_key=_key(idempotency_key, "gettagsfrom", entry.source_path), + ) + except CatalogOperationError: + return problems + if tags: + try: + catalog.settagsto( + entry.target_path, + tags, + idempotency_key=_key(idempotency_key, "settagsto", entry.target_path), + ) + except CatalogOperationError as exc: + problems.append(f"tags not carried over: {exc}") + return problems + + +def foldercopy( + catalog: Catalog, + source_path: str, + target_path: str, + *, + idempotency_key: str, + dry_run: bool = False, + overwrite: bool = False, + skip_existing: bool = True, + verify_views: bool = True, + copy_metadata: bool = True, + qualify_external: bool = True, + tree: FolderTree | None = None, + on_progress: ProgressCallback | None = None, +) -> FolderTransferReport: + """Reproduce the whole `source_path` subtree under `target_path`, and prove it. + + The source is left completely untouched — this is the copy half of + `foldermove`, and useful on its own for taking a snapshot of a folder. + + * `dry_run=True` writes nothing and returns the plan, with each table's + source row count filled in so you can see what a real run would move. + * `overwrite=True` replaces whatever is already at the target, per entry. + * `skip_existing=True` (the default) leaves a target table that is already + there **and already has the source's row count** alone, reporting it as + ``skipped`` — which is what makes an interrupted run resumable by just + running it again. A target that exists with a *different* count fails + the entry rather than being silently accepted, and so does any occupied + target with `skip_existing=False`: without `overwrite=True`, nothing + already at the target is ever destroyed. + * `verify_views=True` (the default) also counts each copied view on both + sides. That is the only real proof a rewritten definition resolves to + the same data, so leave it on unless the views are expensive to run. + * `copy_metadata=True` carries each entry's wiki and tags across + (best-effort — see `_carry_metadata`). + * `tree=` skips the scan and applies a `FolderTree` you already have — + the "look at the plan, then run it" flow, without paying for a second + scan or risking a plan that differs from the one you read. + + Returns a `FolderTransferReport`. A per-entry failure does not raise: the + whole point is to finish the sweep and report, so one bad table does not + hide the state of the other forty. + """ + if tree is None: + tree = folderscan( + catalog, + source_path, + target_path, + idempotency_key=idempotency_key, + qualify_external=qualify_external, + on_progress=on_progress, + ) + + report = FolderTransferReport( + source_path=tree.source_path, + target_path=tree.target_path, + dry_run=dry_run, + notes=list(tree.notes), + ) + report.folders = [EntryResult(entry=entry) for entry in tree.folders] + report.tables = [EntryResult(entry=entry) for entry in tree.tables] + report.views = [ + EntryResult(entry=entry, sql=entry.remapped_sql, notes=list(entry.notes)) + for entry in tree.views + ] + + if dry_run: + for result in report.tables: + result.source_rows = _row_count(catalog, result.entry.source_path, idempotency_key) + for result in report.views: + if result.entry.remapped_sql is None: + result.status = "failed" + result.error = "no usable view definition — see the notes on this entry" + return report + + for result in report.folders: + entry = result.entry + try: + catalog.createfolder( + entry.target_path, + create_parents=True, + idempotency_key=_key(idempotency_key, "createfolder", entry.target_path), + ) + result.status = "created" + _emit(on_progress, f"folder {entry.target_path}") + except CatalogOperationError as exc: + result.status = "failed" + result.error = str(exc) + _emit(on_progress, f"FAILED folder {entry.target_path}: {exc}") + + for result in report.tables: + entry = result.entry + try: + result.source_rows = _row_count(catalog, entry.source_path, idempotency_key) + existing_rows = _row_count(catalog, entry.target_path, idempotency_key) + # Something is already at the target. `overwrite=False` never + # destroys it — not even to "finish" the copy — so the only two + # ways past here are accepting it (skip_existing, and only when + # the counts already agree) or being told to replace it. + if existing_rows is not None and not overwrite: + result.target_rows = existing_rows + if skip_existing and existing_rows == result.source_rows: + result.status = "skipped" + _emit(on_progress, f"table {entry.target_path} (there already, counts agree)") + continue + result.status = "failed" + result.error = ( + f"{entry.target_path!r} already exists with {existing_rows} row(s) and the " + f"source has {result.source_rows} — pass overwrite=True to replace it" + ) + _emit(on_progress, f"FAILED table {entry.target_path}: {result.error}") + continue + catalog.datacopy( + entry.source_path, + entry.target_path, + overwrite=overwrite, + create_target_folder=True, + idempotency_key=_key(idempotency_key, "datacopy", entry.target_path), + ) + result.target_rows = _row_count(catalog, entry.target_path, idempotency_key) + result.status = "copied" + _emit( + on_progress, + f"table {entry.target_path} " + f"({result.source_rows} -> {result.target_rows} rows)", + ) + except CatalogOperationError as exc: + result.status = "failed" + result.error = str(exc) + _emit(on_progress, f"FAILED table {entry.target_path}: {exc}") + + for result in report.views: + entry = result.entry + if entry.remapped_sql is None: + result.status = "failed" + result.error = "no usable view definition — see the notes on this entry" + _emit(on_progress, f"FAILED view {entry.target_path}: {result.error}") + continue + try: + already_there = catalog.entryexists( + entry.target_path, + idempotency_key=_key(idempotency_key, "entryexists", entry.target_path), + ) + if already_there and skip_existing and not overwrite: + result.status = "skipped" + _emit(on_progress, f"view {entry.target_path} (there already)") + else: + # `overwrite` is passed through as given, so an occupied target + # with overwrite=False fails here rather than being replaced. + catalog.createviewas( + entry.target_path, + entry.remapped_sql, + overwrite=overwrite, + create_target_folder=True, + idempotency_key=_key(idempotency_key, "createviewas", entry.target_path), + ) + result.status = "created" + _emit(on_progress, f"view {entry.target_path}") + except CatalogOperationError as exc: + result.status = "failed" + result.error = str(exc) + _emit(on_progress, f"FAILED view {entry.target_path}: {exc}") + + if verify_views: + for result in report.views: + if result.status == "failed": + continue + try: + result.source_rows = _row_count(catalog, result.entry.source_path, idempotency_key) + result.target_rows = _row_count(catalog, result.entry.target_path, idempotency_key) + except CatalogOperationError as exc: + result.status = "failed" + result.error = f"the copied view would not run: {exc}" + continue + if not result.counts_match: + result.status = "failed" + result.error = ( + f"row counts disagree — source {result.source_rows}, " + f"target {result.target_rows}; the rewritten definition is not equivalent" + ) + _emit(on_progress, f"FAILED view {result.entry.target_path}: {result.error}") + + if copy_metadata: + for result in report.results: + if result.status in ("failed", "planned"): + continue + result.notes.extend(_carry_metadata(catalog, result.entry, idempotency_key)) + + return report + + +# -------------------------------------------------------------------------- +# moving +# -------------------------------------------------------------------------- + + +def foldermove( + catalog: Catalog, + source_path: str, + target_path: str, + *, + idempotency_key: str, + dry_run: bool = False, + overwrite: bool = False, + skip_existing: bool = True, + verify_views: bool = True, + copy_metadata: bool = True, + qualify_external: bool = True, + delete_source_root: bool = True, + tree: FolderTree | None = None, + on_progress: ProgressCallback | None = None, +) -> FolderTransferReport: + """Move the whole `source_path` subtree to `target_path`. + + `foldercopy` first — folders, tables (row-counted on both sides), views + (rewritten to point at the copies) — and then, **only if + `report.verified` came back true**, the source is taken apart: views in + reverse dependency order, then tables, then folders deepest-first, then + `source_path` itself unless `delete_source_root=False`. + + Anything less than a clean copy leaves the source exactly where it is, and + says so in `report.notes`. Nothing is ever removed on a `dry_run`. + + All of `foldercopy`'s keyword arguments mean the same thing here. + """ + report = foldercopy( + catalog, + source_path, + target_path, + idempotency_key=idempotency_key, + dry_run=dry_run, + overwrite=overwrite, + skip_existing=skip_existing, + verify_views=verify_views, + copy_metadata=copy_metadata, + qualify_external=qualify_external, + tree=tree, + on_progress=on_progress, + ) + + if dry_run: + report.notes.append( + "dry run — the source would be removed only after the copy above verified clean" + ) + return report + + if not report.verified: + plural = "y" if len(report.failures) == 1 else "ies" + report.notes.append( + f"the copy did not verify ({len(report.failures)} failed entr{plural}) — " + f"{source_path!r} was left exactly as it was. Fix what failed and run this " + "again; entries that already landed are skipped." + ) + _emit(on_progress, f"source {source_path} left untouched — the copy did not verify") + return report + + _emit(on_progress, f"copy verified — removing {source_path}") + + def remove(path: str, delete: Callable[[str], object]) -> None: + try: + delete(path) + report.deleted.append(path) + _emit(on_progress, f"removed {path}") + except CatalogOperationError as exc: + report.delete_errors.append((path, str(exc))) + _emit(on_progress, f"FAILED to remove {path}: {exc}") + + def drop_view(path: str) -> None: + catalog.deleteview(path, idempotency_key=_key(idempotency_key, "deleteview", path)) + + def drop_table(path: str) -> None: + catalog.deletetable(path, idempotency_key=_key(idempotency_key, "deletetable", path)) + + def drop_folder(path: str) -> None: + catalog.deletefolder(path, idempotency_key=_key(idempotency_key, "deletefolder", path)) + + for result in reversed(report.views): + remove(result.entry.source_path, drop_view) + for result in report.tables: + remove(result.entry.source_path, drop_table) + # Deepest first: a folder has to be empty before it will delete, and + # cascade is deliberately not used here — see the module docstring. + for result in sorted( + report.folders, key=lambda item: item.entry.source_path.count("."), reverse=True + ): + remove(result.entry.source_path, drop_folder) + if delete_source_root: + remove(source_path, drop_folder) + + report.source_deleted = not report.delete_errors + if report.delete_errors: + report.notes.append( + "the copy verified and the target is complete, but part of the source could not " + "be removed (listed above) — it is safe to re-run this, or to delete what is " + "left by hand" + ) + return report diff --git a/src/eea_datalakehouse/catalog/operations.py b/src/eea_datalakehouse/catalog/operations.py index e198763..fbdb026 100644 --- a/src/eea_datalakehouse/catalog/operations.py +++ b/src/eea_datalakehouse/catalog/operations.py @@ -206,6 +206,27 @@ def _entry_exists(executor: SqlExecutor, path: str, *, idempotency_key: str) -> return len(rows) > 0 +def entryexists( + executor: SqlExecutor, + path: str, + *, + idempotency_key: str, +) -> bool: + """Whether `path` is a table or view Dremio knows about. + + The public form of the existence check `table2view`/`datacopy`/`datamove` + already run internally. Transport-agnostic (`INFORMATION_SCHEMA`, so REST + or Flight both answer) — unlike `CatalogRestClient.exists`, which goes + through the REST catalog API and also sees folders, spaces and sources. + Fetches no rows from `path` itself, so it costs nothing on a large table. + """ + try: + return _entry_exists(executor, path, idempotency_key=idempotency_key) + except EngineStartingError as exc: + retry_state.record(idempotency_key, "entryexists", path, str(exc), params={"path": path}) + raise + + def _entry_kind(executor: SqlExecutor, path: str, *, idempotency_key: str) -> Literal["TABLE", "VIEW"]: """Whether `path` is a Dremio TABLE or VIEW, via INFORMATION_SCHEMA's own ``TABLE_TYPE`` column — doubles as an existence check. @@ -1198,6 +1219,237 @@ def gettableitemsfrom( return TableInfo(schema=schema, row_count=row_count) +@dataclass(frozen=True, slots=True) +class CatalogEntry: + """One table or view under a schema: its full dot-path, and which it is.""" + + path: str + kind: Literal["TABLE", "VIEW"] + + +def getentriesfrom( + executor: SqlExecutor, + schema_path: str, + *, + idempotency_key: str, +) -> list[CatalogEntry]: + """`gettablesfrom`, but each path carries whether it is a TABLE or a VIEW. + + Same single query over the whole subtree (see `gettablesfrom` for the + `LIKE` reasoning), with ``TABLE_TYPE`` selected as well — so a caller + copying a folder knows per entry what it is without one `_entry_kind` + round-trip per path. It matters: copying a view with + ``CREATE TABLE … AS SELECT`` silently materialises it into a table, + which is a different kind of thing from the original. + + Sorted by path, so two runs enumerate in the same order. + """ + like_pattern = f"{_like_escape(schema_path)}.%" + sql = ( + 'SELECT "TABLE_SCHEMA", "TABLE_NAME", "TABLE_TYPE" FROM INFORMATION_SCHEMA."TABLES" ' + f"WHERE \"TABLE_SCHEMA\" = {_quote_literal(schema_path)} " + f"OR \"TABLE_SCHEMA\" LIKE {_quote_literal(like_pattern)} ESCAPE '\\'" + ) + rows = _fetch_step( + executor, + sql, + operation="getentriesfrom", + target=schema_path, + idempotency_key=idempotency_key, + params={"schema_path": schema_path}, + ) + entries = [ + CatalogEntry( + path=f"{row['TABLE_SCHEMA']}.{row['TABLE_NAME']}", + kind="VIEW" if "VIEW" in str(row.get("TABLE_TYPE", "")).upper() else "TABLE", + ) + for row in rows + ] + return sorted(entries, key=lambda entry: entry.path) + + +@dataclass(frozen=True, slots=True) +class ViewDefinition: + """A view's own SQL, and the catalog context it was written against. + + `context` is the ``sqlContext`` Dremio stores beside the SQL — the schema + an unqualified ``FROM sales`` inside `sql` resolves against. It is empty + when the definition came from `INFORMATION_SCHEMA."VIEWS"`, which has no + equivalent column: an empty `context` means *unknown*, not *none*. + """ + + sql: str + context: tuple[str, ...] = () + + +def getviewsql( + executor: SqlExecutor, + view_path: str, + *, + catalog_rest: CatalogRestClient | None = None, + idempotency_key: str, +) -> ViewDefinition: + """The SQL the view at `view_path` is defined by. + + Two sources, in this order — they answer with different amounts of the + truth, so the better one is tried first: + + 1. `catalog_rest.get_view_definition` (Dremio's catalog entity), which + carries the SQL **and** its ``sqlContext``. Skipped without a + `catalog_rest`, and fallen through if the lookup is rejected — some + Dremio source types answer a nested by-path lookup with a 400 (see + `CatalogRestClient.is_folder`). + 2. ``INFORMATION_SCHEMA."VIEWS"``, which has ``VIEW_DEFINITION`` but no + context — so the result's `context` is empty and any unqualified + reference inside the SQL cannot be resolved from it. + + Raises `CatalogOperationError` if neither answers — a view whose + definition cannot be read is not something a caller should quietly + replace with a guess. + """ + if catalog_rest is not None: + try: + definition = catalog_rest.get_view_definition(view_path) + except CatalogOperationError: + definition = None + if definition is not None: + sql, context = definition + retry_state.clear(idempotency_key) + return ViewDefinition(sql=sql, context=tuple(context)) + + schema_path = _parent_path(view_path) + if schema_path is None: + raise ValueError(f"{view_path!r} has no containing schema") + leaf = view_path.rsplit(".", 1)[-1] + sql_query = ( + 'SELECT "VIEW_DEFINITION" FROM INFORMATION_SCHEMA."VIEWS" ' + f"WHERE \"TABLE_SCHEMA\" = {_quote_literal(schema_path)} " + f"AND \"TABLE_NAME\" = {_quote_literal(leaf)}" + ) + rows = _fetch_step( + executor, + sql_query, + operation="getviewsql", + target=view_path, + idempotency_key=idempotency_key, + params={"view_path": view_path}, + ) + definition_sql = str(rows[0].get("VIEW_DEFINITION", "")) if rows else "" + if not definition_sql.strip(): + raise CatalogOperationError( + f"could not read the view definition of {view_path!r} — " + "it is not a view, or Dremio reports no SQL for it" + ) + return ViewDefinition(sql=definition_sql, context=()) + + +def createviewas( + executor: SqlExecutor, + view_path: str, + sql: str, + *, + overwrite: bool = False, + create_target_folder: bool = True, + catalog_rest: CatalogRestClient | None = None, + idempotency_key: str, +) -> SqlResult: + """Create the view at `view_path` from an arbitrary `sql` query body. + + `table2view`'s general form: that one always writes + ``SELECT * FROM ``, this one takes whatever query you hand it — + which is what a folder copy needs, since a copied view has to select + from the *copied* tables, not the originals (see + `eea_datalakehouse.catalog.foldertree`). + + `sql` is the query body only (no ``CREATE VIEW`` prefix) and is sent + through unchanged — quoting and correctness are the caller's, since the + text usually comes back out of Dremio itself. + + `overwrite=False` (the default) raises `CatalogOperationError` if + anything already sits at `view_path`, rather than letting Dremio answer + with its own less specific error. `overwrite=True` drops whatever is + actually there first, detecting whether it is a table or a view — the + same reasoning as `datacopy`'s own `check_or_drop_target`. + """ + params: dict[str, Any] = { + "view_path": view_path, + "sql": sql, + "overwrite": overwrite, + "create_target_folder": create_target_folder, + } + + def ensure_target_folder() -> None: + if not create_target_folder: + return + parent = _parent_path(view_path) + if parent is None: + return + if catalog_rest is None: + raise CatalogOperationError( + "createviewas(create_target_folder=True) needs catalog_rest= " + "to check/create the target folder" + ) + catalog_rest.ensure_folder_path(parent) + + def check_or_drop_target() -> None: + if not _entry_exists(executor, view_path, idempotency_key=idempotency_key): + return + if not overwrite: + raise CatalogOperationError( + f"target {view_path!r} already exists — pass overwrite=True to replace it" + ) + existing_kind = _entry_kind(executor, view_path, idempotency_key=idempotency_key) + executor.execute( + f"DROP {existing_kind} IF EXISTS {_quote_path(view_path)}", + idempotency_key=idempotency_key, + ) + + def create_view() -> SqlResult: + return executor.execute( + f"CREATE VIEW {_quote_path(view_path)} AS {sql}", idempotency_key=idempotency_key + ) + + return _run_actions( + [ensure_target_folder, check_or_drop_target, create_view], + operation="createviewas", + target=view_path, + idempotency_key=idempotency_key, + params=params, + ) + + +def getfoldersfrom( + catalog_rest: CatalogRestClient, + path: str, + *, + recursive: bool = True, + idempotency_key: str, +) -> list[str]: + """Dot-paths of the folders under `path`, shallowest first. + + REST-only (see module docstring) — folders are not in + `INFORMATION_SCHEMA`, which only knows about tables and views, so an + *empty* folder is invisible to `gettablesfrom`/`getentriesfrom` and can + only be found this way. + + Breadth-first, which is the order they have to be created in on the + other side. `path` itself is not included. + """ + try: + folders = catalog_rest.list_folders(path, recursive=recursive) + except EngineStartingError as exc: + retry_state.record( + idempotency_key, + "getfoldersfrom", + path, + str(exc), + params={"path": path, "recursive": recursive}, + ) + raise + retry_state.clear(idempotency_key) + return folders + + def getwikifrom( catalog_rest: CatalogRestClient, path: str, @@ -1674,6 +1926,11 @@ def delete_dataset(dataset_path: str) -> None: "deletetable": deletetable, "gettablesfrom": gettablesfrom, "gettableitemsfrom": gettableitemsfrom, + "entryexists": entryexists, + "getentriesfrom": getentriesfrom, + "getviewsql": getviewsql, + "createviewas": createviewas, + "getfoldersfrom": getfoldersfrom, "getwikifrom": getwikifrom, "gettagsfrom": gettagsfrom, "setwikito": setwikito, diff --git a/src/eea_datalakehouse/catalog/rest.py b/src/eea_datalakehouse/catalog/rest.py index d0b3344..7f14605 100644 --- a/src/eea_datalakehouse/catalog/rest.py +++ b/src/eea_datalakehouse/catalog/rest.py @@ -230,6 +230,74 @@ def is_table_or_view(self, path: str) -> bool: return False return entity is not None and entity.get("entityType") == "dataset" + def children(self, path: str) -> list[dict[str, Any]]: + """Dremio's own child JSON for every entity directly inside `path`. + + Each child carries at least ``path`` (a list of segments), ``id`` and + ``type``; a container also carries ``containerType`` (see + `_child_is_folder`). Raises `CatalogOperationError` if `path` does not + exist — unlike `is_folder`/`is_table_or_view`, which swallow a failed + lookup, because "I could not read this folder" and "this folder is + empty" are not the same answer for a caller that is about to copy or + delete its contents. + """ + entity = self._lookup_by_path(path) + if entity is None: + raise CatalogOperationError(f"{path!r} does not exist") + return list(entity.get("children") or []) + + def list_folders(self, path: str, *, recursive: bool = True) -> list[str]: + """Dot-paths of the folders inside `path`, shallowest first. + + `path` itself is not included. `recursive=True` (the default) walks + the whole subtree; `False` returns only the direct children. Ordering + is breadth-first, which is the order a caller has to *create* them in + on the other side — a parent before anything under it. + + Same unverified assumption as `delete_folder`'s cascade: a folder + child is recognised by ``type="CONTAINER"`` / + ``containerType="FOLDER"``. + """ + found: list[str] = [] + queue = [path] + while queue: + current = queue.pop(0) + for child in self.children(current): + if not self._child_is_folder(child): + continue + child_path = ".".join(child.get("path") or []) + if not child_path: + continue + found.append(child_path) + if recursive: + queue.append(child_path) + return found + + def get_view_definition(self, path: str) -> tuple[str, list[str]] | None: + """``(sql, sql_context)`` for the view at `path`, or None if `path` is + not a view (or Dremio's entity carries no SQL for it). + + Dremio's catalog entity for a virtual dataset carries both the SQL + *as written* and the ``sqlContext`` it was written against — and the + context is what makes an unqualified ``FROM sales`` inside that SQL + resolvable. `INFORMATION_SCHEMA."VIEWS"` has the definition but not + the context, which is why this REST route is preferred where it + answers (see `getviewsql`). + + Best-effort about *reachability*, not about correctness: a lookup + Dremio rejects outright (the 400 described in `is_folder`) surfaces as + `CatalogOperationError`, so the caller can fall back rather than + silently copy a view with no definition. + """ + entity = self._lookup_by_path(path) + if entity is None: + raise CatalogOperationError(f"{path!r} does not exist") + sql = entity.get("sql") + if not sql: + return None + context = [str(segment) for segment in (entity.get("sqlContext") or [])] + return str(sql), context + def get_wiki(self, path: str) -> str: """The Dremio wiki text attached to the catalog entity at `path`. diff --git a/tests/catalog/conftest.py b/tests/catalog/conftest.py index a9b3220..a2c69c4 100644 --- a/tests/catalog/conftest.py +++ b/tests/catalog/conftest.py @@ -106,6 +106,9 @@ def __init__( self._raise_on_create_folder = raise_on_create_folder self._raise_on_delete_folder = raise_on_delete_folder self._raise_on_exists = raise_on_exists + # Seeded per-test by assignment, since only the folder-tree tests care. + self.view_definitions: dict[str, tuple[str, list[str]]] = {} + self.folder_listing: list[str] = [] def exists(self, path: str) -> bool: if self._raise_on_exists is not None: @@ -183,6 +186,20 @@ def set_tags(self, path: str, tags: list[str]) -> None: raise CatalogOperationError(f"{path!r} does not exist") self._tags[path] = list(tags) + def get_view_definition(self, path: str) -> tuple[str, list[str]] | None: + from eea_datalakehouse.catalog.errors import CatalogOperationError + + if path not in self.existing: + raise CatalogOperationError(f"{path!r} does not exist") + return self.view_definitions.get(path) + + def list_folders(self, path: str, *, recursive: bool = True) -> list[str]: + from eea_datalakehouse.catalog.errors import CatalogOperationError + + if path not in self.existing: + raise CatalogOperationError(f"{path!r} does not exist") + return list(self.folder_listing) + def ensure_folder_path(self, path: str) -> list[str]: from eea_datalakehouse.catalog.errors import CatalogOperationError diff --git a/tests/catalog/test_foldertree.py b/tests/catalog/test_foldertree.py new file mode 100644 index 0000000..40a7dd9 --- /dev/null +++ b/tests/catalog/test_foldertree.py @@ -0,0 +1,688 @@ +"""foldertree — scanning a subtree, rewriting view SQL, copying with proof, +and only then removing the source.""" + +from __future__ import annotations + +from pathlib import Path +from typing import Any + +import pytest + +from eea_datalakehouse.catalog import foldertree, retry_state +from eea_datalakehouse.catalog.errors import CatalogOperationError +from eea_datalakehouse.catalog.foldertree import ( + foldercopy, + foldermove, + folderscan, + remap_view_sql, +) +from eea_datalakehouse.catalog.operations import CatalogEntry, TableInfo, ViewDefinition + + +@pytest.fixture(autouse=True) +def _isolated_state(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + """Never touch the real ~/.cache retry-state file.""" + state_path = tmp_path / "state.json" + monkeypatch.setattr(retry_state, "DEFAULT_STATE_PATH", state_path) + return state_path + + +class FakeCatalog: + """In-memory stand-in for `Catalog`, holding a whole little catalog. + + `tables` maps a dot-path to a row count, `views` maps one to + ``(sql, context)``, `folders` is a set of folder paths. Everything + `foldertree` calls is a real method here, so a test asserts against the + catalog's *state* afterwards rather than against a list of SQL strings — + which is what makes "did the move actually move it" checkable. + """ + + def __init__( + self, + tables: dict[str, int] | None = None, + views: dict[str, tuple[str, tuple[str, ...]]] | None = None, + folders: set[str] | None = None, + wikis: dict[str, str] | None = None, + tags: dict[str, list[str]] | None = None, + fail_copy_of: set[str] | None = None, + short_copy_of: set[str] | None = None, + ) -> None: + self.tables = dict(tables or {}) + self.views = dict(views or {}) + self.folders = set(folders or set()) + self.wikis = dict(wikis or {}) + self.tags = dict(tags or {}) + self.calls: list[tuple[str, str]] = [] + self.keys: list[str] = [] + self._fail_copy_of = set(fail_copy_of or set()) + self._short_copy_of = set(short_copy_of or set()) + + # -- helpers --------------------------------------------------------- + def _record(self, operation: str, path: str, idempotency_key: str) -> None: + self.calls.append((operation, path)) + self.keys.append(idempotency_key) + + def _under(self, prefix: str, paths) -> list[str]: + return [p for p in paths if p == prefix or p.startswith(f"{prefix}.")] + + # -- discovery ------------------------------------------------------- + def getentriesfrom(self, schema_path: str, *, idempotency_key: str) -> list[CatalogEntry]: + self._record("getentriesfrom", schema_path, idempotency_key) + tables = self._under(schema_path, self.tables) + views = self._under(schema_path, self.views) + entries = [CatalogEntry(path=p, kind="TABLE") for p in tables] + entries += [CatalogEntry(path=p, kind="VIEW") for p in views] + return sorted(entries, key=lambda entry: entry.path) + + def getfoldersfrom( + self, path: str, *, recursive: bool = True, idempotency_key: str + ) -> list[str]: + self._record("getfoldersfrom", path, idempotency_key) + if path not in self.folders: + raise CatalogOperationError(f"{path!r} does not exist") + found = [p for p in self.folders if p.startswith(f"{path}.")] + return sorted(found, key=lambda p: (p.count("."), p)) + + def entryexists(self, path: str, *, idempotency_key: str) -> bool: + self._record("entryexists", path, idempotency_key) + return path in self.tables or path in self.views + + def getviewsql(self, view_path: str, *, idempotency_key: str) -> ViewDefinition: + self._record("getviewsql", view_path, idempotency_key) + if view_path not in self.views: + raise CatalogOperationError(f"{view_path!r} is not a view") + sql, context = self.views[view_path] + return ViewDefinition(sql=sql, context=context) + + def gettableitemsfrom(self, path: str, *, idempotency_key: str) -> TableInfo: + self._record("gettableitemsfrom", path, idempotency_key) + if path in self.tables: + return TableInfo(schema={"c": "INTEGER"}, row_count=self.tables[path]) + if path in self.views: + return TableInfo(schema={"c": "INTEGER"}, row_count=self._view_rows(path)) + raise CatalogOperationError(f"{path!r} does not exist") + + def _view_rows(self, view_path: str) -> int: + """A view counts whatever the first thing its SQL names counts. + + Quotes are stripped before matching, so a rewritten + ``"bwd"."archive"."raw"."t"`` really does resolve to ``bwd.archive.raw.t`` + — which is what makes the row-count check on a copied view mean + something. A view naming nothing that exists counts 0, the way a broken + rewrite would show up. + """ + seen: set[str] = set() + path, sql = view_path, self.views[view_path][0].replace('"', "") + while path not in seen: + seen.add(path) + for table, count in self.tables.items(): + if table.lower() in sql.lower(): + return count + nested = next((v for v in self.views if v != path and v.lower() in sql.lower()), None) + if nested is None: + return 0 + path, sql = nested, self.views[nested][0].replace('"', "") + return 0 + + # -- writes ---------------------------------------------------------- + def createfolder( + self, path: str, *, create_parents: bool = False, idempotency_key: str + ) -> None: + self._record("createfolder", path, idempotency_key) + self.folders.add(path) + + def datacopy( + self, + source_path: str, + target_path: str, + *, + overwrite: bool = False, + create_target_folder: bool = False, + idempotency_key: str, + ) -> None: + self._record("datacopy", target_path, idempotency_key) + if source_path in self._fail_copy_of: + raise CatalogOperationError(f"copy of {source_path!r} refused") + rows = self.tables[source_path] + self.tables[target_path] = rows - 1 if source_path in self._short_copy_of else rows + + def createviewas( + self, + view_path: str, + sql: str, + *, + overwrite: bool = False, + create_target_folder: bool = True, + idempotency_key: str, + ) -> None: + self._record("createviewas", view_path, idempotency_key) + self.views[view_path] = (sql, ()) + + def deletetable(self, table_path: str, *, idempotency_key: str) -> None: + self._record("deletetable", table_path, idempotency_key) + self.tables.pop(table_path, None) + + def deleteview(self, view_path: str, *, idempotency_key: str) -> None: + self._record("deleteview", view_path, idempotency_key) + self.views.pop(view_path, None) + + def deletefolder(self, path: str, *, cascade: bool = False, idempotency_key: str) -> None: + self._record("deletefolder", path, idempotency_key) + still_inside = [ + p + for p in (*self.tables, *self.views, *self.folders) + if p.startswith(f"{path}.") + ] + if still_inside: + raise CatalogOperationError(f"folder {path!r} is not empty: {sorted(still_inside)}") + self.folders.discard(path) + + # -- metadata -------------------------------------------------------- + def getwikifrom(self, path: str, *, idempotency_key: str) -> str: + if path not in self.wikis: + raise CatalogOperationError(f"{path!r} has no wiki") + return self.wikis[path] + + def setwikito(self, path: str, text: str, *, idempotency_key: str, **_: Any) -> None: + self.wikis[path] = text + + def gettagsfrom(self, path: str, *, idempotency_key: str) -> list[str]: + return self.tags.get(path, []) + + def settagsto(self, path: str, tags: list[str], *, idempotency_key: str) -> None: + self.tags[path] = list(tags) + + +def make_catalog(**overrides: Any) -> FakeCatalog: + """The tree every test below starts from, unless it says otherwise. + + bwd.draft (folder) + bwd.draft.raw (folder) + bwd.draft.raw.measurements 100 rows + bwd.draft.raw.stations 10 rows + bwd.draft.assessments (view over raw.measurements) + bwd.draft.summary (view over the assessments view) + """ + base: dict[str, Any] = { + "folders": {"bwd", "bwd.draft", "bwd.draft.raw", "bwd.archive"}, + "tables": {"bwd.draft.raw.measurements": 100, "bwd.draft.raw.stations": 10}, + "views": { + "bwd.draft.assessments": ( + 'SELECT * FROM "bwd"."draft"."raw"."measurements" WHERE ok = true', + (), + ), + "bwd.draft.summary": ('SELECT * FROM "bwd"."draft"."assessments"', ()), + }, + } + base.update(overrides) + return FakeCatalog(**base) + + +# --------------------------------------------------------------------------- +# remap_view_sql +# --------------------------------------------------------------------------- + + +ENTRIES = { + "bwd.draft.raw.measurements": "bwd.archive.raw.measurements", + "bwd.draft.assessments": "bwd.archive.assessments", +} + + +def remap(sql: str, **kwargs: Any): + return remap_view_sql( + sql, source_path="bwd.draft", target_path="bwd.archive", entries=ENTRIES, **kwargs + ) + + +def test_rewrites_a_fully_qualified_reference_into_the_target() -> None: + result = remap('SELECT * FROM "bwd"."draft"."raw"."measurements"') + + assert result.sql == 'SELECT * FROM "bwd"."archive"."raw"."measurements"' + assert result.references == ("bwd.draft.raw.measurements",) + + +def test_rewrites_an_unquoted_reference_and_keeps_the_column_it_qualifies() -> None: + result = remap("SELECT bwd.draft.assessments.value FROM bwd.draft.assessments") + + assert result.sql == ( + 'SELECT "bwd"."archive"."assessments"."value" FROM "bwd"."archive"."assessments"' + ) + + +def test_matches_paths_case_insensitively() -> None: + assert remap('SELECT * FROM "BWD"."Draft"."raw"."Measurements"').sql == ( + 'SELECT * FROM "bwd"."archive"."raw"."measurements"' + ) + + +def test_leaves_references_outside_the_moved_subtree_alone() -> None: + sql = 'SELECT * FROM "other"."space"."t" JOIN "bwd"."draft"."assessments" ON 1 = 1' + + assert remap(sql).sql == ( + 'SELECT * FROM "other"."space"."t" JOIN "bwd"."archive"."assessments" ON 1 = 1' + ) + + +def test_never_rewrites_inside_a_string_literal_or_a_comment() -> None: + sql = ( + "SELECT 'bwd.draft.raw.measurements' AS label, " + "'it''s bwd.draft here too' AS quoted " + "-- and bwd.draft.assessments in a comment\n" + "/* and bwd.draft.assessments in a block */ " + 'FROM "bwd"."draft"."assessments"' + ) + result = remap(sql) + + assert "'bwd.draft.raw.measurements'" in result.sql + assert "'it''s bwd.draft here too'" in result.sql + assert "-- and bwd.draft.assessments in a comment" in result.sql + assert "/* and bwd.draft.assessments in a block */" in result.sql + assert result.sql.endswith('FROM "bwd"."archive"."assessments"') + + +def test_does_not_read_a_decimal_number_as_a_path() -> None: + assert remap("SELECT 1.5 AS x FROM t").sql == "SELECT 1.5 AS x FROM t" + + +def test_spells_out_a_context_relative_reference_into_the_subtree() -> None: + # The view said `FROM assessments`, resolved against bwd.draft. After the + # move it has no such context, so the reference has to be written in full. + result = remap("SELECT * FROM assessments", context=("bwd", "draft")) + + assert result.sql == 'SELECT * FROM "bwd"."archive"."assessments"' + assert result.references == ("bwd.draft.assessments",) + + +def test_qualifies_a_context_relative_reference_that_is_not_moving() -> None: + probed: list[str] = [] + + def exists(path: str) -> bool: + probed.append(path) + return path == "bwd.reference.codelist" + + result = remap( + "SELECT * FROM reference.codelist", + context=("bwd",), + external_exists=exists, + ) + + assert result.sql == 'SELECT * FROM "bwd"."reference"."codelist"' + assert probed == ["bwd.reference.codelist"] + assert result.notes # it says what it did + + +def test_keeps_the_column_on_a_context_relative_reference() -> None: + result = remap("SELECT assessments.value FROM assessments", context=("bwd", "draft")) + + assert result.sql == ( + 'SELECT "bwd"."archive"."assessments"."value" FROM "bwd"."archive"."assessments"' + ) + + +def test_does_not_probe_when_no_external_exists_is_given() -> None: + assert remap("SELECT * FROM reference.codelist", context=("bwd",)).sql == ( + "SELECT * FROM reference.codelist" + ) + + +def test_says_so_when_the_context_is_unknown_and_a_bare_name_is_used() -> None: + result = remap("SELECT * FROM assessments") + + assert result.sql == "SELECT * FROM assessments" + assert any("sqlContext is unknown" in note for note in result.notes) + + +def test_falls_back_to_the_source_root_for_a_path_the_scan_did_not_list() -> None: + # A table created between the scan and the rewrite is still under the + # source root, so its prefix is swapped anyway. + assert remap('SELECT * FROM "bwd"."draft"."raw"."brand_new"').sql == ( + 'SELECT * FROM "bwd"."archive"."raw"."brand_new"' + ) + + +# --------------------------------------------------------------------------- +# folderscan +# --------------------------------------------------------------------------- + + +def test_scan_finds_folders_tables_and_views_and_maps_them_to_the_target() -> None: + tree = folderscan(make_catalog(), "bwd.draft", "bwd.archive.2025", idempotency_key="k") + + assert [entry.source_path for entry in tree.folders] == ["bwd.draft.raw"] + assert [entry.target_path for entry in tree.folders] == ["bwd.archive.2025.raw"] + assert [entry.source_path for entry in tree.tables] == [ + "bwd.draft.raw.measurements", + "bwd.draft.raw.stations", + ] + assert [entry.target_path for entry in tree.tables] == [ + "bwd.archive.2025.raw.measurements", + "bwd.archive.2025.raw.stations", + ] + assert len(tree) == 5 + + +def test_scan_puts_a_view_after_the_view_it_selects_from() -> None: + tree = folderscan(make_catalog(), "bwd.draft", "bwd.archive.2025", idempotency_key="k") + + ordered = [entry.source_path for entry in tree.views] + assert ordered.index("bwd.draft.assessments") < ordered.index("bwd.draft.summary") + summary = next(e for e in tree.views if e.source_path == "bwd.draft.summary") + assert summary.depends_on == ("bwd.draft.assessments",) + assert summary.remapped_sql == 'SELECT * FROM "bwd"."archive"."2025"."assessments"' + + +def test_scan_derives_folders_from_entry_paths_when_rest_cannot_list_them() -> None: + catalog = make_catalog(folders=set()) # REST answers "does not exist" + tree = folderscan(catalog, "bwd.draft", "bwd.archive", idempotency_key="k") + + assert [entry.source_path for entry in tree.folders] == ["bwd.draft.raw"] + assert any("rebuilt from the paths" in note for note in tree.notes) + + +def test_scan_refuses_a_target_inside_the_source() -> None: + with pytest.raises(CatalogOperationError, match="inside source"): + folderscan(make_catalog(), "bwd.draft", "bwd.draft.archive", idempotency_key="k") + + +def test_scan_refuses_a_source_inside_the_target() -> None: + with pytest.raises(CatalogOperationError, match="inside target"): + folderscan(make_catalog(), "bwd.draft.raw", "bwd.draft", idempotency_key="k") + + +def test_scan_refuses_the_same_path_twice() -> None: + with pytest.raises(CatalogOperationError, match="same folder"): + folderscan(make_catalog(), "bwd.draft", "BWD.Draft", idempotency_key="k") + + +def test_scan_raises_when_the_source_cannot_be_read_at_all() -> None: + with pytest.raises(CatalogOperationError, match="could not read"): + folderscan(FakeCatalog(), "bwd.nothing", "bwd.archive", idempotency_key="k") + + +# --------------------------------------------------------------------------- +# foldercopy +# --------------------------------------------------------------------------- + + +def test_dry_run_writes_nothing_and_counts_the_source() -> None: + catalog = make_catalog() + report = foldercopy( + catalog, "bwd.draft", "bwd.archive.2025", idempotency_key="k", dry_run=True + ) + + assert [row.status for row in report.results] == ["planned"] * 5 + assert [row.source_rows for row in report.tables] == [100, 10] + assert report.verified is False + assert not any(op in {"datacopy", "createviewas", "createfolder"} for op, _ in catalog.calls) + + +def test_copy_reproduces_the_tree_and_verifies_by_row_count() -> None: + catalog = make_catalog() + report = foldercopy(catalog, "bwd.draft", "bwd.archive.2025", idempotency_key="k") + + assert report.verified is True + assert catalog.tables["bwd.archive.2025.raw.measurements"] == 100 + assert catalog.tables["bwd.archive.2025.raw.stations"] == 10 + assert [(row.source_rows, row.target_rows) for row in report.tables] == [(100, 100), (10, 10)] + # the copied view points at the copy, not back at the original + assert catalog.views["bwd.archive.2025.assessments"][0] == ( + 'SELECT * FROM "bwd"."archive"."2025"."raw"."measurements" WHERE ok = true' + ) + # and it resolves: both sides of every view counted the same real rows, + # rather than both quietly counting nothing + assert [(row.source_rows, row.target_rows) for row in report.views] == [(100, 100), (100, 100)] + # and the source is untouched + assert catalog.tables["bwd.draft.raw.measurements"] == 100 + assert "bwd.draft.assessments" in catalog.views + + +def test_copy_fails_the_entry_when_the_counts_disagree() -> None: + catalog = make_catalog(short_copy_of={"bwd.draft.raw.stations"}) + report = foldercopy(catalog, "bwd.draft", "bwd.archive.2025", idempotency_key="k") + + assert report.verified is False + stations = next(r for r in report.tables if r.entry.name == "stations") + assert (stations.source_rows, stations.target_rows) == (10, 9) + assert stations.counts_match is False + + +def test_one_failed_table_does_not_stop_the_rest_of_the_sweep() -> None: + catalog = make_catalog(fail_copy_of={"bwd.draft.raw.measurements"}) + report = foldercopy(catalog, "bwd.draft", "bwd.archive.2025", idempotency_key="k") + + assert [row.status for row in report.tables] == ["failed", "copied"] + assert catalog.tables["bwd.archive.2025.raw.stations"] == 10 + assert report.verified is False + + +def test_a_stale_view_already_at_the_target_is_caught_by_the_count() -> None: + # Something left over from an earlier, different job sits where this view + # should go. skip_existing leaves it alone — but counting both sides is + # what stops that from passing for a copy. + catalog = make_catalog() + stale_sql = 'SELECT * FROM "bwd"."draft"."raw"."stations"' + catalog.views["bwd.archive.2025.assessments"] = (stale_sql, ()) + + report = foldercopy(catalog, "bwd.draft", "bwd.archive.2025", idempotency_key="k") + + stale = next(r for r in report.views if r.entry.name == "assessments") + assert stale.status == "failed" + assert (stale.source_rows, stale.target_rows) == (100, 10) + assert "not equivalent" in (stale.error or "") + assert report.verified is False + + +def test_views_are_not_counted_when_verify_views_is_off() -> None: + catalog = make_catalog() + report = foldercopy( + catalog, "bwd.draft", "bwd.archive.2025", idempotency_key="k", verify_views=False + ) + + assert [row.source_rows for row in report.views] == [None, None] + assert report.verified is True + + +def test_re_running_skips_what_already_landed() -> None: + catalog = make_catalog() + foldercopy(catalog, "bwd.draft", "bwd.archive.2025", idempotency_key="k") + catalog.calls.clear() + + report = foldercopy(catalog, "bwd.draft", "bwd.archive.2025", idempotency_key="k") + + assert [row.status for row in report.tables] == ["skipped", "skipped"] + assert [row.status for row in report.views] == ["skipped", "skipped"] + assert report.verified is True + assert not any(op == "datacopy" for op, _ in catalog.calls) + + +def test_an_existing_target_with_a_different_count_fails_rather_than_being_accepted() -> None: + catalog = make_catalog() + catalog.tables["bwd.archive.2025.raw.measurements"] = 7 + + report = foldercopy(catalog, "bwd.draft", "bwd.archive.2025", idempotency_key="k") + + failed = next(r for r in report.tables if r.entry.name == "measurements") + assert failed.status == "failed" + assert "overwrite=True" in (failed.error or "") + assert catalog.tables["bwd.archive.2025.raw.measurements"] == 7 # left alone + + +def test_overwrite_replaces_an_existing_target() -> None: + catalog = make_catalog() + catalog.tables["bwd.archive.2025.raw.measurements"] = 7 + + report = foldercopy( + catalog, "bwd.draft", "bwd.archive.2025", idempotency_key="k", overwrite=True + ) + + assert report.verified is True + assert catalog.tables["bwd.archive.2025.raw.measurements"] == 100 + + +def test_metadata_travels_with_the_entries() -> None: + catalog = make_catalog( + wikis={"bwd.draft.raw": "the raw landing zone"}, + tags={"bwd.draft.raw.measurements": ["bronze"]}, + ) + foldercopy(catalog, "bwd.draft", "bwd.archive.2025", idempotency_key="k") + + assert catalog.wikis["bwd.archive.2025.raw"] == "the raw landing zone" + assert catalog.tags["bwd.archive.2025.raw.measurements"] == ["bronze"] + + +def test_every_entry_gets_its_own_idempotency_key() -> None: + catalog = make_catalog() + foldercopy(catalog, "bwd.draft", "bwd.archive.2025", idempotency_key="job-1") + + copy_keys = [ + key for (op, _), key in zip(catalog.calls, catalog.keys, strict=True) if op == "datacopy" + ] + assert copy_keys == [ + "job-1:datacopy:bwd.archive.2025.raw.measurements", + "job-1:datacopy:bwd.archive.2025.raw.stations", + ] + + +def test_a_prepared_tree_is_applied_without_a_second_scan() -> None: + catalog = make_catalog() + tree = folderscan(catalog, "bwd.draft", "bwd.archive.2025", idempotency_key="k") + catalog.calls.clear() + + foldercopy(catalog, "bwd.draft", "bwd.archive.2025", idempotency_key="k", tree=tree) + + assert not any(op == "getentriesfrom" for op, _ in catalog.calls) + + +# --------------------------------------------------------------------------- +# foldermove +# --------------------------------------------------------------------------- + + +def test_move_removes_the_source_only_after_a_verified_copy() -> None: + catalog = make_catalog() + report = foldermove(catalog, "bwd.draft", "bwd.archive.2025", idempotency_key="k") + + assert report.verified is True + assert report.source_deleted is True + assert not [p for p in catalog.tables if p.startswith("bwd.draft")] + assert not [p for p in catalog.views if p.startswith("bwd.draft")] + assert "bwd.draft" not in catalog.folders + assert "bwd.draft.raw" not in catalog.folders + # everything is where it should be on the other side + assert catalog.tables["bwd.archive.2025.raw.measurements"] == 100 + assert "bwd.archive.2025.summary" in catalog.views + + +def test_move_takes_the_source_apart_in_a_deletable_order() -> None: + catalog = make_catalog() + foldermove(catalog, "bwd.draft", "bwd.archive.2025", idempotency_key="k") + + removals = [(op, path) for op, path in catalog.calls if op.startswith("delete")] + order = [path for _, path in removals] + # dependent view before the one it selects from; folders last, deepest first + assert order.index("bwd.draft.summary") < order.index("bwd.draft.assessments") + assert order.index("bwd.draft.raw.measurements") < order.index("bwd.draft.raw") + assert order.index("bwd.draft.raw") < order.index("bwd.draft") + assert order[-1] == "bwd.draft" + + +def test_move_leaves_the_source_alone_when_the_copy_did_not_verify() -> None: + catalog = make_catalog(short_copy_of={"bwd.draft.raw.stations"}) + report = foldermove(catalog, "bwd.draft", "bwd.archive.2025", idempotency_key="k") + + assert report.verified is False + assert report.source_deleted is False + assert report.deleted == [] + assert catalog.tables["bwd.draft.raw.stations"] == 10 + assert "bwd.draft" in catalog.folders + assert any("left exactly as it was" in note for note in report.notes) + + +def test_move_can_keep_the_source_root_folder() -> None: + catalog = make_catalog() + foldermove( + catalog, + "bwd.draft", + "bwd.archive.2025", + idempotency_key="k", + delete_source_root=False, + ) + + assert "bwd.draft" in catalog.folders + assert not [p for p in catalog.tables if p.startswith("bwd.draft.")] + + +def test_move_dry_run_removes_nothing() -> None: + catalog = make_catalog() + report = foldermove( + catalog, "bwd.draft", "bwd.archive.2025", idempotency_key="k", dry_run=True + ) + + assert report.source_deleted is False + assert catalog.tables["bwd.draft.raw.measurements"] == 100 + assert any("dry run" in note for note in report.notes) + + +def test_move_reports_a_source_folder_it_could_not_empty() -> None: + catalog = make_catalog() + # Something nobody told us about turns up inside the source mid-move. + original_deletetable = catalog.deletetable + + def deletetable(table_path: str, *, idempotency_key: str) -> None: + original_deletetable(table_path, idempotency_key=idempotency_key) + catalog.tables.setdefault("bwd.draft.raw.surprise", 1) + + catalog.deletetable = deletetable # type: ignore[method-assign] + report = foldermove(catalog, "bwd.draft", "bwd.archive.2025", idempotency_key="k") + + assert report.source_deleted is False + assert [path for path, _ in report.delete_errors] == ["bwd.draft.raw", "bwd.draft"] + assert any("could not be removed" in note for note in report.notes) + + +def test_summary_reads_as_a_report() -> None: + catalog = make_catalog() + report = foldermove(catalog, "bwd.draft", "bwd.archive.2025", idempotency_key="k") + text = report.summary() + + assert "RESULT: bwd.draft -> bwd.archive.2025" in text + assert "[100 == 100 rows]" in text + assert "verified: True" in text + assert "removed from the source" in text + + +def test_progress_is_reported_as_it_goes() -> None: + messages: list[str] = [] + foldermove( + make_catalog(), + "bwd.draft", + "bwd.archive.2025", + idempotency_key="k", + on_progress=messages.append, + ) + + assert any(message.startswith("scanning") for message in messages) + assert any("copy verified" in message for message in messages) + assert any(message.startswith("removed bwd.draft") for message in messages) + + +def test_module_exports_the_three_verbs() -> None: + assert foldertree.folderscan is folderscan + assert foldertree.foldercopy is foldercopy + assert foldertree.foldermove is foldermove + + +def test_nothing_at_the_target_is_destroyed_without_overwrite() -> None: + # skip_existing=False says "don't accept what's there", not "replace it". + catalog = make_catalog() + catalog.tables["bwd.archive.2025.raw.measurements"] = 100 # same count, still not ours to drop + + report = foldercopy( + catalog, "bwd.draft", "bwd.archive.2025", idempotency_key="k", skip_existing=False + ) + + occupied = next(r for r in report.tables if r.entry.name == "measurements") + assert occupied.status == "failed" + assert "overwrite=True" in (occupied.error or "") + assert not any(op == "datacopy" and path.endswith("measurements") for op, path in catalog.calls) diff --git a/tests/catalog/test_operations.py b/tests/catalog/test_operations.py index 7236e3e..baa2fd0 100644 --- a/tests/catalog/test_operations.py +++ b/tests/catalog/test_operations.py @@ -1437,3 +1437,126 @@ def test_deletefolder_engine_starting_is_remembered_and_retryable(executor) -> N fake_rest._raise_on_delete_folder = None operations.retry_pending(executor, "k", catalog_rest=fake_rest) assert fake_rest.deleted == ["a.b"] + + +# --- getentriesfrom / entryexists / getviewsql / createviewas / deletetable --- +# --- getfoldersfrom ---------------------------------------------------------- + + +def test_getentriesfrom_returns_each_path_with_its_kind() -> None: + fake = FakeExecutor( + rows=[ + {"TABLE_SCHEMA": "bwd.draft", "TABLE_NAME": "v", "TABLE_TYPE": "VIEW"}, + {"TABLE_SCHEMA": "bwd.draft.raw", "TABLE_NAME": "t", "TABLE_TYPE": "TABLE"}, + ] + ) + + entries = operations.getentriesfrom(fake, "bwd.draft", idempotency_key="k") + + assert [(entry.path, entry.kind) for entry in entries] == [ + ("bwd.draft.raw.t", "TABLE"), + ("bwd.draft.v", "VIEW"), + ] + assert '"TABLE_TYPE"' in fake.statements[0] + assert "LIKE 'bwd.draft.%' ESCAPE" in fake.statements[0] + + +def test_getentriesfrom_reads_anything_that_is_not_a_view_as_a_table() -> None: + fake = FakeExecutor( + rows=[{"TABLE_SCHEMA": "a", "TABLE_NAME": "t", "TABLE_TYPE": "SYSTEM_TABLE"}] + ) + + assert operations.getentriesfrom(fake, "a", idempotency_key="k")[0].kind == "TABLE" + + +def test_entryexists_answers_from_information_schema() -> None: + assert operations.entryexists( + FakeExecutor(rows=[{"TABLE_NAME": "t"}]), "a.t", idempotency_key="k" + ) + assert not operations.entryexists(FakeExecutor(rows=[]), "a.t", idempotency_key="k") + + +def test_getviewsql_prefers_the_catalog_entity_and_keeps_its_context() -> None: + fake = FakeExecutor() + catalog_rest = FakeCatalogRest(existing={"a", "a.v"}) + catalog_rest.view_definitions = {"a.v": ("SELECT 1", ["a"])} # type: ignore[attr-defined] + + definition = operations.getviewsql(fake, "a.v", catalog_rest=catalog_rest, idempotency_key="k") + + assert definition.sql == "SELECT 1" + assert definition.context == ("a",) + assert fake.statements == [] # never had to ask INFORMATION_SCHEMA + + +def test_getviewsql_falls_back_to_information_schema_without_a_context() -> None: + fake = FakeExecutor(rows=[{"VIEW_DEFINITION": "SELECT * FROM t"}]) + + definition = operations.getviewsql(fake, "a.v", idempotency_key="k") + + assert definition.sql == "SELECT * FROM t" + assert definition.context == () + assert 'INFORMATION_SCHEMA."VIEWS"' in fake.statements[0] + + +def test_getviewsql_raises_rather_than_guessing_a_definition() -> None: + with pytest.raises(CatalogOperationError, match="could not read the view definition"): + operations.getviewsql(FakeExecutor(rows=[]), "a.v", idempotency_key="k") + + +def test_createviewas_sends_the_query_body_through_unchanged(catalog_rest) -> None: + fake = FakeExecutor(rows=[]) # nothing at the target yet + + operations.createviewas( + fake, "a.v", 'SELECT * FROM "b"."t"', catalog_rest=catalog_rest, idempotency_key="k" + ) + + assert fake.statements[-1] == 'CREATE VIEW "a"."v" AS SELECT * FROM "b"."t"' + + +def test_createviewas_refuses_an_occupied_target_unless_told_to_overwrite(catalog_rest) -> None: + fake = FakeExecutor(rows=[{"TABLE_NAME": "v"}]) + + with pytest.raises(CatalogOperationError, match="overwrite=True"): + operations.createviewas( + fake, "a.v", "SELECT 1", catalog_rest=catalog_rest, idempotency_key="k" + ) + + +def test_createviewas_drops_whatever_kind_is_actually_there_first(catalog_rest) -> None: + fake = FakeExecutor( + rows_sequence=[[{"TABLE_NAME": "v"}], [{"TABLE_TYPE": "BASE TABLE"}]] + ) + + operations.createviewas( + fake, + "a.v", + "SELECT 1", + overwrite=True, + catalog_rest=catalog_rest, + idempotency_key="k", + ) + + assert fake.statements[-2:] == [ + 'DROP TABLE IF EXISTS "a"."v"', + 'CREATE VIEW "a"."v" AS SELECT 1', + ] + + +def test_deletetable_is_the_counterpart_of_deleteview() -> None: + fake = FakeExecutor() + + operations.deletetable(fake, "a.t", idempotency_key="k") + + assert fake.statements == ['DROP TABLE IF EXISTS "a"."t"'] + + +def test_getfoldersfrom_delegates_to_the_rest_catalog() -> None: + catalog_rest = FakeCatalogRest(existing={"a"}) + catalog_rest.folder_listing = ["a.b", "a.b.c"] # type: ignore[attr-defined] + + assert operations.getfoldersfrom(catalog_rest, "a", idempotency_key="k") == ["a.b", "a.b.c"] + + +def test_the_new_operations_can_be_replayed_by_retry_pending() -> None: + for name in ("getentriesfrom", "entryexists", "getviewsql", "createviewas", "deletetable"): + assert name in operations._OPERATIONS diff --git a/tests/catalog/test_rest.py b/tests/catalog/test_rest.py index a4f8d26..98d6ed1 100644 --- a/tests/catalog/test_rest.py +++ b/tests/catalog/test_rest.py @@ -865,3 +865,101 @@ def test_list_datasets_refused_token_raises_auth_error() -> None: with pytest.raises(CatalogAuthError): CatalogRestClient(BASE_URL, "expired").list_datasets(["s", "top"]) + + +# --- children / list_folders / get_view_definition -------------------------- + + +def _folder_child(*path: str) -> dict[str, object]: + return { + "id": "-".join(path), + "path": list(path), + "type": "CONTAINER", + "containerType": "FOLDER", + } + + +def _dataset_child(*path: str) -> dict[str, object]: + return {"id": "-".join(path), "path": list(path), "type": "DATASET"} + + +@respx.mock +def test_children_returns_the_entities_directly_inside_a_folder() -> None: + respx.get(f"{BASE_URL}/api/v3/catalog/by-path/a/b").mock( + return_value=httpx.Response( + 200, + json={ + "entityType": "folder", + "children": [_folder_child("a", "b", "sub"), _dataset_child("a", "b", "t")], + }, + ) + ) + client = CatalogRestClient(BASE_URL, "pat") + + assert [child["id"] for child in client.children("a.b")] == ["a-b-sub", "a-b-t"] + + +@respx.mock +def test_children_raises_rather_than_calling_a_missing_folder_empty() -> None: + respx.get(f"{BASE_URL}/api/v3/catalog/by-path/a/gone").mock(return_value=httpx.Response(404)) + client = CatalogRestClient(BASE_URL, "pat") + + with pytest.raises(CatalogOperationError, match="does not exist"): + client.children("a.gone") + + +@respx.mock +def test_list_folders_walks_the_subtree_shallowest_first() -> None: + respx.get(f"{BASE_URL}/api/v3/catalog/by-path/a").mock( + return_value=httpx.Response( + 200, + json={"children": [_folder_child("a", "b"), _dataset_child("a", "t")]}, + ) + ) + respx.get(f"{BASE_URL}/api/v3/catalog/by-path/a/b").mock( + return_value=httpx.Response(200, json={"children": [_folder_child("a", "b", "c")]}) + ) + respx.get(f"{BASE_URL}/api/v3/catalog/by-path/a/b/c").mock( + return_value=httpx.Response(200, json={"children": []}) + ) + client = CatalogRestClient(BASE_URL, "pat") + + assert client.list_folders("a") == ["a.b", "a.b.c"] + + +@respx.mock +def test_list_folders_can_stay_at_the_top_level() -> None: + respx.get(f"{BASE_URL}/api/v3/catalog/by-path/a").mock( + return_value=httpx.Response(200, json={"children": [_folder_child("a", "b")]}) + ) + client = CatalogRestClient(BASE_URL, "pat") + + assert client.list_folders("a", recursive=False) == ["a.b"] + + +@respx.mock +def test_get_view_definition_returns_the_sql_and_its_context() -> None: + respx.get(f"{BASE_URL}/api/v3/catalog/by-path/a/v").mock( + return_value=httpx.Response( + 200, + json={ + "entityType": "dataset", + "type": "VIRTUAL_DATASET", + "sql": "SELECT * FROM t", + "sqlContext": ["a", "b"], + }, + ) + ) + client = CatalogRestClient(BASE_URL, "pat") + + assert client.get_view_definition("a.v") == ("SELECT * FROM t", ["a", "b"]) + + +@respx.mock +def test_get_view_definition_is_none_for_something_that_is_not_a_view() -> None: + respx.get(f"{BASE_URL}/api/v3/catalog/by-path/a/t").mock( + return_value=httpx.Response(200, json={"entityType": "dataset", "type": "PHYSICAL_DATASET"}) + ) + client = CatalogRestClient(BASE_URL, "pat") + + assert client.get_view_definition("a.t") is None From 8d9b088f0c8c80d0ae58671cdea526086d236382 Mon Sep 17 00:00:00 2001 From: blikij Date: Mon, 19 Oct 2026 11:21:44 +0200 Subject: [PATCH 04/19] =?UTF-8?q?docs:=20review=20the=20DDS=20ingest=20cli?= =?UTF-8?q?ent=20plan=20against=20the=20code=20(PY-8=20=E2=80=A6=20PY-13)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mark PY-1 … PY-7 done with as-built notes, and correct PY-1's premise: 774691a already honoured the proxy hint. A new §5 records main's broken ingest and its repair, where each task stands, and the follow-up tasks PY-8 … PY-13, with PY-11 (the new options in %ingest) left to Oskar. Co-Authored-By: Claude Opus 5.5 --- docs/dds-ingest-robustness-client-plan.md | 238 +++++++++++++++++++++- 1 file changed, 227 insertions(+), 11 deletions(-) diff --git a/docs/dds-ingest-robustness-client-plan.md b/docs/dds-ingest-robustness-client-plan.md index 1f2c41d..2e41e39 100644 --- a/docs/dds-ingest-robustness-client-plan.md +++ b/docs/dds-ingest-robustness-client-plan.md @@ -1,6 +1,8 @@ # DDS ingest robustness — the client half (DI-12) — plan -> Status: **plan only — nothing implemented yet.** Date: 2026-10-16. +> Status: **PY-1 … PY-7 implemented** (2026-10-16). Reviewed against the code on +> 2026-10-19: what is still open, and the follow-up tasks PY-8 … PY-13, are in §5. +> Date: 2026-10-16. Companion plan for **this** repository. The design, the server work and the release schedule live with DDS: @@ -37,7 +39,7 @@ repeats the ⚠ note): | Task | What changes for an existing script | Mitigation | |---|---|---| -| PY-1 | with `upload_mode=auto`, a network failure on the presigned upload now retries through DDS (`/stage`) instead of raising | only where the script fails today; `upload_mode=direct` on the server keeps the old behaviour | +| PY-1 | when the server says `upload_mode: "auto"` (DDS's default) and `FolderIngest` keeps its default `upload_mode="auto"`, a network failure on the presigned upload now retries through DDS (`/stage`) instead of raising | only where the script fails today; `FolderIngest(upload_mode="direct")`, or `direct` on the server, keeps the old behaviour | | PY-2 | an upload that fails part-way now cancels the session (its landed files are deleted) before the exception is raised | nothing is lost: an incomplete upload cannot be retried anyway (`retry` re-runs only the load) | | PY-6 | CSV/JSON ingests without `table_definition` now send locally inferred columns | the inferred schema is printed; `infer_schema=False` restores today's behaviour | @@ -45,10 +47,13 @@ repeats the ⚠ note): ## 2. Tasks -### PY-1 Honour `upload_mode`, with automatic `/stage` fallback — R1 +### PY-1 Honour `upload_mode`, with automatic `/stage` fallback — R1 — **DONE** -**Today:** the client ignores the `upload_mode` that `begin` returns and has no -automatic fallback (G10 in `Documentation/data-digestion.md`). +**Before this task** (corrected 2026-10-19): `FolderIngest` already honoured the +server's `upload_mode: "proxy"` hint and took its own +`upload_mode="auto" | "direct" | "proxy"` — Oskar Esparza, `774691a`, 2026-10-08. +This plan was written on a branch that did not have that commit yet. What was +missing was the automatic fallback (G10 in `Documentation/data-digestion.md`). - Implement the contract's upload-mode rule, the same text the extension's `uploadOne` implements: `proxy` → always `IngestClient.stage`; `direct` → @@ -61,7 +66,17 @@ automatic fallback (G10 in `Documentation/data-digestion.md`). - **Verify:** tests with an unreachable presigned URL under each mode; a file over the multipart threshold in proxy mode. -### PY-2 Cancel when an upload fails; small fixes — R1 +> Landed `9d924f7`. The merge with `774691a` on `main` (`101b09c`) dropped part of +> Oskar's code; repaired on `eea_bliki` on 2026-10-19 (§5.1). As built, Oskar's +> design is kept: `BeginResult.upload_mode` is `None` when the server sends none and +> is read from the top level or from inside `s3`; `FolderIngest(upload_mode=…)` +> decides, and its default `"auto"` follows the server. The fallback is added on +> top: only when both `FolderIngest` and the server say `"auto"` +> (`_falls_back_to_proxy`). DDS sends `upload_mode` on every begin, `"auto"` unless +> configured, so the fallback is on by default. Open: the `/stage` size cap is not +> checked before uploading (PY-10). + +### PY-2 Cancel when an upload fails; small fixes — R1 — **DONE** - `FolderIngest.run()` (`folder.py:218-234`) has no failure handling. On an upload-stage failure, call `IngestClient.cancel(session_id)` before raising @@ -76,7 +91,13 @@ automatic fallback (G10 in `Documentation/data-digestion.md`). - **Verify:** an upload that fails on file 3 of 5 leaves no session and no objects (against a mocked DDS that records the `DELETE`). -### PY-3 Optional table definition — R2 +> Landed `9d924f7`. As built: any upload failure cancels the session, and the +> uploads still queued are cancelled at the first failure; the PUT comment is fixed; +> `.ndjson` is in the format table (`formats.py`). Open: under +> `IngestSession(retry=True)` the cancel hides the upload error (PY-8); the client +> guide does not describe it yet (PY-12). + +### PY-3 Optional table definition — R2 — **DONE** - A `TableDefinition` / `ColumnDef` dataclass in `dds_ingestion/models.py`, matching the contract object of DI-12.1b (`version`, `columns` with optional @@ -98,7 +119,10 @@ automatic fallback (G10 in `Documentation/data-digestion.md`). definition → only set fields sent; against a server without `definition_applied` → the definition goes on commit. -### PY-4 Show the plan, and `dry_run` — R2 +> Landed `6a4faaf`. Open: `IngestClient.retry(definition=…)` for DI-12.6 was not +> built (PY-9). + +### PY-4 Show the plan, and `dry_run` — R2 — **DONE** - When `begin` returns a `plan` (DI-12.2), print it before uploading: placement, table path, storage path, conflict mode, and the definition report. @@ -110,7 +134,13 @@ automatic fallback (G10 in `Documentation/data-digestion.md`). - **Verify:** plan printed when present, nothing printed when absent; `dry_run` against old and new server. -### PY-5 Failure text, `retryable`, `cleanup_on_failure` — R3 +> Landed `6a4faaf`. As built: the dry run is a separate `FolderIngest.dry_run()` / +> `IngestClient.dry_run()` returning a `DryRunResult`, not a `dry_run=` flag (a dry +> run has no session id, and `BeginResult.session_id` stays a `str`). Against a +> server without dry runs it raises `UnsupportedByServerError`, a subclass of +> `IngestApiError`, so callers that catch `IngestApiError` need no change (rule 4). + +### PY-5 Failure text, `retryable`, `cleanup_on_failure` — R3 — **DONE** - Show the server's failure fields verbatim: `error`, `failed_stage`, `retryable`, and the error body's `message`, in the same format as the @@ -125,7 +155,12 @@ automatic fallback (G10 in `Documentation/data-digestion.md`). - **Verify:** attributes populated from a 4xx and from a failed poll; the `cleanup_on_failure` matrix. -### PY-6 CSV/JSON options, inferred columns, local Parquet check — R3/R4 +> Landed `a7f99e9`. As built: `IngestApiError.error_slug` and `.retryable`; a failed +> commit carries the session's own failure text (`StatusResult.describe_failure`) as +> an exception note; `cleanup_on_failure` as planned. `failed_stage` is in that note +> but is not an attribute (PY-13). + +### PY-6 CSV/JSON options, inferred columns, local Parquet check — R3/R4 — **DONE** - One format table in `dds_ingestion/` (extensions, options builder, local schema reader), replacing `folder.py:45` plus the scattered checks; the client @@ -147,7 +182,14 @@ automatic fallback (G10 in `Documentation/data-digestion.md`). caught locally; one end-to-end editable CSV run against the real-Dremio `local-stack` profile (DI-12.15) that used to hit the 501. -### PY-7 `FolderIngest.diagnose()` — R4 +> Landed `a0faf15`. As built: `formats.py` (`FORMATS`, `local_columns`, +> `compare_columns`); `csv_options` / `json_options` are sent only for their own +> format; columns are inferred only for CSV/JSON into an **editable** table, the one +> case where the server builds the table from them; a JSON array cannot be inferred +> and says so. Open: the end-to-end editable CSV run against `local-stack` +> (DI-12.15) has not been done from this client. + +### PY-7 `FolderIngest.diagnose()` — R4 — **DONE** - Checks, in order, and prints each result in plain language: DDS reachable, credential valid, WRITE on the target, the storage self-test verdict @@ -156,6 +198,11 @@ automatic fallback (G10 in `Documentation/data-digestion.md`). - **Server:** DI-12.3 (R2). - **Verify:** one test per failing step. +> Landed `c1149d5`. As built: four steps — DDS reachable; credential accepted +> (`GET /ingest/limits`, or `GET /ingest` on a server without it); storage self-test; +> dry run of the folder. WRITE on the target is covered by the dry run rather than +> checked on its own, so against a server without dry runs (before R2) it is skipped. + --- ## 3. Order @@ -170,6 +217,8 @@ automatic fallback (G10 in `Documentation/data-digestion.md`). Each task can be released as soon as it is ready: by rule 3 it also works against the DDS release before the one it needs, with today's behaviour. +The follow-up tasks from the 2026-10-19 review are ordered in §5.5. + ## 4. Definition of done - Every existing call sends the same request bodies as before (golden tests), @@ -181,3 +230,170 @@ the DDS release before the one it needs, with today's behaviour. columns; Parquet columns are checked locally before upload. - `docs/python-client-guide.md` describes each new option, with one example with and one without a table definition. mypy clean; the test suite passes. + +> 2026-10-19: met except the client guide (PY-12). `dry_run` is a method rather +> than an option (PY-4). mypy is clean for the code these tasks touched; the +> remaining errors are in `dds_ingestion/common/dremio_identity.py`. + +--- + +## 5. Review against the code — 2026-10-19 + +Every task was checked against `eea_bliki` after it was brought up to date with +`main` — which now carries Oskar Esparza's `%catalog` / `%ingest` work and +`774691a` — and against the DDS side: the DI-12 plan's §7.2 progress notes and the +server code. Oskar's work takes priority where the two meet. + +### 5.1 First: `main` cannot ingest + +`101b09c`, which merged `eea_bliki` into `main` on 2026-10-19 in GitHub's conflict +editor, dropped twelve lines of `774691a`: + +- `BeginResult.from_json` raises `NameError: s3` on every begin, so + `FolderIngest.run()` fails before uploading anything. The release workflow moved + the `main` and `v0.1.30` tags to that merge, so `pip install …@main` and + `…@v0.1.30` install it. `@staging` (`v0.1.31-staging`) does not contain + PY-1 … PY-6 and is not affected. +- `FolderIngest(upload_mode="proxy")` reported a proxy upload while uploading to S3. +- 12 tests fail on `main`. + +Repaired on `eea_bliki` on 2026-10-19: Oskar's lines restored as he wrote them, PY-1's +fallback added on top, and only PY-1's own test changed (`upload_mode` is `None` when +the server sends none, as in `774691a`). The whole suite passes. This has to reach +`main` before any task below. + +### 5.2 Where each task stands + +| Task | State | Open | +|---|---|---| +| PY-1 | done; built on `774691a` | `/stage` size cap not checked (PY-10); not in the client guide (PY-12) | +| PY-2 | done | hides the upload error under `IngestSession(retry=True)` (PY-8); not in the guide (PY-12) | +| PY-3 | done | `retry` cannot send a corrected definition (PY-9) | +| PY-4 | done, as a method | — | +| PY-5 | done | `failed_stage` only in the note text (PY-13) | +| PY-6 | done | end-to-end editable CSV run against `local-stack` not done (§5.7) | +| PY-7 | done | — | +| §4 | met except the guide | `docs/python-client-guide.md` describes none of the new options (PY-12) | + +Beyond the tasks: `IngestSession.ingest`, behind `%ingest`, passes `FolderIngest` a +fixed set of options, so none of the options above reaches a notebook user (PY-11). + +### 5.3 Follow-up tasks + +#### PY-8 `retry()` after a cancelled upload + +- **Today:** PY-2 cancels the session after a failed upload, and + `FolderIngest.retry()` then raises `IngestStateError("no session to retry — call + run() first")`. `IngestSession.commit(retry=True)` re-raises that error, so + `%ingest` shows it instead of the upload error: an S3 403 reads "no session to + retry" (checked 2026-10-19). +- Remember the upload error that led to the cancel. `retry()` then: + - re-runs `run()` under a new session when that error was transient (a network + error, or a 5xx from S3 or DDS) **and** the cancel succeeded, dropping the + `idempotency_key` as its "anything else" branch already does; + - otherwise raises `IngestStateError` whose message quotes the upload error. A + failed cancel may have left files in place, and re-running a read-only ingest + stored permanently would add a second copy. +- No change to `IngestSession`. +- **⚠ Compatibility:** `retry()` after a failed upload used to raise; after a + transient failure it now uploads again. The cancel deleted whatever had landed, + so nothing is duplicated. +- **Server:** none. +- **Verify:** `IngestSession(retry=True)` after an S3 403 names the 403; after a + network error in `direct` mode the next attempt uploads and commits; after a + failed cancel nothing is re-uploaded. + +#### PY-9 `retry` with a corrected table definition (rest of PY-3) + +- `IngestClient.retry(session_id, *, definition=None)` and + `FolderIngest.retry(*, table_definition=None)`: with a definition, the resumed + load uses it (DI-12.6), and a re-upload under a new session sends it on begin. + Without one, the request is exactly today's (golden test). +- For a commit that failed with `schema_required`, retryable since DI-12.5: a retry + with the columns fixes it without uploading again. +- **Server:** DI-12.6 (done, R3). +- **Verify:** `schema_required` → `retry(table_definition=…)` → done; the request + without a definition is unchanged; against a server before DI-12.6 the retry + behaves as today. + +#### PY-10 Check file sizes against the server's limits (rest of PY-1) + +- DDS has two caps. `begin` refuses a file over the caller's cap (DI-12.13), but + `POST /ingest/stage` has its own `stage_max_bytes` and refuses a file only when + that file arrives — after other files have uploaded, which PY-2 then cancels. The + two caps are equal unless the server sets a separate stage cap. +- Read `GET /ingest/limits?runner=notebook` once per run. In proxy mode, refuse + before uploading anything when a file exceeds `stage_max_bytes`. With the + fallback, do not fall back for such a file: raise the network error with a note + saying why. A 404 (a server before DI-12.11) keeps today's behaviour. +- `diagnose()` already calls `limits()`: show the caps there and flag the files + over them. +- **Server:** DI-12.11, DI-12.13 (done, R4). +- **Verify:** proxy mode with one file over the stage cap uploads nothing; the + fallback is not attempted for such a file; an older server is unchanged. + +#### PY-11 The new options in `%ingest` — Oskar's decision + +- `IngestSession.ingest` (`dds_ingestion/session.py`) passes `FolderIngest` seven + options: `intent`, `conflict_mode`, `table_name`, `sub_path`, `parallelism`, + `multipart`, `show_progress`. Not `table_definition`, `csv_options`, + `json_options`, `infer_schema`, `cleanup_on_failure` — nor `upload_mode`. +- Proposal: add those six to `IngestSession.ingest` and to `%ingest help`, and + decide whether `dry_run()` and `diagnose()` belong in the notebook layer. +- Why it matters: without `table_definition`, a CSV/JSON ingest into an editable + table whose columns cannot be inferred has no way through `%ingest`. It fails at + commit today, and at begin once `DDS_INGEST_SCHEMA_AT_BEGIN` is on. +- **Owner:** Oskar Esparza — it is his module; nothing changes there without him. +- **Server:** none. + +#### PY-12 Client guide — the open part of §4 + +- `docs/python-client-guide.md` describes: `upload_mode` (`FolderIngest`'s choice, + the server's hint, the fallback); cancel on upload failure; `table_definition`, + with one example with and one without; `csv_options` / `json_options` / + `infer_schema`; `cleanup_on_failure`; `dry_run()`; `diagnose()`; reading + `error_slug`, `retryable` and the failure note. +- The DI-12 plan (§7.2) lists the `upload_mode` and cancel parts as open since R1. + +#### PY-13 Small fixes + +- `failed_stage` as an attribute on the exception from a failed commit (PY-5 named + it; today it is only in the note). +- `IngestOutcome.files_staged`: how many files went through DDS, so a fallback is + visible. `IngestOutcome.upload_mode` stays as it is. + +### 5.4 Not planned yet + +- **Dremio job ids in the failure text** (DI-12.18, R5): only together with the + extension, which does not show them either — DI-12.14 keeps one failure text for + both clients. +- **A local schema-drift warning** before a read-only append, like PY-6's Parquet + check: after DI-12.22 (R6b). +- **Undo for `IngestSession`** once DDS can delete a read-only table (DI-12.21, R6b); + its module docstring notes that it is missing. + +### 5.5 Order + +| Step | What | Why first | +|---|---|---| +| 1 | §5.1 to `main`, with Oskar | every Python ingest from `@main` fails today | +| 2 | PY-8, PY-12, PY-13 | fix and document what is already shipped | +| 3 | PY-9, PY-10 | use server features that are already deployed | +| 4 | PY-11 | when Oskar agrees | + +### 5.6 Coordination + +- Oskar Esparza reviews the §5.1 repair, which restores his `774691a`, and decides + PY-11. +- The DI-12 plan (§7.2) cites PY-7 as `8d79542`; that commit was rebased before it + was pushed and is `c1149d5`. + +### 5.7 Release notes + +- Before `DDS_INGEST_SCHEMA_AT_BEGIN` is turned on (DI-12 plan §7.3), the kernels + also need a Python release with PY-3 and PY-6, from after the §5.1 repair: a script + that sends its columns only on commit (`IngestClient.commit(definition=…)`) is + refused at begin. +- After each deploy: one ingest per format through this client into the production + test area (DI-12 plan §7.1 rule 4), and PY-6's editable-CSV run against + `local-stack`. From d6d9dd772c5cdc087646b85c9cfb3b79249205af Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Mon, 19 Oct 2026 09:23:18 +0000 Subject: [PATCH 05/19] Bump version to 0.1.32 [skip ci] --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 234a52d..e28e181 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "EEADataLakehouse" -version = "0.1.31" +version = "0.1.32" description = "Data preparation and validation utilities for the EEA data lakehouse pipeline." readme = "README.md" requires-python = ">=3.11" From d539890069b79a1542e56d0363a73a9167402f35 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Mon, 19 Oct 2026 09:23:20 +0000 Subject: [PATCH 06/19] Update README install examples to v0.1.32-staging [skip ci] --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 8f445cd..328912d 100644 --- a/README.md +++ b/README.md @@ -36,7 +36,7 @@ pip install "git+https://github.com/eeadata/EEALakeHouse.python.git@staging" ``` # staging's latest release (early access) — pin to the tag the "staging" badge above shows -pip install "git+https://github.com/eeadata/EEALakeHouse.python.git@v0.1.31-staging" +pip install "git+https://github.com/eeadata/EEALakeHouse.python.git@v0.1.32-staging" ``` ## Usage @@ -495,7 +495,7 @@ To pin to one specific release instead, use the exact tag the live badges under %pip install "git+https://github.com/eeadata/EEALakeHouse.python.git@v0.1.30" # staging's latest release (early access) -%pip install "git+https://github.com/eeadata/EEALakeHouse.python.git@v0.1.31-staging" +%pip install "git+https://github.com/eeadata/EEALakeHouse.python.git@v0.1.32-staging" ``` Use the `%pip` magic rather than `!pip` — it installs into the kernel the From 7dad4504042f373142dd5cc01d764db81247f906 Mon Sep 17 00:00:00 2001 From: blikij Date: Mon, 19 Oct 2026 12:03:57 +0200 Subject: [PATCH 07/19] feat(dds_ingestion): retry after a cancelled upload (PY-8) PY-2 cancels the session when an upload fails, and retry() then raised "no session to retry". IngestSession.commit(retry=True) re-raises that, so %ingest showed it instead of the upload error. run() now records the upload failure, and retry() acts on it before anything else: it uploads again under a new session when the error can pass (a network error, a 5xx, or DDS saying retryable) and the cancel went through; otherwise it raises IngestStateError quoting the upload error. IngestSession is unchanged. Co-Authored-By: Claude Opus 5.5 --- src/eea_datalakehouse/dds_ingestion/folder.py | 86 +++++++++++++++++-- .../dds_ingestion/test_folder_upload_modes.py | 86 ++++++++++++++++++- 2 files changed, 162 insertions(+), 10 deletions(-) diff --git a/src/eea_datalakehouse/dds_ingestion/folder.py b/src/eea_datalakehouse/dds_ingestion/folder.py index 89678de..67fbd04 100644 --- a/src/eea_datalakehouse/dds_ingestion/folder.py +++ b/src/eea_datalakehouse/dds_ingestion/folder.py @@ -99,6 +99,29 @@ class IngestOutcome: upload_mode: str = "direct" # how the files went: "direct" (presigned S3) or "proxy" (DDS) +@dataclass(frozen=True, slots=True) +class _UploadFailure: + """Why the last :meth:`FolderIngest.run` stopped in the upload phase, kept for + :meth:`FolderIngest.retry` (PY-8).""" + + error: Exception + session_id: str + cancelled: bool # whether PY-2's cancel went through + + +def _may_succeed_on_retry(err: Exception) -> bool: + """Whether a failed upload can succeed unchanged on a second attempt: a + network-layer error, or a 5xx from S3 or DDS. DDS's own ``retryable`` flag + decides where it gives one.""" + if isinstance(err, httpx.TransportError): + return True + if isinstance(err, IngestApiError): + if err.retryable is not None: + return err.retryable + return err.status_code >= 500 + return False + + def scan_folder(folder: Path, data_format: DataFormat) -> list[FileSpec]: """Recurse ``folder`` and return files matching ``data_format`` only. @@ -217,6 +240,9 @@ def __init__( # command below works from. self._session_id: str | None = None self._begin: BeginResult | None = None + # Set when run() cancelled its session after a failed upload; what + # retry() goes by once there is no session left (PY-8). + self._upload_failure: _UploadFailure | None = None # The client owns the credentials; if the caller did not inject one we # build it from the kernel environment. Creds never leave the client. @@ -277,11 +303,13 @@ def run(self) -> IngestOutcome: :meth:`status`, :meth:`retry` or :meth:`cancel` the transfer. Use it as a context manager (or call :meth:`close`) to release it. """ + self._upload_failure = None begin = self.begin() try: etags, multipart_etags, uploaded, skipped = self._upload_all(begin) except Exception as err: - self._cancel_after_failure(err) + cancelled = self._cancel_after_failure(err) + self._upload_failure = _UploadFailure(err, begin.session_id, cancelled) raise try: commit = self.commit(multipart_etags=multipart_etags or None) @@ -559,6 +587,11 @@ def retry(self) -> IngestOutcome: staged copy is gone. A fresh session is opened and the folder is uploaded again. Any ``idempotency_key`` is dropped for that attempt, or the server would just replay the failed session. + * **An upload of this object's own** :meth:`run` **failed**, so that run + cancelled its session (PY-2) and there is none left to resume. The + folder is uploaded again under a new session if the failure can pass + (a network error, a 5xx) and the cancel went through; otherwise this + raises :class:`IngestStateError` quoting the upload error. **A transfer whose files are stored permanently is never re-uploaded blindly.** There the upload landed in the table's own folder and stayed, @@ -572,6 +605,10 @@ def retry(self) -> IngestOutcome: Raises :class:`IngestStateError` if there is nothing to retry — no session, one that is still running, or one that already succeeded. """ + if self._upload_failure is not None: + # Whether or not its cancel went through, that session never got all + # its files: the server has nothing to resume (PY-8). + return self._rerun_after_failed_upload(self._upload_failure) if self._session_id is None: raise IngestStateError("no session to retry — call run() first") status = self._client.get_status(self._session_id) @@ -614,6 +651,35 @@ def retry(self) -> IngestOutcome: self.idempotency_key = None return self.run() + def _rerun_after_failed_upload(self, failure: _UploadFailure) -> IngestOutcome: + """:meth:`retry` once :meth:`run` cancelled its session after a failed + upload (PY-8). Uploads again only when a second attempt can succeed and + nothing of the first is left behind; otherwise raises, quoting the upload + error, so a caller that retries (``IngestSession.commit(retry=True)``) + shows the cause instead of "no session".""" + if not failure.cancelled: + raise IngestStateError( + f"the upload failed ({failure.error}) and session {failure.session_id} " + "could not be cancelled, so files it uploaded may still be in place; " + "uploading again could add a second copy. Cancel it with " + "FolderIngest.attach(...).cancel(), or wait for the server's sweep, " + "then run the ingest again." + ) from failure.error + if not _may_succeed_on_retry(failure.error): + raise IngestStateError( + f"the upload failed ({failure.error}); its session was cancelled, and " + "uploading again would fail the same way. Fix the cause, then run the " + "ingest again." + ) from failure.error + logger.info( + "the upload of session %s failed (%s); uploading again under a new session", + failure.session_id, + failure.error, + ) + self._begin = None + self.idempotency_key = None + return self.run() + # -- upload phase ----------------------------------------------------- def _after_failed_commit(self, err: IngestApiError) -> None: @@ -635,8 +701,9 @@ def _after_failed_commit(self, err: IngestApiError) -> None: if self.cleanup_on_failure and err.retryable is True: self._cancel_after_failure(err) - def _cancel_after_failure(self, err: Exception) -> None: - """Cancel the session after a failure, and say so on ``err``. + def _cancel_after_failure(self, err: Exception) -> bool: + """Cancel the session after a failure, say so on ``err``, and return + whether the cancel went through. After a failed upload (PY-2): an incomplete client upload cannot be resumed — the server only re-runs the load (:meth:`retry`) — and the @@ -649,7 +716,7 @@ def _cancel_after_failure(self, err: Exception) -> None: """ session_id = self._session_id if session_id is None: - return + return False try: self.cancel() except Exception as cancel_err: # noqa: BLE001 — reported on err instead @@ -658,11 +725,12 @@ def _cancel_after_failure(self, err: Exception) -> None: f"DDS could not cancel session {session_id} ({cancel_err}); the " "server's sweep removes its uploaded files after the session's TTL." ) - else: - err.add_note( - f"Session {session_id} was cancelled and the files it had uploaded " - "were deleted; run the ingest again." - ) + return False + err.add_note( + f"Session {session_id} was cancelled and the files it had uploaded " + "were deleted; run the ingest again." + ) + return True def _already_done(self, session_id: str) -> set[str]: """Return rel_paths already uploaded for this session (resume support). diff --git a/tests/dds_ingestion/test_folder_upload_modes.py b/tests/dds_ingestion/test_folder_upload_modes.py index cc51fe3..8d43bbf 100644 --- a/tests/dds_ingestion/test_folder_upload_modes.py +++ b/tests/dds_ingestion/test_folder_upload_modes.py @@ -1,4 +1,5 @@ -"""PY-1 (upload_mode + /stage fallback) and PY-2 (cancel when an upload fails). +"""PY-1 (upload_mode + /stage fallback), PY-2 (cancel when an upload fails) and +PY-8 (retry after that cancel). Companion to the DDS DI-12 plan; see docs/dds-ingest-robustness-client-plan.md. """ @@ -11,12 +12,14 @@ import httpx import pytest from eea_datalakehouse.dds_ingestion.client import S3UploadError +from eea_datalakehouse.dds_ingestion.folder import IngestStateError from eea_datalakehouse.dds_ingestion.models import ( BeginResult, FileSpec, StageResult, UploadTarget, ) +from eea_datalakehouse.dds_ingestion.session import IngestCommitError, IngestSession from .test_folder import FakeClient, _make_ingest @@ -163,3 +166,84 @@ def test_a_successful_ingest_is_never_cancelled(data_folder: Path) -> None: client = ModeClient() _make_ingest(data_folder, client).run() assert client.cancelled == [] + + +# --- PY-8: retry after a cancelled upload ---------------------------------------- + + +class FlakyClient(ModeClient): + """ModeClient whose failures happen once: the next attempt at that file works.""" + + def upload_file(self, target: UploadTarget, data: bytes) -> str | None: + error = self.fail.pop(target.rel_path, None) + if error is not None: + self.attempted.append(target.rel_path) + raise error + return super().upload_file(target, data) + + +def test_retry_uploads_again_after_a_network_error(data_folder: Path) -> None: + client = FlakyClient(fail={"a.parquet": _network_error()}) + job = _make_ingest(data_folder, client) + with pytest.raises(httpx.ConnectError): + job.run() + + outcome = job.retry() + + assert outcome.commit.status == "committed" + assert len(client.begin_calls) == 2 # a new session + assert client.cancelled == ["sess-X"] # only the first one + + +def test_retry_does_not_upload_again_what_would_fail_the_same_way(data_folder: Path) -> None: + client = FlakyClient(fail={"a.parquet": _s3_error("a.parquet")}) + job = _make_ingest(data_folder, client) + with pytest.raises(S3UploadError): + job.run() + + with pytest.raises(IngestStateError, match="HTTP 403") as exc: + job.retry() + + assert isinstance(exc.value.__cause__, S3UploadError) + assert len(client.begin_calls) == 1 + + +def test_retry_does_not_upload_again_when_the_cancel_failed(data_folder: Path) -> None: + client = FlakyClient( + fail={"a.parquet": _network_error()}, cancel_error=RuntimeError("DDS unreachable") + ) + job = _make_ingest(data_folder, client) + with pytest.raises(httpx.ConnectError): + job.run() + + with pytest.raises(IngestStateError, match="could not be cancelled"): + job.retry() + + assert len(client.begin_calls) == 1 + + +def test_retry_before_any_run_still_says_there_is_no_session(data_folder: Path) -> None: + with pytest.raises(IngestStateError, match="no session to retry"): + _make_ingest(data_folder, ModeClient()).retry() + + +def test_ingest_session_retry_shows_the_upload_error(data_folder: Path) -> None: + client = FlakyClient(fail={"a.parquet": _s3_error("a.parquet")}) + session = IngestSession(client=client) # type: ignore[arg-type] + session.ingest(data_folder, "bio.uploads", data_format="parquet", show_progress=False) + + with pytest.raises(IngestCommitError) as exc: + session.commit(retry=True) + + assert "AccessDenied" in str(exc.value) + assert "no session to retry" not in str(exc.value) + + +def test_ingest_session_retry_recovers_from_a_network_error(data_folder: Path) -> None: + client = FlakyClient(fail={"a.parquet": _network_error()}) + session = IngestSession(client=client) # type: ignore[arg-type] + session.ingest(data_folder, "bio.uploads", data_format="parquet", show_progress=False) + + report = session.commit(retry=True) + + assert [o.commit.status for o in report.outcomes] == ["committed"] From a5900d81074648480abda7209b5ba27e740e8d83 Mon Sep 17 00:00:00 2001 From: blikij Date: Mon, 19 Oct 2026 12:05:18 +0200 Subject: [PATCH 08/19] feat(dds_ingestion): failed_stage on a failed commit; files_staged (PY-13) IngestApiError.failed_stage: where a failed commit broke ("upload" or "load"), taken from the session as the failure note already was. PY-5 named it as an attribute; it was only in the note text. IngestOutcome.files_staged: how many uploaded files went through DDS, so a network-error fallback is visible. IngestOutcome.upload_mode is unchanged. Co-Authored-By: Claude Opus 5.5 --- src/eea_datalakehouse/dds_ingestion/client.py | 4 ++++ src/eea_datalakehouse/dds_ingestion/folder.py | 20 +++++++++++++++---- tests/dds_ingestion/test_failure_reporting.py | 17 ++++++++++++++++ .../dds_ingestion/test_folder_upload_modes.py | 8 ++++++++ 4 files changed, 45 insertions(+), 4 deletions(-) diff --git a/src/eea_datalakehouse/dds_ingestion/client.py b/src/eea_datalakehouse/dds_ingestion/client.py index 9f8ba89..9618edc 100644 --- a/src/eea_datalakehouse/dds_ingestion/client.py +++ b/src/eea_datalakehouse/dds_ingestion/client.py @@ -71,10 +71,14 @@ class IngestApiError(RuntimeError): ``schema_required``; ``""`` when the body carried none) and ``retryable`` whether repeating the request — for a failed transfer, ``retry`` — can succeed unchanged (``None`` from a server predating contract v0.8). + ``failed_stage`` is where a failed commit broke, ``"upload"`` or ``"load"``, + as the session reports it; ``FolderIngest`` sets it, and it is ``None`` + everywhere else. """ error_slug: str = "" retryable: bool | None = None + failed_stage: str | None = None def __init__(self, status_code: int, message: str, *, where: str | None = None) -> None: super().__init__( diff --git a/src/eea_datalakehouse/dds_ingestion/folder.py b/src/eea_datalakehouse/dds_ingestion/folder.py index 67fbd04..b54c194 100644 --- a/src/eea_datalakehouse/dds_ingestion/folder.py +++ b/src/eea_datalakehouse/dds_ingestion/folder.py @@ -97,6 +97,9 @@ class IngestOutcome: etags: dict[str, str] = field(default_factory=dict) resumed: bool = False upload_mode: str = "direct" # how the files went: "direct" (presigned S3) or "proxy" (DDS) + # How many of files_uploaded went through DDS (/stage): all of them with + # "proxy", the network-error fallbacks with "direct" (PY-13). + files_staged: int = 0 @dataclass(frozen=True, slots=True) @@ -243,6 +246,9 @@ def __init__( # Set when run() cancelled its session after a failed upload; what # retry() goes by once there is no session left (PY-8). self._upload_failure: _UploadFailure | None = None + # The files the current run sent through DDS after a network error; + # appended to from the upload threads (list.append is atomic). + self._staged_by_fallback: list[str] = [] # The client owns the credentials; if the caller did not inject one we # build it from the kernel environment. Creds never leave the client. @@ -323,6 +329,7 @@ def run(self) -> IngestOutcome: files_skipped=skipped, etags=etags, upload_mode="proxy" if self._uses_proxy(begin) else "direct", + files_staged=uploaded if self._uses_proxy(begin) else len(self._staged_by_fallback), ) def _uses_proxy(self, begin: BeginResult) -> bool: @@ -687,15 +694,18 @@ def _after_failed_commit(self, err: IngestApiError) -> None: ``cleanup_on_failure``. Best-effort: nothing here replaces ``err``. The note is the session's own failure text (:meth:`StatusResult. - describe_failure`), the same the Transfers panel shows. A failure the - server cannot retry was already cleaned up server-side; a retryable one - is kept for :meth:`retry` unless ``cleanup_on_failure`` is set. + describe_failure`), the same the Transfers panel shows, and + ``err.failed_stage`` its stage. A failure the server cannot retry was + already cleaned up server-side; a retryable one is kept for :meth:`retry` + unless ``cleanup_on_failure`` is set. """ session_id = self._session_id if session_id is None: return try: - err.add_note(self._client.get_status(session_id).describe_failure()) + status = self._client.get_status(session_id) + err.failed_stage = status.failed_stage + err.add_note(status.describe_failure()) except Exception: # noqa: BLE001 — the commit error is what matters logger.debug("could not fetch the failed session's status", exc_info=True) if self.cleanup_on_failure and err.retryable is True: @@ -766,6 +776,7 @@ def _upload_all( targets = list(begin.s3.uploads) proxy = self._uses_proxy(begin) fallback = self._falls_back_to_proxy(begin) + self._staged_by_fallback = [] if proxy: logger.info("uploading through DDS (proxy), not to the presigned S3 targets") done = self._already_done(begin.session_id) @@ -867,6 +878,7 @@ def _stage( S3 with its own credentials. A file the server planned as multipart goes through whole; DDS accepts that at commit (DI-12.10a).""" self._client.stage(session_id, target.rel_path, data) + self._staged_by_fallback.append(target.rel_path) return None, None diff --git a/tests/dds_ingestion/test_failure_reporting.py b/tests/dds_ingestion/test_failure_reporting.py index 6ec2f9e..607825d 100644 --- a/tests/dds_ingestion/test_failure_reporting.py +++ b/tests/dds_ingestion/test_failure_reporting.py @@ -112,6 +112,23 @@ def test_a_failed_commit_is_described_like_the_transfers_panel(data_folder: Path assert client.cancelled == [] # kept for retry by default +def test_a_failed_commit_carries_its_stage(data_folder: Path) -> None: + # PY-13: the stage is an attribute too, not only part of the note. + with pytest.raises(IngestApiError) as exc: + _job(data_folder, FailingCommitClient(retryable=True)).run() + assert exc.value.failed_stage == "load" + + +@respx.mock +def test_an_api_error_has_no_stage_of_its_own(creds: DremioCreds) -> None: + respx.post(f"{BASE_URL}/api/v1/ingest/commit").mock( + return_value=httpx.Response(400, json={"error": "bad_request", "message": "no"}) + ) + with IngestClient(BASE_URL, creds) as client, pytest.raises(IngestApiError) as exc: + client.commit(session_id="s") + assert exc.value.failed_stage is None + + @pytest.mark.parametrize( ("retryable", "cleanup", "cancelled"), [(True, True, True), (False, True, False), (True, False, False)], diff --git a/tests/dds_ingestion/test_folder_upload_modes.py b/tests/dds_ingestion/test_folder_upload_modes.py index 8d43bbf..83e701a 100644 --- a/tests/dds_ingestion/test_folder_upload_modes.py +++ b/tests/dds_ingestion/test_folder_upload_modes.py @@ -91,6 +91,7 @@ def test_proxy_mode_sends_every_file_through_dds(data_folder: Path) -> None: assert sorted(client.staged) == ["a.parquet", "sub/b.parquet"] assert client.attempted == [] assert outcome.commit.status == "committed" + assert (outcome.upload_mode, outcome.files_staged) == ("proxy", 2) def test_auto_mode_falls_back_to_dds_on_a_network_error(data_folder: Path) -> None: @@ -101,6 +102,13 @@ def test_auto_mode_falls_back_to_dds_on_a_network_error(data_folder: Path) -> No assert "sub/b.parquet" in client.uploaded assert outcome.commit.status == "committed" assert client.cancelled == [] + # The fallback shows: one file of two went through DDS (PY-13). + assert (outcome.upload_mode, outcome.files_uploaded, outcome.files_staged) == ("direct", 2, 1) + + +def test_a_plain_direct_upload_stages_nothing(data_folder: Path) -> None: + outcome = _make_ingest(data_folder, ModeClient(upload_mode="auto")).run() + assert outcome.files_staged == 0 def test_auto_mode_does_not_retry_an_s3_http_error_through_dds(data_folder: Path) -> None: From 9b4f429f1f561477c38dd55adef98d9f4d7ff486 Mon Sep 17 00:00:00 2001 From: blikij Date: Mon, 19 Oct 2026 12:06:57 +0200 Subject: [PATCH 09/19] feat(dds_ingestion): retry with a corrected table definition (PY-9) IngestClient.retry(session_id, definition=...) sends DDS's optional retry body (DI-12.6), and FolderIngest.retry(table_definition=...) passes it on: a commit that failed on its columns (schema_required, schema_mismatch) is fixed without uploading again, and a retry that does upload again sends it on begin. Without a definition, both calls are exactly what they were: no request body, and retry(session_id) on the client. Co-Authored-By: Claude Opus 5.5 --- src/eea_datalakehouse/dds_ingestion/client.py | 14 ++- src/eea_datalakehouse/dds_ingestion/folder.py | 19 +++- .../dds_ingestion/test_definition_and_plan.py | 92 ++++++++++++++++++- 3 files changed, 121 insertions(+), 4 deletions(-) diff --git a/src/eea_datalakehouse/dds_ingestion/client.py b/src/eea_datalakehouse/dds_ingestion/client.py index 9618edc..61ef9bd 100644 --- a/src/eea_datalakehouse/dds_ingestion/client.py +++ b/src/eea_datalakehouse/dds_ingestion/client.py @@ -397,7 +397,12 @@ def list_sessions(self, state: str | None = None) -> list[StatusResult]: rows = payload if isinstance(payload, list) else payload.get("sessions", []) return [StatusResult.from_json(row) for row in rows] - def retry(self, session_id: str) -> StatusResult: + def retry( + self, + session_id: str, + *, + definition: TableDefinition | Mapping[str, Any] | None = None, + ) -> StatusResult: """Re-run a failed session's last stage (``POST /api/v1/ingest/{id}/retry``). Only a ``failed`` session can be retried, and only the Dremio load stage @@ -405,9 +410,16 @@ def retry(self, session_id: str) -> StatusResult: (see :attr:`StatusResult.is_resumable`). A session whose upload never completed, or whose failure a re-run cannot fix, raises :class:`IngestApiError` telling you to upload again. + + ``definition`` (contract v0.8, DI-12.6) replaces the stored table + definition before the re-run — the fix for a ``schema_required`` or + ``schema_mismatch`` failure without uploading again. Without it the + request has no body, exactly as before. """ + payload = definition_payload(definition) resp = self._http.post( f"{self._base_url}/api/v1/ingest/{session_id}/retry", + json={"definition": payload} if payload is not None else None, headers=self._auth_headers, ) self._raise_for_status(resp, f"POST /api/v1/ingest/{session_id}/retry") diff --git a/src/eea_datalakehouse/dds_ingestion/folder.py b/src/eea_datalakehouse/dds_ingestion/folder.py index b54c194..a9e64cc 100644 --- a/src/eea_datalakehouse/dds_ingestion/folder.py +++ b/src/eea_datalakehouse/dds_ingestion/folder.py @@ -580,9 +580,17 @@ def cancel(self) -> None: self._client.cancel(self._session_id) self._session_id = None - def retry(self) -> IngestOutcome: + def retry( + self, *, table_definition: TableDefinition | Mapping[str, Any] | None = None + ) -> IngestOutcome: """Re-run this transfer from the step that failed. + ``table_definition`` replaces the one this transfer was given — the fix + for a commit that failed on its columns (``schema_required``, + ``schema_mismatch``). A resumed load uses it without uploading again + (DI-12.6; a server before that ignores it), and a new upload sends it on + begin. + Two cases, decided by the server's own report of where it broke: * **The load failed** (``CREATE TABLE`` / ``COPY INTO``) and the staged @@ -612,6 +620,8 @@ def retry(self) -> IngestOutcome: Raises :class:`IngestStateError` if there is nothing to retry — no session, one that is still running, or one that already succeeded. """ + if table_definition is not None: + self.table_definition = table_definition if self._upload_failure is not None: # Whether or not its cancel went through, that session never got all # its files: the server has nothing to resume (PY-8). @@ -630,7 +640,12 @@ def retry(self) -> IngestOutcome: ) if status.is_resumable: logger.info("resuming the load step of %s", self._session_id) - resumed = self._client.retry(self._session_id) + # Without a definition, the call is exactly the one made before DI-12.6. + resumed = ( + self._client.retry(self._session_id, definition=table_definition) + if table_definition is not None + else self._client.retry(self._session_id) + ) return IngestOutcome( begin=self._begin, commit=CommitResult.from_json(resumed.raw), diff --git a/tests/dds_ingestion/test_definition_and_plan.py b/tests/dds_ingestion/test_definition_and_plan.py index 834b717..00428b1 100644 --- a/tests/dds_ingestion/test_definition_and_plan.py +++ b/tests/dds_ingestion/test_definition_and_plan.py @@ -1,4 +1,5 @@ -"""PY-3 (optional table definition) and PY-4 (show the plan, dry run). +"""PY-3 (optional table definition), PY-4 (show the plan, dry run) and PY-9 +(retry with a corrected definition). Companion to the DDS DI-12 plan; see docs/dds-ingest-robustness-client-plan.md. """ @@ -22,6 +23,7 @@ ColumnDef, DryRunResult, FileSpec, + StatusResult, TableDefinition, definition_payload, ) @@ -266,3 +268,91 @@ def test_folder_dry_run_uploads_nothing_and_shows_the_plan( assert client.begin_calls == [] and client.uploaded == [] and client.commit_calls == [] assert job.session_id is None assert "Importing as a new editable table" in capsys.readouterr().out + + +# --- PY-9: retry with a corrected definition ---------------------------------------- + +DEFINITION_JSON = {"columns": [{"name": "a", "type": "INT", "nullable": False}]} + + +@respx.mock +def test_retry_without_a_definition_sends_todays_request(creds: DremioCreds) -> None: + """Golden: no body, as before DI-12.6.""" + route = respx.post(f"{BASE_URL}/api/v1/ingest/sess-1/retry").mock( + return_value=httpx.Response(200, json={"session_id": "sess-1", "status": "committing"}) + ) + with IngestClient(BASE_URL, creds) as client: + client.retry("sess-1") + request = route.calls.last.request + assert request.read() == b"" + assert "content-type" not in request.headers + + +@respx.mock +def test_retry_sends_a_corrected_definition(creds: DremioCreds) -> None: + route = respx.post(f"{BASE_URL}/api/v1/ingest/sess-1/retry").mock( + return_value=httpx.Response(200, json={"session_id": "sess-1", "status": "committing"}) + ) + with IngestClient(BASE_URL, creds) as client: + client.retry("sess-1", definition=DEFINITION) + assert json.loads(route.calls.last.request.read()) == {"definition": DEFINITION_JSON} + + +class FailedSessionClient(PlanClient): + """A session that failed with ``schema_required``; ``retry`` records its arguments.""" + + def __init__(self, *, resumable: bool) -> None: + super().__init__(plan=PLAN_JSON) + self.status_payload = { + "status": "failed", + "progress": {}, + "failed_stage": "load" if resumable else "upload", + "resumable": resumable, + "error_code": "schema_required", + } + self.retry_calls: list[dict[str, object]] = [] + + def retry(self, session_id: str, **kwargs: object) -> StatusResult: + self.retry_calls.append({"session_id": session_id, **kwargs}) + return StatusResult.from_json({"session_id": session_id, "status": "done", "progress": {}}) + + +def _attached(folder: Path, client: FakeClient) -> FolderIngest: + return FolderIngest.attach( + "sess-1", + folder, + "bio/uploads", + data_format="csv", + intent="editable", + show_progress=False, + client=client, # type: ignore[arg-type] + ) + + +def test_retry_resumes_the_load_with_the_corrected_definition(tmp_path: Path) -> None: + client = FailedSessionClient(resumable=True) + job = _attached(_csv_folder(tmp_path), client) + + outcome = job.retry(table_definition=DEFINITION) + + assert outcome.resumed + assert client.retry_calls == [{"session_id": "sess-1", "definition": DEFINITION}] + assert client.begin_calls == [] # nothing uploaded again + + +def test_retry_without_a_definition_calls_retry_as_before(tmp_path: Path) -> None: + client = FailedSessionClient(resumable=True) + _attached(_csv_folder(tmp_path), client).retry() + assert client.retry_calls == [{"session_id": "sess-1"}] + + +def test_a_retry_that_uploads_again_sends_the_corrected_definition_on_begin( + tmp_path: Path, +) -> None: + client = FailedSessionClient(resumable=False) + job = _attached(_csv_folder(tmp_path), client) + + job.retry(table_definition=DEFINITION) + + assert client.retry_calls == [] + assert client.begin_calls[0]["definition"] == DEFINITION From 2a554b23facd8fbe22095222bf87e885313922a9 Mon Sep 17 00:00:00 2001 From: blikij Date: Mon, 19 Oct 2026 12:09:35 +0200 Subject: [PATCH 10/19] feat(dds_ingestion): check files against DDS's /stage cap (PY-10) POST /ingest/stage has its own cap (stage_max_bytes) and DDS checks it only when each file arrives, after the files ahead of it have uploaded. FolderIngest now reads GET /ingest/limits (DI-12.11) at most once per run, and only when a file may go through DDS: - upload_mode proxy: a file over the cap is refused before anything is uploaded (IngestApiError, retryable False; the session is cancelled). - the network-error fallback: a file over the cap is not sent through DDS; the network error gets a note saying why. A server without the endpoint (404), or a failed request, keeps today's behaviour. A direct upload makes no extra request. diagnose() gains a "File sizes" step: the files against max_bytes and stage_max_bytes. Co-Authored-By: Claude Opus 5.5 --- src/eea_datalakehouse/dds_ingestion/folder.py | 114 ++++++++++++++++-- tests/dds_ingestion/test_diagnose.py | 61 +++++++++- .../dds_ingestion/test_folder_upload_modes.py | 70 ++++++++++- 3 files changed, 231 insertions(+), 14 deletions(-) diff --git a/src/eea_datalakehouse/dds_ingestion/folder.py b/src/eea_datalakehouse/dds_ingestion/folder.py index a9e64cc..4dcc7e9 100644 --- a/src/eea_datalakehouse/dds_ingestion/folder.py +++ b/src/eea_datalakehouse/dds_ingestion/folder.py @@ -17,6 +17,7 @@ from __future__ import annotations import logging +import threading from collections.abc import Mapping from concurrent.futures import ThreadPoolExecutor, as_completed from dataclasses import dataclass, field @@ -249,6 +250,11 @@ def __init__( # The files the current run sent through DDS after a network error; # appended to from the upload threads (list.append is atomic). self._staged_by_fallback: list[str] = [] + # The largest file POST /ingest/stage accepts (PY-10): asked of DDS at + # most once per run, and only when a file may go through it. + self._stage_cap: int | None = None + self._stage_cap_asked = False + self._stage_cap_lock = threading.Lock() # The client owns the credentials; if the caller did not inject one we # build it from the kernel environment. Creds never leave the client. @@ -468,11 +474,12 @@ def diagnose(self) -> Diagnosis: """Check, in plain language, whether this transfer would work — without starting it (PY-7). Nothing is uploaded and no session is created. - In order: DDS reachable; the credential accepted; DDS's own S3 write test; - and a dry run of this exact folder, which covers the privileges on the - target, the target folder, collisions and the table definition. A check - that cannot run (DDS unreachable, a server predating it) is reported as - skipped, with the reason, instead of failing the ones after it. + In order: DDS reachable; the credential accepted; the files against DDS's + size limits (PY-10); DDS's own S3 write test; and a dry run of this exact + folder, which covers the privileges on the target, the target folder, + collisions and the table definition. A check that cannot run (DDS + unreachable, a server predating it) is reported as skipped, with the + reason, instead of failing the ones after it. """ result = Diagnosis() @@ -486,13 +493,19 @@ def add(name: str, ok: bool | None, detail: str) -> None: self._client.health() except (httpx.HTTPError, IngestApiError) as exc: add("DDS reachable", False, str(exc)) - for name in ("Credential accepted", "Storage write test", "Dry run of this folder"): + for name in ( + "Credential accepted", + "File sizes", + "Storage write test", + "Dry run of this folder", + ): add(name, None, "skipped: DDS is not reachable") return result add("DDS reachable", True, "the service answers") + limits: dict[str, Any] | None = None try: - self._client.limits("notebook") + limits = self._client.limits("notebook") add("Credential accepted", True, "DDS accepts your Dremio credential") except IngestApiError as exc: if exc.status_code == 404: # a server predating GET /ingest/limits @@ -504,6 +517,8 @@ def add(name: str, ok: bool | None, detail: str) -> None: else: add("Credential accepted", False, exc.message) + add("File sizes", *self._check_file_sizes(limits)) + try: verdict = self._client.storage_selftest().get("storage_write_test") or {} if verdict.get("ok"): @@ -525,6 +540,38 @@ def add(name: str, ok: bool | None, detail: str) -> None: add("Dry run of this folder", False, str(getattr(exc, "message", exc))) return result + def _check_file_sizes(self, limits: Mapping[str, Any] | None) -> tuple[bool | None, str]: + """:meth:`diagnose`'s size step (PY-10): this folder's files against the + caps DDS reports — ``max_bytes`` for any file of this caller, which begin + enforces, and ``stage_max_bytes`` for a file sent through DDS.""" + if limits is None: + return None, "skipped: DDS did not report its size limits" + files = scan_folder(self.folder, self.data_format) + if not files: + return None, f"skipped: no {self.data_format} files under {self.folder}" + + def over(cap: object) -> list[str]: + if not isinstance(cap, int): + return [] + return [f"{f.rel_path} ({f.size:,} bytes)" for f in files if f.size > cap] + + max_bytes, stage_max = limits.get("max_bytes"), limits.get("stage_max_bytes") + if too_big := over(max_bytes): + return False, f"larger than DDS accepts ({max_bytes:,} bytes): {', '.join(too_big)}" + if too_big_to_stage := over(stage_max): + names = ", ".join(too_big_to_stage) + if self.upload_mode == "proxy": + return False, ( + f"larger than DDS accepts through /stage ({stage_max:,} bytes), where " + f"upload_mode='proxy' sends every file: {names}" + ) + return True, ( + f"fine for a direct upload, but larger than DDS accepts through /stage " + f"({stage_max:,} bytes), so these cannot fall back to it: {names}" + ) + largest = max(files, key=lambda f: f.size) + return True, f"within DDS's limits (largest: {largest.rel_path}, {largest.size:,} bytes)" + def _report_plan(self, plan: BeginPlan) -> None: """Show what commit will do: printed when ``show_progress`` (the same switch as the progress bar), always logged; the parts of the table @@ -792,11 +839,14 @@ def _upload_all( proxy = self._uses_proxy(begin) fallback = self._falls_back_to_proxy(begin) self._staged_by_fallback = [] + self._stage_cap_asked = False if proxy: logger.info("uploading through DDS (proxy), not to the presigned S3 targets") done = self._already_done(begin.session_id) pending = [t for t in targets if t.rel_path not in done] skipped = len(targets) - len(pending) + if proxy: + self._refuse_files_over_the_stage_cap(pending) # Single-shot ETags (rel_path -> ETag) and multipart commit entries # ({rel_path, upload_id, parts:[{part_number, etag}]}) are collected @@ -878,6 +928,13 @@ def _upload_one( except httpx.TransportError as exc: if not fallback: raise + cap = self._stage_limit() + if cap is not None and len(data) > cap: + exc.add_note( + f"Not sent through DDS instead: {target.rel_path!r} is {len(data):,} " + f"bytes, more than POST /api/v1/ingest/stage accepts ({cap:,} bytes)." + ) + raise logger.warning( "presigned upload of %s failed at the network layer (%s); " "sending it through DDS instead", @@ -896,6 +953,49 @@ def _stage( self._staged_by_fallback.append(target.rel_path) return None, None + def _stage_limit(self) -> int | None: + """The largest file ``POST /api/v1/ingest/stage`` accepts, from ``GET + /api/v1/ingest/limits`` (DI-12.11), asked at most once per run (PY-10). + + ``None`` when DDS does not say — a server before DI-12.11 answers 404 — + or the question fails: this is a check ahead of DDS's own, never a + reason to fail an upload by itself. + """ + with self._stage_cap_lock: + if not self._stage_cap_asked: + self._stage_cap_asked = True + try: + value = self._client.limits("notebook").get("stage_max_bytes") + except Exception: # noqa: BLE001 — best-effort, see the docstring + logger.debug("could not read the ingest limits", exc_info=True) + value = None + self._stage_cap = value if isinstance(value, int) else None + return self._stage_cap + + def _refuse_files_over_the_stage_cap(self, targets: list[UploadTarget]) -> None: + """With ``proxy`` every file goes through ``POST /ingest/stage``, which DDS + caps on its own and checks only when each file arrives (PY-10). Refuse an + oversized file before anything is uploaded, rather than after the files + ahead of it.""" + cap = self._stage_limit() + if cap is None: + return + too_big: list[str] = [] + for target in targets: + size = (self.folder / target.rel_path).stat().st_size + if size > cap: + too_big.append(f"{target.rel_path!r} ({size:,} bytes)") + if not too_big: + return + err = IngestApiError( + 400, + f"DDS accepts files of up to {cap:,} bytes through POST /api/v1/ingest/stage, " + f"which this upload goes through; too large: {', '.join(too_big)}", + where="local size check against GET /api/v1/ingest/limits (nothing was uploaded)", + ) + err.retryable = False + raise err + def ingest_folder( folder: str | Path, diff --git a/tests/dds_ingestion/test_diagnose.py b/tests/dds_ingestion/test_diagnose.py index 1d20fa9..440aab0 100644 --- a/tests/dds_ingestion/test_diagnose.py +++ b/tests/dds_ingestion/test_diagnose.py @@ -27,10 +27,15 @@ } -def _diagnose(folder: Path, creds: DremioCreds) -> Diagnosis: +def _diagnose(folder: Path, creds: DremioCreds, **kwargs: object) -> Diagnosis: with IngestClient(BASE_URL, creds) as client: job = FolderIngest( - folder, "bio/uploads", data_format="parquet", show_progress=False, client=client + folder, + "bio/uploads", + data_format="parquet", + show_progress=False, + client=client, + **kwargs, # type: ignore[arg-type] ) return job.diagnose() @@ -43,7 +48,7 @@ def _summary(result: Diagnosis) -> list[tuple[str, bool | None]]: def test_everything_green(data_folder: Path, creds: DremioCreds) -> None: respx.get(f"{BASE_URL}/health").mock(return_value=httpx.Response(200, json={"status": "ok"})) respx.get(f"{BASE_URL}/api/v1/ingest/limits").mock( - return_value=httpx.Response(200, json={"max_bytes": 1}) + return_value=httpx.Response(200, json={"max_bytes": 1000, "stage_max_bytes": 1000}) ) respx.post(f"{BASE_URL}/storage-selftest").mock( return_value=httpx.Response(200, json={"storage_write_test": {"ok": True}}) @@ -58,10 +63,11 @@ def test_everything_green(data_folder: Path, creds: DremioCreds) -> None: assert _summary(result) == [ ("DDS reachable", True), ("Credential accepted", True), + ("File sizes", True), ("Storage write test", True), ("Dry run of this folder", True), ] - assert result.steps[3].detail == "Importing as a new editable table at bio/uploads/t." + assert result.steps[4].detail == "Importing as a new editable table at bio/uploads/t." @respx.mock @@ -72,6 +78,7 @@ def test_an_unreachable_dds_skips_the_rest(data_folder: Path, creds: DremioCreds assert _summary(result) == [ ("DDS reachable", False), ("Credential accepted", None), + ("File sizes", None), ("Storage write test", None), ("Dry run of this folder", None), ] @@ -118,6 +125,7 @@ def test_an_older_server(data_folder: Path, creds: DremioCreds) -> None: assert _summary(result) == [ ("DDS reachable", True), ("Credential accepted", True), + ("File sizes", None), ("Storage write test", None), ("Dry run of this folder", None), ] @@ -145,3 +153,48 @@ def test_no_write_privilege_on_the_target(data_folder: Path, creds: DremioCreds) assert steps["Storage write test"].ok is False assert "HTTP 501" in steps["Storage write test"].detail assert steps["Dry run of this folder"].detail == "no WRITE privilege on bio/uploads" + + +# --- PY-10: the files against DDS's size limits ------------------------------------- + + +def _mock_a_working_dds(limits: dict[str, object]) -> None: + respx.get(f"{BASE_URL}/health").mock(return_value=httpx.Response(200)) + respx.get(f"{BASE_URL}/api/v1/ingest/limits").mock( + return_value=httpx.Response(200, json=limits) + ) + respx.post(f"{BASE_URL}/storage-selftest").mock( + return_value=httpx.Response(200, json={"storage_write_test": {"ok": True}}) + ) + respx.post(f"{BASE_URL}/api/v1/ingest/begin").mock( + return_value=httpx.Response(200, json=DRY_RUN) + ) + + +def _sizes(result: Diagnosis) -> tuple[bool | None, str]: + step = next(step for step in result.steps if step.name == "File sizes") + return step.ok, step.detail + + +@respx.mock +def test_a_file_larger_than_dds_accepts(data_folder: Path, creds: DremioCreds) -> None: + # data_folder: a.parquet is 6 bytes, sub/b.parquet 7. + _mock_a_working_dds({"max_bytes": 6, "stage_max_bytes": 6}) + ok, detail = _sizes(_diagnose(data_folder, creds)) + assert ok is False + assert detail == "larger than DDS accepts (6 bytes): sub/b.parquet (7 bytes)" + + +@respx.mock +def test_a_file_over_the_stage_cap_matters_only_through_dds( + data_folder: Path, creds: DremioCreds +) -> None: + _mock_a_working_dds({"max_bytes": 100, "stage_max_bytes": 6}) + + ok, detail = _sizes(_diagnose(data_folder, creds)) + assert ok is True + assert "cannot fall back to it: sub/b.parquet (7 bytes)" in detail + + ok, detail = _sizes(_diagnose(data_folder, creds, upload_mode="proxy")) + assert ok is False + assert "upload_mode='proxy' sends every file: sub/b.parquet (7 bytes)" in detail diff --git a/tests/dds_ingestion/test_folder_upload_modes.py b/tests/dds_ingestion/test_folder_upload_modes.py index 83e701a..6f88c28 100644 --- a/tests/dds_ingestion/test_folder_upload_modes.py +++ b/tests/dds_ingestion/test_folder_upload_modes.py @@ -1,5 +1,5 @@ -"""PY-1 (upload_mode + /stage fallback), PY-2 (cancel when an upload fails) and -PY-8 (retry after that cancel). +"""PY-1 (upload_mode + /stage fallback), PY-2 (cancel when an upload fails), +PY-8 (retry after that cancel) and PY-10 (the /stage size cap). Companion to the DDS DI-12 plan; see docs/dds-ingest-robustness-client-plan.md. """ @@ -11,7 +11,7 @@ import httpx import pytest -from eea_datalakehouse.dds_ingestion.client import S3UploadError +from eea_datalakehouse.dds_ingestion.client import IngestApiError, S3UploadError from eea_datalakehouse.dds_ingestion.folder import IngestStateError from eea_datalakehouse.dds_ingestion.models import ( BeginResult, @@ -255,3 +255,67 @@ def test_ingest_session_retry_recovers_from_a_network_error(data_folder: Path) - report = session.commit(retry=True) assert [o.commit.status for o in report.outcomes] == ["committed"] + + +# --- PY-10: the /stage size cap --------------------------------------------------- + + +class CappedClient(FlakyClient): + """FlakyClient that answers GET /ingest/limits; ``stage_max_bytes=None`` is a + server before DI-12.11 (404).""" + + def __init__(self, *, stage_max_bytes: int | None, **kwargs: object) -> None: + super().__init__(**kwargs) # type: ignore[arg-type] + self.stage_max_bytes = stage_max_bytes + self.limits_calls = 0 + + def limits(self, runner: str = "notebook") -> dict[str, object]: + self.limits_calls += 1 + if self.stage_max_bytes is None: + raise IngestApiError(404, "Not Found", where="GET /api/v1/ingest/limits") + return {"max_bytes": 1000, "stage_max_bytes": self.stage_max_bytes} + + +def test_proxy_refuses_a_file_over_the_stage_cap_before_uploading_anything( + data_folder: Path, +) -> None: + # data_folder: a.parquet is 6 bytes, sub/b.parquet 7. + client = CappedClient(upload_mode="proxy", stage_max_bytes=6) + with pytest.raises(IngestApiError, match=r"too large: 'sub/b.parquet' \(7 bytes\)") as exc: + _make_ingest(data_folder, client).run() + + assert client.staged == [] + assert client.cancelled == ["sess-X"] + assert exc.value.retryable is False + + +def test_proxy_against_a_server_without_limits_uploads_as_before(data_folder: Path) -> None: + client = CappedClient(upload_mode="proxy", stage_max_bytes=None) + _make_ingest(data_folder, client).run() + assert sorted(client.staged) == ["a.parquet", "sub/b.parquet"] + + +def test_no_fallback_for_a_file_over_the_stage_cap(data_folder: Path) -> None: + client = CappedClient( + upload_mode="auto", stage_max_bytes=5, fail={"a.parquet": _network_error()} + ) + with pytest.raises(httpx.ConnectError) as exc: + _make_ingest(data_folder, client).run() + + assert client.staged == [] + assert any("Not sent through DDS instead" in note for note in exc.value.__notes__) + + +def test_the_limits_are_asked_only_when_a_file_may_go_through_dds(data_folder: Path) -> None: + plain = CappedClient(upload_mode="auto", stage_max_bytes=100) + _make_ingest(data_folder, plain).run() + assert plain.limits_calls == 0 # a direct upload sends what it sent before + + two_fallbacks = CappedClient( + upload_mode="auto", + stage_max_bytes=100, + fail={"a.parquet": _network_error(), "sub/b.parquet": _network_error()}, + ) + outcome = _make_ingest(data_folder, two_fallbacks).run() + assert outcome.files_staged == 2 + assert two_fallbacks.limits_calls == 1 From f512d2f833f34390f76fe724542adc86671c6dd2 Mon Sep 17 00:00:00 2001 From: blikij Date: Mon, 19 Oct 2026 12:13:10 +0200 Subject: [PATCH 11/19] =?UTF-8?q?docs(dds=5Fingestion):=20the=20client=20g?= =?UTF-8?q?uide=20covers=20PY-1=20=E2=80=A6=20PY-10=20(PY-12)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New recipes: CSV/JSON into an editable table, with and without a table definition; checking a transfer first (diagnose, dry_run); fixing the columns and resuming (retry with a definition). The "S3 is unreachable" recipe uses upload_mode="proxy" instead of driving /stage by hand. Part 1 gains three sections — table definitions and inferred columns; upload_mode, including the fallback and the /stage cap; dry_run and diagnose — and its reference tables catch up: FolderIngest's parameters and methods, retry's decisions after a failed upload, IngestOutcome, IngestClient's endpoints, the models, and what an IngestApiError carries. Co-Authored-By: Claude Opus 5.5 --- docs/python-client-guide.md | 288 ++++++++++++++++++++++++++++++++---- 1 file changed, 261 insertions(+), 27 deletions(-) diff --git a/docs/python-client-guide.md b/docs/python-client-guide.md index f7ca4e9..6a98376 100644 --- a/docs/python-client-guide.md +++ b/docs/python-client-guide.md @@ -44,6 +44,9 @@ behaviour behind those snippets. - [`FolderIngest`](#folderingest) - [`intent` — what a transfer leaves behind](#intent--what-a-transfer-leaves-behind) - [`sub_path` — read-only data that accumulates](#sub_path--read-only-data-that-accumulates) + - [Table definitions, CSV/JSON options, inferred columns](#table-definitions-csvjson-options-inferred-columns) + - [`upload_mode` — how the bytes travel](#upload_mode--how-the-bytes-travel) + - [Checking a transfer first: `dry_run()` and `diagnose()`](#checking-a-transfer-first-dry_run-and-diagnose) - [Managing, resuming and abandoning a transfer](#managing-resuming-and-abandoning-a-transfer) - [`IngestOutcome`, `scan_folder`, `ingest_folder`](#ingestoutcome-scan_folder-ingest_folder) - [`IngestClient` — one method per endpoint](#ingestclient--one-method-per-endpoint) @@ -135,7 +138,12 @@ fewest lines that actually work. | …and keep the handle, so a failure is resumable | `job = FolderIngest(..., **_dds); job.run()` | | add this year to a table that grows | `ingest(..., intent="read_only", table_name="t", sub_path="2026")` | | redo one year only | `ingest(..., sub_path="2026", conflict_mode="replace")` | +| CSV / JSON into an editable table | `ingest("./csv", "path.to.target", data_format="csv", intent="editable")` — columns inferred | +| …with the column types I choose | `ingest(..., table_definition=TableDefinition(columns=(ColumnDef("v", "DOUBLE"),)))` | +| check a transfer before starting it | `job.diagnose()` · `job.dry_run()` | | resume a failed transfer | `job.retry()` | +| …after fixing the columns | `job.retry(table_definition=…)` | +| S3 is unreachable from this kernel | `FolderIngest(..., upload_mode="proxy", **_dds).run()` | | pick a transfer up in a later kernel | `FolderIngest.attach(session_id, ..., **_dds)` | | abandon a transfer | `job.cancel()` | | see my transfers | `IngestClient(**_dds).list_sessions(state="failed")` | @@ -250,6 +258,47 @@ ingest("./bw_2026", "water_management_resources.bathing_water.bwd.reference", table_name="water_temperature", sub_path="2026", conflict_mode="replace") ``` +### CSV or JSON into an editable table + +An editable table is built from column types. For CSV and JSON the client reads +every file locally, infers them, prints them (`Inferred columns: …`) and sends +them with the transfer — nothing to write: + +```python +out = ingest("./stations_csv", "biodiversity.uploads", data_format="csv", intent="editable") +``` + +To choose the types yourself, pass a table definition — and parsing hints, if +the files need them: + +```python +from eea_datalakehouse.dds_ingestion import ColumnDef, TableDefinition + +columns = TableDefinition(columns=( + ColumnDef("station_id", "VARCHAR"), + ColumnDef("measured_at", "TIMESTAMP"), + ColumnDef("value", "DOUBLE"), +)) +out = ingest("./stations_csv", "biodiversity.uploads", data_format="csv", intent="editable", + table_definition=columns, csv_options={"delimiter": ";"}) +``` + +A JSON *array* file cannot be inferred — pass its columns this way. Details: +[Table definitions](#table-definitions-csvjson-options-inferred-columns). + +### Check a transfer before you start it + +```python +job = FolderIngest("./my_data", "biodiversity.uploads", data_format="parquet", **_dds) +job.diagnose() # one ✓ / ✗ line per check, ending with a dry run of this folder +job.dry_run() # only the dry run: what commit would do +``` + +Neither uploads anything or opens a session. `diagnose()` says, in plain +language, which of DDS, your credential, the file sizes, DDS's storage and the +target is the problem — see +[Checking a transfer first](#checking-a-transfer-first-dry_run-and-diagnose). + ### The transfer failed — resume it without re-uploading ```python @@ -264,8 +313,25 @@ except Exception: `retry()` decides from the server's own report. It re-runs just the load step when the staged files survived, starts a fresh session when they did not, and **raises rather than guessing** for a permanently-stored transfer that a blind -re-run would duplicate. The full decision table is -[here](#managing-resuming-and-abandoning-a-transfer). +re-run would duplicate. An *upload* failure has already cancelled the session +(the error's note says so); `retry()` then uploads again if the failure can pass +— a network error, a 5xx — and otherwise raises with the upload error. The full +decision table is [here](#managing-resuming-and-abandoning-a-transfer). + +### The load failed on its columns — fix them and resume + +A commit that failed with `schema_required` or `schema_mismatch` (the error's +`error_slug`) needs no new upload — only the right columns: + +```python +from eea_datalakehouse.dds_ingestion import ColumnDef, TableDefinition + +job.retry(table_definition=TableDefinition(columns=(ColumnDef("value", "DOUBLE"),))) +``` + +The corrected definition replaces the stored one and only the load re-runs. It +needs a DDS server with DI-12.6; an older one ignores the definition and the +retry fails as before. ### Pick a transfer up in a later kernel @@ -331,23 +397,21 @@ dataflow level and below are created. ### S3 is unreachable from this kernel — send the bytes through the server -The fallback when presigned uploads cannot connect. Slower, since every byte -transits DDS: +Every file goes through DDS (`POST /api/v1/ingest/stage`) instead of to the +presigned S3 targets. Slower, since every byte transits DDS: ```python -from pathlib import Path -from eea_datalakehouse.dds_ingestion import scan_folder - -folder = Path("./my_data") -with IngestClient(**_dds) as c: - begin = c.begin(target_catalog_path="biodiversity.uploads", intent="read_only", - data_format="parquet", conflict_mode="fail", - files=scan_folder(folder, "parquet")) - for t in begin.s3.uploads: - c.stage(begin.session_id, t.rel_path, (folder / t.rel_path).read_bytes()) - print(c.commit(session_id=begin.session_id)) +job = FolderIngest("./my_data", "biodiversity.uploads", data_format="parquet", + upload_mode="proxy", **_dds) +out = job.run() +print(out.upload_mode, out.files_staged) # "proxy", every file ``` +Usually you do not need to ask. With the default `upload_mode="auto"` the client +follows the server, which can say `proxy` itself; and when it says `auto` (DDS's +default), a file whose presigned upload fails at the network layer goes through +DDS on its own. See [`upload_mode`](#upload_mode--how-the-bytes-travel). + ### Tell staged from permanent Whether your uploaded files *are* the table matters before you re-run anything: @@ -666,12 +730,18 @@ begin → upload (all files, in parallel) → commit server validates the target *here*, so an unauthorised or non-existent catalog folder fails before a single byte moves. It answers with an [`S3Plan`](#models): a bucket, a key prefix, and one presigned - [`UploadTarget`](#models) per file. + [`UploadTarget`](#models) per file — and, from a server with contract v0.8, + a [`BeginPlan`](#models) saying what commit will do, which `run()` shows + before uploading. 2. **upload** — each file's bytes go straight from your machine to object - storage using the target exactly as issued. Small files use a presigned POST - policy (or a legacy presigned PUT); large ones use S3 multipart with a + storage using the target exactly as issued. Small files use a presigned PUT + (older servers issue a POST policy); large ones use S3 multipart with a presigned URL per part. The DDS bearer token is **never** sent on these - requests — each presigned URL carries its own auth. + requests — each presigned URL carries its own auth. Where S3 cannot be + reached from your kernel the bytes go through DDS instead — see + [`upload_mode`](#upload_mode--how-the-bytes-travel). If an upload fails, + `run()` cancels the session before raising, so the files that did land are + deleted rather than left behind. 3. **`commit`** — the server loads what landed into the table. This is where the Dremio work happens (`CREATE TABLE` + `COPY INTO` on a managed catalog), so it is the step most likely to fail, and the one @@ -695,7 +765,13 @@ FolderIngest( parallelism=4, idempotency_key=None, multipart=None, + table_definition=None, # TableDefinition | dict + cleanup_on_failure=False, + csv_options=None, # dict, sent only with data_format="csv" + json_options=None, # dict, sent only with data_format="json" + infer_schema=True, show_progress=True, + upload_mode="auto", # "auto" | "direct" | "proxy" client=None, # inject an IngestClient (tests, custom transport) base_url=None, # else DDS_BASE_URL creds=None, # else _DREMIO_USER / _DREMIO_PWD @@ -714,7 +790,12 @@ FolderIngest( | `parallelism` | Concurrent uploads (a `ThreadPoolExecutor`). Must be ≥ 1 — `ValueError` otherwise. | | `idempotency_key` | Passed to `begin` so a repeated call replays the same session rather than starting a second one. **Dropped automatically** when `retry()` has to start over, or the server would just replay the failed session. | | `multipart` | `True`/`False` forces the server's multipart decision; `None` leaves it to the server. | -| `show_progress` | `False` suppresses the progress bar entirely. | +| `table_definition` | The table's column types (and, for later servers, partitioning and sorting) — a `TableDefinition` or a dict in the contract's shape. See [Table definitions](#table-definitions-csvjson-options-inferred-columns). | +| `cleanup_on_failure` | After a commit failure the server *could* retry, cancel the session — deleting its uploaded files — instead of keeping it for `retry()`. Default `False`: kept. | +| `csv_options` / `json_options` | Parsing hints sent on begin, each only for its own format. CSV: `delimiter`, `quote`, `header`, `encoding`. JSON: `format`, `"lines"` or `"array"`. Inference uses them too. | +| `infer_schema` | For CSV/JSON into an `editable` table without a `table_definition`: infer the columns locally from every file and send them. `False` turns that off. | +| `show_progress` | `False` suppresses the progress bar, and the plan, inferred columns and `diagnose()` lines that are otherwise printed. | +| `upload_mode` | `"auto"` follows the server; `"direct"` uploads only to S3; `"proxy"` sends every file through DDS. `ValueError` otherwise. See [`upload_mode`](#upload_mode--how-the-bytes-travel). | | `client` / `base_url` / `creds` | Injection points. Pass `client=` and the object never builds its own (and never closes yours). Otherwise `base_url` defaults to `DDS_BASE_URL` and `creds` to `load_creds()`. | Credentials live in the `IngestClient`, never on `FolderIngest` itself. @@ -723,12 +804,14 @@ Credentials live in the `IngestClient`, never on `FolderIngest` itself. | Method | Does | |---|---| -| `run() -> IngestOutcome` | The whole `begin → upload → commit` flow. Leaves the client open so you can still `status()`/`retry()`/`cancel()`. | +| `run() -> IngestOutcome` | The whole `begin → upload → commit` flow. Cancels the session if an upload fails. Leaves the client open so you can still `status()`/`retry()`/`cancel()`. | | `begin() -> BeginResult` | Step 1 alone. Raises `FileNotFoundError` if the scan found no files of `data_format`. Sets `session_id`. | +| `dry_run() -> DryRunResult` | Every check `begin` runs, and what commit would do — **nothing uploaded, no session**. See [Checking a transfer first](#checking-a-transfer-first-dry_run-and-diagnose). | +| `diagnose() -> Diagnosis` | Plain-language checks, one per line, ending with a dry run. See [Checking a transfer first](#checking-a-transfer-first-dry_run-and-diagnose). | | `commit(*, multipart_etags=None) -> CommitResult` | Step 3 alone. `IngestStateError` if nothing has begun. | | `status() -> StatusResult` | Server-side state of this transfer. | | `estimate() -> EstimateResult` | Row count + size class of the staged data, between upload and commit. | -| `retry() -> IngestOutcome` | Resume or re-run — see [below](#managing-resuming-and-abandoning-a-transfer). | +| `retry(*, table_definition=None) -> IngestOutcome` | Resume or re-run, optionally with corrected columns — see [below](#managing-resuming-and-abandoning-a-transfer). | | `cancel() -> None` | Delete the staged data and drop the session. Does **not** drop a table an earlier successful commit already created. Clears `session_id`. | | `close() -> None` | Release the HTTP client, but only if this object created it. | | `session_id` (property) | `None` until `begin()` runs or `attach()` supplies one. | @@ -791,6 +874,112 @@ FolderIngest( nothing: `scan_folder` keeps sub-folders and the server preserves the original `rel_path`. +## Table definitions, CSV/JSON options, inferred columns + +A table definition says what the table should look like. Every field is +optional, and only the fields you set are sent: + +```python +from eea_datalakehouse.dds_ingestion import ColumnDef, TableDefinition + +TableDefinition( + columns=(ColumnDef("station_id", "VARCHAR"), ColumnDef("value", "DOUBLE")), + partition_by=(), # accepted; the plan says whether this server applies it + sort_by=(), + version=None, # None → version 1 +) +``` + +A dict in the contract's shape works anywhere a `TableDefinition` does: +`{"columns": [{"name": "value", "type": "DOUBLE"}]}`. Types are Dremio's +(`VARCHAR`, `BIGINT`, `DOUBLE`, `DECIMAL(10,2)`, `TIMESTAMP`, …). +`ColumnDef.nullable` and `description` are accepted by the server but not +applied yet. + +**Where it goes.** On `begin`. A server with contract v0.8 answers with a plan +(`BeginResult.plan`); `run()` prints `plan.describe()` — the sentence the +JupyterLab extension shows — and logs a warning for each part of the definition +this server does not apply (`plan.definition_ignored`). A server without plans +ignored it, so it is sent again on `commit`. + +**When you give none — inferred columns.** For CSV or JSON into an `editable` +table (the one case where the server builds the table from columns), the client +reads every file with pyarrow, widens the types across files, prints them as +`Inferred columns: …`, and sends them as the definition. `csv_options` and +`json_options` are used for that reading too. A file that cannot be inferred — a +JSON *array*, for one — logs a warning and sends nothing; the server then asks +for the columns. `infer_schema=False` turns inference off. Parquet needs none: +its files carry their own schema. + +**Parquet with columns given.** They are checked against the files' own footers +before anything is uploaded, with the server's comparison rules. A mismatch +raises `IngestApiError` (422, `error_slug="schema_mismatch"`) in the server's +wording, and `where` says it was the local check. The server checks again at +commit and stays the authority. + +**After a failure.** `retry(table_definition=…)` sends corrected columns with +the retry — see [Managing](#managing-resuming-and-abandoning-a-transfer). + +## `upload_mode` — how the bytes travel + +Two parties decide: `FolderIngest(upload_mode=…)` and the server's hint in +`BeginResult.upload_mode` (DDS's `DDS_INGEST_UPLOAD_MODE`). + +| `FolderIngest(upload_mode=…)` | the server says | the files go | +|---|---|---| +| `"auto"` (default) | `"proxy"` | all through DDS (`POST /api/v1/ingest/stage`) | +| `"auto"` | `"auto"` — DDS's default | to S3; a file whose upload fails at the network layer goes through DDS instead | +| `"auto"` | `"direct"`, or nothing (an older server: `None`) | to S3 only | +| `"direct"` | anything | to S3 only | +| `"proxy"` | anything | all through DDS | + +- **Only a network-layer failure falls back** — an unreachable endpoint, DNS, TLS, + a timeout. An HTTP error from S3 is a signing or permission problem that DDS + would not fix, so it is raised. +- **`/stage` has its own size cap.** DDS checks it only when each file arrives, + so the client asks first (`GET /api/v1/ingest/limits`, at most once per run and + only when a file may go through DDS). With `"proxy"`, a file over the cap is + refused before anything is uploaded. With the fallback, such a file is not + sent through DDS, and the network error carries a note saying why. A server + without that endpoint is not checked locally. +- A file the server planned as multipart goes through DDS whole. +- `IngestOutcome.upload_mode` is `"proxy"` when every file went through DDS, else + `"direct"`; `IngestOutcome.files_staged` counts the files that went through + DDS, so a fallback shows. + +## Checking a transfer first: `dry_run()` and `diagnose()` + +Neither uploads anything or leaves a session behind. + +**`dry_run() -> DryRunResult`** runs every check `begin` runs — privileges, the +target folder, storage, collisions, the table definition — and returns what +commit would do: `plan` (a `BeginPlan`), `key_prefix` and `collision`. The plan +is printed too. It raises what a real `begin` would raise. A DDS server without +dry runs would open a real session instead; the client cancels it straight away +and raises `UnsupportedByServerError`. + +**`diagnose() -> Diagnosis`** runs these checks in order and prints one line +each (`✓` passed, `✗` failed, `–` skipped, with the reason): + +| Step | Checks | Skipped when | +|---|---|---| +| DDS reachable | `GET /health` | — if it fails, every step after it is skipped | +| Credential accepted | `GET /api/v1/ingest/limits`, which needs a login (`GET /api/v1/ingest` on a server without it) | — | +| File sizes | each file against `max_bytes` (what begin accepts) and `stage_max_bytes` (what `/stage` accepts; failing only with `upload_mode="proxy"`) | DDS reported no limits | +| Storage write test | DDS's own S3 write test (`POST /storage-selftest`) | no verdict yet, or an older server | +| Dry run of this folder | everything `dry_run()` checks, including WRITE on the target | a server without dry runs | + +``` +✓ DDS reachable: the service answers +✓ Credential accepted: DDS accepts your Dremio credential +✓ File sizes: within DDS's limits (largest: sub/b.parquet, 7 bytes) +✓ Storage write test: DDS can write to its S3 storage +✓ Dry run of this folder: Importing as a new editable table at bio/uploads/t. +``` + +`Diagnosis.steps` holds one `DiagnosisStep(name, ok, detail)` per line, `ok` +being `None` for a skipped step, and `Diagnosis.ok` is true when no step failed. + ## Managing, resuming and abandoning a transfer ```python @@ -812,8 +1001,22 @@ with FolderIngest(folder="./my_data", target_catalog_path="catalog.theme.sub", | failed, not resumable, staged placement | Opens a fresh session and uploads the folder again, dropping any `idempotency_key` first. | | `done` | `IngestStateError` — already succeeded, nothing to retry. | | `pending` / `uploading` / `committing` | `IngestStateError` — wait for it to finish. | +| — this object's own `run()` failed **in the upload**, so it cancelled the session | Uploads again under a new session when the failure can pass — a network error, a 5xx, or DDS saying `retryable` — and the cancel went through. Otherwise `IngestStateError`, quoting the upload error. The server is not asked: that session never got all its files. | | no session at all | `IngestStateError`. | +**With corrected columns.** `retry(table_definition=…)` replaces the +definition this transfer was given. A resumed load uses it without uploading +again — the fix for a `schema_required` or `schema_mismatch` failure (DI-12.6; a +server before that ignores it) — and an upload under a new session sends it on +`begin`. + +**What a failure leaves behind.** A failed *upload* cancels the session before +the error reaches you, deleting the files that did land; the error's note says +so, or says the cancel failed and the server's sweep will remove them. A failed +*commit* keeps the session for `retry()` when the server says it can be retried, +unless `cleanup_on_failure=True`; one it cannot retry the server has already +cleaned up. + > ⚠ **A transfer whose files are stored permanently is never re-uploaded > blindly.** There the upload landed in the table's own folder and stayed, so a > fresh session would add a *second* copy: the server numbers an incoming name @@ -856,6 +1059,8 @@ class IngestOutcome: files_skipped: int etags: dict[str, str] # rel_path -> single-shot ETag (multipart entries are not here) resumed: bool = False + upload_mode: str = "direct" # "proxy" when every file went through DDS + files_staged: int = 0 # files that went through DDS: all with "proxy", the fallbacks otherwise ``` ```python @@ -887,16 +1092,20 @@ IngestClient(base_url, creds, *, timeout=60.0, http_client=None) | Method | Endpoint / action | |---|---| -| `begin(*, target_catalog_path, intent, data_format, conflict_mode, files, table_name=None, sub_path=None, idempotency_key=None, multipart=None)` | `POST /api/v1/ingest/begin` → `BeginResult` | +| `begin(*, target_catalog_path, intent, data_format, conflict_mode, files, table_name=None, sub_path=None, idempotency_key=None, multipart=None, definition=None, csv_options=None, json_options=None)` | `POST /api/v1/ingest/begin` → `BeginResult`. A server that took `definition` returns `BeginResult.plan`. Only the fields that were set are sent. | +| `dry_run(*, …)` | `POST /api/v1/ingest/begin` with `dry_run: true` → `DryRunResult`. Takes `begin`'s arguments except `idempotency_key` and `multipart`. Against a server without dry runs: cancels the session it opened and raises `UnsupportedByServerError`. | | `commit(*, session_id, definition=None, multipart_etags=None)` | `POST /api/v1/ingest/commit` → `CommitResult` | | `get_status(session_id)` | `GET /api/v1/ingest/{id}` → `StatusResult` | | `list_sessions(state=None)` | `GET /api/v1/ingest` → `list[StatusResult]`. `state` filters server-side: `"active"`, `"failed"`, `"all"`. | -| `retry(session_id)` | `POST /api/v1/ingest/{id}/retry` → `StatusResult` | +| `retry(session_id, *, definition=None)` | `POST /api/v1/ingest/{id}/retry` → `StatusResult`. With `definition`, the body `{"definition": …}` replaces the stored one (DI-12.6); without it, no body, as before. | | `cancel(session_id)` | `DELETE /api/v1/ingest/{id}` | | `estimate(session_id)` | `POST /api/v1/ingest/estimate` → `EstimateResult`. On a managed catalog the count comes from staged Parquet footers; CSV/JSON have none and report `0`. | | `stage(session_id, rel_path, data)` | `POST /api/v1/ingest/stage` → `StageResult`. Uploads one file **through the server** — the fallback when the S3 endpoint is not reachable from this kernel. Slower (the bytes transit the server), so prefer `upload_file`. | -| `upload_file(target, data)` | To S3, not DDS. Presigned POST policy (form `fields` first, `file` part **last**, so S3 honours the policy conditions) or legacy presigned PUT. Returns the `ETag` header or `None`. | +| `upload_file(target, data)` | To S3, not DDS. Presigned PUT with the target's signed `headers` — the normal path since contract v0.7 — or, from an older server, a POST policy (form `fields` first, `file` part **last**, so S3 honours the policy conditions). Returns the `ETag` header or `None`. An `httpx.TransportError` (unreachable, TLS, timeout) passes through unchanged. | | `upload_file_multipart(target, data)` | To S3. Splits `data` into `len(target.parts)` chunks (ceil division; last part takes the remainder) and `PUT`s each to its presigned part URL. Returns `[{"part_number", "etag"}, …]` for `commit`'s `multipart_etags`. Raises if S3 omits an ETag for any part. | +| `health()` | `GET /health` (no login); raises unless DDS answers. | +| `limits(runner="notebook")` | `GET /api/v1/ingest/limits` → dict: `max_bytes` (the largest file begin accepts from this kind of client), `stage_max_bytes` (through `/stage`), `multipart_threshold`. Needs a login. | +| `storage_selftest()` | `POST /storage-selftest` (no login) → dict; `storage_write_test.ok` says whether DDS can write to its S3. | | `close()` | Closes the `httpx.Client` **only if** the client created it. Also a context manager. | Both upload methods enforce `target.max_bytes` locally (a `413` @@ -916,7 +1125,11 @@ working when the server adds fields. | `UploadPart` | `part_number`, `url` | | `UploadTarget` | `rel_path`, `url`, `method="POST"`, `fields`, `headers`, `max_bytes`, `upload_id`, `parts`; `is_multipart` (true when `upload_id` **and** `parts` are set) | | `S3Plan` | `bucket`, `key_prefix`, `uploads: tuple[UploadTarget, ...]` | -| `BeginResult` | `session_id`, `status`, `s3: S3Plan`, `collision` | +| `BeginResult` | `session_id`, `status`, `s3: S3Plan`, `collision`, `upload_mode` (the server's hint; `None` when it sends none), `plan: BeginPlan \| None` | +| `BeginPlan` | `placement`, `table_path`, `storage_path`, `action` (`"create"`, `"append"`, `"replace"`, `"fail"`), `definition_source`, `definition_applied`, `definition_ignored`; `describe()` — one sentence, the extension's wording | +| `DryRunResult` | `plan: BeginPlan`, `key_prefix`, `collision` | +| `TableDefinition` | `columns: tuple[ColumnDef, ...]`, `partition_by`, `sort_by`, `version`; `as_payload()` sends only what was set | +| `ColumnDef` | `name`, `type` (a Dremio type), `nullable`, `description` | | `CommitResult` | `session_id`, `status`, `table_path`, `record_count`, `storage_path` | | `Progress` | `files_done`, `files_total`, `step` | | `StatusResult` | `status`, `progress`, `raw` — plus the properties below | @@ -943,6 +1156,9 @@ client does not model yet is still reachable. | `error` | why it failed, including the stage (`"Dremio load failed: …"`, `"S3 upload failed: …"`). `None` unless failed. | | `is_terminal` | status is `done`, `failed` or `cancelled` | | `is_resumable` | failed **and** the server's own `resumable` flag is truthy | +| `failed_stage` | where a failed transfer broke: `"upload"` or `"load"` | +| `error_code` | the failure's slug, e.g. `"schema_required"` (contract v0.8) | +| `describe_failure()` | one sentence on the failure — stage, slug, reason, and what a retry would do — worded exactly as the JupyterLab Transfers panel words it | Lifecycle: `pending` → `uploading` → `committing` → `done`, or `failed` / `cancelled`. @@ -980,9 +1196,22 @@ RuntimeError ├── IngestStateError asked for locally-impossible state, before any HTTP call └── IngestApiError a DDS call returned non-success ├── StorageUnavailableError DDS cannot write to its own S3 (503) + ├── UnsupportedByServerError the DDS server predates the feature (e.g. dry runs) └── S3UploadError a presigned upload failed — NOT a DDS error ``` +**What every `IngestApiError` tells you**, besides its message: + +| Attribute | Meaning | +|---|---| +| `error_slug` | DDS's machine-readable `error`, e.g. `schema_required`, `schema_mismatch`; `""` when the body had none | +| `retryable` | whether repeating — for a failed transfer, `retry()` — can succeed unchanged; `None` from a server before contract v0.8 | +| `failed_stage` | on a failed commit from `FolderIngest`: `"upload"` or `"load"`, from the session; `None` otherwise | + +A failed commit also carries the session's own failure text as an exception +note — `StatusResult.describe_failure()`, the sentence the Transfers panel +shows. A failed upload carries a note on what happened to its session. + **`IngestApiError`** carries `status_code`, `message` and `where` — the call that failed, e.g. `POST /api/v1/ingest/begin`. `where` is in the message because a transfer makes several calls to two different systems, and "API error 500" alone @@ -1007,9 +1236,14 @@ an unreachable endpoint, a gateway error. It adds `rel_path` and `url`, and its message names both, so you can tell "your upload never landed" from "the Document Service refused the request". +**`UnsupportedByServerError`** — the DDS server is older than the feature a call +needs (e.g. `dry_run()`). A subclass of `IngestApiError`, so code that catches +that keeps working. + **`IngestStateError`** is raised locally, before any HTTP call — committing a transfer that never began, retrying one that is still running, cancelling -nothing. +nothing, or retrying an upload that would fail the same way (its message quotes +the upload error). ## Notebook helpers (`dds_ingestion.common`) From 95f72747fd97555c7992e5f8b5bde2e2021ed366 Mon Sep 17 00:00:00 2001 From: blikij Date: Mon, 19 Oct 2026 12:14:18 +0200 Subject: [PATCH 12/19] docs: mark PY-8, PY-9, PY-10, PY-12 and PY-13 done in the client plan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each follow-up task gets its as-built note and commit, the earlier tasks' "open" notes point at what closed them, and §5.2 / §5.5 say what is left: the §5.1 repair reaching main, PY-11 (Oskar's decision), and the end-to-end runs after deploy. Co-Authored-By: Claude Opus 5.5 --- docs/dds-ingest-robustness-client-plan.md | 104 ++++++++++++++-------- 1 file changed, 68 insertions(+), 36 deletions(-) diff --git a/docs/dds-ingest-robustness-client-plan.md b/docs/dds-ingest-robustness-client-plan.md index 2e41e39..d436544 100644 --- a/docs/dds-ingest-robustness-client-plan.md +++ b/docs/dds-ingest-robustness-client-plan.md @@ -1,8 +1,9 @@ # DDS ingest robustness — the client half (DI-12) — plan > Status: **PY-1 … PY-7 implemented** (2026-10-16). Reviewed against the code on -> 2026-10-19: what is still open, and the follow-up tasks PY-8 … PY-13, are in §5. -> Date: 2026-10-16. +> 2026-10-19 (§5); the follow-up tasks **PY-8, PY-9, PY-10, PY-12 and PY-13 +> implemented** the same day. Open: the §5.1 repair reaching `main`, and PY-11, +> which is Oskar's decision. Date: 2026-10-16. Companion plan for **this** repository. The design, the server work and the release schedule live with DDS: @@ -73,8 +74,8 @@ missing was the automatic fallback (G10 in `Documentation/data-digestion.md`). > decides, and its default `"auto"` follows the server. The fallback is added on > top: only when both `FolderIngest` and the server say `"auto"` > (`_falls_back_to_proxy`). DDS sends `upload_mode` on every begin, `"auto"` unless -> configured, so the fallback is on by default. Open: the `/stage` size cap is not -> checked before uploading (PY-10). +> configured, so the fallback is on by default. The `/stage` size cap is checked +> before uploading since PY-10. ### PY-2 Cancel when an upload fails; small fixes — R1 — **DONE** @@ -93,9 +94,9 @@ missing was the automatic fallback (G10 in `Documentation/data-digestion.md`). > Landed `9d924f7`. As built: any upload failure cancels the session, and the > uploads still queued are cancelled at the first failure; the PUT comment is fixed; -> `.ndjson` is in the format table (`formats.py`). Open: under -> `IngestSession(retry=True)` the cancel hides the upload error (PY-8); the client -> guide does not describe it yet (PY-12). +> `.ndjson` is in the format table (`formats.py`). Under +> `IngestSession(retry=True)` the cancel hid the upload error until PY-8; the +> client guide describes it since PY-12. ### PY-3 Optional table definition — R2 — **DONE** @@ -119,8 +120,8 @@ missing was the automatic fallback (G10 in `Documentation/data-digestion.md`). definition → only set fields sent; against a server without `definition_applied` → the definition goes on commit. -> Landed `6a4faaf`. Open: `IngestClient.retry(definition=…)` for DI-12.6 was not -> built (PY-9). +> Landed `6a4faaf`. `IngestClient.retry(definition=…)` for DI-12.6 followed in +> PY-9. ### PY-4 Show the plan, and `dry_run` — R2 — **DONE** @@ -157,8 +158,8 @@ missing was the automatic fallback (G10 in `Documentation/data-digestion.md`). > Landed `a7f99e9`. As built: `IngestApiError.error_slug` and `.retryable`; a failed > commit carries the session's own failure text (`StatusResult.describe_failure`) as -> an exception note; `cleanup_on_failure` as planned. `failed_stage` is in that note -> but is not an attribute (PY-13). +> an exception note; `cleanup_on_failure` as planned. `failed_stage` became an +> attribute in PY-13. ### PY-6 CSV/JSON options, inferred columns, local Parquet check — R3/R4 — **DONE** @@ -231,9 +232,9 @@ The follow-up tasks from the 2026-10-19 review are ordered in §5.5. - `docs/python-client-guide.md` describes each new option, with one example with and one without a table definition. mypy clean; the test suite passes. -> 2026-10-19: met except the client guide (PY-12). `dry_run` is a method rather -> than an option (PY-4). mypy is clean for the code these tasks touched; the -> remaining errors are in `dds_ingestion/common/dremio_identity.py`. +> 2026-10-19: met — the client guide since PY-12 (`f512d2f`). `dry_run` is a +> method rather than an option (PY-4). mypy is clean for the code these tasks +> touched; the remaining errors are in `dds_ingestion/common/dremio_identity.py`. --- @@ -264,23 +265,24 @@ the server sends none, as in `774691a`). The whole suite passes. This has to rea ### 5.2 Where each task stands -| Task | State | Open | -|---|---|---| -| PY-1 | done; built on `774691a` | `/stage` size cap not checked (PY-10); not in the client guide (PY-12) | -| PY-2 | done | hides the upload error under `IngestSession(retry=True)` (PY-8); not in the guide (PY-12) | -| PY-3 | done | `retry` cannot send a corrected definition (PY-9) | -| PY-4 | done, as a method | — | -| PY-5 | done | `failed_stage` only in the note text (PY-13) | -| PY-6 | done | end-to-end editable CSV run against `local-stack` not done (§5.7) | -| PY-7 | done | — | -| §4 | met except the guide | `docs/python-client-guide.md` describes none of the new options (PY-12) | +| Task | State | Found in the review | Follow-up | +|---|---|---|---| +| PY-1 | done; built on `774691a` | `/stage` size cap not checked; not in the client guide | PY-10, PY-12 — done | +| PY-2 | done | hides the upload error under `IngestSession(retry=True)`; not in the guide | PY-8, PY-12 — done | +| PY-3 | done | `retry` cannot send a corrected definition | PY-9 — done | +| PY-4 | done, as a method | — | — | +| PY-5 | done | `failed_stage` only in the note text | PY-13 — done | +| PY-6 | done | end-to-end editable CSV run against `local-stack` not done | §5.7 — open | +| PY-7 | done | — | — | +| §4 | met | `docs/python-client-guide.md` described none of the new options | PY-12 — done | Beyond the tasks: `IngestSession.ingest`, behind `%ingest`, passes `FolderIngest` a -fixed set of options, so none of the options above reaches a notebook user (PY-11). +fixed set of options, so none of the options above reaches a notebook user (PY-11, +open). ### 5.3 Follow-up tasks -#### PY-8 `retry()` after a cancelled upload +#### PY-8 `retry()` after a cancelled upload — **DONE** - **Today:** PY-2 cancels the session after a failed upload, and `FolderIngest.retry()` then raises `IngestStateError("no session to retry — call @@ -303,7 +305,14 @@ fixed set of options, so none of the options above reaches a notebook user (PY-1 network error in `direct` mode the next attempt uploads and commits; after a failed cancel nothing is re-uploaded. -#### PY-9 `retry` with a corrected table definition (rest of PY-3) +> Landed `7dad450`. As built: `run()` records the upload failure, and `retry()` +> looks at it before anything else — also when the cancel failed and a session id +> is left, because that session never got all its files and asking the server +> about it would only say "uploading". "Can pass" (`_may_succeed_on_retry`) is a +> network error, or an `IngestApiError` whose `retryable` is true or, without +> one, whose status is 5xx. + +#### PY-9 `retry` with a corrected table definition (rest of PY-3) — **DONE** - `IngestClient.retry(session_id, *, definition=None)` and `FolderIngest.retry(*, table_definition=None)`: with a definition, the resumed @@ -316,7 +325,13 @@ fixed set of options, so none of the options above reaches a notebook user (PY-1 without a definition is unchanged; against a server before DI-12.6 the retry behaves as today. -#### PY-10 Check file sizes against the server's limits (rest of PY-1) +> Landed `9b4f429`. As built: without a definition, `FolderIngest.retry()` still +> calls `client.retry(session_id)` with no keyword, so a client written against +> the old signature — two test fakes in `IngestSession`'s tests are — keeps +> working. The behaviour against a server before DI-12.6 is from that server's +> code (its endpoint takes no body), not a test. + +#### PY-10 Check file sizes against the server's limits (rest of PY-1) — **DONE** - DDS has two caps. `begin` refuses a file over the caller's cap (DI-12.13), but `POST /ingest/stage` has its own `stage_max_bytes` and refuses a file only when @@ -332,6 +347,13 @@ fixed set of options, so none of the options above reaches a notebook user (PY-1 - **Verify:** proxy mode with one file over the stage cap uploads nothing; the fallback is not attempted for such a file; an older server is unchanged. +> Landed `2a554b2`. As built: proxy mode asks for the limits before uploading; the +> fallback asks only when a file actually falls back, so a plain direct upload +> makes no extra request. Best-effort: a 404, or any failure of the limits +> request, leaves DDS's own check as the only one. `diagnose()` gained a "File +> sizes" step after the credential step: a file over `max_bytes` fails it, one +> over `stage_max_bytes` fails it only with `upload_mode="proxy"`. + #### PY-11 The new options in `%ingest` — Oskar's decision - `IngestSession.ingest` (`dds_ingestion/session.py`) passes `FolderIngest` seven @@ -346,7 +368,7 @@ fixed set of options, so none of the options above reaches a notebook user (PY-1 - **Owner:** Oskar Esparza — it is his module; nothing changes there without him. - **Server:** none. -#### PY-12 Client guide — the open part of §4 +#### PY-12 Client guide — the open part of §4 — **DONE** - `docs/python-client-guide.md` describes: `upload_mode` (`FolderIngest`'s choice, the server's hint, the fallback); cancel on upload failure; `table_definition`, @@ -355,13 +377,20 @@ fixed set of options, so none of the options above reaches a notebook user (PY-1 `error_slug`, `retryable` and the failure note. - The DI-12 plan (§7.2) lists the `upload_mode` and cancel parts as open since R1. -#### PY-13 Small fixes +> Landed `f512d2f`, covering PY-8, PY-9, PY-10 and PY-13 as well. As built: Part 1's +> reference tables also caught up with PY-3 … PY-7 (models, `IngestClient` +> endpoints, errors), and the "S3 is unreachable" recipe uses +> `upload_mode="proxy"` instead of driving `/stage` by hand. + +#### PY-13 Small fixes — **DONE** - `failed_stage` as an attribute on the exception from a failed commit (PY-5 named it; today it is only in the note). - `IngestOutcome.files_staged`: how many files went through DDS, so a fallback is visible. `IngestOutcome.upload_mode` stays as it is. +> Landed `a5900d8`. + ### 5.4 Not planned yet - **Dremio job ids in the failure text** (DI-12.18, R5): only together with the @@ -374,12 +403,15 @@ fixed set of options, so none of the options above reaches a notebook user (PY-1 ### 5.5 Order -| Step | What | Why first | -|---|---|---| -| 1 | §5.1 to `main`, with Oskar | every Python ingest from `@main` fails today | -| 2 | PY-8, PY-12, PY-13 | fix and document what is already shipped | -| 3 | PY-9, PY-10 | use server features that are already deployed | -| 4 | PY-11 | when Oskar agrees | +| Step | What | Why first | State | +|---|---|---|---| +| 1 | §5.1 to `main`, with Oskar | every Python ingest from `@main` fails today | on `eea_bliki` (`60cad54`), not yet on `main` | +| 2 | PY-8, PY-12, PY-13 | fix and document what is already shipped | done 2026-10-19 | +| 3 | PY-9, PY-10 | use server features that are already deployed | done 2026-10-19 | +| 4 | PY-11 | when Oskar agrees | open | + +Steps 2 and 3 were built in the order PY-8, PY-13, PY-9, PY-10, PY-12, so the guide +documents all of them at once. ### 5.6 Coordination From e1c262ded229f8700bbec7d83f5a8cb98c594627 Mon Sep 17 00:00:00 2001 From: blikij Date: Mon, 19 Oct 2026 13:21:57 +0200 Subject: [PATCH 13/19] feat(notebook): the one-line ingest options in %ingest (PY-11) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit IngestSession.ingest — and so `%ingest ingest(...)` — forwards four more FolderIngest options, each of which fits on the one line that queues the ingest: upload_mode="proxy" (S3 unreachable), csv_options, json_options, and table_definition as a dict. Left out on purpose: infer_schema (turning inference off only makes DDS ask for the columns, which table_definition gives anyway) and cleanup_on_failure (its cancel after a failed commit would leave commit(retry=True) nothing to retry). The queue-then-commit design is unchanged; %ingest help shows the new parameters from the signature. Co-Authored-By: Claude Opus 5.5 --- README.md | 5 +++ .../dds_ingestion/session.py | 26 ++++++++++- src/eea_datalakehouse/notebook/magics.py | 8 +++- tests/dds_ingestion/test_ingest_session.py | 45 +++++++++++++++++++ tests/notebook/test_magics.py | 2 + 5 files changed, 83 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 94f26c3..7e0a397 100644 --- a/README.md +++ b/README.md @@ -324,6 +324,11 @@ 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. +`%ingest ingest(...)` also takes what a one-off ingest may need, each on that same line: +`upload_mode="proxy"` when S3 is unreachable from the kernel, `csv_options={"delimiter": ";"}`, +`json_options={"format": "array"}`, and `table_definition={"columns": [{"name": "value", +"type": "DOUBLE"}]}` for the column types of a CSV or JSON table. + 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 diff --git a/src/eea_datalakehouse/dds_ingestion/session.py b/src/eea_datalakehouse/dds_ingestion/session.py index 0e8556f..7b216b4 100644 --- a/src/eea_datalakehouse/dds_ingestion/session.py +++ b/src/eea_datalakehouse/dds_ingestion/session.py @@ -30,6 +30,7 @@ from __future__ import annotations +from collections.abc import Mapping from dataclasses import dataclass, field from pathlib import Path from typing import Any @@ -37,7 +38,7 @@ from .client import IngestClient from .credentials import DremioCreds from .folder import DEFAULT_PARALLELISM, FolderIngest, IngestOutcome, IngestStateError -from .models import DataFormat, Intent +from .models import DataFormat, Intent, TableDefinition, UploadMode class IngestSessionError(RuntimeError): @@ -124,12 +125,29 @@ def ingest( parallelism: int = DEFAULT_PARALLELISM, multipart: bool | None = None, show_progress: bool = True, + table_definition: TableDefinition | Mapping[str, Any] | None = None, + csv_options: Mapping[str, Any] | None = None, + json_options: Mapping[str, Any] | None = None, + upload_mode: UploadMode = "auto", ) -> IngestSession: """Queue one folder ingest — see `FolderIngest` for what each argument means. No `idempotency_key` here: unlike the catalog side, a queued ingest's own key would only ever be used once per `commit()`, so there is nothing to encapsulate — `FolderIngest` already defaults - it to `None` (a fresh session every run).""" + it to `None` (a fresh session every run). + + Each of the last four still fits on the one `%ingest` line that queues + the ingest:: + + upload_mode="proxy" # S3 is unreachable from this kernel + csv_options={"delimiter": ";"} # CSV parsing hints + json_options={"format": "array"} # a JSON array rather than JSON lines + table_definition={"columns": [{"name": "value", "type": "DOUBLE"}]} + + `FolderIngest`'s `infer_schema` and `cleanup_on_failure` are left out on + purpose: turning inference off only makes DDS ask for the columns, which + `table_definition` gives anyway, and a cleanup after a failed commit + would leave `commit(retry=True)` nothing to retry.""" self._pending.append( _QueuedIngest( folder=folder, @@ -143,6 +161,10 @@ def ingest( "parallelism": parallelism, "multipart": multipart, "show_progress": show_progress, + "table_definition": table_definition, + "csv_options": csv_options, + "json_options": json_options, + "upload_mode": upload_mode, }, ) ) diff --git a/src/eea_datalakehouse/notebook/magics.py b/src/eea_datalakehouse/notebook/magics.py index 42e5483..fbee0c7 100644 --- a/src/eea_datalakehouse/notebook/magics.py +++ b/src/eea_datalakehouse/notebook/magics.py @@ -286,7 +286,13 @@ ), ] _INGEST_HELP = [ - ("ingest", "Queue one folder ingest (see FolderIngest for what each argument means)."), + ( + "ingest", + "Queue one folder ingest (see FolderIngest for what each argument means). " + "upload_mode='proxy' when S3 is unreachable; csv_options={'delimiter': ';'}; " + "table_definition={'columns': [{'name': 'value', 'type': 'DOUBLE'}]} for the column " + "types of a CSV/JSON table.", + ), ("commit", "Run every queued ingest in order; stops at the first failure."), ] _SDI_HELP = [ diff --git a/tests/dds_ingestion/test_ingest_session.py b/tests/dds_ingestion/test_ingest_session.py index 1995248..4c3aa5b 100644 --- a/tests/dds_ingestion/test_ingest_session.py +++ b/tests/dds_ingestion/test_ingest_session.py @@ -15,6 +15,7 @@ BeginResult, CommitResult, S3Plan, + StageResult, StatusResult, UploadTarget, ) @@ -166,3 +167,47 @@ def retry(self, session_id: str) -> StatusResult: assert len(report.outcomes) == 1 assert report.outcomes[0].resumed is True assert client.retry_calls == ["sess-1"] + + +def test_the_one_line_options_reach_folder_ingest(tmp_path: Path) -> None: + # PY-11: upload_mode, csv_options, json_options and table_definition, each + # given on the one line that queues the ingest. + class RecordingClient(_StubClient): + def __init__(self) -> None: + super().__init__() + self.begin_kwargs: list[dict[str, Any]] = [] + self.staged: list[str] = [] + + def begin(self, *, files: list[Any], **kwargs: Any) -> BeginResult: + self.begin_kwargs.append(kwargs) + return super().begin(files=files, **kwargs) + + def stage(self, session_id: str, rel_path: str, data: bytes) -> StageResult: + self.staged.append(rel_path) + return StageResult(rel_path=rel_path, key=f"p/{rel_path}", bytes_written=len(data)) + + csv_folder = tmp_path / "csv" + csv_folder.mkdir() + (csv_folder / "a.csv").write_text("a;b\n1;2\n") + json_folder = tmp_path / "json" + json_folder.mkdir() + (json_folder / "a.json").write_text('[{"a": 1}]') + columns = {"columns": [{"name": "a", "type": "BIGINT"}]} + + client = RecordingClient() + session = IngestSession(client=client) # type: ignore[arg-type] + session.ingest( + csv_folder, "bio.uploads.c", data_format="csv", intent="editable", show_progress=False, + csv_options={"delimiter": ";"}, table_definition=columns, upload_mode="proxy", + ) + session.ingest( + json_folder, "bio.uploads.j", data_format="json", show_progress=False, + json_options={"format": "array"}, + ) + session.commit() + + csv_begin, json_begin = client.begin_kwargs + assert csv_begin["csv_options"] == {"delimiter": ";"} + assert csv_begin["definition"] == columns + assert client.staged == ["a.csv"] # upload_mode="proxy": through DDS, not S3 + assert json_begin["json_options"] == {"format": "array"} diff --git a/tests/notebook/test_magics.py b/tests/notebook/test_magics.py index 31fbdd3..ad76560 100644 --- a/tests/notebook/test_magics.py +++ b/tests/notebook/test_magics.py @@ -399,6 +399,8 @@ def test_ingest_help_lists_methods( assert ">ingest<" in table_html and ">commit<" in table_html assert "folder, target_catalog_path" in table_html assert "retry=False, max_retries=3" in table_html + # PY-11: the one-line options show, with their defaults. + assert "csv_options=None" in table_html and "upload_mode='auto'" in table_html assert "str | None" not in table_html # no Python type-hint syntax leaking into the table assert _magics_instance(ip)._ingest_session is None From a02a561d3c39cfda96fb8b1ab11d26f97332e373 Mon Sep 17 00:00:00 2001 From: blikij Date: Mon, 19 Oct 2026 13:23:32 +0200 Subject: [PATCH 14/19] feat(notebook): %ingest diagnose checks what is queued (PY-11) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit IngestSession.diagnose() runs FolderIngest.diagnose() for every queued ingest and prints its lines — DDS reachable, the credential, the file sizes, DDS's storage, and a dry run — under a header naming the ingest. Nothing is uploaded and the queue is kept, so it fits between ingest(...) and commit: `%ingest diagnose`, one line, like `%ingest commit`. A missing folder or credential is printed as the finding instead of stopping the check. No separate dry_run verb: diagnose ends with one. Co-Authored-By: Claude Opus 5.5 --- README.md | 1 + docs/notebook-facade-for-data-scientists.md | 1 + .../dds_ingestion/session.py | 30 +++++++++++++ src/eea_datalakehouse/notebook/magics.py | 8 +++- tests/dds_ingestion/test_ingest_session.py | 45 +++++++++++++++++++ tests/notebook/test_magics.py | 6 +++ 6 files changed, 90 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 7e0a397..dcb0a29 100644 --- a/README.md +++ b/README.md @@ -316,6 +316,7 @@ import eea_datalakehouse.notebook # registers %catalog/%ingest — no %load_ext %ingest ingest(folder="./bw_2026", target_catalog_path="bwd.reference", data_format="parquet", table_name="water_temperature") +%ingest diagnose # optional: check the queued ingests first; nothing is uploaded %ingest commit(retry=True) ``` diff --git a/docs/notebook-facade-for-data-scientists.md b/docs/notebook-facade-for-data-scientists.md index 4b0b238..a06b414 100644 --- a/docs/notebook-facade-for-data-scientists.md +++ b/docs/notebook-facade-for-data-scientists.md @@ -201,6 +201,7 @@ the second call in a session. import eea_datalakehouse.notebook %ingest ingest(folder="./bw_2026", target_catalog_path="bwd.reference", data_format="parquet") +%ingest diagnose # optional: check what is queued; uploads nothing, keeps the queue %ingest commit(retry=True) %catalog set_tags(".water_temperature", ["reviewed"]) diff --git a/src/eea_datalakehouse/dds_ingestion/session.py b/src/eea_datalakehouse/dds_ingestion/session.py index 7b216b4..d2837ed 100644 --- a/src/eea_datalakehouse/dds_ingestion/session.py +++ b/src/eea_datalakehouse/dds_ingestion/session.py @@ -170,6 +170,36 @@ def ingest( ) return self + def diagnose(self) -> None: + """Check every queued ingest without running it — `FolderIngest.diagnose` + for each, one printed line per check: DDS reachable, the credential, the + file sizes, DDS's storage, and a dry run of that folder into its target. + Nothing is uploaded and the queue is kept, so a `commit()` afterwards + runs exactly these ingests.""" + if not self._pending: + print("nothing queued — queue an ingest(...) first, then diagnose") + return + for item in self._pending: + print(f"{item.describe()}:") + try: + job = FolderIngest( + item.folder, + item.target_catalog_path, + data_format=item.data_format, + client=self._client, + base_url=self._base_url, + creds=self._creds, + **{**item.kwargs, "show_progress": True}, # the lines are the point + ) + except (OSError, ValueError, RuntimeError) as exc: + # A missing folder or credential is itself the finding. + print(f"✗ cannot check it: {exc}") + continue + try: + job.diagnose() + finally: + job.close() + def commit(self, *, retry: bool = False, max_retries: int = 3) -> IngestCommitReport: """Run every queued ingest, in order. Stops at the first failure — see the module docstring: nothing already succeeded can be rolled diff --git a/src/eea_datalakehouse/notebook/magics.py b/src/eea_datalakehouse/notebook/magics.py index fbee0c7..3a2a3f7 100644 --- a/src/eea_datalakehouse/notebook/magics.py +++ b/src/eea_datalakehouse/notebook/magics.py @@ -18,6 +18,7 @@ %ingest ingest(folder="./bw_2026", target_catalog_path="bwd.reference", data_format="parquet", table_name="water_temperature") + %ingest diagnose %ingest commit(retry=True) %sdi get_xml("070d9baa-448d-4168-8514-7dadb3ad876d") @@ -108,7 +109,7 @@ ), "ingest": ( '%ingest ingest(folder="./data", target_catalog_path="a.b", data_format="parquet")' - " | %ingest commit" + " | %ingest diagnose | %ingest commit" ), "sdi": '%sdi get_xml("") | %sdi resolve_series("")', "metadata": ( @@ -293,6 +294,11 @@ "table_definition={'columns': [{'name': 'value', 'type': 'DOUBLE'}]} for the column " "types of a CSV/JSON table.", ), + ( + "diagnose", + "Check every queued ingest without running it: DDS, your credential, the file " + "sizes, DDS's storage, and a dry run. Nothing is uploaded; the queue is kept.", + ), ("commit", "Run every queued ingest in order; stops at the first failure."), ] _SDI_HELP = [ diff --git a/tests/dds_ingestion/test_ingest_session.py b/tests/dds_ingestion/test_ingest_session.py index 4c3aa5b..ca2b013 100644 --- a/tests/dds_ingestion/test_ingest_session.py +++ b/tests/dds_ingestion/test_ingest_session.py @@ -11,6 +11,7 @@ import pytest +from eea_datalakehouse.dds_ingestion.folder import Diagnosis, FolderIngest from eea_datalakehouse.dds_ingestion.models import ( BeginResult, CommitResult, @@ -211,3 +212,47 @@ def stage(self, session_id: str, rel_path: str, data: bytes) -> StageResult: assert csv_begin["definition"] == columns assert client.staged == ["a.csv"] # upload_mode="proxy": through DDS, not S3 assert json_begin["json_options"] == {"format": "array"} + + +def test_diagnose_checks_every_queued_ingest_and_keeps_the_queue( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + checked: list[tuple[Path, bool]] = [] + + def fake_diagnose(self: FolderIngest) -> Diagnosis: + checked.append((self.folder, self.show_progress)) + return Diagnosis() + + monkeypatch.setattr(FolderIngest, "diagnose", fake_diagnose) + client = _StubClient() + session = IngestSession(client=client) # type: ignore[arg-type] + a, b = _make_folder(tmp_path, "a"), _make_folder(tmp_path, "b") + session.ingest(a, "bio.uploads.a", data_format="parquet", show_progress=False) + session.ingest(b, "bio.uploads.b", data_format="parquet", show_progress=False) + + session.diagnose() + + assert checked == [(a, True), (b, True)] # the lines print even for a quiet ingest + assert client.begin_calls == 0 + assert repr(session) == "IngestSession(pending=2)" + assert f"ingest {a!r} -> 'bio.uploads.a':" in capsys.readouterr().out + + +def test_diagnose_reports_a_missing_folder_and_goes_on( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + monkeypatch.setattr(FolderIngest, "diagnose", lambda self: Diagnosis()) + session = IngestSession(client=_StubClient()) # type: ignore[arg-type] + session.ingest(tmp_path / "gone", "bio.uploads.x", data_format="parquet") + session.ingest(_make_folder(tmp_path, "b"), "bio.uploads.b", data_format="parquet") + + session.diagnose() + + out = capsys.readouterr().out + assert "✗ cannot check it: not a directory" in out + assert "'bio.uploads.b':" in out + + +def test_diagnose_with_nothing_queued_says_so(capsys: pytest.CaptureFixture[str]) -> None: + IngestSession(client=_StubClient()).diagnose() # type: ignore[arg-type] + assert "nothing queued" in capsys.readouterr().out diff --git a/tests/notebook/test_magics.py b/tests/notebook/test_magics.py index ad76560..bc6c87d 100644 --- a/tests/notebook/test_magics.py +++ b/tests/notebook/test_magics.py @@ -271,6 +271,11 @@ def test_ingest_magic_queues_onto_one_shared_session(ip: Any) -> None: assert repr(session) == "IngestSession(pending=1)" +def test_ingest_diagnose_with_nothing_queued(ip: Any, capsys: pytest.CaptureFixture[str]) -> None: + ip.run_line_magic("ingest", "diagnose") # bare name, like commit + assert "nothing queued" in capsys.readouterr().out + + def test_empty_line_prints_usage(ip: Any, capsys: pytest.CaptureFixture[str]) -> None: # No credentials needed to reach the usage message — it's printed before # a session is ever built. @@ -397,6 +402,7 @@ def test_ingest_help_lists_methods( table_html = displayed[0].data assert "Command" in table_html and "Parameters" in table_html and "Description" in table_html assert ">ingest<" in table_html and ">commit<" in table_html + assert ">diagnose<" in table_html assert "folder, target_catalog_path" in table_html assert "retry=False, max_retries=3" in table_html # PY-11: the one-line options show, with their defaults. From ee1559e8049d3861c9f3db81456d8ca36fd720bb Mon Sep 17 00:00:00 2001 From: blikij Date: Mon, 19 Oct 2026 13:24:05 +0200 Subject: [PATCH 15/19] docs: mark PY-11 done in the client plan The scope agreed after talking to Oskar (keep IngestSession's concept; only what fits on one %ingest line), what was added and what was left out, and why. The main repair is shown as PR #97, left to the user and Oskar. Co-Authored-By: Claude Opus 5.5 --- docs/dds-ingest-robustness-client-plan.md | 33 +++++++++++++++-------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/docs/dds-ingest-robustness-client-plan.md b/docs/dds-ingest-robustness-client-plan.md index d436544..8aadc40 100644 --- a/docs/dds-ingest-robustness-client-plan.md +++ b/docs/dds-ingest-robustness-client-plan.md @@ -1,9 +1,9 @@ # DDS ingest robustness — the client half (DI-12) — plan > Status: **PY-1 … PY-7 implemented** (2026-10-16). Reviewed against the code on -> 2026-10-19 (§5); the follow-up tasks **PY-8, PY-9, PY-10, PY-12 and PY-13 -> implemented** the same day. Open: the §5.1 repair reaching `main`, and PY-11, -> which is Oskar's decision. Date: 2026-10-16. +> 2026-10-19 (§5); the follow-up tasks **PY-8 … PY-13 implemented** the same day, +> PY-11 with the scope agreed with Oskar. Open: the §5.1 repair reaching `main` +> (PR #97, left to the user and Oskar). Date: 2026-10-16. Companion plan for **this** repository. The design, the server work and the release schedule live with DDS: @@ -276,9 +276,9 @@ the server sends none, as in `774691a`). The whole suite passes. This has to rea | PY-7 | done | — | — | | §4 | met | `docs/python-client-guide.md` described none of the new options | PY-12 — done | -Beyond the tasks: `IngestSession.ingest`, behind `%ingest`, passes `FolderIngest` a -fixed set of options, so none of the options above reaches a notebook user (PY-11, -open). +Beyond the tasks: `IngestSession.ingest`, behind `%ingest`, passed `FolderIngest` a +fixed set of options, so none of the options above reached a notebook user (PY-11 — +done). ### 5.3 Follow-up tasks @@ -354,7 +354,7 @@ open). > sizes" step after the credential step: a file over `max_bytes` fails it, one > over `stage_max_bytes` fails it only with `upload_mode="proxy"`. -#### PY-11 The new options in `%ingest` — Oskar's decision +#### PY-11 The new options in `%ingest` — Oskar's decision — **DONE** - `IngestSession.ingest` (`dds_ingestion/session.py`) passes `FolderIngest` seven options: `intent`, `conflict_mode`, `table_name`, `sub_path`, `parallelism`, @@ -368,6 +368,17 @@ open). - **Owner:** Oskar Esparza — it is his module; nothing changes there without him. - **Server:** none. +> Landed `e1c262d` and `a02a561`. Scope set on 2026-10-19 after talking to Oskar: +> keep `IngestSession`'s logic and queue-then-commit concept, and add only what a +> one-off user can give on the **one** `%ingest` line — what cannot be, stays out. +> As built: `upload_mode`, `csv_options`, `json_options` and `table_definition` (a +> dict on that line) are forwarded. `infer_schema` and `cleanup_on_failure` are +> left out: turning inference off only makes DDS ask for the columns, which +> `table_definition` gives anyway, and a cleanup after a failed commit would leave +> `commit(retry=True)` nothing to retry. One verb, `%ingest diagnose`, runs +> `FolderIngest.diagnose()` for every queued ingest and keeps the queue; no +> `dry_run` verb, since `diagnose` ends with one. + #### PY-12 Client guide — the open part of §4 — **DONE** - `docs/python-client-guide.md` describes: `upload_mode` (`FolderIngest`'s choice, @@ -405,18 +416,18 @@ open). | Step | What | Why first | State | |---|---|---|---| -| 1 | §5.1 to `main`, with Oskar | every Python ingest from `@main` fails today | on `eea_bliki` (`60cad54`), not yet on `main` | +| 1 | §5.1 to `main`, with Oskar | every Python ingest from `@main` fails today | on `eea_bliki` (`60cad54`); PR #97 into `main` is open, and merging it is left to the user and Oskar | | 2 | PY-8, PY-12, PY-13 | fix and document what is already shipped | done 2026-10-19 | | 3 | PY-9, PY-10 | use server features that are already deployed | done 2026-10-19 | -| 4 | PY-11 | when Oskar agrees | open | +| 4 | PY-11 | when Oskar agrees | done 2026-10-19 | Steps 2 and 3 were built in the order PY-8, PY-13, PY-9, PY-10, PY-12, so the guide documents all of them at once. ### 5.6 Coordination -- Oskar Esparza reviews the §5.1 repair, which restores his `774691a`, and decides - PY-11. +- Oskar Esparza reviews the §5.1 repair (PR #97), which restores his `774691a`. + PY-11's scope was agreed with him. - The DI-12 plan (§7.2) cites PY-7 as `8d79542`; that commit was rebased before it was pushed and is `c1149d5`. From 1a01be1627f42026d5ef09c6b8c33ce67e369cb3 Mon Sep 17 00:00:00 2001 From: blikij Date: Mon, 19 Oct 2026 13:31:08 +0200 Subject: [PATCH 16/19] feat(notebook): cleanup_on_failure in %ingest; retry after a cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit IngestSession.ingest — and so `%ingest ingest(...)` — forwards cleanup_on_failure too: for most one-off ingests a retry will not help, and a failed commit should leave nothing behind. Its cancel used to leave retry() with no session ("no session to retry"), which is what commit(retry=True) would have shown. A commit that cleanup_on_failure cancelled is now recorded like a failed upload (PY-8), so retry() uploads again under a new session — the failure was retryable, or the cleanup would not have run. If the cancel itself failed, the session is still there and retry() resumes it on the server as before. Co-Authored-By: Claude Opus 5.5 --- README.md | 5 +- docs/dds-ingest-robustness-client-plan.md | 16 +++-- docs/python-client-guide.md | 4 +- src/eea_datalakehouse/dds_ingestion/folder.py | 70 ++++++++++--------- .../dds_ingestion/session.py | 14 ++-- tests/dds_ingestion/test_failure_reporting.py | 54 ++++++++++++++ tests/dds_ingestion/test_ingest_session.py | 32 +++++++++ tests/notebook/test_magics.py | 1 + 8 files changed, 149 insertions(+), 47 deletions(-) diff --git a/README.md b/README.md index dcb0a29..1db7899 100644 --- a/README.md +++ b/README.md @@ -327,8 +327,9 @@ custodian, not necessarily a developer. `%ingest help` does the same for `Ingest `%ingest ingest(...)` also takes what a one-off ingest may need, each on that same line: `upload_mode="proxy"` when S3 is unreachable from the kernel, `csv_options={"delimiter": ";"}`, -`json_options={"format": "array"}`, and `table_definition={"columns": [{"name": "value", -"type": "DOUBLE"}]}` for the column types of a CSV or JSON table. +`json_options={"format": "array"}`, `table_definition={"columns": [{"name": "value", +"type": "DOUBLE"}]}` for the column types of a CSV or JSON table, and `cleanup_on_failure=True` +so a failed commit deletes its upload instead of leaving it for a retry. 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. diff --git a/docs/dds-ingest-robustness-client-plan.md b/docs/dds-ingest-robustness-client-plan.md index 8aadc40..57c2ba2 100644 --- a/docs/dds-ingest-robustness-client-plan.md +++ b/docs/dds-ingest-robustness-client-plan.md @@ -310,7 +310,9 @@ done). > is left, because that session never got all its files and asking the server > about it would only say "uploading". "Can pass" (`_may_succeed_on_retry`) is a > network error, or an `IngestApiError` whose `retryable` is true or, without -> one, whose status is 5xx. +> one, whose status is 5xx. Since PY-11, a commit that `cleanup_on_failure` cancelled is +> recorded the same way, so a retry after a cleanup uploads again instead of +> finding no session. #### PY-9 `retry` with a corrected table definition (rest of PY-3) — **DONE** @@ -371,11 +373,13 @@ done). > Landed `e1c262d` and `a02a561`. Scope set on 2026-10-19 after talking to Oskar: > keep `IngestSession`'s logic and queue-then-commit concept, and add only what a > one-off user can give on the **one** `%ingest` line — what cannot be, stays out. -> As built: `upload_mode`, `csv_options`, `json_options` and `table_definition` (a -> dict on that line) are forwarded. `infer_schema` and `cleanup_on_failure` are -> left out: turning inference off only makes DDS ask for the columns, which -> `table_definition` gives anyway, and a cleanup after a failed commit would leave -> `commit(retry=True)` nothing to retry. One verb, `%ingest diagnose`, runs +> As built: `upload_mode`, `csv_options`, `json_options`, `table_definition` (a +> dict on that line) and `cleanup_on_failure` are forwarded — the last at the +> user's request, since for most one-off ingests a retry will not help and a failed +> commit should leave nothing behind; `commit(retry=True)` then uploads a +> cleaned-up ingest again (PY-8). `infer_schema` is left out: turning inference off +> only makes DDS ask for the columns, which `table_definition` gives anyway. One +> verb, `%ingest diagnose`, runs > `FolderIngest.diagnose()` for every queued ingest and keeps the queue; no > `dry_run` verb, since `diagnose` ends with one. diff --git a/docs/python-client-guide.md b/docs/python-client-guide.md index 6a98376..d07be02 100644 --- a/docs/python-client-guide.md +++ b/docs/python-client-guide.md @@ -791,7 +791,7 @@ FolderIngest( | `idempotency_key` | Passed to `begin` so a repeated call replays the same session rather than starting a second one. **Dropped automatically** when `retry()` has to start over, or the server would just replay the failed session. | | `multipart` | `True`/`False` forces the server's multipart decision; `None` leaves it to the server. | | `table_definition` | The table's column types (and, for later servers, partitioning and sorting) — a `TableDefinition` or a dict in the contract's shape. See [Table definitions](#table-definitions-csvjson-options-inferred-columns). | -| `cleanup_on_failure` | After a commit failure the server *could* retry, cancel the session — deleting its uploaded files — instead of keeping it for `retry()`. Default `False`: kept. | +| `cleanup_on_failure` | After a commit failure the server *could* retry, cancel the session — deleting its uploaded files — instead of keeping it for `retry()`. Default `False`: kept. With it, a `retry()` afterwards uploads again under a new session. | | `csv_options` / `json_options` | Parsing hints sent on begin, each only for its own format. CSV: `delimiter`, `quote`, `header`, `encoding`. JSON: `format`, `"lines"` or `"array"`. Inference uses them too. | | `infer_schema` | For CSV/JSON into an `editable` table without a `table_definition`: infer the columns locally from every file and send them. `False` turns that off. | | `show_progress` | `False` suppresses the progress bar, and the plan, inferred columns and `diagnose()` lines that are otherwise printed. | @@ -1001,7 +1001,7 @@ with FolderIngest(folder="./my_data", target_catalog_path="catalog.theme.sub", | failed, not resumable, staged placement | Opens a fresh session and uploads the folder again, dropping any `idempotency_key` first. | | `done` | `IngestStateError` — already succeeded, nothing to retry. | | `pending` / `uploading` / `committing` | `IngestStateError` — wait for it to finish. | -| — this object's own `run()` failed **in the upload**, so it cancelled the session | Uploads again under a new session when the failure can pass — a network error, a 5xx, or DDS saying `retryable` — and the cancel went through. Otherwise `IngestStateError`, quoting the upload error. The server is not asked: that session never got all its files. | +| — this object's own `run()` cancelled the session: after a failed **upload**, or after a failed commit with `cleanup_on_failure=True` | Uploads again under a new session when the failure can pass — a network error, a 5xx, or DDS saying `retryable` — and the cancel went through. Otherwise `IngestStateError`, quoting the error. The server is not asked: that session is gone, or never got all its files. | | no session at all | `IngestStateError`. | **With corrected columns.** `retry(table_definition=…)` replaces the diff --git a/src/eea_datalakehouse/dds_ingestion/folder.py b/src/eea_datalakehouse/dds_ingestion/folder.py index 4dcc7e9..848818b 100644 --- a/src/eea_datalakehouse/dds_ingestion/folder.py +++ b/src/eea_datalakehouse/dds_ingestion/folder.py @@ -104,13 +104,15 @@ class IngestOutcome: @dataclass(frozen=True, slots=True) -class _UploadFailure: - """Why the last :meth:`FolderIngest.run` stopped in the upload phase, kept for - :meth:`FolderIngest.retry` (PY-8).""" +class _FailedRun: + """Why the last :meth:`FolderIngest.run` stopped and cancelled its session — + an upload failure (PY-2), or a commit failure with ``cleanup_on_failure`` — + kept for :meth:`FolderIngest.retry` (PY-8).""" + stage: str # "upload" | "commit" error: Exception session_id: str - cancelled: bool # whether PY-2's cancel went through + cancelled: bool # whether the cancel went through def _may_succeed_on_retry(err: Exception) -> bool: @@ -226,7 +228,8 @@ def __init__( # begin; the begin plan says which parts this server uses. self.table_definition = table_definition # DI-12.26: after a commit failure the server could retry, drop the - # session (and its uploaded files) instead of keeping it for retry(). + # session (and its uploaded files) instead of keeping it for retry(); + # a retry() then uploads again under a new session. self.cleanup_on_failure = cleanup_on_failure # Parsing hints sent on begin (the server already accepts them). self.csv_options = csv_options @@ -244,9 +247,9 @@ def __init__( # command below works from. self._session_id: str | None = None self._begin: BeginResult | None = None - # Set when run() cancelled its session after a failed upload; what - # retry() goes by once there is no session left (PY-8). - self._upload_failure: _UploadFailure | None = None + # Set when run() cancelled its session after a failure; what retry() + # goes by once there is no session left (PY-8). + self._failed_run: _FailedRun | None = None # The files the current run sent through DDS after a network error; # appended to from the upload threads (list.append is atomic). self._staged_by_fallback: list[str] = [] @@ -315,13 +318,13 @@ def run(self) -> IngestOutcome: :meth:`status`, :meth:`retry` or :meth:`cancel` the transfer. Use it as a context manager (or call :meth:`close`) to release it. """ - self._upload_failure = None + self._failed_run = None begin = self.begin() try: etags, multipart_etags, uploaded, skipped = self._upload_all(begin) except Exception as err: cancelled = self._cancel_after_failure(err) - self._upload_failure = _UploadFailure(err, begin.session_id, cancelled) + self._failed_run = _FailedRun("upload", err, begin.session_id, cancelled) raise try: commit = self.commit(multipart_etags=multipart_etags or None) @@ -649,11 +652,12 @@ def retry( staged copy is gone. A fresh session is opened and the folder is uploaded again. Any ``idempotency_key`` is dropped for that attempt, or the server would just replay the failed session. - * **An upload of this object's own** :meth:`run` **failed**, so that run - cancelled its session (PY-2) and there is none left to resume. The - folder is uploaded again under a new session if the failure can pass - (a network error, a 5xx) and the cancel went through; otherwise this - raises :class:`IngestStateError` quoting the upload error. + * **This object's own** :meth:`run` **cancelled its session** — after a + failed upload (PY-2), or after a failed commit with + ``cleanup_on_failure`` — so there is none left to resume. The folder is + uploaded again under a new session if the failure can pass (a network + error, a 5xx, DDS saying ``retryable``) and the cancel went through; + otherwise this raises :class:`IngestStateError` quoting the error. **A transfer whose files are stored permanently is never re-uploaded blindly.** There the upload landed in the table's own folder and stayed, @@ -669,10 +673,10 @@ def retry( """ if table_definition is not None: self.table_definition = table_definition - if self._upload_failure is not None: - # Whether or not its cancel went through, that session never got all - # its files: the server has nothing to resume (PY-8). - return self._rerun_after_failed_upload(self._upload_failure) + if self._failed_run is not None: + # That session was cancelled, or never got all its files: the server + # has nothing to resume (PY-8). + return self._rerun_after_cancel(self._failed_run) if self._session_id is None: raise IngestStateError("no session to retry — call run() first") status = self._client.get_status(self._session_id) @@ -720,15 +724,15 @@ def retry( self.idempotency_key = None return self.run() - def _rerun_after_failed_upload(self, failure: _UploadFailure) -> IngestOutcome: - """:meth:`retry` once :meth:`run` cancelled its session after a failed - upload (PY-8). Uploads again only when a second attempt can succeed and - nothing of the first is left behind; otherwise raises, quoting the upload - error, so a caller that retries (``IngestSession.commit(retry=True)``) - shows the cause instead of "no session".""" + def _rerun_after_cancel(self, failure: _FailedRun) -> IngestOutcome: + """:meth:`retry` once :meth:`run` cancelled its session after a failure + (PY-8). Uploads again only when a second attempt can succeed and nothing + of the first is left behind; otherwise raises, quoting the error, so a + caller that retries (``IngestSession.commit(retry=True)``) shows the + cause instead of "no session".""" if not failure.cancelled: raise IngestStateError( - f"the upload failed ({failure.error}) and session {failure.session_id} " + f"the {failure.stage} failed ({failure.error}) and session {failure.session_id} " "could not be cancelled, so files it uploaded may still be in place; " "uploading again could add a second copy. Cancel it with " "FolderIngest.attach(...).cancel(), or wait for the server's sweep, " @@ -736,12 +740,13 @@ def _rerun_after_failed_upload(self, failure: _UploadFailure) -> IngestOutcome: ) from failure.error if not _may_succeed_on_retry(failure.error): raise IngestStateError( - f"the upload failed ({failure.error}); its session was cancelled, and " - "uploading again would fail the same way. Fix the cause, then run the " - "ingest again." + f"the {failure.stage} failed ({failure.error}); its session was " + "cancelled, and running it again would fail the same way. Fix the " + "cause, then run the ingest again." ) from failure.error logger.info( - "the upload of session %s failed (%s); uploading again under a new session", + "the %s of session %s failed (%s); uploading again under a new session", + failure.stage, failure.session_id, failure.error, ) @@ -770,8 +775,9 @@ def _after_failed_commit(self, err: IngestApiError) -> None: err.add_note(status.describe_failure()) except Exception: # noqa: BLE001 — the commit error is what matters logger.debug("could not fetch the failed session's status", exc_info=True) - if self.cleanup_on_failure and err.retryable is True: - self._cancel_after_failure(err) + if self.cleanup_on_failure and err.retryable is True and self._cancel_after_failure(err): + # The upload is gone with the session, so retry() starts over (PY-8). + self._failed_run = _FailedRun("commit", err, session_id, cancelled=True) def _cancel_after_failure(self, err: Exception) -> bool: """Cancel the session after a failure, say so on ``err``, and return diff --git a/src/eea_datalakehouse/dds_ingestion/session.py b/src/eea_datalakehouse/dds_ingestion/session.py index d2837ed..32a09b7 100644 --- a/src/eea_datalakehouse/dds_ingestion/session.py +++ b/src/eea_datalakehouse/dds_ingestion/session.py @@ -126,6 +126,7 @@ def ingest( multipart: bool | None = None, show_progress: bool = True, table_definition: TableDefinition | Mapping[str, Any] | None = None, + cleanup_on_failure: bool = False, csv_options: Mapping[str, Any] | None = None, json_options: Mapping[str, Any] | None = None, upload_mode: UploadMode = "auto", @@ -136,18 +137,20 @@ def ingest( there is nothing to encapsulate — `FolderIngest` already defaults it to `None` (a fresh session every run). - Each of the last four still fits on the one `%ingest` line that queues + Each of the last five still fits on the one `%ingest` line that queues the ingest:: upload_mode="proxy" # S3 is unreachable from this kernel csv_options={"delimiter": ";"} # CSV parsing hints json_options={"format": "array"} # a JSON array rather than JSON lines table_definition={"columns": [{"name": "value", "type": "DOUBLE"}]} + cleanup_on_failure=True # a failed commit leaves nothing behind - `FolderIngest`'s `infer_schema` and `cleanup_on_failure` are left out on - purpose: turning inference off only makes DDS ask for the columns, which - `table_definition` gives anyway, and a cleanup after a failed commit - would leave `commit(retry=True)` nothing to retry.""" + With `cleanup_on_failure=True`, `commit(retry=True)` uploads a cleaned-up + ingest again under a new session (see `FolderIngest.retry`). + `FolderIngest`'s `infer_schema` is left out on purpose: turning + inference off only makes DDS ask for the columns, which + `table_definition` gives anyway.""" self._pending.append( _QueuedIngest( folder=folder, @@ -162,6 +165,7 @@ def ingest( "multipart": multipart, "show_progress": show_progress, "table_definition": table_definition, + "cleanup_on_failure": cleanup_on_failure, "csv_options": csv_options, "json_options": json_options, "upload_mode": upload_mode, diff --git a/tests/dds_ingestion/test_failure_reporting.py b/tests/dds_ingestion/test_failure_reporting.py index 607825d..8bd82cf 100644 --- a/tests/dds_ingestion/test_failure_reporting.py +++ b/tests/dds_ingestion/test_failure_reporting.py @@ -140,3 +140,57 @@ def test_cleanup_on_failure( with pytest.raises(IngestApiError): _job(data_folder, client, cleanup_on_failure=cleanup).run() assert (client.cancelled == ["sess-X"]) is cancelled + + +# --- a retry after cleanup_on_failure (PY-8) ----------------------------------------- + + +class FlakyCommitClient(FailingCommitClient): + """Fails its first commit (retryable), then commits.""" + + def __init__(self) -> None: + super().__init__(retryable=True) + self.commits = 0 + + def commit(self, *, session_id: str, **kwargs: object) -> CommitResult: + self.commits += 1 + if self.commits == 1: + return super().commit(session_id=session_id, **kwargs) + return FakeClient.commit(self, session_id=session_id, **kwargs) + + +def test_a_retry_after_a_cleanup_uploads_again(data_folder: Path) -> None: + client = FlakyCommitClient() + job = _job(data_folder, client, cleanup_on_failure=True) + with pytest.raises(IngestApiError): + job.run() + assert client.cancelled == ["sess-X"] + + outcome = job.retry() + + assert outcome.commit.status == "committed" + assert len(client.begin_calls) == 2 # the upload was gone with the session + + +def test_a_cleanup_whose_cancel_failed_is_resumed_on_the_server(data_folder: Path) -> None: + class CancelFailsClient(FailingCommitClient): + def __init__(self) -> None: + super().__init__(retryable=True) + self.retried: list[str] = [] + + def cancel(self, session_id: str) -> None: + raise RuntimeError("DDS unreachable") + + def retry(self, session_id: str) -> StatusResult: + self.retried.append(session_id) + return StatusResult.from_json({"session_id": session_id, "status": "done"}) + + client = CancelFailsClient() + job = _job(data_folder, client, cleanup_on_failure=True) + with pytest.raises(IngestApiError): + job.run() + + outcome = job.retry() + + assert outcome.resumed and client.retried == ["sess-X"] + assert len(client.begin_calls) == 1 diff --git a/tests/dds_ingestion/test_ingest_session.py b/tests/dds_ingestion/test_ingest_session.py index ca2b013..0f6dfb5 100644 --- a/tests/dds_ingestion/test_ingest_session.py +++ b/tests/dds_ingestion/test_ingest_session.py @@ -11,6 +11,7 @@ import pytest +from eea_datalakehouse.dds_ingestion.client import IngestApiError from eea_datalakehouse.dds_ingestion.folder import Diagnosis, FolderIngest from eea_datalakehouse.dds_ingestion.models import ( BeginResult, @@ -256,3 +257,34 @@ def test_diagnose_reports_a_missing_folder_and_goes_on( def test_diagnose_with_nothing_queued_says_so(capsys: pytest.CaptureFixture[str]) -> None: IngestSession(client=_StubClient()).diagnose() # type: ignore[arg-type] assert "nothing queued" in capsys.readouterr().out + + +def test_cleanup_on_failure_with_retry_uploads_again(tmp_path: Path) -> None: + class FailsOnceClient(_StubClient): + def __init__(self) -> None: + super().__init__() + self.cancelled: list[str] = [] + + def commit(self, *, session_id: str, **kwargs: Any) -> CommitResult: + if not self.commit_calls: + self.commit_calls.append(session_id) + err = IngestApiError(503, "Dremio is busy", where="POST commit") + err.retryable = True + raise err + return super().commit(session_id=session_id, **kwargs) + + def cancel(self, session_id: str) -> None: + self.cancelled.append(session_id) + + client = FailsOnceClient() + session = IngestSession(client=client) # type: ignore[arg-type] + session.ingest( + _make_folder(tmp_path, "a"), "bio.uploads", data_format="parquet", + show_progress=False, cleanup_on_failure=True, + ) + + report = session.commit(retry=True) + + assert [o.commit.status for o in report.outcomes] == ["done"] + assert client.cancelled == ["sess-1"] # the failed commit left nothing behind + assert client.begin_calls == 2 # and the retry uploaded again diff --git a/tests/notebook/test_magics.py b/tests/notebook/test_magics.py index bc6c87d..e10fca1 100644 --- a/tests/notebook/test_magics.py +++ b/tests/notebook/test_magics.py @@ -407,6 +407,7 @@ def test_ingest_help_lists_methods( assert "retry=False, max_retries=3" in table_html # PY-11: the one-line options show, with their defaults. assert "csv_options=None" in table_html and "upload_mode='auto'" in table_html + assert "cleanup_on_failure=False" in table_html assert "str | None" not in table_html # no Python type-hint syntax leaking into the table assert _magics_instance(ip)._ingest_session is None From 75e141a5a2afe087e8f9f1a3f7517cbd59c68434 Mon Sep 17 00:00:00 2001 From: blikij Date: Mon, 19 Oct 2026 13:38:46 +0200 Subject: [PATCH 17/19] fix(dds_ingestion): refuse CSV that is not UTF-8 before uploading MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DDS accepts csv_options["encoding"] but passes no encoding on to Dremio, which reads CSV as UTF-8, so a Latin-1 or Windows-1252 file loads garbled. Worse, PY-6's inference read its text columns as binary (pyarrow's reading of text that is not UTF-8) and sent VARBINARY columns. The client now refuses, before begin, with a message naming the cause: - a csv_options encoding that is not UTF-8 (utf8, UTF-8, utf_8 are); - while inferring columns, a file that is not valid UTF-8 — a binary column from a CSV is that — named in the error. Only that failure path reads the files again, to name the file. The request body is unchanged: an encoding given is still sent as is. Co-Authored-By: Claude Opus 5.5 --- docs/dds-ingest-robustness-client-plan.md | 10 ++++- docs/python-client-guide.md | 9 ++++- src/eea_datalakehouse/dds_ingestion/folder.py | 31 ++++++++++++++- .../dds_ingestion/formats.py | 33 +++++++++++++++- tests/dds_ingestion/test_folder_formats.py | 38 +++++++++++++++++++ 5 files changed, 117 insertions(+), 4 deletions(-) diff --git a/docs/dds-ingest-robustness-client-plan.md b/docs/dds-ingest-robustness-client-plan.md index 57c2ba2..1176f74 100644 --- a/docs/dds-ingest-robustness-client-plan.md +++ b/docs/dds-ingest-robustness-client-plan.md @@ -188,7 +188,11 @@ missing was the automatic fallback (G10 in `Documentation/data-digestion.md`). > format; columns are inferred only for CSV/JSON into an **editable** table, the one > case where the server builds the table from them; a JSON array cannot be inferred > and says so. Open: the end-to-end editable CSV run against `local-stack` -> (DI-12.15) has not been done from this client. +> (DI-12.15) has not been done from this client. Fixed 2026-10-19: a CSV whose text is not +> UTF-8 was inferred as `VARBINARY` columns (pyarrow reads such text as binary) and +> sent. Since DDS passes no `encoding` on to Dremio, which reads CSV as UTF-8, the +> client now refuses a non-UTF-8 `csv_options` `encoding`, and a file that is not +> UTF-8, before anything is uploaded. ### PY-7 `FolderIngest.diagnose()` — R4 — **DONE** @@ -415,6 +419,10 @@ done). check: after DI-12.22 (R6b). - **Undo for `IngestSession`** once DDS can delete a read-only table (DI-12.21, R6b); its module docstring notes that it is missing. +- **Non-UTF-8 CSV that actually loads.** Today the client refuses it (PY-6 note). + Either DDS honours `csv_options['encoding']` — server work: it accepts the field + and uses it nowhere — or the client converts the files to UTF-8 before `begin`, + which changes their sizes and so has to happen before the file list is sent. ### 5.5 Order diff --git a/docs/python-client-guide.md b/docs/python-client-guide.md index d07be02..67d836c 100644 --- a/docs/python-client-guide.md +++ b/docs/python-client-guide.md @@ -792,7 +792,7 @@ FolderIngest( | `multipart` | `True`/`False` forces the server's multipart decision; `None` leaves it to the server. | | `table_definition` | The table's column types (and, for later servers, partitioning and sorting) — a `TableDefinition` or a dict in the contract's shape. See [Table definitions](#table-definitions-csvjson-options-inferred-columns). | | `cleanup_on_failure` | After a commit failure the server *could* retry, cancel the session — deleting its uploaded files — instead of keeping it for `retry()`. Default `False`: kept. With it, a `retry()` afterwards uploads again under a new session. | -| `csv_options` / `json_options` | Parsing hints sent on begin, each only for its own format. CSV: `delimiter`, `quote`, `header`, `encoding`. JSON: `format`, `"lines"` or `"array"`. Inference uses them too. | +| `csv_options` / `json_options` | Parsing hints sent on begin, each only for its own format. CSV: `delimiter`, `quote`, `header` — DDS also accepts `encoding` but passes it on to nothing, so anything other than UTF-8 is refused (see [CSV must be UTF-8](#table-definitions-csvjson-options-inferred-columns)). JSON: `format`, `"lines"` or `"array"`. Inference uses them too. | | `infer_schema` | For CSV/JSON into an `editable` table without a `table_definition`: infer the columns locally from every file and send them. `False` turns that off. | | `show_progress` | `False` suppresses the progress bar, and the plan, inferred columns and `diagnose()` lines that are otherwise printed. | | `upload_mode` | `"auto"` follows the server; `"direct"` uploads only to S3; `"proxy"` sends every file through DDS. `ValueError` otherwise. See [`upload_mode`](#upload_mode--how-the-bytes-travel). | @@ -911,6 +911,13 @@ JSON *array*, for one — logs a warning and sends nothing; the server then asks for the columns. `infer_schema=False` turns inference off. Parquet needs none: its files carry their own schema. +**CSV must be UTF-8.** Dremio reads CSV as UTF-8 and DDS passes no encoding on +to it, so a Latin-1 or Windows-1252 file would load garbled — and inference alone +would read its text columns as binary. The client refuses such a file before +anything is uploaded: a `csv_options` `encoding` other than UTF-8 (however +spelled: `utf8`, `UTF-8` are fine), or, while inferring columns, a file that is +not valid UTF-8, which the error names. Save the files as UTF-8 first. + **Parquet with columns given.** They are checked against the files' own footers before anything is uploaded, with the server's comparison rules. A mismatch raises `IngestApiError` (422, `error_slug="schema_mismatch"`) in the server's diff --git a/src/eea_datalakehouse/dds_ingestion/folder.py b/src/eea_datalakehouse/dds_ingestion/folder.py index 848818b..a1c482e 100644 --- a/src/eea_datalakehouse/dds_ingestion/folder.py +++ b/src/eea_datalakehouse/dds_ingestion/folder.py @@ -29,7 +29,7 @@ from .client import IngestApiError, IngestClient, UnsupportedByServerError from .credentials import DremioCreds, load_base_url, load_creds -from .formats import FORMATS, compare_columns, local_columns +from .formats import FORMATS, compare_columns, first_non_utf8, is_utf8, local_columns from .models import ( BeginPlan, BeginResult, @@ -361,6 +361,7 @@ def begin(self) -> BeginResult: The server validates the target here — an unauthorised or non-existent catalog folder fails now, before a single byte is uploaded. """ + self._check_csv_encoding_option() files = scan_folder(self.folder, self.data_format) if not files: raise FileNotFoundError( @@ -396,6 +397,7 @@ def dry_run(self) -> DryRunResult: real begin would raise, or :class:`UnsupportedByServerError` against a DDS server that predates dry runs. """ + self._check_csv_encoding_option() files = scan_folder(self.folder, self.data_format) if not files: raise FileNotFoundError( @@ -442,6 +444,13 @@ def _definition_to_send( try: columns = local_columns(self.folder, rel_paths, self.data_format, options) except (ValueError, pa.ArrowException) as exc: + # A file that is not UTF-8 would load garbled, not just untyped: say so. + bad = first_non_utf8(self.folder, rel_paths) if self.data_format == "csv" else None + if bad is not None: + raise self._encoding_error( + f"{bad!r} is not valid UTF-8, and Dremio reads CSV as UTF-8, so its " + "text would load garbled: save it as UTF-8 and run again." + ) from exc logger.warning( "could not infer the %s columns locally (%s); the server will ask for " "them — pass table_definition=…", @@ -456,6 +465,26 @@ def _definition_to_send( print(f"Inferred columns: {summary}") return inferred + def _check_csv_encoding_option(self) -> None: + """Refuse a CSV ``encoding`` other than UTF-8 before anything starts: DDS + accepts the option but passes no encoding on to Dremio, which reads CSV as + UTF-8, so such a file would load garbled rather than fail.""" + encoding = (self.csv_options or {}).get("encoding") + if self.data_format == "csv" and encoding and not is_utf8(str(encoding)): + raise self._encoding_error( + f"csv_options asks for encoding {encoding!r}, but DDS ignores it and " + "Dremio reads CSV as UTF-8, so the text would load garbled: save the " + "files as UTF-8, drop the encoding option, and run again." + ) + + @staticmethod + def _encoding_error(message: str) -> IngestApiError: + err = IngestApiError( + 400, message, where="local CSV encoding check (nothing was uploaded)" + ) + err.retryable = False + return err + def _check_parquet_columns(self, rel_paths: list[str], given: list[Mapping[str, Any]]) -> None: """Refuse given Parquet columns that contradict the local footers, before anything is uploaded — the same check the server runs at commit (DI-12.1d).""" diff --git a/src/eea_datalakehouse/dds_ingestion/formats.py b/src/eea_datalakehouse/dds_ingestion/formats.py index 967666a..71780d3 100644 --- a/src/eea_datalakehouse/dds_ingestion/formats.py +++ b/src/eea_datalakehouse/dds_ingestion/formats.py @@ -15,6 +15,7 @@ from __future__ import annotations +import codecs import re from collections.abc import Callable, Mapping, Sequence from dataclasses import dataclass @@ -97,7 +98,12 @@ def _csv_schema(path: Path, options: Mapping[str, Any] | None) -> Columns: delimiter=opts.get("delimiter") or ",", quote_char=opts.get("quote") or '"' ) read = pa_csv.ReadOptions(autogenerate_column_names=opts.get("header") is False) - return _columns(pa_csv.read_csv(path, read_options=read, parse_options=parse).schema) + schema = pa_csv.read_csv(path, read_options=read, parse_options=parse).schema + # pyarrow reads text that is not valid UTF-8 as binary, and a CSV has no binary + # columns: without this, a Latin-1 file would become VARBINARY columns. + if any(pa.types.is_binary(f.type) or pa.types.is_large_binary(f.type) for f in schema): + raise ValueError(f"{path.name} is not valid UTF-8") + return _columns(schema) def _json_schema(path: Path, options: Mapping[str, Any] | None) -> Columns: @@ -125,6 +131,31 @@ class ClientFormat: } +def is_utf8(encoding: str) -> bool: + """Whether ``encoding`` names UTF-8, however it is spelled (``utf8``, + ``UTF-8``, ``utf_8``); an unknown name is not.""" + try: + return codecs.lookup(encoding).name == "utf-8" + except LookupError: + return False + + +def first_non_utf8(folder: Path, rel_paths: Sequence[str]) -> str | None: + """The first of the files that is not valid UTF-8, read in 1 MiB chunks; + ``None`` when every one is. Dremio reads CSV as UTF-8, and DDS passes no + encoding on to it.""" + for rel in rel_paths: + decoder = codecs.getincrementaldecoder("utf-8")() + with (folder / rel).open("rb") as fh: + try: + while chunk := fh.read(1 << 20): + decoder.decode(chunk) + decoder.decode(b"", final=True) + except UnicodeDecodeError: + return rel + return None + + def local_columns( folder: Path, rel_paths: Sequence[str], fmt: DataFormat, options: Mapping[str, Any] | None ) -> Columns: diff --git a/tests/dds_ingestion/test_folder_formats.py b/tests/dds_ingestion/test_folder_formats.py index e2a654b..6941fb5 100644 --- a/tests/dds_ingestion/test_folder_formats.py +++ b/tests/dds_ingestion/test_folder_formats.py @@ -167,3 +167,41 @@ def test_csv_options_reach_begin_only_for_csv(creds: DremioCreds) -> None: sent = [json.loads(call.request.read()) for call in route.calls] assert sent[0]["csv_options"] == {"delimiter": ";"} assert "csv_options" not in sent[1] + + +# --- the CSV encoding: DDS passes none on, and Dremio reads CSV as UTF-8 ---------- + +LATIN_1_CSV = "navn;by\nJørgen;Århus\n".encode("latin-1") + + +def test_a_non_utf8_encoding_option_is_refused_before_anything_starts(tmp_path: Path) -> None: + (tmp_path / "a.csv").write_bytes(LATIN_1_CSV) + client = FakeClient() + job = _job( + tmp_path, "csv", client, intent="editable", + csv_options={"delimiter": ";", "encoding": "latin-1"}, + ) + with pytest.raises(IngestApiError, match="DDS ignores it") as exc: + job.run() + assert exc.value.retryable is False + assert client.begin_calls == [] + + +@pytest.mark.parametrize("spelling", ["utf8", "UTF-8", "utf_8"]) +def test_utf8_however_spelled_is_fine(tmp_path: Path, spelling: str) -> None: + (tmp_path / "a.csv").write_text("navn;by\nJørgen;Århus\n", encoding="utf-8") + client = FakeClient() + options = {"delimiter": ";", "encoding": spelling} + _job(tmp_path, "csv", client, intent="editable", csv_options=options).run() + assert _sent_columns(client) == [("navn", "VARCHAR"), ("by", "VARCHAR")] + assert client.begin_calls[0]["csv_options"] == options # still sent as given + + +def test_a_csv_that_is_not_utf8_is_named_before_anything_starts(tmp_path: Path) -> None: + (tmp_path / "a.csv").write_text("navn;by\nAnna;Oslo\n", encoding="utf-8") + (tmp_path / "b.csv").write_bytes(LATIN_1_CSV) + client = FakeClient() + job = _job(tmp_path, "csv", client, intent="editable", csv_options={"delimiter": ";"}) + with pytest.raises(IngestApiError, match="'b.csv' is not valid UTF-8"): + job.run() + assert client.begin_calls == [] From 2a9555580398766e095f8a67852d20946b6c240b Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Mon, 19 Oct 2026 12:08:17 +0000 Subject: [PATCH 18/19] Bump version to 0.1.33 [skip ci] --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index e28e181..7219c2c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "EEADataLakehouse" -version = "0.1.32" +version = "0.1.33" description = "Data preparation and validation utilities for the EEA data lakehouse pipeline." readme = "README.md" requires-python = ">=3.11" From a7eaa928e2a8f41bf5503ed22a591873a413cee1 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Mon, 19 Oct 2026 12:08:19 +0000 Subject: [PATCH 19/19] Update README install examples to v0.1.33-staging [skip ci] --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 4f688f1..11810b6 100644 --- a/README.md +++ b/README.md @@ -36,7 +36,7 @@ pip install "git+https://github.com/eeadata/EEALakeHouse.python.git@staging" ``` # staging's latest release (early access) — pin to the tag the "staging" badge above shows -pip install "git+https://github.com/eeadata/EEALakeHouse.python.git@v0.1.32-staging" +pip install "git+https://github.com/eeadata/EEALakeHouse.python.git@v0.1.33-staging" ``` ## Usage @@ -535,7 +535,7 @@ To pin to one specific release instead, use the exact tag the live badges under %pip install "git+https://github.com/eeadata/EEALakeHouse.python.git@v0.1.30" # staging's latest release (early access) -%pip install "git+https://github.com/eeadata/EEALakeHouse.python.git@v0.1.32-staging" +%pip install "git+https://github.com/eeadata/EEALakeHouse.python.git@v0.1.33-staging" ``` Use the `%pip` magic rather than `!pip` — it installs into the kernel the