sqlite3: Support 'size' keyword argument in Cursor::fetchmany
#6118
Conversation
WalkthroughCursor.fetchmany in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor PyCode as Python Code
participant Cursor as Cursor.fetchmany
participant Args as FetchManyArgs parser
participant DB as SQLite Cursor::next
PyCode->>Cursor: fetchmany(size=?)
Cursor->>Args: parse into FetchManyArgs (optional size)
Args-->>Cursor: size or None
alt parse error
Cursor-->>PyCode: raise TypeError/ValueError
else valid
Cursor->>DB: Cursor::next repeatedly
loop until rows exhausted or (max_rows > 0 and count == max_rows)
DB-->>Cursor: next row
Cursor-->>PyCode: accumulate rows
end
Cursor-->>PyCode: return list of rows
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (11)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
stdlib/src/sqlite.rs
Outdated
| fn fetchmany( | ||
| zelf: &Py<Self>, | ||
| max_rows: OptionalArg<c_int>, | ||
| mut args: FuncArgs, |
There was a problem hiding this comment.
Can #[derive(FromArgs)] be applicable? Or is there any blocker not allowing to do it?
There was a problem hiding this comment.
We can change it. However, I believe we can't use the FromArgs derive macro because the error messages would be different.
--- Testing with too many positional arguments ---
-fetchmany() takes from 0 to 1 positional arguments but 2 were given (cpython)
+expected at most 2 arguments, got 3 (FromArgs)
--- Testing with an unexpected keyword argument ---
-fetchmany() got an unexpected keyword argument 'unexpected_kw' (cpython)
+Unexpected keyword argument unexpected_kw (FromArgs)
--- Testing with both positional and keyword for the same argument ---
-fetchmany() got multiple values for argument 'size' (cpython)
+Unexpected keyword argument size (FromArgs)There was a problem hiding this comment.
If you think the current error messages are acceptable, I believe we can apply this version as is.
There was a problem hiding this comment.
Yeah, those error messages are fine - there's discrepancy in so many places, that if we wanted to make the error messages line up we'd just change FromArgs all at once.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
stdlib/src/sqlite.rs (2)
1700-1705: Quote unexpected keyword name to match CPython error text.CPython formats this as "got an unexpected keyword argument 'name'". Minor UX polish and closer compatibility.
Apply this diff:
- if let Some((name_str, _)) = args.kwargs.into_iter().next() { - return Err(vm.new_type_error(format!( - "fetchmany() got an unexpected keyword argument {name_str}", - ))); - } + if let Some((name, _)) = args.kwargs.into_iter().next() { + return Err(vm.new_type_error(format!( + "fetchmany() got an unexpected keyword argument '{}'", + name.as_str(), + ))); + }
1706-1715: Semantics match CPython for size <= 0; optional micro-optimizations.
- The “only enforce when max_rows > 0” behavior aligns with CPython’s implementation, where a non-positive size results in unbounded fetching (counter break check never triggers). (chromium.googlesource.com)
- Micro: preallocate when max_rows > 0 to reduce reallocations.
Apply this diff:
- let mut list = vec![]; + let mut list = if max_rows > 0 { + Vec::with_capacity(max_rows as usize) + } else { + Vec::new() + };No change in behavior; marginal perf improvement when many rows are fetched.
Also applies to: 1720-1720
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Lib/test/test_sqlite3/test_dbapi.pyis excluded by!Lib/**
📒 Files selected for processing (1)
stdlib/src/sqlite.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style (cargo fmtto format)
Always run clippy to lint code (cargo clippy) before completing tasks. Fix any warnings or lints that are introduced by your changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
stdlib/src/sqlite.rs
⏰ 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). (11)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run tests under miri
🔇 Additional comments (1)
stdlib/src/sqlite.rs (1)
1685-1689: Good change: accept keyword argsizeviaFuncArgs.The manual parse cleanly supports both positional and keyword forms and keeps the default from
arraysizewhen omitted. Looks consistent with the rest of the module’s argument handling.
Summary by CodeRabbit