Skip to content

Spool index backend - #751

Merged
d-chambers merged 97 commits into
devfrom
spool-index-backend
Jul 18, 2026
Merged

Spool index backend#751
d-chambers merged 97 commits into
devfrom
spool-index-backend

Conversation

@d-chambers

@d-chambers d-chambers commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Replaces DASCore's PyTables/HDF5 spool index with a relational SQLite metadata engine and unifies every spool type (in-memory, directory, and their unions) on top of it. Selection, candidate identification, chunk planning, and union now go through one lazy PatchCatalog, so the same pushdown-capable semantics apply whether patches come from files or memory. The old ChunkManager and PyTables index are removed.

Motivation

The old index stored a flat, per-directory PyTables table and only served directory spools; in-memory and union spools each had their own ad-hoc metadata paths, and chunking went through a separate ChunkManager. Behavior diverged across spool types, queries kept everything in memory, and scaling to large archives was awkward. A relational index with a real query planner lets one code path answer counts, selections, and chunk plans lazily and push work down to the database.

What changed

  • Relational backend (io/index/). A normalized seven-table schema (sources, patches, attrs, coordinate definitions, links) with keys, value-kind checks, and cascade foreign keys, validated before any mutation. Coordinate definitions separate an exact fingerprint from a summary-only dedup key. Nullable nanosecond columns are fetched without a float64 round trip (ns epochs exceed 2**53). Ingest normalizes unit-bearing values to canonical base SI, and the query builder compiles the selector spec to SQL (attrs exact, regex as candidate + residual, coordinate predicates candidacy-only with exact re-trim at load).
  • One engine for every spool. PatchCatalog owns the backend, a resolver, and the directory synchronizer; directory and memory spools share it, so lazy selection, pushdown, and chunk planning are identical across types. spool + spool unions catalogs table-to-table. In-memory patches carry an eager instance identity, giving spools set semantics by patch identity.
  • Chunk planning. ChunkManager is replaced by an inspectable ChunkPlan, exposed publicly as Spool.chunk_plan(...) with the same arguments as chunk.
  • Unit-aware selection. Numeric coordinate summaries are stored in canonical SI, so a bare range means SI regardless of native units and the exact per-patch trim defers its representation until each patch is known — correct across mixed unitful/unitless archives. Coordinate selection accepts a (start, stop) range or a boolean mask; scalar/membership selectors are rejected eagerly.
  • Relocations/removals. PyTables dropped from pyproject.toml/environment.yml; the chunk planner, path classifiers, and relative-selection helpers moved to utils/.

Summary by CodeRabbit

  • New Features

    • Added SQLite-backed directory indexing with incremental updates and a catalog-based patch selection workflow.
    • Expanded spool functionality: richer select options (_attrs/_coords, samples, relative), chunk_plan, and chunk(..., group, missing_dim), plus spool-union support (spool + spool).
    • Added config controls for chunk grouping (sampling_group_tolerance, groupby_attrs).
  • Bug Fixes

    • Improved patch identity/serialization behavior and corrected selection/trim exactness, including unit-aware and regex-chaining query handling.
    • Refined chunk/merge validation and edge-case behavior.
  • Documentation

    • Updated spool index/selection/chunking docs and directory indexer tutorial for the SQLite model.
  • Chores

    • Migrated HDF5 handling/tests from PyTables to h5py; updated related configs and fixtures.

Changelog

  • changed breaking: the spool hierarchy collapses to one concrete dc.Spool; MemorySpool, DirectorySpool, FileSpool, the dascore.clients package, and the select_kwargs parameter are removed — use dc.spool(...) and spool.has_live_patches.
  • changed breaking: Spool.update() is allowed only on a root spool; any derived spool (select, slice, sort, chunk, +) raises.
  • changed breaking: Spool.select no longer accepts coordinate boolean masks — use spool.map(lambda p: p.select(dim=mask)); spool[bool_array] still selects membership and samples=True ranges still apply per patch.
  • changed breaking: directory indexes use a seven-table SQLite schema in .dascore_index.sqlite3; the PyTables index is gone, so existing indexes must be deleted and rebuilt.
  • changed breaking: SQLite supports concurrent readers and one serialized writer on local filesystems; network filesystems with unreliable locking are not supported.
  • removed breaking: PyTablesReader, PyTablesWriter, and the HDF5Reader/HDF5Writer aliases from dascore.utils.hdf5; use H5Reader/H5Writer.
  • changed: spools present patches in a defined order — construction order for patch lists, time order for directories, and rows without a value for the ordering key sort last — and equality compares effective contents rather than backing.
  • changed: a + b bakes each operand's pending trims and sort order into the union instead of dropping them, including per-patch time order for interleaved multi-patch files.
  • changed: Spool.sort(...) accepts any coordinate of the spool, not just time/distance, and contiguous descending-coordinate patches now merge under chunk(dim=None).
  • changed: chunking is defined on dimensions only and is unit-aware — compatible spellings such as metres and feet merge, incompatible dimensionalities can never plan together — and chunking a restructured spool along a different dimension plans over its current patches, while re-chunking the same dimension still re-plans from the original members.
  • changed: Spool.select validates unknown names and quantity dimensionality, composes chained regexes with AND, supports explicit _attrs/_coords namespaces, and skips (with a warning) attribute values whose units are dimensionally incompatible across files instead of failing the index update.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@coderabbitai

coderabbitai Bot commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The PR replaces legacy directory indexing with SQLite-backed catalogs, adds lazy spool selection, union, and chunk planning, normalizes patch identities, and migrates HDF5 utilities and tests from PyTables to h5py.

Changes

Indexing, catalogs, and directory spools

Layer / File(s) Summary
SQLite index and directory synchronization
dascore/io/index/*, dascore/io/indexer.py
Adds a versioned SQLite schema, normalized ingestion records, typed queries, transactional source updates, incremental directory scanning, and catalog-based patch resolution.
DirectorySpool integration
dascore/clients/dirspool.py, dascore/core/summary.py, dascore/io/core.py
Routes directory spools through PatchCatalog, normalizes source patch IDs, and resolves loaded patches through catalog resolvers with exact trimming.
Catalog-native spool operations
dascore/core/spool.py, dascore/utils/pd.py
Adds lazy selection, _attrs/_coords namespaces, relative and sample-local selection, catalog unions, patch identity semantics, and materialized-spool transitions.
Metadata-only chunk planning
dascore/utils/chunk_plan.py, dascore/utils/chunk.py, dascore/utils/coordmanager.py, dascore/utils/patch.py, dascore/config.py
Replaces ChunkManager with deterministic ChunkPlan generation, grouping and tolerance controls, missing-dimension policies, conflict handling, and updated coordinate merge assembly.
HDF5 migration and supporting updates
dascore/utils/hdf5.py, dascore/io/h5simple/utils.py, environment.yml, pyproject.toml
Replaces PyTables-oriented HDF5 handling and dependencies with managed h5py resources, including local and remote read/write paths.
Tests and documentation
tests/**, docs/**, scripts/_templates/_quarto.yml
Adds coverage and documentation for indexing, selection, chunk planning, unions, identity, incremental updates, and the h5py migration.

Possibly related PRs

  • DASDAE/dascore#532 — overlaps in coordinate-merge conflict handling and merge-kwargs routing.
  • DASDAE/dascore#584 — overlaps in spool iteration behavior for differing coordinate ranges.
  • DASDAE/dascore#743 — overlaps in DirectorySpool patch-loading and resolver behavior.

Suggested labels: spool, IO

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title is related to the main change, but it is broad and generic rather than specific. Use a more specific title such as "Replace PyTables spool index with SQLite PatchCatalog backend".
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The description covers the problem, motivation, major changes, and removals, so it is mostly complete.
Docstring Coverage ✅ Passed Docstring coverage is 91.35% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch spool-index-backend

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot added IO Work for reading/writing different formats spool related to Spool class labels Jul 11, 2026
@codecov

codecov Bot commented Jul 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (a79e8e5) to head (e9a873c).

Additional details and impacted files
@@             Coverage Diff             @@
##               dev      #751     +/-   ##
===========================================
  Coverage   100.00%   100.00%             
===========================================
  Files          152       161      +9     
  Lines        14929     16619   +1690     
===========================================
+ Hits         14929     16619   +1690     
Flag Coverage Δ
network 49.66% <41.93%> (-1.97%) ⬇️
unittests 99.98% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

🧹 Nitpick comments (5)
dascore/io/index/query.py (1)

216-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add strict=True to zip() for safety.

If rows["value_kind"] and rows["column_name"] differ in length, zip() silently truncates. Using strict=True catches this at runtime.

♻️ Proposed fix
-    columns = dict(zip(rows["value_kind"], rows["column_name"]))
+    columns = dict(zip(rows["value_kind"], rows["column_name"], strict=True))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@dascore/io/index/query.py` at line 216, Update the zip call that builds
columns in the query logic to pass strict=True, ensuring mismatched value_kind
and column_name lengths raise an error instead of being silently truncated.

Source: Linters/SAST tools

dascore/core/patch.py (1)

103-105: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Minor: declare _instance_id alongside other private fields.

The other private/public fields (_data, data, coords, etc.) are declared as class-level annotations; _instance_id isn't, which makes it a bit harder to discover as part of the object's persistent state.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@dascore/core/patch.py` around lines 103 - 105, Declare the _instance_id field
as a class-level annotation alongside the existing _data, data, and coords
fields, while retaining its eager uuid4().hex assignment in the initializer.
dascore/io/index/ingest.py (2)

233-239: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add stacklevel to the reserved-attr-name warning.

Without stacklevel=2, the warning's reported source location points into ingest.py instead of the caller's ingest call, making it harder to trace which write triggered it.

♻️ Suggested fix
-            warnings.warn(msg, UserWarning)
+            warnings.warn(msg, UserWarning, stacklevel=2)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@dascore/io/index/ingest.py` around lines 233 - 239, Update the warnings.warn
call in the reserved-attribute handling block to pass stacklevel=2, so the
warning points to the caller of the ingest operation rather than ingest.py.

Source: Linters/SAST tools


211-216: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Bare except Exception: pass swallows all errors in the datetime fallback.

This is presumably intentional (classify-or-skip for unclassifiable attrs), but catching blind Exception can mask real bugs (e.g., a broken to_datetime64 call) instead of just skipping non-datetime-like values.

♻️ Suggested narrowing
     try:
         return TypedValue("time", to_int(to_datetime64(value)))
-    except Exception:
+    except (TypeError, ValueError, OverflowError):
         pass
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@dascore/io/index/ingest.py` around lines 211 - 216, Narrow the exception
handling around the datetime fallback in the ingest classification flow to catch
only the expected conversion/parsing errors from to_datetime64 and to_int.
Preserve returning the time TypedValue for valid datetime-like inputs and
returning None for unsupported values, while allowing unexpected programming or
runtime errors to propagate.

Source: Linters/SAST tools

dascore/io/index/indexer.py (1)

32-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider sharing the hidden-columns constant with catalog.py.

_SPOOL_HIDDEN_COLUMNS here duplicates the inline list in catalog.py's to_df() (["n_dims", "sample_count_total", "shape"]). Extracting a single shared constant would prevent silent drift if one site changes without the other.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@dascore/io/index/indexer.py` around lines 32 - 35, Share the hidden-column
definition between the indexer and catalog code instead of maintaining duplicate
values. Extract or reuse a single module-level constant for "n_dims",
"sample_count_total", and "shape", update _SPOOL_HIDDEN_COLUMNS and catalog.py's
to_df() to reference it, and preserve the existing filtering behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@dascore/core/spool.py`:
- Around line 807-834: Update _as_catalog_member and _chunk_working_df to
preserve DirectorySpool constructor select_kwargs: retain the selection in the
catalog view or apply the equivalent filter before returning catalog membership
and before constructing chunk-planner input. Ensure catalog-native unions and
chunking cannot reintroduce rows excluded by the constructor selection, while
preserving existing behavior for unrestricted spools.
- Around line 1330-1342: Update __rich__ to guard against a missing dataframe
before calling len(df) or accessing df.columns. Preserve the existing time-span
rendering for non-empty dataframes containing time_min, while allowing
MemorySpool() with _df set to None to return the base representation without
error.

In `@dascore/io/index/indexer.py`:
- Around line 92-125: Update _find_index_path so the explicit index_path branch
returns the same absolute Path value recorded in the index map, rather than the
original potentially relative Path(index_path). Preserve the existing map update
behavior and all other path-selection branches.

In `@dascore/utils/patch.py`:
- Around line 630-634: Update the field construction near coord_fields in the
patch formatting logic to flatten all zipped min/max coordinate pairs into a
single list before concatenating with attrs. Replace the invalid multi-argument
list conversion while preserving the existing field order and reindex behavior
for single- and multi-coordinate inputs.

In `@dascore/utils/paths.py`:
- Around line 33-44: The directory_writable() helper should return False for any
filesystem OSError, including read-only mount errors. Move the
probe.parent.mkdir, file creation, and cleanup operation inside the try block,
catch OSError, and preserve the existing successful cleanup and True return
behavior.

In `@dascore/utils/pd.py`:
- Around line 57-59: Update the validation guard in the relative-select logic to
require both envelope columns, lo_col and the corresponding upper-bound column,
before either is accessed. Preserve the existing InvalidSpoolQueryError path for
missing columns or empty data so incomplete metadata cannot reach the query and
raise KeyError.

In `@tests/test_core/test_patch_chunk.py`:
- Around line 309-316: Strengthen test_merge_unequal_other by comparing the
sorted distance_min/distance_max envelopes of distance_adjacent with those in
out, in addition to the existing length assertion. Verify that each unequal
coordinate is preserved exactly and outputs are not duplicated or altered.

In `@tests/test_core/test_spool.py`:
- Around line 968-974: Update test_repr_without_time_coordinate to match
MemorySpool.__rich__: since the patch has no time_min coordinate, assert that
the rendered output does not contain “Time Span,” unless the renderer is
explicitly changed to add a no-time placeholder.

---

Nitpick comments:
In `@dascore/core/patch.py`:
- Around line 103-105: Declare the _instance_id field as a class-level
annotation alongside the existing _data, data, and coords fields, while
retaining its eager uuid4().hex assignment in the initializer.

In `@dascore/io/index/indexer.py`:
- Around line 32-35: Share the hidden-column definition between the indexer and
catalog code instead of maintaining duplicate values. Extract or reuse a single
module-level constant for "n_dims", "sample_count_total", and "shape", update
_SPOOL_HIDDEN_COLUMNS and catalog.py's to_df() to reference it, and preserve the
existing filtering behavior.

In `@dascore/io/index/ingest.py`:
- Around line 233-239: Update the warnings.warn call in the reserved-attribute
handling block to pass stacklevel=2, so the warning points to the caller of the
ingest operation rather than ingest.py.
- Around line 211-216: Narrow the exception handling around the datetime
fallback in the ingest classification flow to catch only the expected
conversion/parsing errors from to_datetime64 and to_int. Preserve returning the
time TypedValue for valid datetime-like inputs and returning None for
unsupported values, while allowing unexpected programming or runtime errors to
propagate.

In `@dascore/io/index/query.py`:
- Line 216: Update the zip call that builds columns in the query logic to pass
strict=True, ensuring mismatched value_kind and column_name lengths raise an
error instead of being silently truncated.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d2796497-c7c1-41a9-97e7-6597244d2f78

📥 Commits

Reviewing files that changed from the base of the PR and between 1a200d8 and 8590ee4.

📒 Files selected for processing (62)
  • dascore/clients/dirspool.py
  • dascore/clients/filespool.py
  • dascore/config.py
  • dascore/core/coords.py
  • dascore/core/patch.py
  • dascore/core/spool.py
  • dascore/core/summary.py
  • dascore/exceptions.py
  • dascore/io/core.py
  • dascore/io/index/__init__.py
  • dascore/io/index/backend.py
  • dascore/io/index/catalog.py
  • dascore/io/index/dialect.py
  • dascore/io/index/indexer.py
  • dascore/io/index/ingest.py
  • dascore/io/index/lite.py
  • dascore/io/index/query.py
  • dascore/io/index/schema.py
  • dascore/io/indexer.py
  • dascore/utils/chunk.py
  • dascore/utils/chunk_plan.py
  • dascore/utils/coordmanager.py
  • dascore/utils/hdf5.py
  • dascore/utils/patch.py
  • dascore/utils/paths.py
  • dascore/utils/pd.py
  • docs/changelog.qmd
  • docs/contributing/new_format.qmd
  • docs/notes/notes.qmd
  • docs/notes/spool_chunking.qmd
  • docs/notes/spool_index.qmd
  • docs/notes/spool_selection.qmd
  • docs/tutorial/file_io.qmd
  • environment.yml
  • pyproject.toml
  • scripts/_templates/_quarto.yml
  • tests/conftest.py
  • tests/test_clients/test_dirspool.py
  • tests/test_core/test_patch_chunk.py
  • tests/test_core/test_spool.py
  • tests/test_core/test_spool_select_spec.py
  • tests/test_io/test_dasdae/test_dasdae.py
  • tests/test_io/test_febus/test_febusg1.py
  • tests/test_io/test_h5simple/test_h5simple.py
  • tests/test_io/test_index/test_catalog.py
  • tests/test_io/test_index/test_db_dirspool.py
  • tests/test_io/test_index/test_heterogeneity_stress.py
  • tests/test_io/test_index/test_index_contract.py
  • tests/test_io/test_index/test_index_edge_cases.py
  • tests/test_io/test_index/test_plan.py
  • tests/test_io/test_index/test_schema.py
  • tests/test_io/test_index/test_union.py
  • tests/test_io/test_indexer.py
  • tests/test_io/test_io_core.py
  • tests/test_io/test_pickle/test_pickle.py
  • tests/test_io/test_prodml/test_prod_ml.py
  • tests/test_io/test_terra15/test_terra15.py
  • tests/test_utils/test_chunk.py
  • tests/test_utils/test_config.py
  • tests/test_utils/test_hdf_utils.py
  • tests/test_utils/test_io_utils.py
  • tests/test_utils/test_patch_utils.py
💤 Files with no reviewable changes (3)
  • tests/test_io/test_dasdae/test_dasdae.py
  • environment.yml
  • dascore/clients/filespool.py

Comment thread dascore/core/spool.py Outdated
Comment thread dascore/core/spool.py
Comment thread dascore/io/index/indexer.py
Comment thread dascore/utils/patch.py
Comment thread dascore/utils/pd.py Outdated
Comment thread tests/test_core/test_patch_chunk.py
Comment thread tests/test_core/test_spool.py Outdated
@coderabbitai coderabbitai Bot added documentation Improvements or additions to documentation and removed IO Work for reading/writing different formats labels Jul 11, 2026
@github-actions

github-actions Bot commented Jul 11, 2026

Copy link
Copy Markdown

✅ Documentation built:
👉 Download
Note: You must be logged in to github and a DASDAE member to access the link.

d-chambers added a commit that referenced this pull request Jul 12, 2026
CodeRabbit findings on #751, verified and fixed:

- Union of a directory spool built with constructor select_kwargs
  reintroduced the excluded rows (_as_catalog_member treated any
  catalog-native spool as the whole catalog). Carry the restricted
  patch ids when select_kwargs are set.
- A bare MemorySpool() has no dataframe, so len(), iteration, and repr
  raised TypeError. Treat a missing frame as an empty spool.
- get_patch_names crashed for multi-coordinate naming
  (list(*coord_fields) unpacks multiple pairs); flatten the min/max
  fields explicitly.
- directory_writable let a read-only mount's OSError (e.g. EROFS from
  mkdir) escape instead of returning False; guard the whole probe.
- relative_ranges_to_absolute read {name}_max without checking it,
  raising KeyError instead of the documented InvalidSpoolQueryError.
- _find_index_path returned the un-absolutized index_path while
  recording the absolute form in the index map; return the absolute
  path.

Regression tests added for each; touched modules stay at 100% coverage.
@coderabbitai coderabbitai Bot added IO Work for reading/writing different formats patch related to Patch class and removed documentation Improvements or additions to documentation labels Jul 12, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@dascore/utils/paths.py`:
- Around line 37-45: Wrap the cleanup call in the writable-directory probe’s
existing OSError handling so transient failures from os.remove(probe) return
False rather than escaping. Update the probe flow around probe.parent.mkdir,
open, and os.remove while preserving the existing True result only when creation
and cleanup both succeed.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3a6e4c6e-0fb8-42dd-b3db-9153ff4bd08f

📥 Commits

Reviewing files that changed from the base of the PR and between 2cd1db4 and c3e0b87.

📒 Files selected for processing (15)
  • dascore/core/spool.py
  • dascore/io/index/indexer.py
  • dascore/io/index/ingest.py
  • dascore/utils/patch.py
  • dascore/utils/paths.py
  • dascore/utils/pd.py
  • tests/conftest.py
  • tests/test_clients/test_dirspool.py
  • tests/test_core/test_spool.py
  • tests/test_io/test_index/test_heterogeneity_stress.py
  • tests/test_io/test_index/test_index_contract.py
  • tests/test_io/test_index/test_union.py
  • tests/test_utils/test_patch_utils.py
  • tests/test_utils/test_paths.py
  • tests/test_utils/test_pd.py
🚧 Files skipped from review as they are similar to previous changes (11)
  • tests/test_utils/test_patch_utils.py
  • tests/test_io/test_index/test_index_contract.py
  • dascore/utils/pd.py
  • dascore/io/index/indexer.py
  • tests/test_core/test_spool.py
  • tests/test_io/test_index/test_union.py
  • tests/test_io/test_index/test_heterogeneity_stress.py
  • dascore/utils/patch.py
  • tests/test_clients/test_dirspool.py
  • dascore/io/index/ingest.py
  • dascore/core/spool.py

Comment thread dascore/utils/paths.py
@coderabbitai coderabbitai Bot added documentation Improvements or additions to documentation and removed patch related to Patch class labels Jul 12, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
dascore/utils/paths.py (1)

36-42: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Use a unique, exclusive temporary probe file.

The predictable sentinel can clobber an existing file, and open(..., "w") follows an attacker-created symlink in a writable directory. Use tempfile.mkstemp(dir=probe.parent, prefix=...) (with O_EXCL semantics) and retain the existing suppressed cleanup.

🛡️ Proposed fix
+import tempfile
+
 def directory_writable(path) -> bool:
     ...
-    name = "._dascore_write_test_delete_me"
-    probe = Path(path) / name
+    probe = None
     try:
         probe.parent.mkdir(exist_ok=True, parents=True)
-        open(probe, "w").close()
+        fd, probe_name = tempfile.mkstemp(
+            dir=probe.parent,
+            prefix="._dascore_write_test_",
+        )
+        os.close(fd)
+        probe = Path(probe_name)
     except OSError:
         return False
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@dascore/utils/paths.py` around lines 36 - 42, Replace the predictable probe
path and open call in the path-writability check with
tempfile.mkstemp(dir=probe.parent, prefix=...), retaining the existing
suppressed cleanup and closing the returned file descriptor. Preserve the
surrounding mkdir and exception handling while ensuring the probe is uniquely
created with exclusive semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/test_core/test_patch_chunk.py`:
- Around line 318-323: Update the _distance_envelopes helper’s zip call to pass
an explicit strict argument, using strict=True to preserve the expected
equal-length behavior while satisfying Ruff B905.

---

Outside diff comments:
In `@dascore/utils/paths.py`:
- Around line 36-42: Replace the predictable probe path and open call in the
path-writability check with tempfile.mkstemp(dir=probe.parent, prefix=...),
retaining the existing suppressed cleanup and closing the returned file
descriptor. Preserve the surrounding mkdir and exception handling while ensuring
the probe is uniquely created with exclusive semantics.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e7c866a4-437f-4b87-ad4d-ab0d9c470bfa

📥 Commits

Reviewing files that changed from the base of the PR and between c3e0b87 and 36e639d.

📒 Files selected for processing (4)
  • dascore/core/spool.py
  • dascore/utils/paths.py
  • tests/test_core/test_patch_chunk.py
  • tests/test_core/test_spool.py
🚧 Files skipped from review as they are similar to previous changes (2)
  • tests/test_core/test_spool.py
  • dascore/core/spool.py

Comment thread tests/test_core/test_patch_chunk.py
Six-table summary index: sources, patches (frozen structural columns plus
time/distance envelopes), attrs (lazily-added typed columns, one per
attr/kind), coords (tall, typed min/max/step), attr_meta, meta_data.

- DuckDB, SQLite (STRICT), and Parquet-manifest backends behind one
  abstract interface; shared SQL generation with a small dialect layer.
- Ingest from PatchSummary with pint base-SI unit normalization and
  sanitized dynamic column names.
- Query layer implements the selector semantics spec: attrs-first name
  resolution, kind dispatch, envelope candidacy with no false negatives,
  regex as pandas residual.
- Contract test suite runs identically against all three backends
  (36 tests x 3).
- Secondary indexes (SQLite EXISTS otherwise quadratic) and dataframe
  bulk ingest for DuckDB (executemany binds row-at-a-time).
- DBDirectoryIndexer: directory walk via _iter_filesystem, per-source
  (mtime_ns, size_bytes) change detection instead of a global watermark,
  stale-source removal folded into update(), scan of changed files only.
- DirectorySpool gains an index_engine parameter selecting the duckdb,
  sqlite, or parquet backend; default behavior unchanged (HDF5 index).
- Derived spools share the indexer connection (deepcopy-safe), matching
  the single-writer model.
- Integration tests: patch loading, time select, chunk merge, and
  add/modify/delete lifecycle against real files for all backends.
SQLite caps bound variables (32766 by default), so replacing or deleting
many sources in one call overflowed the parameter list at ~500k sources.
Chunk both the path lookup and the id deletions. Also keep a failed
rollback from masking the original write error.
Stress test: 300 summaries with randomized dims (1-3 of 10 names incl.
a hostile one), coord dtypes (datetime/timedelta/float/int/str), units,
and attrs with mixed kinds per name and names that collide after
sanitization. Verifies ingest completeness and the no-false-negative
query contract against references computed from the raw summaries.

Fixes:
- attr names that sanitize to the same identifier ("Shot Number" vs
  "shot_number") collided on the attrs column; attr_meta is now the
  single source of truth for column names with deterministic numeric
  suffixes on collision.
- multi-kind attrs coalesce in object space; pandas nullable
  BooleanArray refuses cross-dtype fills in Series.where.
The database index replaces the HDF5/PyTables index entirely:

- Delete HDFPatchIndexManager, _HDF5Store, open_hdf5_file, the kernel
  query, and the PyTables reader/writer wrappers; H5Reader stands alone.
  io/indexer.py keeps AbstractIndexer and the index-location map helpers.
- Drop tables>=3.7 from dependencies (it was only used by the index; all
  FiberIO readers use h5py) and remove the pytables warning filters. Add
  duckdb as an optional extra (needed for the duckdb/parquet engines;
  sqlite default has no extra dependency).
- DirectorySpool now defaults to index_engine="sqlite".
  DBDirectoryIndexer gains: read-only-archive index relocation
  (config-backed, engine-keyed map), update(paths=...), one automatic
  update on first query of a brand-new index, and directory-format scan
  units - a directory FiberIO (e.g. XMLBinary) indexes as one source
  keyed by the directory with aggregate stats (max member mtime, summed
  size), mirroring dc.scan's skip protocol. Every visited path gets a
  sources row even when it yields no patches, so non-fiber files don't
  force perpetual rescans; the root-as-source path is spelled ".".
- Ingest hardening: container/array values can no longer slip through
  the datetime fallback into the index.
- Tests: port conftest and io tests from tables to h5py (pytables
  duck-typing tests use importorskip); rewrite indexer tests for
  DBDirectoryIndexer; add an edge-case suite bringing dascore/io/index
  to 100% line coverage. The #583 skip-warning test now asserts the
  fixed behavior (index-level distance selection) and the febus chunk
  test passes conflict="keep_first", matching in-memory spool semantics
  that the old index masked by dropping non-schema attrs.
Split the coords table into coord_defs (one row per unique coordinate
summary) and patch_coords (patch -> name/dims -> def links). The def
key is the CoordSummary fingerprint when the scan provides one (exact
value identity, truncated to 128 bits) or a hash of the stored summary
fields otherwise (lossless for the index, too weak for value-identity
claims). Name and dims stay on the link since two patches can share
values under different names.

Defs are upserted with batched key lookups, reused across writes, and
left orphaned on source deletion (a rebuild compacts them). Coord
predicates join patch_coords x coord_defs; query timings are unchanged.

This is groundwork, not a storage win: time coords are unique per file
so their defs don't dedup (index grows ~40% on time-indexed archives),
but shared coords collapse to single rows, chunk/merge can later
recognize shared coordinates by coord_def_id equality instead of value
comparison, and coord_defs is the natural home for exact coord arrays
if full-coords storage lands.
The flat relation now carries {name}_min/{name}_max/{name}_step for
every coord in the result beyond the time/distance envelopes cached on
patches, restoring parity with memory-spool dataframes (chunking on any
dim now works from a directory spool). Every coord also gets a private
_{name}_def_key column: the globally-stable coordinate identity that
future chunk/merge grouping keys on (underscore-prefixed so it does not
yet participate in merge-compatibility comparisons).
Groundwork for merged/universal spools: base_uri is stored as "" (never
NULL) so plain equality works on every engine; source replacement and
deletion are scoped by base_uri; identical relative paths under
different bases coexist. The flat relation prefixes base_uri onto paths
only when non-empty.
The catalog owns the index tables (via any backend) and composed
selection state; resolvers turn flat-relation rows into patches
(FileResolver through dc.read with trim hints -- remoteness belongs to
the path layer; LiveResolver from an in-memory registry with synthetic
memory:// source identities); the directory indexer plugs in as the
syncer for directory-backed catalogs.

Laziness: from_patches does no metadata work until the first metadata
operation (backend bootstrap costs ~10s of ms; holding a patch list is
free). select composes Query predicates with eager name validation and
no SQL; realization runs one query per view. samples selections are
patch-local; relative bounds resolve against the view envelope; coord
range predicates re-apply exactly at patch load (two-stage select).
Mutation is root-only; views share backend and resolver.

Also: build_query_sql accepts AND-composed query sequences.
Patch-list memory spools now build their managing dataframes from the
catalog's flat relation and resolve patches through the shared
LiveResolver, completing stage 1: one metadata engine for every spool
type. Spools created from other spools/dataframes keep the legacy
flat-dump path. Derived spools share the catalog (deepcopy-safe);
catalogs pickle by rebuilding their backend from registered patches.

Correctness fixes surfaced by the rewire:
- ns-epoch integers never pass through float64 (which corrupts them by
  ~100 ns and breaks merge boundary arithmetic): exact masked
  conversion in _flatten, and duckdb/parquet fetch through arrow with
  integer_object_nulls (df() floats nullable BIGINTs).
- numeric envelope columns coerce object-None to float NaN so sorting
  and chunking work on coords without steps.
- all-relative time results serve timedelta envelopes so chunking on
  relative time keeps working (#553).
- MemorySpool drops synthetic identity columns (path, file_format,
  file_version, source_patch_id) before chunk merge-compat comparisons,
  as DirectorySpool always has.
- get_patch_names ignores memory:// synthetic paths and renders absent
  name-field columns as empty, so generated names (and DASDAE group
  names) are identical whichever metadata engine produced the frame.
DataFrameSpool.select now implements the selector semantics spec at the
user-facing surface, for memory and directory spools alike:

- Unknown names raise InvalidSpoolQueryError with the valid attribute
  and coordinate names (closes #435). Bare names resolve attrs-first,
  then coords.
- _attrs / _coords dict kwargs disambiguate explicitly and validate
  against their own namespace only.
- samples=True selections are coordinate-only and patch-local: they
  never exclude patches; the selection is recorded and applied to each
  patch as it loads, surviving chunk and other derived spools
  (closes the second failure mode of #447).
- relative=True resolves range bounds against the spool's coordinate
  envelope, mirroring Patch.select semantics at spool scope
  (closes #362).

InvalidSpoolQueryError moves to dascore.exceptions (avoiding a circular
import); the index query module re-imports it from there.
One relative-bound resolver (query.relative_offset) serves catalog and
spool selects; ingest uses dataclasses.replace; select drops its
split_df_query call, which strict name validation made dead (every
validated name matches a dataframe column or coord range, so the extra
kwargs were always empty).
patch.summary is a cached_property; building fresh PatchSummary objects
in _live_records discarded fingerprints and summaries the patch already
had. Reusing them makes catalog ingest of previously-summarized patches
~3x faster (first get_contents at 300 patches: 208 -> 75 ms) and turns
the remaining summary cost into once-per-patch-lifetime instead of
once-per-spool.
…e removed

The PatchAssembler loses its orphaned indexing front end and post-
select plumbing (the plan resolver only consumes the merge internals),
get_column_names_from_dim loses its last caller, and the planned-
catalog converters simplify to the pandas scalar forms that actually
reach them. New tests pin the operation-order compositions the review
demanded: collapse with value and quantity residuals, regex+window+
attr-sort chains, attr membership arrays, envelope-column sort names,
membership-restricted pickling of union views, third-party BaseSpool
members, negative samples windows, and complete-envelope-overlap
merges. Touched modules are back to 100% coverage locally.
…eep every coordinate

Combining spools now preserves each operand's current contents: __add__
auto-materializes operands carrying union-lossy lazy state (coordinate
and samples residual trims, sort specs) into identity-plan derived
catalogs — table work only, no patch loads — while membership-style
state still unions by rows so identity dedup keeps working. Derived
catalogs record every coordinate of their members (numeric, time, and
string), aggregated from the member source rows, with def-key identity
kept only when values provably survive assembly; identity claims are
likewise dropped for any coordinate riding a residual-trimmed dim.
Sorting accepts any known coordinate (non-hot names order by their
coord_defs envelope minimum through a correlated subquery), samples
envelopes use the last included index and respect orientation, sampling
groups compare magnitudes against a stable anchor (descending
contiguous patches now merge; tolerance chains can no longer drift),
concatenating an empty spool returns an empty spool, and the removed
PyTables reader/writer aliases are documented in the changelog. The
_attrs/_coords namespaces additionally accept a name or collection of
names tagging bare kwargs.
…ings at merge

The planner grouped patches by raw SI envelope magnitudes with no
knowledge of the chunked dimension's units, so a metre patch and a
seconds patch with contiguous magnitudes planned into one output whose
assembly failed only at patch access. The canonical (base) unit now
rides the flat relation as a private per-coord column and joins the
sampling partition: incompatible dimensionality — and unitful next to
unitless, which assembly refuses too — can never share an output.
Compatible spellings of one dimensionality (metres with feet) still
plan together and now genuinely merge: assembly converts each member's
merge-dim units to the first member's, and the raw-concatenation merge
fallback reattaches the verified common unit it previously dropped.
Derived catalogs carry the canonical units on their coordinate
definitions, so unit metadata survives restructuring.
…ectories, whole transactions

Chunking a restructured spool along a different dimension now plans
over the spool's current output rows (loaded through the plan
resolver), so it keeps the boundaries the earlier operation assembled;
re-chunking the same dimension still collapses to the trimmed members.
Directory catalogs carry a per-patch default presentation order (ORDER
BY time with the ordinal/patch-id tiebreak) because source-grain
ordinals cannot interleave a multi-patch file that straddles a patch of
another file; the default order is a catalog contract, not view state,
so roots still update. The membership-restricted resolver keeps the
plan routes its rows reference (mixed planned/live views survive
pickling and process-backed map), the segmented-coordinate write guard
keys on what a spool resolves rather than where its members live (plan-
assembled file-backed spools are guarded; purely file-backed spools
still skip inspection), the SQLite statement lock now spans whole
transactions so shared-connection readers can never observe a
half-applied source replacement, and only mark_initial_update_done —
after renumbering succeeds — sets the initial-update marker, keeping
the reopen recovery path alive when a sync dies mid-way.
A patch carrying the chunk name solely as a non-dimensional coordinate
cannot be trimmed or merged along it, but envelope presence made the
planner treat it as chunkable: merge outputs failed with CoordError
only at patch access, and segmenting happened to slice the riding
dimension for numeric coordinates while producing wrong plans for
datetime ones. Such patches now fall under missing_dim with the rest of
the dimension-less rows — the default raise names how many ride the
name as a coordinate, and missing_dim='drop' excludes them.
The per-patch directory order lived only in the catalog's default-order
spec, so combining a directory spool fell back to source-record
transfer and re-lost the interleaved presentation the order exists to
provide — even against an empty spool, breaking order-sensitive
equality with itself. The union handoff now bakes the effective order
into an identity plan, but only when record-grain transfer would
actually present rows differently, so ordinary archives keep record
transfer and same-source deduplication. Ordering also treats missing
values consistently: rows without a value for the order key sort last
under any direction, matching the ordinal renumberer's
missing-time-last rule instead of SQLite's NULLs-first default.
@d-chambers
d-chambers force-pushed the spool-index-backend branch from 812a6c4 to 5ca467d Compare July 18, 2026 12:23
Spool equality compared raw rows plus residual state, so a trimmed view
never equaled its union-materialized twin despite identical contents,
and the view's rows still carried the untrimmed coordinate def keys — an
identity a view cannot restate honestly without loading. Equality now
folds samples residuals into the compared envelopes (value residuals
already present in the realized rows), keeps presented-but-empty rows,
and drops def keys and private bookkeeping from the comparison; data
values were never compared here anyway. The planner's samples
adjustment also resolves negative indices per patch from the
envelope-derived sample count (unknown counts keep the candidacy
envelope), so chunking a tail selection reports the envelopes the
loaded patches actually have.
The ordering section still described source-ordinal renumbering as the
whole directory contract; it now documents the per-patch time
presentation (missing-time last, ordinal/patch-id tiebreaks) with
renumbering demoted to the stability/dedup grain. Federation documents
the row-vs-baked transfer split, plan routing, and effective-contents
equality; the flat relation lists the canonical-units column; the
chunking note covers one-dim-per-call chaining, unit and orientation
partitioning, and non-dim coordinates counting as missing — each new
claim with an executable cell. The spool tutorial loses the stale
warning that some spool types lack concatenate.
@d-chambers
d-chambers merged commit 2aa3c00 into dev Jul 18, 2026
26 checks passed
@d-chambers
d-chambers deleted the spool-index-backend branch July 18, 2026 13:46
@coderabbitai coderabbitai Bot mentioned this pull request Aug 1, 2026
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

IO Work for reading/writing different formats spool related to Spool class

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant