Add own-namespace negative comparator suites: DDL/DML/BATCH (PR 4/7)#1527
Merged
Merged
Conversation
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. - 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 the per-element update counts Isolation: these suites mutate catalog objects, so each runs in its own namespace under comparator_ddl_tests (seeded fresh and dropped in a finally), keeping them safe on the shared connections. Validated live against the peco serverless warehouse (Thrift-vs-SEA, shadow) across two runs: 21/21 cases, identical results, and positive STATEMENT_DDL/STATEMENT_DML run in the same JVM still PASS afterward (no shared-connection poisoning). Also fixes an 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. Added a regression test. Registered the three suites in the TestSuite enum (auto-included under DEFAULT_PARAMS via allExcept). README suite table updated. 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
…(PR 5a/8) (#1528) ## What Introduces the **fresh-connection mechanism** for negative suites whose cases mutate or destroy connection/session state, plus the first two such suites. Builds on #1524–#1527. > Splits the planned PR 5 into **5a** (this — mechanism + the two lower-risk suites) and **5b** (later — `NEGATIVE_CONNECTION`, `NEGATIVE_CANCEL_TIMEOUT`, `NEGATIVE_VOLUME`). The series is now 8 PRs. ### Mechanism - `config/ConnectionFactory.openFresh(side)` — a dedicated, **uncached** connection - `ConnectionManager.openUncached(url)` — `DriverManager.getConnection`, not cached - `SuiteProvider`: backward-compatible **default 5-arg** `execute(conn1, conn2, ConnectionFactory, tc, label)` delegating to the 4-arg form — only isolation suites override it; the 12 existing providers are untouched - `JDBCDriverComparisonTest` builds a per-config factory bound to the resolved LEFT/RIGHT URLs and calls the 5-arg form ### Suites Each opens its own fresh connections and closes them in a `finally`, so they never poison the shared connections: - `NEGATIVE_CONNECTION_STATE` — `setCatalog`/`setSchema`/`setClientInfo`/`USE` to a nonexistent target - `NEGATIVE_TRANSACTION` — commit/rollback with autocommit on; DDL inside a manual transaction (throwaway table dropped in a `finally`) ## Included fix: one-sided diff mis-filing `ErrorDiffs.compare` returns a **flat** list, so callers that dumped it into `dataDifferences` mis-filed one-sided diffs (which belong in `metadataDifferences`) and dropped them from the CSV summary — surfaced live as an empty summary on the transaction case. New `ErrorDiffs.foldInto(result, ...)` preserves the metadata/data split; migrated **all five** negative providers (the two new + DDL/DML/Batch from #1527) to it. Kept `compare()` for the DML/Volume positive suites with a javadoc warning. Added `foldInto` unit tests. ## Live validation Ran against the peco serverless warehouse (Thrift-vs-SEA, `ERROR_COMPARISON_MODE=shadow`). Positive suites run in the **same JVM after** the state-mutating negatives and still **PASS** — no shared-connection poisoning. Surfaced a real one-sided divergence: `DDL inside a manual transaction` returns `false` on one side and throws `42P07` on the other. ## Testing `ErrorComparatorTest` — 20/20 (added `foldInto` coverage); two live shadow runs; clean compile; Isaac Review (`/review --uncommitted`) — 0 findings. 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 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
ErrorDiffsbridge, under theshadowdefault.NEGATIVE_STATEMENT_DDLIF EXISTS), missing schema/catalog, malformed syntaxNEGATIVE_STATEMENT_DMLexecuteUpdateon a SELECT (wrong method), write overflowNEGATIVE_STATEMENT_BATCHexecuteBatchpartial/full failure and a ResultSet-producing command; when both sides throwBatchUpdateException, also compares per-element update countsIsolation
These suites mutate catalog objects, so each runs in its own namespace under
comparator_ddl_tests(seeded fresh, dropped in afinally), keeping them safe on the shared connections.Included fix: NPE in the shared
ErrorDiffs.compareThe neither-threw case is now guarded centrally (returns empty, mirroring the deep path's
NOT_APPLICABLE) before reachinglegacyClassOnly, which dereferences the nullerror()of a RETURNED outcome. The new suites callErrorDiffs.compareunconditionally, so anoff-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 withif 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, positiveSTATEMENT_DDL/STATEMENT_DMLrun 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.