Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdded a new helper 🚥 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
AND d.didIN :data_idsclause is unlikely to work as intended withtext()and a list parameter; consider using an expanding bindparam (e.g.,bindparam('data_ids', expanding=True)) or explicitly generating theIN (...)list to ensure the IDs are bound correctly. - Instead of suppressing C901 on
list_datasets, consider pushing more of the condition-building into separate helper functions (e.g., one for visibility/user logic, one for ID/name/version filters) to keep this endpoint’s core query assembly simpler and easier to maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `AND d.`did` IN :data_ids` clause is unlikely to work as intended with `text()` and a list parameter; consider using an expanding bindparam (e.g., `bindparam('data_ids', expanding=True)`) or explicitly generating the `IN (...)` list to ensure the IDs are bound correctly.
- Instead of suppressing C901 on `list_datasets`, consider pushing more of the condition-building into separate helper functions (e.g., one for visibility/user logic, one for ID/name/version filters) to keep this endpoint’s core query assembly simpler and easier to maintain.
## Individual Comments
### Comment 1
<location path="src/routers/openml/datasets.py" line_range="162-163" />
<code_context>
+ clauses.append("AND `version`=:data_version")
+ parameters["data_version"] = data_version
+
+ if data_id:
+ clauses.append("AND d.`did` IN :data_ids")
+ parameters["data_ids"] = data_id
</code_context>
<issue_to_address>
**issue (bug_risk):** Binding a list to `IN :data_ids` with `text()` is unlikely to work as intended
With `sqlalchemy.text()`, binding a Python list to `IN :data_ids` won’t automatically expand to `IN (..., ...)`; most dialects treat it as a single value (the list repr), causing errors or empty results when `data_id` is set.
You could instead:
- Use an expanding bindparam (e.g. `bindparam("data_ids", expanding=True)` with `d."did" IN :data_ids`), or
- Dynamically build `IN (:id_0, :id_1, ...)` with separate params, or
- Use SQLAlchemy Core/ORM with `in_()` so expansion is handled for you.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #286 +/- ##
=======================================
Coverage ? 53.50%
=======================================
Files ? 34
Lines ? 1497
Branches ? 130
=======================================
Hits ? 801
Misses ? 694
Partials ? 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/routers/openml/datasets.py`:
- Around line 162-164: The IN-clause handling for data_id is incorrect: change
the truthy check to "data_id is not None" so empty lists are not skipped, and
bind the parameter for the "AND d.`did` IN :data_ids" clause using
SQLAlchemy.bindparam with expanding=True (i.e., set parameters["data_ids"] via
bindparam("data_ids", value=data_id, expanding=True)) so the list expands
correctly at execution; update the code that builds clauses and parameters (look
for variables named data_id, clauses, parameters and the IN :data_ids fragment)
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3fda056e-e7b8-48b5-b810-7b238d7e40bf
📒 Files selected for processing (1)
src/routers/openml/datasets.py
1 participant
Refactors the list dataset function to make it easier to read and check that input parameters are provided correctly.