Repository navigation
Add dict-like behavior to TableProxy for proper equality comparison - #200
Conversation
- Add __eq__ method to DictSQLite and TableProxy for dict equality comparison - Add table() method to DictSQLite for getting TableProxy instances - Add keys(), values(), items(), __len__ methods to TableProxy - Fix __iter__ in TableProxy to yield only keys (standard dict behavior) - Fix test_version2_multi_tables to use items() instead of iter() - Add comprehensive dict-like behavior tests Co-authored-by: harumaki4649 <83683593+harumaki4649@users.noreply.github.com>
…uality - Extract _deserialize_value helper method to avoid code duplication - Optimize values() and items() methods to use single DB query - Add documentation notes about return types differing from dict views - Fix test to use db.table() instead of direct TableProxy instantiation Co-authored-by: harumaki4649 <83683593+harumaki4649@users.noreply.github.com>
Co-authored-by: harumaki4649 <83683593+harumaki4649@users.noreply.github.com>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bandit found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
- Replace `not (x == y)` with `x != y` to remove superfluous parens
- Add pylint disable comments for intentional `db == {}` equality tests
Co-authored-by: harumaki4649 <83683593+harumaki4649@users.noreply.github.com>
Fixed in 0e64f3d:
|
選択肢 A（現状維持: current table のみを比較） が適切です。 理由:
ドキュメントでの明確化については、必要であれば追加できますが、現在の動作は既存APIの一貫性を保っています。 |
1) Add nosec B608 comments for SQL queries using _quote_ident() - These queries are safe because table names are properly quoted 3) Change items()/values() to return raw values instead of proxies - Matches standard dict behavior for equality comparison - Add memory usage warning in docstrings 4) Fix table() method to not modify self.table_name - Directly execute CREATE TABLE SQL without side effects - Eliminates potential race condition with operation_queue 5) Add memory usage documentation for items()/values() Co-authored-by: harumaki4649 <83683593+harumaki4649@users.noreply.github.com>
対応を完了しました (81cf935): 1) Bandit セキュリティ警告
3) items()/values() の返り値
4) table() の副作用排除
5) メモリ使用量のドキュメント
|
変更3のデメリット (items()/values() が生値を返す):
変更4のデメリット (table() が self.table_name を変更しない):
|
- Break long SQL lines (505, 513) to comply with 100-char limit - Consolidate docstrings to reduce module line count under 1000 Co-authored-by: harumaki4649 <83683593+harumaki4649@users.noreply.github.com>
いいえ、やり残したことはありません。すべての対応が完了しています：
PR はマージ可能な状態です。 |
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive dict-like behavior to the TableProxy class and DictSQLite to enable proper equality comparisons and standard dictionary operations. The changes address an issue where TableProxy instances couldn't be compared with regular Python dictionaries, breaking the module's core promise of being "usable like a regular dict."
Key Changes:
- Added
table()method to DictSQLite for creating/accessing TableProxy instances without modifying the current table - Implemented
__eq__methods in both TableProxy and DictSQLite for dict equality comparisons - Added
keys(),values(),items(), and__len__methods to TableProxy for complete dict-like interface - Fixed
__iter__in TableProxy to yield only keys (matching standard dict behavior) - Refactored value deserialization logic into
_deserialize_value()method to avoid code duplication - Added 21 comprehensive tests covering dict-like behavior scenarios
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| dictsqlite/main.py | Added table() method, __eq__ methods for both DictSQLite and TableProxy, dict-like methods (keys(), values(), items(), __len__) to TableProxy, refactored _deserialize_value(), fixed __iter__ to yield only keys, and added nosec comments for SQL queries |
| tests/test_dict_like_behavior.py | New comprehensive test suite with 21 tests covering dict-like behavior for DictSQLite and TableProxy, including equality comparisons, iteration, and the original issue scenario |
| tests/test_transactions_and_tables.py | Updated test to use items() method instead of directly iterating over TableProxy (aligning with standard dict behavior) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def __eq__(self, other): | ||
| """辞書との等価比較をサポート。""" | ||
| if isinstance(other, dict): | ||
| return dict(self.items()) == other | ||
| if isinstance(other, DictSQLite.TableProxy): | ||
| return dict(self.items()) == dict(other.items()) | ||
| return NotImplemented |
There was a problem hiding this comment.
Missing documentation: The __eq__ method lacks a docstring. While there's a brief inline comment, a proper docstring should document the behavior, including what types it can compare with (dict, TableProxy) and that it loads all table data into memory for comparison.
| def __iter__(self): | ||
| """辞書と同様に、キーのみをイテレートする。""" | ||
| for row in self.get_all_rows(): | ||
| yield row[0] | ||
|
|
||
| def __len__(self): | ||
| """テーブル内のエントリ数を返す。""" | ||
| result_queue = queue.Queue() | ||
| self.db.operation_queue.put(( | ||
| self.db._fetchone, # pylint: disable=protected-access | ||
| (f"SELECT COUNT(*) FROM {self.db._quote_ident(self.table_name)}",), # nosec B608 | ||
| {}, result_queue | ||
| )) | ||
| result = result_queue.get() | ||
| if isinstance(result, Exception): | ||
| raise result | ||
| return result[0] if result else 0 |
There was a problem hiding this comment.
Missing documentation: The __len__ and __iter__ methods lack docstrings. While __iter__ has a brief inline comment, proper docstrings would better document their behavior and any performance considerations (e.g., __iter__ streams data, while __len__ uses COUNT which is efficient).
| if table_name not in self.tables(): | ||
| schema = '(key TEXT PRIMARY KEY, value TEXT)' | ||
| create_table_sql = ( | ||
| "CREATE TABLE IF NOT EXISTS " | ||
| f"{self._quote_ident(table_name)} " | ||
| f"{schema}" | ||
| ) | ||
| self.operation_queue.put((self._execute, (create_table_sql,), {}, None)) | ||
| self.operation_queue.join() |
There was a problem hiding this comment.
Potential race condition: The check if table_name not in self.tables() followed by table creation is not atomic. If multiple threads call table() with the same name simultaneously, both could pass the check and attempt to create the table. While CREATE TABLE IF NOT EXISTS handles this gracefully, consider adding operation_queue.join() after checking self.tables() to ensure the check is synchronized, or simply always use CREATE TABLE IF NOT EXISTS without the conditional check for better thread safety.
| if table_name not in self.tables(): | |
| schema = '(key TEXT PRIMARY KEY, value TEXT)' | |
| create_table_sql = ( | |
| "CREATE TABLE IF NOT EXISTS " | |
| f"{self._quote_ident(table_name)} " | |
| f"{schema}" | |
| ) | |
| self.operation_queue.put((self._execute, (create_table_sql,), {}, None)) | |
| self.operation_queue.join() | |
| schema = '(key TEXT PRIMARY KEY, value TEXT)' | |
| create_table_sql = ( | |
| "CREATE TABLE IF NOT EXISTS " | |
| f"{self._quote_ident(table_name)} " | |
| f"{schema}" | |
| ) | |
| self.operation_queue.put((self._execute, (create_table_sql,), {}, None)) | |
| self.operation_queue.join() |
| """辞書との等価比較をサポート。""" | ||
| if isinstance(other, dict): | ||
| return dict(self.items()) == other | ||
| if isinstance(other, DictSQLite.TableProxy): | ||
| return dict(self.items()) == dict(other.items()) |
There was a problem hiding this comment.
Performance concern: The __eq__ method calls self.items() which retrieves all data from the table. For large tables, this could be very memory-intensive and slow. Consider documenting this performance characteristic in the docstring, or implementing a more efficient comparison that streams data instead of loading everything into memory.
| """辞書との等価比較をサポート。""" | |
| if isinstance(other, dict): | |
| return dict(self.items()) == other | |
| if isinstance(other, DictSQLite.TableProxy): | |
| return dict(self.items()) == dict(other.items()) | |
| """ | |
| 辞書または他のTableProxyとの等価比較をサポート。 | |
| 注意: この比較はテーブル全体を逐次的に走査するため、大規模テーブルでは時間がかかる場合があります。 | |
| ただし、全データを一度にメモリに読み込まず、1件ずつ比較するため、メモリ効率は向上しています。 | |
| """ | |
| # Compare with dict | |
| if isinstance(other, dict): | |
| if len(self) != len(other): | |
| return False | |
| for k in self: | |
| try: | |
| v = self[k] | |
| except KeyError: | |
| return False | |
| if k not in other or other[k] != v: | |
| return False | |
| # Check for extra keys in other | |
| for k in other: | |
| if k not in self: | |
| return False | |
| return True | |
| # Compare with another TableProxy | |
| if isinstance(other, DictSQLite.TableProxy): | |
| if len(self) != len(other): | |
| return False | |
| for k in self: | |
| try: | |
| v1 = self[k] | |
| v2 = other[k] | |
| except KeyError: | |
| return False | |
| if v1 != v2: | |
| return False | |
| # Check for extra keys in other | |
| for k in other: | |
| if k not in self: | |
| return False | |
| return True |
| def __eq__(self, other): | ||
| """辞書との等価比較をサポート。""" | ||
| if isinstance(other, dict): | ||
| proxy = self.TableProxy(self, self.table_name) | ||
| return dict(proxy.items()) == other | ||
| if isinstance(other, DictSQLite): | ||
| proxy1 = self.TableProxy(self, self.table_name) | ||
| proxy2 = other.TableProxy(other, other.table_name) | ||
| return dict(proxy1.items()) == dict(proxy2.items()) | ||
| return NotImplemented |
There was a problem hiding this comment.
Performance concern: Similar to TableProxy's __eq__, this method loads all table data into memory via proxy.items(). For large tables, this could cause memory issues. Consider adding a docstring warning about this behavior or implementing a more efficient streaming comparison.
| def __repr__(self): | ||
| return f"{dict(self)}" | ||
| return f"{dict(self.items())}" |
There was a problem hiding this comment.
Performance concern: The __repr__ method calls dict(self.items()) which loads all table data into memory. For large tables, calling repr() (e.g., in debugging or logging) could cause significant memory usage and performance issues. Consider truncating the output for large tables or adding a warning in the docstring.
| def __eq__(self, other): | ||
| """辞書との等価比較をサポート。""" | ||
| if isinstance(other, dict): | ||
| proxy = self.TableProxy(self, self.table_name) | ||
| return dict(proxy.items()) == other | ||
| if isinstance(other, DictSQLite): | ||
| proxy1 = self.TableProxy(self, self.table_name) | ||
| proxy2 = other.TableProxy(other, other.table_name) | ||
| return dict(proxy1.items()) == dict(proxy2.items()) | ||
| return NotImplemented |
There was a problem hiding this comment.
Missing documentation: The __eq__ method lacks a docstring explaining its behavior, particularly that it loads all table data into memory for comparison. This is important information for users working with large datasets. Consider adding a docstring that documents the memory implications and the types it can compare with (dict, DictSQLite).
| def keys(self): | ||
| """テーブル内の全キー一覧をリストで返す。""" | ||
| return list(self) | ||
|
|
||
| def values(self): | ||
| """テーブル内の全値一覧をリストで返す。大規模テーブルではメモリ注意。""" | ||
| result = [] | ||
| for row in self.get_all_rows(): | ||
| key = row[0] | ||
| key, raw_value_str = row[0], row[1] | ||
| try: | ||
| # __getitem__ を経由して正しいプロキシオブジェクトを取得 | ||
| yield key, self[key] | ||
| except (KeyError, json.JSONDecodeError): | ||
| # デコードできない値はそのまま返す | ||
| yield key, row[1] | ||
| raw_value = self._deserialize_value(raw_value_str, key) | ||
| result.append(raw_value) | ||
| except (json.JSONDecodeError, pickle.UnpicklingError, ValueError, TypeError): | ||
| result.append(raw_value_str) | ||
| return result | ||
|
|
||
| def items(self): | ||
| """テーブル内の全(key, value)ペアをリストで返す。大規模テーブルではメモリ注意。""" | ||
| result = [] | ||
| for row in self.get_all_rows(): | ||
| key, raw_value_str = row[0], row[1] | ||
| try: | ||
| raw_value = self._deserialize_value(raw_value_str, key) | ||
| result.append((key, raw_value)) | ||
| except (json.JSONDecodeError, pickle.UnpicklingError, ValueError, TypeError): | ||
| result.append((key, raw_value_str)) | ||
| return result |
There was a problem hiding this comment.
Missing documentation: The keys(), values(), and items() methods lack docstrings. While brief inline comments mention memory concerns, proper docstrings would better document the return types, behavior differences from standard dict (returns list instead of dict_keys/dict_values/dict_items views), and memory implications for large tables.
| db = DictSQLite(str(db_path)) | ||
| try: | ||
| # Testing __eq__ explicitly, not boolean truthiness | ||
| assert db == {} # pylint: disable=use-implicit-booleaness-not-comparison | ||
| finally: | ||
| db.close() |
There was a problem hiding this comment.
Instance of context-manager class DictSQLite is closed in a finally block. Consider using 'with' statement.
| db = DictSQLite(str(db_path)) | |
| try: | |
| # Testing __eq__ explicitly, not boolean truthiness | |
| assert db == {} # pylint: disable=use-implicit-booleaness-not-comparison | |
| finally: | |
| db.close() | |
| with DictSQLite(str(db_path)) as db: | |
| # Testing __eq__ explicitly, not boolean truthiness | |
| assert db == {} # pylint: disable=use-implicit-booleaness-not-comparison |
| assert table2 == {"tkey2": 'tvalue2'} | ||
|
|
||
| finally: | ||
| db.close() |
There was a problem hiding this comment.
Instance of context-manager class DictSQLite is closed in a finally block. Consider using 'with' statement.
GitHub Copilot レビューコメントの確認と対応提案1. ドキュメント不足の指摘 (comment_id: 2594620619, 2594620623, 2594620636, 2594620639)指摘内容: 対応提案:
2. 潜在的な競合状態 (comment_id: 2594620625)指摘内容: 対応提案:
3. パフォーマンスの懸念 (comment_id: 2594620627, 2594620630, 2594620632)指摘内容: 対応提案:
4. テストでの with 文使用 (comment_id: 2594620642, 2594620650)指摘内容: 対応提案: テストコードを items_iter()/values_iter() 提案への返答賛成です。以下の理由と実装計画を提案します： メリット
実装方針への同意点
懸念点と提案
結論本PRでは Phase 1 の対応のみ を行い、iterator 実装は別PRとして切り出すことを推奨します。 コード変更の承認をいただければ、上記 Phase 1 の修正を実施します。 |
Phase 1 changes: - Add docstrings to __eq__, __len__, __iter__, keys(), values(), items() - Document performance characteristics and memory implications - Fix race condition in table() by always using CREATE TABLE IF NOT EXISTS - Update tests to use with statements instead of try/finally Co-authored-by: harumaki4649 <83683593+harumaki4649@users.noreply.github.com>
Phase 1 完了 (c28b456): 実施した変更:
items_iter()/values_iter() の代替提案複雑な実装を避け、既存の 提案:
|
大規模データ処理の最適化提案現状の課題Python の dict は完全にメモリ内で動作するため高速ですが、DictSQLite は SQLite 経由のため I/O オーバーヘッドがあります。 推奨アプローチ（段階的）1. 短期（本PR範囲）: 現状維持 + ドキュメント
2. 中期: SQLite の最適化 # WAL モードでの並行読み取り改善
db = DictSQLite('test.db', journal_mode='WAL')
# バッチ操作の追加
def update_many(self, items):
"""複数キーを1トランザクションで更新"""3. 長期: Rust バインディング活用
本 PR での対応案現時点ではドキュメントでの推奨パターン明記が現実的です。Rust 最適化は別 issue/PR として切り出すことを推奨します。 本 PR をこのままマージして、パフォーマンス最適化は次のステップとして進めますか？ |
Python+Rust 設計での高速化オプション現在利用可能な最適化パラメータ（v4系）# 1. persist_mode="memory" - 完全インメモリモード
db = DictSQLiteV4(":memory:", persist_mode="memory")
# → SQLite I/O を完全に回避、Python dict に近い速度
# 2. hot_capacity - LRU キャッシュサイズ
db = DictSQLiteV4(path, hot_capacity=1_000_000)
# → 頻繁アクセスのキーをメモリに保持
# 3. buffer_size - 書き込みバッファ
db = DictSQLiteV4(path, buffer_size=500)
# → バッチ書き込みで I/O 削減Python dict 並みの速度を実現する方法1. 純粋インメモリモード（最速） db = DictSQLiteV4(":memory:", persist_mode="memory", hot_capacity=10_000_000)→ 永続化不要なら Python dict とほぼ同等 2. ハイブリッドモード（永続化 + 高速） db = DictSQLiteV4(path,
persist_mode="lazy", # 遅延書き込み
hot_capacity=1_000_000, # 100万件キャッシュ
buffer_size=1000 # 大きめバッファ
)3. Rust 側の追加最適化案（将来）
結論本 PR（v1系）は基本機能実装が目的です。高速化は v4 系の 本 PR をマージして進めますか？ |
将来の Issue 用まとめ## パフォーマンス最適化提案: DictSQLite の大規模データ処理改善
### 背景
- DictSQLite は SQLite 経由のため、純粋な Python dict より I/O オーバーヘッドがある
- 大規模テーブルでの `items()`/`values()` は全データをメモリにロードする
### 現在利用可能な最適化（v4系）
```python
# 1. persist_mode="memory" - 完全インメモリモード
db = DictSQLiteV4(":memory:", persist_mode="memory")
# 2. hot_capacity - LRU キャッシュサイズ
db = DictSQLiteV4(path, hot_capacity=1_000_000)
# 3. buffer_size - 書き込みバッファ
db = DictSQLiteV4(path, buffer_size=500)将来の最適化案1. ストリーミングイテレータ
2. Rust 側の最適化
3. バッチ操作 API def update_many(self, items): ...
def get_many(self, keys): ...推奨パターン（現状）# メモリ効率の良い逐次処理
for key in table:
process(key, table[key]) |
table()method to DictSQLite for getting TableProxy instances__eq__method to TableProxy for dict equality comparisons__eq__method to DictSQLite for dict equality comparisonskeys()method to TableProxyvalues()method to TableProxyitems()method to TableProxy__len__method to TableProxy__iter__in TableProxy to yield only keys (standard dict behavior)Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.