feat(wren-ai-service): Add invalid SQL tracking to AskResultResponse #1356
Conversation
paopa
added
module/ai-service
WalkthroughThe changes update the SQL handling in the ask service by introducing a new optional attribute, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AskService
participant SQLGenerator
Client->>AskService: Call ask() method
AskService->>SQLGenerator: Request SQL generation
SQLGenerator-->>AskService: Return failed run result (invalid SQL & error message)
AskService->>AskService: Extract invalid_sql & error_message
AskService-->>Client: Respond with AskResultResponse (includes invalid_sql)
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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 (1)
wren-ai-service/src/web/v1/services/ask.py (1)
469-471: Consider extracting this logic to a helper function.This extraction logic is duplicated in multiple places in the code (here and in lines 473-475). Consider creating a helper function to avoid repetition.
+def _extract_invalid_sql_info(failed_result): + return failed_result["sql"], failed_result["error"] + -invalid = failed_dry_run_results[0] -invalid_sql = invalid["sql"] -error_message = invalid["error"] +invalid_sql, error_message = _extract_invalid_sql_info(failed_dry_run_results[0])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wren-ai-service/src/web/v1/services/ask.py(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: pytest
- GitHub Check: pytest
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
wren-ai-service/src/web/v1/services/ask.py (7)
99-99: Excellent addition for debugging!Adding the
invalid_sqlfield to theAskResultResponseclass will significantly improve error reporting and debugging capabilities. This allows clients to access the problematic SQL statement directly when queries fail, making troubleshooting more efficient.
376-377: Good practice to initialize variables early.Explicitly initializing these variables at the beginning of the code block improves readability and prevents potential issues with undefined variables later in the function.
473-475: Duplicate extraction logic.This is the same extraction logic used earlier in the code. Using the proposed helper function would also apply here.
504-504: Good implementation of the invalid_sql field.Properly passing the captured invalid SQL to the response ensures clients receive the detailed error information. This enhances the debugging experience for developers working with the API.
643-647: Improved code formatting.Using parentheses for multiline assignments makes the code more readable and consistent with modern Python style.
716-720: Consistent formatting approach.Applying the same formatting style throughout the codebase improves maintainability. This change aligns with the previous formatting improvements.
376-505:Details
❓ Verification inconclusive
Verify error handling consistency.
The improvements for tracking invalid SQL are only implemented for the
AskResultResponsebut not forAskFeedbackResultResponse. Consider whether similar tracking of invalid SQL should be added to the feedback flow for consistency.
🏁 Script executed:
#!/bin/bash # Verify if there are instances where AskFeedbackResultResponse might benefit from having invalid_sql field # Check for failure cases in the feedback flow rg -A 5 "invalid_generation_results.*feedback" wren-ai-service/src/web/v1/services/ask.pyLength of output: 91
Consistently track SQL errors across response flows.
The current implementation tracks invalid SQL in the main response via the
AskResultResponse, but our verification indicates that no similar handling is applied to the feedback flow (i.e. withinAskFeedbackResultResponse). Please review the feedback pipeline to determine if incorporating aninvalid_sql(and related error data) field there could improve error diagnostics and overall consistency.
paopa
force-pushed
the
feat/return-invalid-sql-if-correcting-fails
branch
from
4404ffc to
3fb0601
Compare
This PR adds tracking of invalid SQL queries in the AskResultResponse model and updates the error handling logic to capture and return invalid SQL statements.
Changes:
invalid_sqlfield to AskResultResponse modelSummary by CodeRabbit
New Features
Style