Add read-only negative-case comparator suites (PR 3/7)#1526
Merged
Conversation
Adds the first batch of NEGATIVE_* suites — error-provoking inputs the positive suites never run — comparing each endpoint's error behavior (class, SQLState, vendor code, message) via the PR 1/2 engine under the shadow default. All five are read-only and safe on the shared connections. - NEGATIVE_STATEMENT_SELECT: missing table/column, syntax error, 1/0, runtime cast failure, executeQuery on an INSERT (wrong method) - NEGATIVE_STATEMENT_OTHER: SHOW/DESCRIBE/EXPLAIN/SET on bad targets, getMoreResults() after consumption, getUpdateCount() before execute() - NEGATIVE_PARAM_BINDING: out-of-range setInt index, too-few params, type mismatch at execute, setBigDecimal over precision/scale - NEGATIVE_PREPARED_METADATA: clearParameters then unbound execute, getMetaData() before execute on invalid SQL / missing table / column - NEGATIVE_TYPE_CONVERSION: getInt/getByte overflow, non-numeric getInt, getObject(col, Integer.class) on VARCHAR, getInt on ARRAY, getBigDecimal on STRUCT Each provider captures result-or-throwable per side and folds through the existing ResultSetComparator / Captures.compareCall path. Registered in the TestSuite enum (auto-included under DEFAULT_PARAMS via allExcept). README suite table updated. Also formats config/Endpoint.java, which was committed to comparator-v2 in a non-Spotless-clean state and reformats on every build; folding the fix in here stops the phantom diff from recurring. No behavior change in that file. Validated live against the peco serverless warehouse (Thrift-vs-SEA, shadow mode): 27/27 cases executed, no harness errors. Observed divergences are message-wording only (same class/SQLState/server-code), which message normalization in PR 7 will address; plus one genuine one-sided finding (getMoreResults returns Boolean vs a STATEMENT_CLOSED exception). NO_CHANGELOG=true (test-only comparator tooling). Co-authored-by: Isaac Signed-off-by: Sreekanth Vadigi <sreekanth.vadigi@databricks.com>
sreekanth-db
added a commit
that referenced
this pull request
Jul 3, 2026
…1527) ## What PR 4 of the error / negative-case comparison series (builds on #1524, #1525, #1526). Adds the error-provoking DDL, DML, and batch suites — each compares the two endpoints' **error behavior** (class, SQLState, vendor code, message) via the gate-aware `ErrorDiffs` bridge, under the `shadow` default. | Suite | Cases | |---|---| | `NEGATIVE_STATEMENT_DDL` | DROP/CREATE/ALTER on missing or duplicate objects (± `IF EXISTS`), missing schema/catalog, malformed syntax | | `NEGATIVE_STATEMENT_DML` | type-mismatch INSERT, NOT NULL violation, DML on a missing table, `executeUpdate` on a SELECT (wrong method), write overflow | | `NEGATIVE_STATEMENT_BATCH` | `executeBatch` partial/full failure and a ResultSet-producing command; when both sides throw `BatchUpdateException`, also compares per-element update counts | ## Isolation These suites mutate catalog objects, so each runs in **its own namespace** under `comparator_ddl_tests` (seeded fresh, dropped in a `finally`), keeping them safe on the shared connections. ## Included fix: NPE in the shared `ErrorDiffs.compare` The neither-threw case is now guarded **centrally** (returns empty, mirroring the deep path's `NOT_APPLICABLE`) before reaching `legacyClassOnly`, which dereferences the null `error()` of a RETURNED outcome. The new suites call `ErrorDiffs.compare` unconditionally, so an `off`-mode both-succeed case (e.g. `DROP TABLE IF EXISTS`) would otherwise NPE and abort. This touches the shared helper introduced in #1525; added a regression test. (Prior callers were unaffected because they guarded with `if threw`.) ## Live validation Ran against the peco serverless warehouse (Thrift-vs-SEA, `ERROR_COMPARISON_MODE=shadow`) **twice** — 21/21 cases, identical results both runs. Critically, positive `STATEMENT_DDL` / `STATEMENT_DML` run in the **same JVM after** the destructive negatives and still **PASS** — confirming no shared-connection poisoning. Observed divergences are the expected Thrift-Hive-wrapper-vs-SEA-direct-text kind (same server error codes: `CAST_INVALID_INPUT`, `SCHEMA_NOT_FOUND`, `TABLE_OR_VIEW_NOT_FOUND`, etc.), which message normalization in PR 7 will address. ## Testing `ErrorComparatorTest` — 18/18 (added the neither-threw guard regression test); two consistent live shadow runs; clean compile; Isaac Review (`/review --uncommitted`) — 0 findings after the NPE fix. NO_CHANGELOG=true (test-only comparator tooling, no driver behavior change). This pull request and its description were written by Isaac. Signed-off-by: Sreekanth Vadigi <sreekanth.vadigi@databricks.com>
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
1 participant
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.
What
PR 3 of the error / negative-case comparison series (builds on #1524, #1525). Adds the first batch of
NEGATIVE_*suites — error-provoking inputs the positive suites never run — comparing each endpoint's error behavior (exception class, SQLState, vendor code, message) via the engine from PR 1/2, under theshadowdefault. All five are read-only and safe on the shared connections.NEGATIVE_STATEMENT_SELECTSELETsyntax error,1/0,CAST('x' AS INT),executeQueryon an INSERT (wrong method)NEGATIVE_STATEMENT_OTHERgetMoreResults()after consumption,getUpdateCount()beforeexecute()NEGATIVE_PARAM_BINDINGsetIntindex, too-few params, type mismatch at execute,setBigDecimalover precision/scaleNEGATIVE_PREPARED_METADATAclearParametersthen unbound execute;getMetaData()before execute on invalid SQL / missing table / missing columnNEGATIVE_TYPE_CONVERSIONgetInt/getByteoverflow, non-numericgetInt,getObject(col, Integer.class)on VARCHAR,getInton ARRAY,getBigDecimalon STRUCTEach provider captures result-or-throwable per side and folds through the existing
ResultSetComparator/Captures.compareCallpath. Registered in theTestSuiteenum (auto-included underDEFAULT_PARAMSviaallExcept). README suite table updated.Note on
Endpoint.javaThis PR also Spotless-formats
config/Endpoint.java, which was committed tocomparator-v2in a non-Spotless-clean state and reformats on every build (a phantom diff for anyone runningmvn spotless:apply). Folding the fix in here stops it recurring. No behavior change in that file — formatting only.Live validation
Ran against the peco serverless warehouse (Thrift-vs-SEA,
ERROR_COMPARISON_MODE=shadow) via a narrowed local runner: 27/27 cases executed, no harness errors. Observed divergences are message-wording only (same class / SQLState / server error code; Thrift's Hive-wrapper prose vs SEA's direct text) — exactly what message normalization in PR 7 will address — plus one genuine one-sided finding:getMoreResults()returnsBooleanon one side and throws aSTATEMENT_CLOSEDexception on the other.Testing
Live shadow run (above); clean compile; Isaac Review (
/review --uncommitted) — 0 findings (all raised comments filtered as not-real, confirming the read-only / shared-connection design).NO_CHANGELOG=true (test-only comparator tooling, no driver behavior change).
This pull request and its description were written by Isaac.