Add DuckDB #222
Add DuckDB #222
Conversation
- Add DuckDBStore implementation with BaseStore and BaseContextManagerStore - Support both in-memory (:memory:) and persistent (file-based) storage - Include seed support following current store patterns - Add comprehensive test suite (294 tests passing) - Update pyproject.toml with duckdb>=1.0.0 optional dependency - Mark as unstable API (_stable_api = False) Implements #11 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: William Easton <strawgate@users.noreply.github.com>
- Add connection ownership tracking with _owns_connection flag - Add closed-state guards to prevent use-after-close errors - Document metadata column duplication rationale - Add error handling to __del__ with try-except - Remove invalid @OverRide decorators from test methods All 294 tests passing ✅ Co-authored-by: William Easton <strawgate@users.noreply.github.com>
Resolved conflicts by accepting relaxed version constraints from main (#139) while keeping DuckDB store additions from this branch. Co-authored-by: William Easton <strawgate@users.noreply.github.com>
Co-authored-by: William Easton <strawgate@users.noreply.github.com>
Remove unrelated documentation and workflow changes that were accidentally included in the DuckDB store PR. These changes included license modifications, wrapper documentation updates, and workflow instruction changes that should be in separate PRs. Co-authored-by: William Easton <strawgate@users.noreply.github.com>
- Add use_json_column parameter (defaults to True) for native JSON vs TEXT storage - Update schema to use native DuckDB types (JSON, TIMESTAMP) for better queryability - Store value data separately from metadata to eliminate duplication - Metadata (created_at, ttl, expires_at) in native columns for efficient SQL queries - Add comprehensive tests for both JSON and TEXT modes - Add test_native_sql_queryability() demonstrating direct SQL queries on stored data - Fix timezone handling to ensure consistency with ManagedEntry expectations - All 411 tests passing (404 passed, 7 skipped) This enables users to query the DuckDB database directly with SQL for analytics and data exploration while maintaining full compatibility with the key-value API. Co-authored-by: William Easton <strawgate@users.noreply.github.com>
…kDB store - Add table_name parameter with default 'kv_entries' for multi-tenancy - Rename use_json_column to native_storage for consistency with Elasticsearch - Split value storage into separate value_json (TEXT) and value_dict (JSON) columns - Add SQL helper functions for generating CREATE, SELECT, INSERT, DELETE statements - Update tests to use native_storage parameter and verify both storage modes - Add test_custom_table_name() and test_native_vs_stringified_storage() - All 406 tests passing, 7 skipped Following MongoDB/Elasticsearch pattern with separated value columns. Co-authored-by: William Easton <strawgate@users.noreply.github.com>
- Add DuckDBSerializationAdapter class following MongoDB/Elasticsearch patterns - Move inline imports (json, timezone) to module level - Update _get_managed_entry() to use adapter.load_dict() - Update _put_managed_entry() to use adapter.dump_dict() - Fix floating point precision issue in SQL queryability test - All tests passing (405 passed, 7 skipped) Aligns DuckDB store with SerializationAdapter refactoring from PR #184 Co-authored-by: William Easton <strawgate@users.noreply.github.com>
- Add pyright: ignore comments for intentional _connection access in tests - Add null assertions for fetchone() results - Move type ignore comments outside SQL strings to avoid DuckDB parse errors - All 406 DuckDB tests now passing Co-authored-by: William Easton <strawgate@users.noreply.github.com>
- Add version, key, and collection fields to serialized JSON documents - Update DuckDBSerializationAdapter to store metadata in JSON columns - Split prepare_load into helper methods for better code organization - Add type annotations to satisfy type checker - Regenerate sync library Co-authored-by: William Easton <strawgate@users.noreply.github.com>
- Remove native_storage parameter from DuckDBSerializationAdapter and DuckDBStore - Remove value_json TEXT column from schema, use only value_dict JSON column - Update all SQL statements to work with single value_dict column - Remove TestDuckDBStoreTextMode test class - Remove test_text_mode_storage() and test_native_vs_stringified_storage() tests - Update test_native_sql_queryability() to use correct JSON path for nested value - Regenerate sync library with simplified implementation This simplification focuses on native JSON storage for better queryability and removes the complexity of supporting dual storage modes. Co-authored-by: William Easton <strawgate@users.noreply.github.com>
|
Warning Rate limit exceeded@strawgate has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 56 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds DuckDB-backed key-value store implementations (sync and async), serialization adapters, package exports, tests, and updates project metadata to include DuckDB (and pytz) as optional/dev dependencies; also silences E501 for generated code files. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant DuckDBStore
participant Adapter as SerializationAdapter
participant DuckDB as DuckDBConnection
Note over Client,DuckDBStore: Put / Get operations (async for aio)
Client->>DuckDBStore: put(key, value, collection, ttl?)
DuckDBStore->>Adapter: prepare_dump(managed_entry)
Adapter-->>DuckDBStore: row_payload (JSON, timestamps)
DuckDBStore->>DuckDB: EXECUTE INSERT/UPDATE SQL (params)
DuckDB-->>DuckDBStore: OK
Client->>DuckDBStore: get(key, collection)
DuckDBStore->>DuckDB: EXECUTE SELECT SQL (params)
DuckDB-->>DuckDBStore: row (json + meta)
DuckDBStore->>Adapter: prepare_load(row)
Adapter-->>DuckDBStore: ManagedEntry (with UTC timestamps)
DuckDBStore-->>Client: ManagedEntry / value
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
key-value/key-value-aio/pyproject.toml(2 hunks)key-value/key-value-aio/src/key_value/aio/stores/duckdb/__init__.py(1 hunks)key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py(1 hunks)key-value/key-value-aio/tests/stores/duckdb/__init__.py(1 hunks)key-value/key-value-aio/tests/stores/duckdb/test_duckdb.py(1 hunks)key-value/key-value-sync/pyproject.toml(2 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/__init__.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py(1 hunks)key-value/key-value-sync/src/key_value/sync/stores/duckdb/__init__.py(1 hunks)key-value/key-value-sync/tests/code_gen/stores/duckdb/__init__.py(1 hunks)key-value/key-value-sync/tests/code_gen/stores/duckdb/test_duckdb.py(1 hunks)pyproject.toml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/__init__.py (2)
key-value/key-value-sync/tests/code_gen/stores/duckdb/test_duckdb.py (3)
store(27-31)store(41-47)store(58-62)key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py (1)
DuckDBStore(117-374)
key-value/key-value-sync/tests/code_gen/stores/duckdb/test_duckdb.py (1)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py (1)
DuckDBStore(117-374)
key-value/key-value-sync/src/key_value/sync/stores/duckdb/__init__.py (2)
key-value/key-value-sync/tests/code_gen/stores/duckdb/test_duckdb.py (3)
store(27-31)store(41-47)store(58-62)key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py (1)
DuckDBStore(117-374)
key-value/key-value-aio/src/key_value/aio/stores/duckdb/__init__.py (2)
key-value/key-value-aio/tests/stores/duckdb/test_duckdb.py (3)
store(24-28)store(38-44)store(55-59)key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py (1)
DuckDBStore(114-420)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py (3)
key-value/key-value-shared/src/key_value/shared/errors/key_value.py (1)
DeserializationError(14-15)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(15-63)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (3)
SerializationAdapter(38-184)load_dict(67-106)dump_dict(114-158)
key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py (3)
key-value/key-value-shared/src/key_value/shared/errors/key_value.py (1)
DeserializationError(14-15)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(15-63)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (3)
SerializationAdapter(38-184)load_dict(67-106)dump_dict(114-158)
key-value/key-value-aio/tests/stores/duckdb/test_duckdb.py (2)
key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py (1)
DuckDBStore(114-420)key-value/key-value-sync/tests/code_gen/stores/duckdb/test_duckdb.py (4)
get_client_from_store(19-20)store(27-31)store(41-47)store(58-62)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, windows-2022, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, macos-latest, key-value/key-value-shared)
- GitHub Check: test_quick (3.10, windows-2022, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, macos-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-aio)
🔇 Additional comments (3)
key-value/key-value-sync/src/key_value/sync/stores/duckdb/__init__.py (1)
1-6: Auto-generated package initializer—correctly structured.The file properly re-exports
DuckDBStorefrom the code-gen module with appropriate auto-generation warnings in place. The warnings correctly direct maintainers to modify the original source file rather than this generated file.Since this is auto-generated, the substantive review should focus on the implementation in
key_value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py. Ensure the original source template maintains this same export pattern and that the code-generation script correctly produces this file on each build.key-value/key-value-aio/tests/stores/duckdb/__init__.py (1)
1-1: LGTM—minimal but correct package marker.This is an appropriate way to initialize a test package. The comment clearly documents the package's purpose. If you need to share test fixtures or utilities across multiple test modules in this package, consider importing and exposing them here in the future, but for now this is fine.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/__init__.py (1)
1-6: LGTM! Auto-generated code is correct.The auto-generated package initializer follows the expected pattern with a clear warning header, correct import path, and appropriate public API exposure.
key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py
Outdated
Show resolved
Hide resolved
key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py
Outdated
Show resolved
Hide resolved
key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py
Show resolved
Hide resolved
key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py
Outdated
Show resolved
Hide resolved
Test Failure AnalysisSummary: All test jobs for and packages are failing due to an import error: Root Cause: The test files are importing In both test files:
The problematic import is: from _duckdb import DuckDBPyConnectionThe Suggested Solution: Change the import statement in the async test file from: from _duckdb import DuckDBPyConnectionto: from duckdb import DuckDBPyConnectionFiles to modify:
After fixing the async version, run This follows the pattern used by other store tests (e.g., Redis tests import from Detailed AnalysisError from test logs:Verification:The from duckdb import DuckDBPyConnection # ✓ This works
import duckdb
conn = duckdb.connect(':memory:')
isinstance(conn, DuckDBPyConnection) # TruePattern from other stores:Looking at from redis.asyncio.client import Redis # Uses public API, not private _redisRelated FilesFiles causing the failure:
Reference implementation:
Related configuration:
|
|
@claude please address PR feedback and fix tests |
|
Claude finished @strawgate's task —— View job PR Feedback Addressed and Tests Fixed ✅All critical issues from the code review have been fixed and changes have been committed and pushed. Summary of Changes1. Import Error (Critical) ✅
2. Naive Timestamp Handling (Critical) ✅
3. SQL Injection Vulnerability (Critical) ✅
4. JSON Storage Optimization ✅
Test ResultsStatus: 4,129 tests passed ✅ | 259 skipped | 1 failed All DuckDB store core functionality tests pass:
Note on the 1 failing test: The CommitChanges committed and pushed to branch --- | |
- Fix import error: use public duckdb module instead of private _duckdb - Fix naive timestamp handling: use replace(tzinfo=UTC) for naive datetimes - Add SQL injection protection: validate table names with regex - Change JSON storage to use dict directly instead of json.dumps() Fixes all critical issues identified in code review: - Resolves ModuleNotFoundError that was causing all tests to fail - Prevents ValueError when DuckDB returns naive timestamps - Protects against SQL injection via malicious table names - Improves JSON queryability in DuckDB Co-authored-by: William Easton <strawgate@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py(1 hunks)key-value/key-value-aio/tests/stores/duckdb/test_duckdb.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py(1 hunks)key-value/key-value-sync/tests/code_gen/stores/duckdb/test_duckdb.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py (3)
key-value/key-value-shared/src/key_value/shared/errors/key_value.py (1)
DeserializationError(14-15)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(15-63)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (3)
SerializationAdapter(38-184)load_dict(67-106)dump_dict(114-158)
key-value/key-value-sync/tests/code_gen/stores/duckdb/test_duckdb.py (1)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py (1)
DuckDBStore(124-392)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py (3)
key-value/key-value-shared/src/key_value/shared/errors/key_value.py (1)
DeserializationError(14-15)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(15-63)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (3)
SerializationAdapter(38-184)load_dict(67-106)dump_dict(114-158)
key-value/key-value-aio/tests/stores/duckdb/test_duckdb.py (1)
key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py (1)
DuckDBStore(121-438)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: test_quick (3.10, windows-2022, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, windows-2022, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, macos-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, macos-14, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-aio)
🔇 Additional comments (13)
key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py (13)
1-18: LGTM: Clean imports with appropriate error handling.The conditional import with a clear error message properly handles the optional DuckDB dependency.
33-60: LGTM: Clean serialization logic.The method correctly extracts fields and builds the JSON document for DuckDB storage.
63-86: LGTM: Well-structured deserialization.The method properly delegates JSON parsing and timestamp conversion to helper methods.
88-102: LGTM: Robust JSON parsing with comprehensive error handling.The method handles all expected input types and provides clear error messages.
104-118: Past feedback properly addressed.The timestamp conversion correctly handles both naive and aware timestamps using
replace(tzinfo=timezone.utc)for naive timestamps andastimezone(tz=timezone.utc)for aware timestamps. This matches the fix suggested in the previous review.
185-232: Verify connection ownership behavior is intentional.Line 209 sets
self._owns_connection = Trueeven for provided connections, meaning the store will close connections it didn't create. While this is documented in the docstring (lines 156-158), it's unconventional and could lead to issues if users expect to reuse connections.Consider whether connection ownership should be:
Falsefor provided connections (more conventional)- Or keeping current behavior if there's a specific reason (thread-safety, transaction management, etc.)
If the current behavior is intentional, the implementation is correct.
226-229: LGTM: Proper SQL injection protection.The table name validation using regex prevents SQL injection attacks while allowing valid SQL identifiers.
234-307: LGTM: Well-structured SQL generation with proper parameterization.The SQL helper methods correctly use parameterized queries with placeholders for user data, while the validated table name is safely interpolated. The indexes support efficient querying by collection and expiration.
310-331: LGTM: Clean schema initialization.The setup method creates the necessary table and indexes for efficient storage and querying.
334-373: Good defensive programming with closed store check.The method properly checks if the store is closed before operating, preventing use-after-close errors.
376-405: LGTM: Straightforward put operation.The method correctly uses the adapter to serialize the entry before storage.
408-421: LGTM: Proper delete with existence check.The method uses the RETURNING clause to determine if a row was actually deleted, providing accurate feedback.
424-438: LGTM: Safe cleanup with appropriate error suppression.The
_closemethod is idempotent, and__del__properly suppresses exceptions during interpreter shutdown to avoid cleanup errors.
| if created_at is not None and isinstance(created_at, datetime): | ||
| if created_at.tzinfo is None: | ||
| document["created_at"] = created_at.replace(tzinfo=timezone.utc) | ||
| else: | ||
| document["created_at"] = created_at.astimezone(tz=timezone.utc) | ||
| if expires_at is not None and isinstance(expires_at, datetime): | ||
| if expires_at.tzinfo is None: | ||
| document["expires_at"] = expires_at.replace(tzinfo=timezone.utc) | ||
| else: | ||
| document["expires_at"] = expires_at.astimezone(tz=timezone.utc) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Eliminate duplicate timestamp conversion logic.
This timestamp conversion code duplicates the logic in _convert_timestamps_to_utc (lines 104-118). The _get_managed_entry method should reuse the existing helper instead of reimplementing it.
Apply this diff to eliminate duplication:
- if created_at is not None and isinstance(created_at, datetime):
- if created_at.tzinfo is None:
- document["created_at"] = created_at.replace(tzinfo=timezone.utc)
- else:
- document["created_at"] = created_at.astimezone(tz=timezone.utc)
- if expires_at is not None and isinstance(expires_at, datetime):
- if expires_at.tzinfo is None:
- document["expires_at"] = expires_at.replace(tzinfo=timezone.utc)
- else:
- document["expires_at"] = expires_at.astimezone(tz=timezone.utc)
+ if created_at is not None:
+ document["created_at"] = created_at
+ if expires_at is not None:
+ document["expires_at"] = expires_at
+
+ # Convert timestamps using the adapter's helper method
+ self._adapter._convert_timestamps_to_utc(document)Alternatively, if you prefer not to call a private method of the adapter, you could move _convert_timestamps_to_utc to be a module-level helper function or a method of the store class.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py around lines
359 to 368, the timestamp conversion logic is duplicated; remove the inline
created_at/expires_at conversion and instead call the existing
_convert_timestamps_to_utc helper (or if calling a private adapter helper is
undesirable, refactor _convert_timestamps_to_utc into a module-level function or
a public store method and call that). Ensure the replacement handles both
created_at and expires_at the same way as the original helper and preserves
timezone-aware and naive datetime handling by returning/assigning UTC-aware
datetimes to document["created_at"] and document["expires_at"].
| try: | ||
| import duckdb | ||
| except ImportError as e: | ||
| msg = "DuckDBStore requires py-key-value-aio[duckdb]" | ||
| raise ImportError(msg) from e |
There was a problem hiding this comment.
Fix the ImportError guidance for sync users.
Line 20 throws an ImportError telling users to install py-key-value-aio[duckdb], but this module lives in the sync package. That message will send sync users to the wrong extra. Please update the hint to reference the correct sync extra (for example, py-key-value-sync[duckdb]) so the remediation is accurate.
Apply this diff:
- msg = "DuckDBStore requires py-key-value-aio[duckdb]"
+ msg = "DuckDBStore requires py-key-value-sync[duckdb]"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| import duckdb | |
| except ImportError as e: | |
| msg = "DuckDBStore requires py-key-value-aio[duckdb]" | |
| raise ImportError(msg) from e | |
| try: | |
| import duckdb | |
| except ImportError as e: | |
| msg = "DuckDBStore requires py-key-value-sync[duckdb]" | |
| raise ImportError(msg) from e |
🤖 Prompt for AI Agents
In key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py
around lines 17 to 21, the ImportError message incorrectly tells users to
install the async extra `py-key-value-aio[duckdb]`; update the hint to reference
the sync package extra (e.g. `py-key-value-sync[duckdb]`) so sync users get the
correct remediation while keeping the same raise ImportError(... ) from e
behavior.
|
@claude please resolve merge conflicts |
|
Claude finished @strawgate's task —— View job Merge Conflicts Resolved ✅All merge conflicts have been successfully resolved and pushed to the PR branch. What Was DoneConflicts Resolved:
Additional Changes:
Files Merged from main:
Commits Created
All changes have been pushed to the branch and are ready for review. | Branch |
Co-authored-by: William Easton <strawgate@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 9
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py(1 hunks)key-value/key-value-aio/tests/stores/duckdb/test_duckdb.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py(1 hunks)key-value/key-value-sync/tests/code_gen/stores/duckdb/test_duckdb.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
key-value/key-value-sync/tests/code_gen/stores/duckdb/test_duckdb.py (1)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py (1)
DuckDBStore(66-331)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py (3)
key-value/key-value-shared/src/key_value/shared/errors/key_value.py (1)
DeserializationError(14-15)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (2)
ManagedEntry(15-63)value_as_json(41-43)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (3)
SerializationAdapter(38-184)load_dict(67-106)dump_dict(114-158)
key-value/key-value-aio/tests/stores/duckdb/test_duckdb.py (1)
key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py (1)
DuckDBStore(63-381)
key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py (3)
key-value/key-value-shared/src/key_value/shared/errors/key_value.py (1)
DeserializationError(14-15)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (2)
ManagedEntry(15-63)value_as_json(41-43)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (3)
SerializationAdapter(38-184)load_dict(67-106)dump_dict(114-158)
🔇 Additional comments (27)
key-value/key-value-sync/tests/code_gen/stores/duckdb/test_duckdb.py (10)
1-16: LGTM!The auto-generated file warning and imports are appropriate for this test module.
18-20: LGTM!The helper function appropriately accesses the private connection for testing purposes with proper type-checking suppression.
22-34: LGTM!The test class properly sets up an in-memory DuckDB store with appropriate cleanup and skip markers.
36-50: LGTM!The fixture correctly manages the temporary directory lifecycle, ensuring it exists throughout the test and is cleaned up only after the store is closed.
63-105: LGTM!Excellent test coverage for native SQL queryability, verifying both JSON field extraction and timestamp-based queries.
106-121: LGTM!The test properly verifies different database path initialization modes with appropriate cleanup.
122-138: LGTM!Excellent test demonstrating data persistence across store instances using the same database file.
139-153: LGTM!Critical security test properly verifying SQL injection protection through malicious collection and key values.
154-179: LGTM!Excellent edge case coverage testing both large data storage (1MB) and comprehensive Unicode support across multiple character sets.
193-220: LGTM!Comprehensive test for custom table name functionality, including both positive verification of the custom table and negative verification that the default table doesn't exist.
key-value/key-value-aio/tests/stores/duckdb/test_duckdb.py (7)
1-17: LGTM! Imports and helper function are well-structured.The helper function appropriately accesses the private connection attribute for testing purposes, with proper type hints and pyright ignore.
19-31: LGTM! Test class properly structured.The fixture correctly manages the in-memory store lifecycle with proper cleanup, and the unbounded test skip is appropriately justified.
33-47: LGTM! Persistent store fixture is well-implemented.The fixture correctly manages resources by closing the store before the temporary directory is cleaned up.
49-58: LGTM! Fixture is properly implemented.The store fixture provides clean setup and teardown for DuckDB-specific tests.
136-150: LGTM! SQL injection test is well-designed.The test properly validates that malicious input is handled safely through parameterized queries, and verifies the store remains functional afterward.
151-160: LGTM! Large data test is appropriate.The test validates handling of 1MB values, which is a reasonable size for verifying the store can handle large payloads.
161-176: LGTM! Comprehensive Unicode test.The test thoroughly validates Unicode support across multiple languages, emoji, and special characters.
key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py (10)
30-33: LGTM!The passthrough implementation is appropriate since DuckDB handles datetime and JSON types natively.
35-44: LGTM!The method correctly handles DuckDB's naive timestamps by delegating to the conversion helper.
46-61: LGTM!The timestamp conversion correctly handles both naive and timezone-aware datetimes, treating naive timestamps from DuckDB as UTC before any timezone operations.
87-174: Constructor implementation is solid.The overloaded signatures provide clear type hints, input validation prevents SQL injection, and connection ownership semantics are well-documented. The mutual exclusivity check for
connectionanddatabase_pathparameters ensures correct usage.
176-250: LGTM!The SQL generators use parameterized queries to prevent injection, and the table name validation in the constructor ensures safe interpolation. The schema design with
TIMESTAMPTZ,JSONcolumns, and appropriate indexes supports efficient querying and expiration management.
253-274: LGTM!The setup method cleanly initializes the schema with appropriate indexes. The
IF NOT EXISTSclauses ensure idempotent execution.
277-312: LGTM!The retrieval logic correctly delegates timestamp conversion to the adapter (avoiding the duplication that was previously flagged), handles missing entries gracefully, and protects against operations on closed stores.
315-348: LGTM!The storage logic correctly validates JSON serializability (line 332) and uses the adapter for serialization. The closed-state check and parameterized query provide appropriate safety guarantees.
351-364: LGTM!The deletion logic correctly uses the
RETURNINGclause to determine success and maintains consistency with the closed-state checks in other methods.
367-381: LGTM!The cleanup methods properly respect connection ownership and handle edge cases like partial initialization and interpreter shutdown. The exception suppression in
__del__is appropriate for avoiding cleanup-related crashes.
key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py
Outdated
Show resolved
Hide resolved
| msg = "Table name must start with a letter or underscore and contain only letters, digits, or underscores" | ||
| raise ValueError(msg) | ||
| self._table_name = table_name | ||
| self._stable_api = False |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify or remove unused _stable_api attribute.
The _stable_api attribute is set to False but doesn't appear to be used anywhere in this file or referenced in the base classes. If this is for future use or internal purposes, consider adding a comment explaining its purpose.
🤖 Prompt for AI Agents
In key-value/key-value-aio/src/key_value/aio/stores/duckdb/store.py around line
172, the attribute `_stable_api = False` is set but never used; either remove
this unused attribute to avoid dead code, or if you intend it for
future/internals, replace it with a one-line clarifying comment explaining its
purpose (e.g., "reserved for future API stability flag") and keep it, ensuring
linters are satisfied; do not change behavior elsewhere.
| async def test_native_sql_queryability(self): | ||
| """Test that users can query the database directly with SQL.""" | ||
| store = DuckDBStore() | ||
|
|
||
| # Store some test data with known metadata | ||
| await store.put(collection="products", key="item1", value={"name": "Widget", "price": 10.99}, ttl=3600) | ||
| await store.put(collection="products", key="item2", value={"name": "Gadget", "price": 25.50}, ttl=7200) | ||
| await store.put(collection="orders", key="order1", value={"total": 100.00, "items": 3}) | ||
|
|
||
| # Query directly via SQL to verify native storage | ||
| # Check that value is stored as JSON (can extract fields) | ||
| result = ( | ||
| get_client_from_store(store) | ||
| .execute(""" | ||
| SELECT key, value->'name' as name, value->'price' as price | ||
| FROM kv_entries | ||
| WHERE collection = 'products' | ||
| ORDER BY key | ||
| """) | ||
| .fetchall() | ||
| ) # pyright: ignore[reportPrivateUsage] | ||
|
|
||
| assert len(result) == 2 | ||
| assert result[0][0] == "item1" | ||
| assert result[0][1] == '"Widget"' # JSON strings are quoted | ||
| assert result[1][0] == "item2" | ||
|
|
||
| # Query by expiration timestamp | ||
| count_result = ( | ||
| get_client_from_store(store) | ||
| .execute(""" | ||
| SELECT COUNT(*) | ||
| FROM kv_entries | ||
| WHERE expires_at > now() OR expires_at IS NULL | ||
| """) | ||
| .fetchone() | ||
| ) # pyright: ignore[reportPrivateUsage] | ||
|
|
||
| assert count_result is not None | ||
| assert count_result[0] == 3 # All 3 entries should not be expired | ||
|
|
||
| await store.close() | ||
|
|
There was a problem hiding this comment.
Use fixture or async context manager for resource safety.
The store is created directly and only closed at the end. If any assertion fails or an exception occurs before line 101, the store won't be closed, potentially leaking database connections.
Consider using the existing store fixture or wrap in an async context manager:
- async def test_native_sql_queryability(self):
+ async def test_native_sql_queryability(self, store: DuckDBStore):
"""Test that users can query the database directly with SQL."""
- store = DuckDBStore()
-
# Store some test data with known metadata
await store.put(collection="products", key="item1", value={"name": "Widget", "price": 10.99}, ttl=3600)
await store.put(collection="products", key="item2", value={"name": "Gadget", "price": 25.50}, ttl=7200)
await store.put(collection="orders", key="order1", value={"total": 100.00, "items": 3})
# Query directly via SQL to verify native storage
# Check that value is stored as JSON (can extract fields)
result = (
get_client_from_store(store)
.execute("""
SELECT key, value->'name' as name, value->'price' as price
FROM kv_entries
WHERE collection = 'products'
ORDER BY key
""")
.fetchall()
) # pyright: ignore[reportPrivateUsage]
assert len(result) == 2
assert result[0][0] == "item1"
assert result[0][1] == '"Widget"' # JSON strings are quoted
assert result[1][0] == "item2"
# Query by expiration timestamp
count_result = (
get_client_from_store(store)
.execute("""
SELECT COUNT(*)
FROM kv_entries
WHERE expires_at > now() OR expires_at IS NULL
""")
.fetchone()
) # pyright: ignore[reportPrivateUsage]
assert count_result is not None
assert count_result[0] == 3 # All 3 entries should not be expired
-
- await store.close()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def test_native_sql_queryability(self): | |
| """Test that users can query the database directly with SQL.""" | |
| store = DuckDBStore() | |
| # Store some test data with known metadata | |
| await store.put(collection="products", key="item1", value={"name": "Widget", "price": 10.99}, ttl=3600) | |
| await store.put(collection="products", key="item2", value={"name": "Gadget", "price": 25.50}, ttl=7200) | |
| await store.put(collection="orders", key="order1", value={"total": 100.00, "items": 3}) | |
| # Query directly via SQL to verify native storage | |
| # Check that value is stored as JSON (can extract fields) | |
| result = ( | |
| get_client_from_store(store) | |
| .execute(""" | |
| SELECT key, value->'name' as name, value->'price' as price | |
| FROM kv_entries | |
| WHERE collection = 'products' | |
| ORDER BY key | |
| """) | |
| .fetchall() | |
| ) # pyright: ignore[reportPrivateUsage] | |
| assert len(result) == 2 | |
| assert result[0][0] == "item1" | |
| assert result[0][1] == '"Widget"' # JSON strings are quoted | |
| assert result[1][0] == "item2" | |
| # Query by expiration timestamp | |
| count_result = ( | |
| get_client_from_store(store) | |
| .execute(""" | |
| SELECT COUNT(*) | |
| FROM kv_entries | |
| WHERE expires_at > now() OR expires_at IS NULL | |
| """) | |
| .fetchone() | |
| ) # pyright: ignore[reportPrivateUsage] | |
| assert count_result is not None | |
| assert count_result[0] == 3 # All 3 entries should not be expired | |
| await store.close() | |
| async def test_native_sql_queryability(self, store: DuckDBStore): | |
| """Test that users can query the database directly with SQL.""" | |
| # Store some test data with known metadata | |
| await store.put(collection="products", key="item1", value={"name": "Widget", "price": 10.99}, ttl=3600) | |
| await store.put(collection="products", key="item2", value={"name": "Gadget", "price": 25.50}, ttl=7200) | |
| await store.put(collection="orders", key="order1", value={"total": 100.00, "items": 3}) | |
| # Query directly via SQL to verify native storage | |
| # Check that value is stored as JSON (can extract fields) | |
| result = ( | |
| get_client_from_store(store) | |
| .execute(""" | |
| SELECT key, value->'name' as name, value->'price' as price | |
| FROM kv_entries | |
| WHERE collection = 'products' | |
| ORDER BY key | |
| """) | |
| .fetchall() | |
| ) # pyright: ignore[reportPrivateUsage] | |
| assert len(result) == 2 | |
| assert result[0][0] == "item1" | |
| assert result[0][1] == '"Widget"' # JSON strings are quoted | |
| assert result[1][0] == "item2" | |
| # Query by expiration timestamp | |
| count_result = ( | |
| get_client_from_store(store) | |
| .execute(""" | |
| SELECT COUNT(*) | |
| FROM kv_entries | |
| WHERE expires_at > now() OR expires_at IS NULL | |
| """) | |
| .fetchone() | |
| ) # pyright: ignore[reportPrivateUsage] | |
| assert count_result is not None | |
| assert count_result[0] == 3 # All 3 entries should not be expired |
🤖 Prompt for AI Agents
In key-value/key-value-aio/tests/stores/duckdb/test_duckdb.py around lines 60 to
102, the test constructs DuckDBStore() directly and only calls await
store.close() at the end which can leak resources if an assertion or exception
occurs; update the test to either use the existing store fixture (replace manual
instantiation with the fixture parameter) or wrap creation in an async context
manager or try/finally so the store is always closed (e.g., use "async with
DuckDBStore() as store:" or "store = DuckDBStore(); try: ... finally: await
store.close()") to guarantee cleanup on failure.
| async def test_database_path_initialization(self): | ||
| """Test that store can be initialized with different database path options.""" | ||
| # In-memory (default) | ||
| store1 = DuckDBStore() | ||
| await store1.put(collection="test", key="key1", value={"test": "value1"}) | ||
| result1 = await store1.get(collection="test", key="key1") | ||
| assert result1 == {"test": "value1"} | ||
| await store1.close() | ||
|
|
||
| # Explicit in-memory | ||
| store2 = DuckDBStore(database_path=":memory:") | ||
| await store2.put(collection="test", key="key2", value={"test": "value2"}) | ||
| result2 = await store2.get(collection="test", key="key2") | ||
| assert result2 == {"test": "value2"} | ||
| await store2.close() | ||
|
|
There was a problem hiding this comment.
Use async context manager for resource safety.
Multiple stores are created and closed manually. If any assertion fails before the close() calls, stores will leak.
Apply this diff to use async context managers:
async def test_database_path_initialization(self):
"""Test that store can be initialized with different database path options."""
# In-memory (default)
- store1 = DuckDBStore()
- await store1.put(collection="test", key="key1", value={"test": "value1"})
- result1 = await store1.get(collection="test", key="key1")
- assert result1 == {"test": "value1"}
- await store1.close()
+ async with DuckDBStore() as store1:
+ await store1.put(collection="test", key="key1", value={"test": "value1"})
+ result1 = await store1.get(collection="test", key="key1")
+ assert result1 == {"test": "value1"}
# Explicit in-memory
- store2 = DuckDBStore(database_path=":memory:")
- await store2.put(collection="test", key="key2", value={"test": "value2"})
- result2 = await store2.get(collection="test", key="key2")
- assert result2 == {"test": "value2"}
- await store2.close()
+ async with DuckDBStore(database_path=":memory:") as store2:
+ await store2.put(collection="test", key="key2", value={"test": "value2"})
+ result2 = await store2.get(collection="test", key="key2")
+ assert result2 == {"test": "value2"}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In key-value/key-value-aio/tests/stores/duckdb/test_duckdb.py around lines 103
to 118, the test manually constructs and closes DuckDBStore instances which can
leak resources if assertions fail; change each store creation to use an async
context manager (async with DuckDBStore(...) as store:) and move the
put/get/assert calls inside that block, removing the explicit await
store.close() calls so resources are always cleaned up even on failures.
| async def test_persistent_database(self): | ||
| """Test that data persists across store instances when using file database.""" | ||
| with TemporaryDirectory() as temp_dir: | ||
| db_path = Path(temp_dir) / "persist_test.db" | ||
|
|
||
| # Store data in first instance | ||
| store1 = DuckDBStore(database_path=db_path) | ||
| await store1.put(collection="test", key="persist_key", value={"data": "persistent"}) | ||
| await store1.close() | ||
|
|
||
| # Create second instance with same database file | ||
| store2 = DuckDBStore(database_path=db_path) | ||
| result = await store2.get(collection="test", key="persist_key") | ||
| await store2.close() | ||
|
|
||
| assert result == {"data": "persistent"} | ||
|
|
There was a problem hiding this comment.
Use async context manager for resource safety.
Both store instances should use async context managers to ensure cleanup even if assertions fail.
Apply this diff:
async def test_persistent_database(self):
"""Test that data persists across store instances when using file database."""
with TemporaryDirectory() as temp_dir:
db_path = Path(temp_dir) / "persist_test.db"
# Store data in first instance
- store1 = DuckDBStore(database_path=db_path)
- await store1.put(collection="test", key="persist_key", value={"data": "persistent"})
- await store1.close()
+ async with DuckDBStore(database_path=db_path) as store1:
+ await store1.put(collection="test", key="persist_key", value={"data": "persistent"})
# Create second instance with same database file
- store2 = DuckDBStore(database_path=db_path)
- result = await store2.get(collection="test", key="persist_key")
- await store2.close()
+ async with DuckDBStore(database_path=db_path) as store2:
+ result = await store2.get(collection="test", key="persist_key")
- assert result == {"data": "persistent"}
+ assert result == {"data": "persistent"}🤖 Prompt for AI Agents
In key-value/key-value-aio/tests/stores/duckdb/test_duckdb.py around lines 119
to 135, the test opens DuckDBStore instances without using async context
managers which risks leaking resources if assertions fail; update the test to
use async with for both store instances (e.g. async with
DuckDBStore(database_path=db_path) as store1: await store1.put(...)) and (async
with DuckDBStore(database_path=db_path) as store2: result = await
store2.get(...)), remove the explicit await store.close() calls, and keep the
same assertions so the stores are always cleaned up even on test failures.
| async def test_connection_initialization(self): | ||
| """Test that store can be initialized with existing DuckDB connection.""" | ||
| import duckdb | ||
|
|
||
| conn = duckdb.connect(":memory:") | ||
| store = DuckDBStore(connection=conn) | ||
|
|
||
| await store.put(collection="test", key="conn_test", value={"test": "value"}) | ||
| result = await store.get(collection="test", key="conn_test") | ||
| assert result == {"test": "value"} | ||
|
|
||
| await store.close() | ||
|
|
There was a problem hiding this comment.
Use async context manager for resource safety.
The connection is created directly and the store takes ownership, but if an assertion or exception occurs before line 188, resources will leak.
Apply this diff:
async def test_connection_initialization(self):
"""Test that store can be initialized with existing DuckDB connection."""
import duckdb
conn = duckdb.connect(":memory:")
- store = DuckDBStore(connection=conn)
-
- await store.put(collection="test", key="conn_test", value={"test": "value"})
- result = await store.get(collection="test", key="conn_test")
- assert result == {"test": "value"}
-
- await store.close()
+ async with DuckDBStore(connection=conn) as store:
+ await store.put(collection="test", key="conn_test", value={"test": "value"})
+ result = await store.get(collection="test", key="conn_test")
+ assert result == {"test": "value"}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def test_connection_initialization(self): | |
| """Test that store can be initialized with existing DuckDB connection.""" | |
| import duckdb | |
| conn = duckdb.connect(":memory:") | |
| store = DuckDBStore(connection=conn) | |
| await store.put(collection="test", key="conn_test", value={"test": "value"}) | |
| result = await store.get(collection="test", key="conn_test") | |
| assert result == {"test": "value"} | |
| await store.close() | |
| async def test_connection_initialization(self): | |
| """Test that store can be initialized with existing DuckDB connection.""" | |
| import duckdb | |
| conn = duckdb.connect(":memory:") | |
| async with DuckDBStore(connection=conn) as store: | |
| await store.put(collection="test", key="conn_test", value={"test": "value"}) | |
| result = await store.get(collection="test", key="conn_test") | |
| assert result == {"test": "value"} |
🤖 Prompt for AI Agents
In key-value/key-value-aio/tests/stores/duckdb/test_duckdb.py around lines 177
to 189, the test creates a DuckDB connection directly and passes it to the store
but does not guarantee the connection (or the store) is closed if an
assertion/exception occurs; wrap the connection creation in a context manager
(e.g., with duckdb.connect(":memory:") as conn or with contextlib.closing(conn))
so the connection is closed automatically, and ensure the store is closed in a
finally block or after the context so resources are always released even on test
failures.
|
|
||
| async def test_connection_initialization(self): | ||
| """Test that store can be initialized with existing DuckDB connection.""" | ||
| import duckdb |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Move import to module level.
Importing inside a function is an anti-pattern that can affect readability and potentially cause issues with import caching.
Move the import to the top of the file with other imports:
import pytest
from duckdb import CatalogException, DuckDBPyConnection
+import duckdb
from inline_snapshot import snapshotThen remove line 179 from the test function.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In key-value/key-value-aio/tests/stores/duckdb/test_duckdb.py around line 179,
an inline "import duckdb" is present inside the test function; move that import
to the module-level imports at the top of the file alongside other imports and
then remove the import statement at line 179 from the test body so the module
imports are centralized and the test uses the already-imported duckdb.
| async def test_custom_table_name(self): | ||
| """Test that store can use custom table name.""" | ||
| custom_table = "my_custom_kv_table" | ||
| store = DuckDBStore(table_name=custom_table) | ||
|
|
||
| # Store some data | ||
| await store.put(collection="test", key="key1", value={"data": "value"}) | ||
|
|
||
| # Verify the custom table exists and contains the data | ||
| tables = ( | ||
| get_client_from_store(store) | ||
| .table(custom_table) | ||
| .filter(filter_expr="key = 'key1'") | ||
| .select("key", "collection") | ||
| .execute() | ||
| .fetchone() | ||
| ) | ||
|
|
||
| assert tables == snapshot(("key1", "test")) | ||
|
|
||
| # Verify default table doesn't exist | ||
| with pytest.raises(CatalogException): | ||
| get_client_from_store(store).table("kv_entries") | ||
|
|
||
| await store.close() | ||
|
|
There was a problem hiding this comment.
Use async context manager for resource safety.
The store is created directly and only closed at the end. If any assertion or exception occurs, the store won't be closed.
Apply this diff:
async def test_custom_table_name(self):
"""Test that store can use custom table name."""
custom_table = "my_custom_kv_table"
- store = DuckDBStore(table_name=custom_table)
-
- # Store some data
- await store.put(collection="test", key="key1", value={"data": "value"})
-
- # Verify the custom table exists and contains the data
- tables = (
- get_client_from_store(store)
- .table(custom_table)
- .filter(filter_expr="key = 'key1'")
- .select("key", "collection")
- .execute()
- .fetchone()
- )
-
- assert tables == snapshot(("key1", "test"))
-
- # Verify default table doesn't exist
- with pytest.raises(CatalogException):
- get_client_from_store(store).table("kv_entries")
-
- await store.close()
+ async with DuckDBStore(table_name=custom_table) as store:
+ # Store some data
+ await store.put(collection="test", key="key1", value={"data": "value"})
+
+ # Verify the custom table exists and contains the data
+ tables = (
+ get_client_from_store(store)
+ .table(custom_table)
+ .filter(filter_expr="key = 'key1'")
+ .select("key", "collection")
+ .execute()
+ .fetchone()
+ )
+
+ assert tables == snapshot(("key1", "test"))
+
+ # Verify default table doesn't exist
+ with pytest.raises(CatalogException):
+ get_client_from_store(store).table("kv_entries")🤖 Prompt for AI Agents
In key-value/key-value-aio/tests/stores/duckdb/test_duckdb.py around lines 190
to 215, wrap the DuckDBStore creation and usage in an async context manager to
ensure it is always closed; replace direct instantiation `store =
DuckDBStore(table_name=custom_table)` and the final `await store.close()` with
`async with DuckDBStore(table_name=custom_table) as store:` and indent the test
body accordingly so the store is automatically closed even if assertions fail.
| def test_connection_initialization(self): | ||
| """Test that store can be initialized with existing DuckDB connection.""" | ||
| import duckdb | ||
|
|
||
| conn = duckdb.connect(":memory:") | ||
| store = DuckDBStore(connection=conn) | ||
|
|
||
| store.put(collection="test", key="conn_test", value={"test": "value"}) | ||
| result = store.get(collection="test", key="conn_test") | ||
| assert result == {"test": "value"} | ||
|
|
||
| store.close() | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove redundant local import.
The duckdb module is already imported at the module level (line 9), so the local import on line 182 is unnecessary.
Apply this diff to remove the redundant import:
def test_connection_initialization(self):
"""Test that store can be initialized with existing DuckDB connection."""
- import duckdb
-
conn = duckdb.connect(":memory:")
store = DuckDBStore(connection=conn)🤖 Prompt for AI Agents
In key-value/key-value-sync/tests/code_gen/stores/duckdb/test_duckdb.py around
lines 180 to 192, there is a redundant local import of duckdb on line 182;
remove that local import statement and rely on the module-level import at the
top of the file (line 9) so the test uses the already-imported duckdb, leaving
the rest of the test unchanged.
Co-authored-by: William Easton <strawgate@users.noreply.github.com>
Test Failure AnalysisSummary: The workflow failed on a single job: Root Cause: The According to PyPI, Python 3.13 support was added in DuckDB 1.4.1 (released October 2025), which is why the Suggested Solution: Update the DuckDB version constraint to require at least version 1.4.1 for Python 3.13+ compatibility: # In key-value/key-value-aio/pyproject.toml
duckdb = [
"duckdb>=1.4.1 ; python_version >= '3.13'", # Python 3.13+ requires DuckDB 1.4.1+
"duckdb>=1.0.0 ; python_version < '3.13'"
]This follows the same pattern already used for other dependencies like After making this change:
Detailed AnalysisWorkflow Run Details
What Worked
What Failed
Build Failure DetailsThe compilation error occurs in DuckDB's C++ source code: This is a known incompatibility between older DuckDB versions and the stricter C++ compilation requirements in macOS toolchains used with Python 3.13. Why This MattersThe CI workflow runs
The "oldest dependencies" test is important because it verifies that the minimum version constraints in Related FilesFiles to Modify
Reference Files
|
|
Summary by CodeRabbit
New Features
Chores
Tests