Repository navigation
fix(text2sql): isolate missing-database errors in batch_compare_queries - #560
Closed
cyberspace-cs wants to merge 1 commit into
Closed
cyberspace-cs wants to merge 1 commit into
cyberspace-cs wants to merge 1 commit into
Conversation
…es (OpenDCAI#556) A single triple whose db_id is not registered previously made the whole batch return an error result, so every valid comparison (including rows whose database exists and whose SQL matches) failed too. Validate each triple individually, execute only those whose database exists, and place a per-row error result for missing databases while preserving input order. Empty batch still returns []. Adds cpu-only regression tests covering isolation, order independence, per-db error messages, the normal unequal path and the empty batch.
Contributor
Author
|
Closing this as a duplicate — I missed #557 when I opened it, and it fixes the same root cause (per-row isolation, original order preserved) with the same approach. Two PRs for one issue only splits review attention, so #557 is the one that should land. If the extra cases in my branch are useful (missing database in the first position, and a batch where every database is missing), I am happy to port them into #557 as a follow-up commit rather than keep this open. Sorry for the noise. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #556.
DatabaseManager.batch_compare_queriespreviously validated every distinctdb_idup front and, when any one was missing, returned an error result for all triples:Because
SQLExecutionClassifiersubmits the whole dataframe in one call, a single row pointing at a missing database made every other comparison fail with"Predicted SQL failed; Ground truth SQL failed", corrupting the difficulty labels for the entire dataset — even rows whose database exists and whose SQL matches.Fix
batch_execute_queriescall, as before).[]; existing equal / unequal comparison behavior is unchanged.Behavior
For the reproducer in #556:
(concert, same sql, same sql)False, Database 'missing_db' not foundTrue, [](missing_db, ...)False, Database 'missing_db' not foundTests
Added
test/cpu_only/test_batch_compare_queries_missing_db.py(cpu-only, SQLite, no model/network):[]pytest -m cpu test/cpu_only/test_batch_compare_queries_missing_db.py→ 5 passed.