fix(wren-ai-service): Validate the SQL generated for recommended ques… #1937
Conversation
✨ 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
wren-ai-service/src/web/v1/services/question_recommendation.py (3)
34-35: Validate and clamp retry countPrevent negative or non-int values from misconfigurations.
Apply:
- max_sql_correction_retries: int = 3, + max_sql_correction_retries: int = 3,And assign (see Line 43):
- self._max_sql_correction_retries = max_sql_correction_retries + # Clamp and coerce + self._max_sql_correction_retries = max(0, int(max_sql_correction_retries))
43-43: Use clamped value for internal state — see suggested diff aboveStores the validated count; no other changes needed.
132-167: Refactor SQL correction loop in question_recommendation.py for readability and parity
- Replace the
while … elsewith afor attempt in range(1, self._max_sql_correction_retries + 1):loop and emit alogger.debug("Request %s: SQL correction attempt %d/%d", request_id, attempt, self._max_sql_correction_retries)before each retry.- Include the same dry-plan flags used in the ask flow—add
use_dry_plan=ask_request.use_dry_planandallow_dry_plan_fallback=ask_request.allow_dry_plan_fallbackto thesql_correction.run(…)invocation.
📜 Review details
Configuration used: CodeRabbit UI
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 selected for processing (2)
wren-ai-service/src/globals.py(1 hunks)wren-ai-service/src/web/v1/services/question_recommendation.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
wren-ai-service/src/web/v1/services/question_recommendation.py (2)
wren-ai-service/src/pipelines/generation/sql_correction.py (2)
post_process(97-111)run(146-174)wren-ai-service/src/pipelines/generation/utils/sql.py (1)
run(29-69)
🔇 Additional comments (4)
wren-ai-service/src/globals.py (3)
223-223: Addition of sql_correction to QuestionRecommendation pipelines — LGTMThis unblocks the correction loop in QuestionRecommendation.
226-226: Settings.max_sql_correction_retries defined and documented
Settings.max_sql_correction_retriesis declared with a default of 3 insrc/config.pyand is included in all example configs and docs.
216-218: Remove invalid fallback suggestion
The proposed fallback topipe_components["sql_generation"]is incorrect—nosql_generationcomponent is defined in any of the configuration files (they only declarequestion_recommendationandquestion_recommendation_sql_generation) so that lookup would itself raise a KeyError.Likely an incorrect or invalid review comment.
wren-ai-service/src/web/v1/services/question_recommendation.py (1)
169-170: Valid-SQL assignment path — LGTMHappy path uses the initial valid result; consistent with the new correction path.
|
@JakeCowton thanks for contribution, I'll take a look in a few days |
Currently the SQL generated for recommended questions does not check if the SQL is valid.
This PR runs a configurable number of retries to fix any invalid SQL, bringing it inline with how SQL for regular asks are generated.
Summary by CodeRabbit