Replace pure-Python implementation with Rust (libpgdump) via PyO3 - #18
Merged
Merged
Conversation
Replace the pure-Python binary format parser with PyO3 bindings to the libpgdump Rust crate, gaining multi-format support (Custom, Directory, Tar), additional compression algorithms (LZ4, Zstd), and improved performance. - Add Cargo.toml and src/ with PyO3 bindings (PyDump, PyEntry, error mapping) - Switch build backend from hatchling to maturin - Rewrite pgdumplib/dump.py as thin wrapper over the native module - Remove toposort dependency (entry ordering handled by libpgdump) - Remove dead constants (BLK_DATA, BLK_BLOBS, FORMAT_*, ZLIB_*, VERSION) - Update SUPPORTED_COMPRESSION_ALGORITHMS to include LZ4 and Zstd - Add set_format() and set_compression() methods to Dump - Update CI to install Rust toolchain and build with maturin - Update deploy workflow to build platform-specific wheels via maturin-action - Remove edge case tests that tested old pure-Python binary parser internals - Bump version to 5.0.0 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughReplaces Python-only dump parsing with a Rust PyO3 extension module ( Changes
Sequence DiagramsequenceDiagram
autonumber
participant Client as Python client
participant Wrapper as pgdumplib.Dump (Python)
participant PyO3 as pgdumplib._pgdumplib (PyO3)
participant Core as libpgdump (Rust core)
Client->>Wrapper: Dump.load(filepath)
Wrapper->>PyO3: Dump.load(path)
PyO3->>Core: parse/load archive
Core-->>PyO3: entries, metadata, data
PyO3-->>Wrapper: Dump handle (methods/metadata)
Client->>Wrapper: table_data_writer(...) / write bytes
Wrapper->>PyO3: add_entry(...) / set_entry_data(...)
PyO3->>Core: mutate entry data
Core-->>PyO3: ack
PyO3-->>Wrapper: ok
Client->>Wrapper: save(path)
Wrapper->>PyO3: save(path)
PyO3->>Core: serialize to archive
Core-->>PyO3: ok
PyO3-->>Wrapper: ok
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
The bootstrap script was skipping virtualenv creation when CI=true, but maturin develop requires a virtualenv. Remove the CI guard so the venv is always created. Also remove the redundant "Build native extension" step from the workflow since bootstrap already runs maturin develop, fix coverage.xml path to match pyproject.toml config, and use .venv/bin/ prefixed commands for lint and test steps. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CLAUDE.md (1)
13-14:⚠️ Potential issue | 🟡 MinorDocumentation mentions toposort but dependency was removed.
The PR summary indicates that the
toposortdependency was removed, but Line 13 still states "Manage dump entries with proper dependency resolution using topological sorting." This should be updated to reflect that dependency resolution is now handled by the Rustlibpgdumplayer.📝 Suggested fix
-- Manage dump entries with proper dependency resolution using topological sorting +- Manage dump entries with proper dependency resolution🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` around lines 13 - 14, Update the CLAUDE.md sentence that currently reads "Manage dump entries with proper dependency resolution using topological sorting" to remove the reference to the removed toposort dependency and state that dependency resolution is now performed by the Rust libpgdump layer; locate the exact sentence in CLAUDE.md (the string "Manage dump entries with proper dependency resolution using topological sorting") and replace it with a concise note such as "Manage dump entries with dependency resolution performed by the Rust libpgdump layer" or similar wording that makes it clear the toposort JS dependency was removed.
🧹 Nitpick comments (6)
tests/test_edge_cases.py (1)
26-28: Test relies on internal API and hardcoded dump_id.The test accesses
_dump(internal Rust backend) and assumes dump_id1is the encoding entry. This coupling to internal implementation details makes the test fragile if the Rust backend changes entry ordering or IDs.Consider documenting why dump_id
1is expected to be the encoding entry, or using a lookup method if one exists.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_edge_cases.py` around lines 26 - 28, The test directly uses the internal Rust backend via dmp._dump.update_entry(1, ...) and assumes dump_id 1 is the encoding entry, which is fragile; change the test to avoid internal APIs by finding the encoding entry dynamically (e.g., call the public lookup/search method on dmp to locate the entry by its type/name or metadata and use its id) or, if no public lookup exists, add/use a public helper on the dmp object to resolve the encoding entry id instead of hardcoding 1; alternatively document in the test why id 1 is guaranteed (but preferred: replace dmp._dump.update_entry with a public API call that finds the correct entry id and then updates it)..github/workflows/testing.yaml (1)
85-88: Redundant maturin installation and build.The
bootstrapscript (line 76) already runspip install maturinandmaturin develop --extras dev. This step duplicates that work, installing maturin again and rebuilding the extension.Consider removing this step since bootstrap handles it, or removing the maturin commands from bootstrap if you prefer explicit CI steps.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/testing.yaml around lines 85 - 88, The "Build native extension" CI step duplicates work already performed by the earlier "bootstrap" step (which installs maturin and runs maturin develop --extras dev); remove the redundant step by deleting the job block named "Build native extension" that runs ".venv/bin/pip install maturin" and ".venv/bin/maturin develop" from the workflow, or alternatively remove the maturin install/build calls from the "bootstrap" step so only one place performs the native build—ensure you keep a single canonical place (either "bootstrap" or the explicit build step) to avoid double installation and rebuilds.src/dump.rs (3)
36-38: Consider handling poisoned mutex instead of unwrap.Throughout this file,
.lock().unwrap()is used which will panic if the mutex is poisoned (i.e., a thread panicked while holding the lock). While this is acceptable for most use cases, production code might benefit from more graceful error handling.♻️ Example alternative
fn save(&self, path: &str) -> PyResult<()> { - self.inner.lock().unwrap().save(path).map_err(to_pyerr) + self.inner + .lock() + .map_err(|_| PyRuntimeError::new_err("Lock poisoned"))? + .save(path) + .map_err(to_pyerr) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dump.rs` around lines 36 - 38, The code currently calls .lock().unwrap() (e.g., in save on self.inner) which will panic on a poisoned mutex; change these to handle PoisonError by replacing .lock().unwrap() with a safe map_err branch that converts the PoisonError into a PyResult error instead of panicking — for example, use .lock().map_err(|pe| /* convert pe to PyErr via to_pyerr or create a PyRuntimeError with context */ )? and then continue calling .save(path).map_err(to_pyerr); update every other occurrence of .lock().unwrap() in this file (references: save, self.inner, any other methods using inner.lock()) so mutex poisoning is returned as a PyErr rather than causing a panic.
144-163: Memory consideration:blobsloads all blob data into memory.Similar to
table_data, theblobsmethod loads all blob OIDs and data into memory as aVec<Bound<'py, PyTuple>>. For dumps with many large blobs, this could be memory-intensive. Consider documenting this behavior or providing a streaming alternative in the future.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dump.rs` around lines 144 - 163, The blobs method currently collects all blob OIDs and data into memory (function blobs, returning Vec<Bound<'py, PyTuple>>), so add a clear doc comment on blobs explaining it loads all blob data into memory and may be memory-intensive for large dumps, and (optionally) add a streaming alternative named blobs_iter (or blobs_stream) that yields items lazily (e.g., return a PyIterator/Python generator or implement an iterator that yields (oid, PyBytes) tuples instead of collecting into a Vec) so callers can consume blobs one-by-one without an in-memory Vec; update public API docs accordingly.
133-142: Memory consideration:table_dataloads all rows into memory.The
table_datamethod collects all rows into aVec<String>before returning. For large tables, this could consume significant memory. The Python side iterates over this, so a streaming iterator would be more memory-efficient. However, PyO3's iterator support can be complex, so this tradeoff may be acceptable for the initial implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dump.rs` around lines 133 - 142, The current table_data method (in dump.rs, function table_data) eagerly collects all rows into a Vec<String> which can OOM for large tables; change the Rust->Python boundary to return a Python iterator instead of Vec<String>: change the Rust signature to return PyResult<PyObject> (or another PyIterator-compatible type), and wrap the underlying dump.table_data(namespace, table) iterator (after mapping errors with to_pyerr) in a PyO3-compatible iterator/generator so rows are yielded lazily to Python rather than collected in memory; keep using dump.table_data and to_pyerr but do not call collect() — yield each String through the PyO3 iterator implementation.pgdumplib/dump.py (1)
146-153: Acknowledged:formatproperty shadows Python builtin.Ruff flags
formatas shadowing the Python builtin. While technically true, this is a common pattern for properties representing format types. Renaming todump_formatwould be cleaner but would break the public API.♻️ Alternative naming (API-breaking)
`@property` - def format(self) -> str: + def dump_format(self) -> str: """The dump format (Custom, Directory, Tar)""" return self._format - `@format.setter` - def format(self, value: str) -> None: + `@dump_format.setter` + def dump_format(self, value: str) -> None: self._format = value🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pgdumplib/dump.py` around lines 146 - 153, The property name format shadows a Python builtin; to avoid breaking the public API, add a new canonical property dump_format that gets/sets self._format, then make the existing format getter/setter thin wrappers that call dump_format and emit a DeprecationWarning (use warnings.warn) so callers keep working while signaling migration; update references to use dump_format internally and keep the same _format backing field; ensure you modify both the format getter and format.setter to delegate to dump_format.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Line 38: The document still mentions "toposort" for dependency resolution
under the "Dependency Resolution" section; remove that reference and update the
wording to state that entry ordering is now handled by the Rust layer (instead
of using toposort). Specifically, edit the "Dependency Resolution" paragraph
that mentions `dump_id` and `toposort` to remove the `toposort` claim and
replace it with a short note that the Rust layer enforces correct ordering of
entries/dumps.
In `@pgdumplib/dump.py`:
- Around line 380-405: The code creates a new TABLE DATA entry (via
self._dump.add_entry) before checking for an existing entry, which can leave
orphan entries when table_data_writer is called multiple times; change the logic
in table_data_writer to call _find_existing_table_data(entry.namespace,
entry.tag, ...) before adding a new entry and, if an existing entry is found,
reuse that existing dump_id (and its copy_stmt) for writing instead of creating
a new entry; only call self._dump.add_entry when no existing entry exists, and
ensure you call self._dump.set_entry_data on the chosen dump_id (existing or
newly created) after collecting data from TableData to avoid leaving empty
duplicate TABLE DATA entries.
- Around line 282-301: The code currently preserves the original compression in
_loaded_compression but then forces the in-memory Dump to uncompressed via
_dump.set_compression(constants.COMPRESSION_NONE), so calling save() writes an
uncompressed file while compression_algorithm still reports the original; update
this by either documenting the behavior in the Dump.load()/class docstring to
state that loads normalize to no compression but report original via
compression_algorithm, or change the load path to preserve compression by
removing the _dump.set_compression(constants.COMPRESSION_NONE) call (or store
and reapply the original compression in save()), and ensure related APIs
(_loaded_compression, compression_algorithm, save()) remain consistent with the
chosen approach.
In `@src/errors.rs`:
- Around line 8-21: The map currently uses .unwrap() on a.into_pyobject(py)
which can panic; change the mapping to propagate conversion errors instead: call
a.into_pyobject(py)? (or map to a PyResult and collect::<PyResult<Vec<_>>>()?)
so the iterator returns a PyResult<Vec<_>> and then ?-propagate the error before
creating the PyTuple; update the surrounding function (to_pyerr / the closure
passed to Python::try_attach) to return PyResult so you can use ? for error
propagation rather than unwrapping. Ensure you still convert each successful
PyObject to .into_any() after the ?-checked conversion.
---
Outside diff comments:
In `@CLAUDE.md`:
- Around line 13-14: Update the CLAUDE.md sentence that currently reads "Manage
dump entries with proper dependency resolution using topological sorting" to
remove the reference to the removed toposort dependency and state that
dependency resolution is now performed by the Rust libpgdump layer; locate the
exact sentence in CLAUDE.md (the string "Manage dump entries with proper
dependency resolution using topological sorting") and replace it with a concise
note such as "Manage dump entries with dependency resolution performed by the
Rust libpgdump layer" or similar wording that makes it clear the toposort JS
dependency was removed.
---
Nitpick comments:
In @.github/workflows/testing.yaml:
- Around line 85-88: The "Build native extension" CI step duplicates work
already performed by the earlier "bootstrap" step (which installs maturin and
runs maturin develop --extras dev); remove the redundant step by deleting the
job block named "Build native extension" that runs ".venv/bin/pip install
maturin" and ".venv/bin/maturin develop" from the workflow, or alternatively
remove the maturin install/build calls from the "bootstrap" step so only one
place performs the native build—ensure you keep a single canonical place (either
"bootstrap" or the explicit build step) to avoid double installation and
rebuilds.
In `@pgdumplib/dump.py`:
- Around line 146-153: The property name format shadows a Python builtin; to
avoid breaking the public API, add a new canonical property dump_format that
gets/sets self._format, then make the existing format getter/setter thin
wrappers that call dump_format and emit a DeprecationWarning (use warnings.warn)
so callers keep working while signaling migration; update references to use
dump_format internally and keep the same _format backing field; ensure you
modify both the format getter and format.setter to delegate to dump_format.
In `@src/dump.rs`:
- Around line 36-38: The code currently calls .lock().unwrap() (e.g., in save on
self.inner) which will panic on a poisoned mutex; change these to handle
PoisonError by replacing .lock().unwrap() with a safe map_err branch that
converts the PoisonError into a PyResult error instead of panicking — for
example, use .lock().map_err(|pe| /* convert pe to PyErr via to_pyerr or create
a PyRuntimeError with context */ )? and then continue calling
.save(path).map_err(to_pyerr); update every other occurrence of .lock().unwrap()
in this file (references: save, self.inner, any other methods using
inner.lock()) so mutex poisoning is returned as a PyErr rather than causing a
panic.
- Around line 144-163: The blobs method currently collects all blob OIDs and
data into memory (function blobs, returning Vec<Bound<'py, PyTuple>>), so add a
clear doc comment on blobs explaining it loads all blob data into memory and may
be memory-intensive for large dumps, and (optionally) add a streaming
alternative named blobs_iter (or blobs_stream) that yields items lazily (e.g.,
return a PyIterator/Python generator or implement an iterator that yields (oid,
PyBytes) tuples instead of collecting into a Vec) so callers can consume blobs
one-by-one without an in-memory Vec; update public API docs accordingly.
- Around line 133-142: The current table_data method (in dump.rs, function
table_data) eagerly collects all rows into a Vec<String> which can OOM for large
tables; change the Rust->Python boundary to return a Python iterator instead of
Vec<String>: change the Rust signature to return PyResult<PyObject> (or another
PyIterator-compatible type), and wrap the underlying dump.table_data(namespace,
table) iterator (after mapping errors with to_pyerr) in a PyO3-compatible
iterator/generator so rows are yielded lazily to Python rather than collected in
memory; keep using dump.table_data and to_pyerr but do not call collect() —
yield each String through the PyO3 iterator implementation.
In `@tests/test_edge_cases.py`:
- Around line 26-28: The test directly uses the internal Rust backend via
dmp._dump.update_entry(1, ...) and assumes dump_id 1 is the encoding entry,
which is fragile; change the test to avoid internal APIs by finding the encoding
entry dynamically (e.g., call the public lookup/search method on dmp to locate
the entry by its type/name or metadata and use its id) or, if no public lookup
exists, add/use a public helper on the dmp object to resolve the encoding entry
id instead of hardcoding 1; alternatively document in the test why id 1 is
guaranteed (but preferred: replace dmp._dump.update_entry with a public API call
that finds the correct entry id and then updates it).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d7266902-77b4-4380-90b4-5e59ec31c2f1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
.github/workflows/deploy.yaml.github/workflows/testing.yaml.gitignoreCLAUDE.mdCargo.tomlbootstrappgdumplib/__init__.pypgdumplib/constants.pypgdumplib/dump.pypyproject.tomlsrc/dump.rssrc/entry.rssrc/errors.rssrc/lib.rstests/test_edge_cases.py
- Update CLAUDE.md to replace stale toposort reference with Rust layer for dependency resolution - Document that loaded dumps save without compression by default in the load() docstring - Fix table_data_writer to check for existing TABLE DATA entries before creating new ones, preventing orphan entries on repeated calls for the same table Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Owner
Author
PR Monitor SummaryResponse to CodeRabbit's Review
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
pgdumplib/dump.py (1)
47-69: Watch the new memory profile ofTableData.This now keeps the entire table payload in
io.BytesIOuntil the context exits, andset_entry_data()then copies those bytes again into Rust. If large table exports are a supported path, this is a meaningful regression from the old temp-file-backed writer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pgdumplib/dump.py` around lines 47 - 69, The current TableData implementation (seen in __init__, append, get_data) keeps the entire table in-memory via io.BytesIO, causing a memory/regression when large exports are used and set_entry_data copies those bytes into Rust; replace the in-memory buffer with a disk-backed or spooled temporary file (e.g., tempfile.SpooledTemporaryFile or tempfile.TemporaryFile) in __init__, change append to write encoded bytes into that file, and update get_data (or provide an alternative like get_fileobj) to either seek/read only when absolutely needed or return the file-like object so set_entry_data can stream from disk without a full extra copy. Ensure you close/unlink the temp file when the context exits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Line 21: The README line describing pgdumplib/dump.py is outdated: update the
sentence about TableData to remove the claim it uses temporary gzip-compressed
files and instead state that TableData buffers rows in an in-memory io.BytesIO
buffer and materializes them when get_data() is called; mention Dump and
TableData by name so readers can locate the implementation and adjust any
wording about lifecycle or memory behavior accordingly.
In `@pgdumplib/dump.py`:
- Around line 291-296: The preflight always calls _check_file_format on any
regular file (path.is_file()), which only recognizes constants.MAGIC ("PGDMP")
and therefore prevents tar archives (which are regular files) from reaching
_pgdumplib.Dump.load; change the guard so you only run _check_file_format when
the file actually starts with constants.MAGIC (e.g., open path, read the first
len(constants.MAGIC) bytes and compare to constants.MAGIC), otherwise skip the
format check and call _pgdumplib.Dump.load(str(path)) so tar support can be
handled by Dump.load.
- Around line 151-154: The property setter Dump.format currently only updates
the Python-side cache (self._format) and must also update the native/backend
dump so save() uses the new format; modify Dump.format setter to set
self._format = value and also propagate the change to the underlying native
object (self._dump) by calling the backend's API (e.g.,
self._dump.set_format(value) or assigning to self._dump.format if that is the
native attribute), ensuring the setter validates/normalizes the value the same
way before updating both places.
- Around line 225-248: The code validates dump_id but never passes it to the
underlying _dump.add_entry (and the bound Rust method _pgdumplib.Dump.add_entry
in src/dump.rs currently lacks a dump_id parameter), so callers thinking they
set a stable ID are silently ignored; modify the Python API (in the function
that checks dump_id and calls self._dump.add_entry) to raise a clear
NotImplementedError (or ValueError with a message like "explicit dump_id not
supported yet") whenever dump_id is not None to fail fast, and add a TODO
comment instructing that when src/dump.rs/_pgdumplib.Dump.add_entry is extended
to accept dump_id the call site should pass dump_id through to _dump.add_entry
and remove the failure.
- Around line 388-416: When reusing an existing TABLE DATA entry from
_find_existing_table_data, validate that the persisted COPY statement matches
the newly constructed copy_stmt (including column names and order) before
reusing dump_id; if they differ, do not reuse existing and instead create a new
entry via self._dump.add_entry (constants.TABLE_DATA) or raise an explicit
error. Locate the logic around copy_stmt, existing =
self._find_existing_table_data(...), and the subsequent branch that sets
dump_id; use self._dump.entry_data(existing) or the entry metadata to retrieve
the stored copy_stmt for comparison and only set dump_id = existing when they
are identical. Ensure TableData(writer) and subsequent set_entry_data still
operate on the correct dump_id when a new entry is created.
---
Nitpick comments:
In `@pgdumplib/dump.py`:
- Around line 47-69: The current TableData implementation (seen in __init__,
append, get_data) keeps the entire table in-memory via io.BytesIO, causing a
memory/regression when large exports are used and set_entry_data copies those
bytes into Rust; replace the in-memory buffer with a disk-backed or spooled
temporary file (e.g., tempfile.SpooledTemporaryFile or tempfile.TemporaryFile)
in __init__, change append to write encoded bytes into that file, and update
get_data (or provide an alternative like get_fileobj) to either seek/read only
when absolutely needed or return the file-like object so set_entry_data can
stream from disk without a full extra copy. Ensure you close/unlink the temp
file when the context exits.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fb8c86c0-9535-419a-8c57-9aa5401d5053
📒 Files selected for processing (2)
CLAUDE.mdpgdumplib/dump.py
- Sync format property setter with Rust backend via set_format() - Fail fast with NotImplementedError for custom dump_id (unsupported in Rust) - Allow tar archives through _check_file_format to Rust backend - Validate column layout matches when reusing TABLE DATA entries - Update CLAUDE.md to reflect in-memory TableData buffering Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
CLAUDE.md (1)
13-13: Inconsistent reference to dependency resolution method.Line 13 still mentions "topological sorting" but line 38 correctly states the Rust layer now handles entry ordering. This creates inconsistency in the documentation.
📝 Suggested fix
-- Handle BLOB data -- Manage dump entries with proper dependency resolution using topological sorting +- Handle BLOB data +- Manage dump entries with proper dependency resolution🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` at line 13, Update the inconsistent phrasing: replace the sentence "Manage dump entries with proper dependency resolution using topological sorting" with wording that reflects the Rust layer handling entry ordering (e.g., "Manage dump entries; entry ordering and dependency resolution are handled by the Rust layer") so the doc matches the later line stating the Rust layer now handles entry ordering and removes mention of topological sorting.tests/test_dump.py (1)
226-237: Test assertion creates coupling to libpgdump's error message wording.The test asserts
self.assertIn('invalid magic bytes', ...), which couples to the exact error message from thelibpgdumpRust crate. Persrc/errors.rs,InvalidHeadererrors pass the message directly through toPyValueErrorwithout modification, meaning any future change to libpgdump's error wording will break this test even though the actual behavior (rejecting invalid files with aValueError) remains correct.Consider either:
- Accepting this coupling (if testing the full error flow including exact wording is intentional)
- Using a less specific assertion that checks only for
ValueErrortype🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_dump.py` around lines 226 - 237, The test test_invalid_binary_format couples to libpgdump's exact error wording by asserting 'invalid magic bytes' in the exception string; instead drop the brittle substring check and rely only on the exception type: keep the with self.assertRaises(ValueError) around pgdumplib.load(temp_name) and remove the self.assertIn(...) line (or replace it with a generic check like assertIsInstance(context.exception, ValueError) if you prefer), so the test verifies the ValueError behavior without depending on libpgdump's exact message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@CLAUDE.md`:
- Line 13: Update the inconsistent phrasing: replace the sentence "Manage dump
entries with proper dependency resolution using topological sorting" with
wording that reflects the Rust layer handling entry ordering (e.g., "Manage dump
entries; entry ordering and dependency resolution are handled by the Rust
layer") so the doc matches the later line stating the Rust layer now handles
entry ordering and removes mention of topological sorting.
In `@tests/test_dump.py`:
- Around line 226-237: The test test_invalid_binary_format couples to
libpgdump's exact error wording by asserting 'invalid magic bytes' in the
exception string; instead drop the brittle substring check and rely only on the
exception type: keep the with self.assertRaises(ValueError) around
pgdumplib.load(temp_name) and remove the self.assertIn(...) line (or replace it
with a generic check like assertIsInstance(context.exception, ValueError) if you
prefer), so the test verifies the ValueError behavior without depending on
libpgdump's exact message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 20e88bd3-7d77-4bc6-8dec-0e31e7639569
📒 Files selected for processing (4)
CLAUDE.mdpgdumplib/dump.pytests/test_dump.pytests/test_edge_cases.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_edge_cases.py
- Fix inconsistent phrasing in CLAUDE.md line 13 to match line 38: replace topological sorting mention with Rust layer reference - Remove brittle assertIn on libpgdump's exact error wording in test_invalid_binary_format; rely only on ValueError exception type Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
-Fc, Directory-Fd, Tar-Ft), additional compression (LZ4, Zstd), and improved performanceChanges
New files:
Cargo.toml— PyO3 crate depending onlibpgdump = "2"src/lib.rs— PyO3 module registrationsrc/dump.rs—PyDumpclass wrappinglibpgdump::Dumpsrc/entry.rs—PyEntryclass exposing entry fieldssrc/errors.rs— Error mapping from Rust to Python exceptionsRewritten:
pgdumplib/dump.py— Thin wrapper over the native module (from ~1200 lines of binary I/O to ~500 lines of delegation)Updated:
pyproject.toml— maturin build backend, removedtoposortdependencypgdumplib/constants.py— Removed dead binary-parser constants, updatedSUPPORTED_COMPRESSION_ALGORITHMSbootstrap— Usesmaturin developinstead ofpip install -e.github/workflows/testing.yaml— Adds Rust toolchain step.github/workflows/deploy.yaml— Usesmaturin-actionfor cross-platform wheel buildstests/test_edge_cases.py— Removed 7 tests that tested old pure-Python binary parser internalsNew capabilities
dump.set_format('Directory')/dump.set_format('Tar')— write in directory or tar formatdump.set_compression('lz4')/dump.set_compression('zstd')— LZ4 and Zstd compressionload,new,Dump,Entry, converters, constants, exceptions)Test plan
maturin developbuilds cleanlypg_restorecan read dumps created by the new implementation🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests