-
Notifications
You must be signed in to change notification settings - Fork 58
feat: support matching sqlstate with [statement|query] error (XX000)
#269
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Bugen Zhao <[email protected]>
- Introduced `SqlState` variant in `ExpectedError` enum for SQL state code matching. - Enhanced `is_match` method to include SQL state comparison. - Updated `FakeDBError` struct to store SQL state information. - Added comprehensive tests for SQL state error handling in `error_sqlstate.slt` and `error_sqlstate_parsing.slt`. Signed-off-by: Bugen Zhao <[email protected]>
Signed-off-by: Bugen Zhao <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request enhances the sqllogictest framework by introducing support for SQL state codes in error handling and matching. Key changes involve updates to error structures (e.g., adding an optional sqlstate field), modifications to parser and runner logic for SQL state extraction and matching, and expanded test coverage via new and updated test files.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
tests/slt/error_sqlstate_parsing.slt | New tests for error parsing with SQL state codes |
tests/slt/error_sqlstate.slt | Expanded test cases verifying SQL state matching |
tests/harness.rs | Updated FakeDBError struct and error generation |
sqllogictest/src/runner.rs | Runner now extracts SQL state for error messages |
sqllogictest/src/parser.rs | Parser now supports inline SQL state patterns |
sqllogictest-engines/src/postgres/simple.rs | Added error_sql_state method for Postgres (Simple) |
sqllogictest-engines/src/postgres/extended.rs | Added error_sql_state method for Postgres Extended |
sqllogictest-engines/src/mysql.rs | Added error_sql_state method for MySQL |
sqllogictest-bin/src/engines.rs | Updated EnginesError structure and error mapping |
Comments suppressed due to low confidence (1)
sqllogictest/src/runner.rs:292
- The error format string in TestErrorKind::ErrorMismatch concatenates the SQL state using an empty placeholder, which can be confusing. Consider refactoring the formatting to make the concatenation explicit and improve clarity.
)]
// Order matters: more specific patterns should come first | ||
if sql.contains("non_existent_column") || sql.contains("missing_column") { | ||
return Err(FakeDBError::with_sql_state( | ||
"column \"missing_column\" does not exist".to_string(), | ||
"42703".to_string(), | ||
)); | ||
} | ||
if sql.contains("FORM") || sql.contains("SELEKT") || sql.contains("INVALID SYNTAX") { | ||
return Err(FakeDBError::with_sql_state( | ||
"syntax error at or near \"FORM\"".to_string(), | ||
"42601".to_string(), | ||
)); | ||
} | ||
if sql.contains("1/0") || sql.contains("/0") { | ||
return Err(FakeDBError::with_sql_state( | ||
"division by zero".to_string(), | ||
"22012".to_string(), | ||
)); | ||
} | ||
if sql.contains("duplicate") || sql.contains("UNIQUE") || sql.contains("violation") { | ||
return Err(FakeDBError::with_sql_state( | ||
"duplicate key value violates unique constraint".to_string(), | ||
"23505".to_string(), | ||
)); | ||
} | ||
if sql.contains("non_existent_table") | ||
|| sql.contains("missing_table") | ||
|| sql.contains("table_that_does_not_exist") | ||
|| sql.contains("final_missing_table") | ||
|| sql.contains("query_missing_table") | ||
|| sql.contains("another_missing_table") | ||
|| sql.contains("yet_another_missing_table") | ||
|| sql.contains("definitely_missing_table") | ||
|| sql.contains("postgres_missing_table") | ||
|| sql.contains("some_missing_table") | ||
{ | ||
return Err(FakeDBError::with_sql_state( | ||
"relation \"missing_table\" does not exist".to_string(), | ||
"42P01".to_string(), | ||
)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The chain of conditional checks for generating FakeDBError with SQL state is quite long. Refactoring these checks into a separate function or using a mapping of error patterns to SQL state codes could enhance readability and maintainability.
// Order matters: more specific patterns should come first | |
if sql.contains("non_existent_column") || sql.contains("missing_column") { | |
return Err(FakeDBError::with_sql_state( | |
"column \"missing_column\" does not exist".to_string(), | |
"42703".to_string(), | |
)); | |
} | |
if sql.contains("FORM") || sql.contains("SELEKT") || sql.contains("INVALID SYNTAX") { | |
return Err(FakeDBError::with_sql_state( | |
"syntax error at or near \"FORM\"".to_string(), | |
"42601".to_string(), | |
)); | |
} | |
if sql.contains("1/0") || sql.contains("/0") { | |
return Err(FakeDBError::with_sql_state( | |
"division by zero".to_string(), | |
"22012".to_string(), | |
)); | |
} | |
if sql.contains("duplicate") || sql.contains("UNIQUE") || sql.contains("violation") { | |
return Err(FakeDBError::with_sql_state( | |
"duplicate key value violates unique constraint".to_string(), | |
"23505".to_string(), | |
)); | |
} | |
if sql.contains("non_existent_table") | |
|| sql.contains("missing_table") | |
|| sql.contains("table_that_does_not_exist") | |
|| sql.contains("final_missing_table") | |
|| sql.contains("query_missing_table") | |
|| sql.contains("another_missing_table") | |
|| sql.contains("yet_another_missing_table") | |
|| sql.contains("definitely_missing_table") | |
|| sql.contains("postgres_missing_table") | |
|| sql.contains("some_missing_table") | |
{ | |
return Err(FakeDBError::with_sql_state( | |
"relation \"missing_table\" does not exist".to_string(), | |
"42P01".to_string(), | |
)); | |
} | |
for (patterns, message, sql_state) in get_error_mapping() { | |
if patterns.iter().any(|pattern| sql.contains(pattern)) { | |
return Err(FakeDBError::with_sql_state(message.to_string(), sql_state.to_string())); | |
} | |
} |
Copilot uses AI. Check for mistakes.
Self::new_inline(tokens.join(" ")) | ||
let joined = tokens.join(" "); | ||
|
||
// Check if this is a sqlstate error pattern: error(sqlstate) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can be slightly more rigorous here: sqlstate is 5 digit number
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Please add some comment and doc (in README) for the feature and indicate SQLSTATE are five-character error codes that follow the SQL standard's conventions
This pull request introduces support for SQL state codes in error handling across the
sqllogictest
framework. The changes enhance error matching capabilities by incorporating SQL state codes, enabling more precise validation of expected errors. The updates span multiple modules, including error struct modifications, trait updates, and test enhancements.Enhancements to Error Handling:
EnginesError
struct insqllogictest-bin/src/engines.rs
to include an optionalsqlstate
field and added awithout_state
constructor for errors without SQL state codes. This change replaces the previous error mapping logic withEnginesError::without_state
for MySQL, Postgres, and other engines. [1] [2]error_sql_state
methods in theAsyncDB
trait and its implementations for MySQL, Postgres (Simple and Extended), and other engines to extract SQL state codes from errors. [1] [2] [3] [4]SQL State Matching in Test Runner:
ExpectedError
enum insqllogictest/src/parser.rs
to support a newSqlState
variant for matching errors based on SQL state codes. Updated parsing, formatting, and matching logic to handle this new variant. [1] [2] [3]sqllogictest/src/runner.rs
) to utilize SQL state codes during error validation. Updated theTestErrorKind::ErrorMismatch
variant to include the actual SQL state when applicable. [1] [2] [3]Test Enhancements:
FakeDB
implementation intests/harness.rs
to generate errors with specific SQL state codes for various scenarios, such as syntax errors or missing columns. Added anerror_sql_state
method to retrieve SQL state codes fromFakeDBError
. [1] [2]These changes improve the framework's ability to test database behavior with greater accuracy and flexibility by leveraging SQL state codes for error validation.