Text to SQL RFT example #324
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| patterns = [ | ||
| r"(?:FROM|JOIN)\\s+([a-zA-Z_][a-zA-Z0-9_]*)", | ||
| r'(?:FROM|JOIN)\\s+"([^"]+)"', | ||
| r"(?:FROM|JOIN)\\s+`([^`]+)`", |
There was a problem hiding this comment.
Bug: Regex Escaping Flaw Breaks SQL Parsing
The regex patterns use double-escaped backslashes (\\s, \\*) which will match literal backslash characters followed by s or *, not whitespace or comment delimiters. The patterns should use single backslashes (\s, \*) to properly match SQL whitespace and block comments. This causes extract_tables to fail at extracting table names from SQL queries.
| urllib.request.urlretrieve(url, path) | ||
| print(f"Downloaded: {path}") | ||
| # df = pd.read_csv(path, header=None, names=COLUMN_NAMES[name], na_values=["\\N"]) | ||
| con.execute(f'CREATE OR REPLACE TABLE "{name}" AS SELECT * FROM df') |
There was a problem hiding this comment.
Bug: Undefined Variable Blocks Critical Process
The code references variable df when creating tables, but df is never defined because the pd.read_csv line is commented out. This causes a NameError when the script runs, preventing the database tables from being created.
| return 0 | ||
| ascii_table = ev["result"]["content"][0]["text"] | ||
| pred = parse_duckdb_ascii(ascii_table) | ||
| return 1 if are_equal(pred, ground_truth) else 0 |
There was a problem hiding this comment.
Bug: Unhandled HTTP errors crash evaluation function.
The run_eval function doesn't handle exceptions from the HTTP request, so if requests.post raises an exception (network error, timeout, etc.), the function crashes instead of returning 0. The exception handling only covers the comparison logic, not the MCP server communication.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try: | ||
| gt_vals = sorted([sorted(map(norm, r.values())) for r in ground_truth]) | ||
| pr_vals = sorted([sorted(map(norm, r.values())) for r in pred]) | ||
| ok = gt_vals == pr_vals | ||
| return {"score": 1 if ok else 0, "reason": "match" if ok else f"mismatch: gt={ground_truth} pred={pred}"} |
There was a problem hiding this comment.
The evaluator normalizes both ground_truth and pred by sorting only the value lists (sorted(map(norm, r.values()))). This drops the association between column names and values, so a prediction that returns the right set of values but assigns them to the wrong columns (e.g., swapping origin and destination) will be marked as a perfect match. That produces false positives and hides invalid SQL generations. Consider normalizing on keyed tuples (e.g., sorting each row by column name and comparing dicts) so column/value alignment is preserved.
Useful? React with 👍 / 👎.
benjibc
force-pushed
the
text_to_sql_example
branch
from
2dbf46d to
6026e78
Compare
| ) | ||
| content: Optional[ | ||
| Union[str, List[Union[ChatCompletionContentPartTextParam, ChatCompletionContentPartImageParam]]] | ||
| ] = Field(default="", description="The content of the message.") |
There was a problem hiding this comment.
Bug: Image content silently dropped by text extraction
The content field now accepts ChatCompletionContentPartImageParam, but existing code throughout the codebase (like _coerce_content_to_str functions in benchmark tests) only handles ChatCompletionContentPartTextParam by accessing the text attribute. When image parts are present, they're silently skipped since they have image_url instead of text, causing data loss without any error indication.
Note
Extends
Message.contentto accept image content parts alongside text.Written by Cursor Bugbot for commit 6026e78. This will update automatically on new commits. Configure here.