fix(jdbc-driver): actually run connection queries on the query and stream paths

[igorlukanin]

Issue

JDBCDriver.query() passed the array returned by prepareConnectionQueries() as queryPromised's options argument, but queryPromised reads options.prepareConnectionQueries off it — which on an array is undefined. The replay loop therefore ran zero times, and no connection query was ever executed on the query path.

// query()
const promise = this.queryPromised(queryWithParams, cancelObj, this.prepareConnectionQueries());

// queryPromised()
const prepareConnectionQueries = options.prepareConnectionQueries || [];  // undefined -> []

stream() was separately missing the replay entirely, so neither path applied it. The defect dates back to the driver's original commit.

supported-drivers.ts ships prepareConnectionQueries: ["SET time_zone = '+00:00'"] for MySQL, so MySQL accessed through this driver has never applied it — timestamps are read in the server's local timezone rather than UTC. Any driver config passing prepareConnectionQueries explicitly was silently ignored too.

Fix

• query() passes a proper options object so queryPromised reads the statements. Fixing the caller rather than widening queryPromised's contract keeps that method's options shape intact — it is protected with this as its only call site.
• stream() replays the same statements before opening the stream, inside the existing try so a failing connection query still releases the pooled connection through the catch.

prepareConnectionQueries() itself is unchanged, as is every driver's statement list.

Blast radius

mysql is the only dbType in supported-drivers.ts with a non-empty list — athena, sparksql and hive all ship [], and DatabricksDriver resolves to [] because its dbType is absent from SupportedDrivers (and it overrides none of query/stream/queryPromised/prepareConnectionQueries). Reaching the non-empty case requires CUBEJS_DB_TYPE=jdbc; a plain CUBEJS_DB_TYPE=mysql routes to the native MySQL driver, not this one.

So the change is confined to MySQL-over-JDBC and to configs that set prepareConnectionQueries explicitly. In both cases the behaviour moves from "silently ignored" to "does what it says" — but it does mean MySQL-over-JDBC starts returning UTC timestamps where it previously returned local-time ones. Worth a release note for anyone who compensated for the old behaviour downstream.

Tests

test/unit/connection-queries.test.ts — 8 tests over both paths, following the existing escape-dialect.test.ts pattern (JVM mocked out, driver built via Object.create with a stub pool). Per path: dbType built-ins replay before the primary query, explicit config replays, an empty-array dbType runs none, and a failing connection query still releases the connection exactly once.

Each was verified to fail on the unfixed code: reverting the fix turns the 5 replay-asserting tests red while the 3 negative controls stay green.

Out of scope

Two pre-existing issues this touches but does not change:

• executeStatement() never closes the JDBC Statement it creates. The query path already leaks these; the stream path now creates one more per connection query, held for the stream's lifetime. Fixing it means changing the shared query path.
• withConnection() (used for metadata/schema queries) does not replay connection queries, so it stays asymmetric with query()/stream().

[claude]

Claude finished @igorlukanin's task in 2m 41s —— View job

---

Review complete — fix is correct; 2 medium, 2 low. No blockers.

Full review

• Read the diff (git diff origin/master...HEAD)
• Read JDBCDriver.ts in full
• Verified the blast-radius claims against supported-drivers.ts and DatabricksDriver.ts
• Reviewed the new test file
• Posted inline comments

Verdict

The diagnosis is right and the fix is the minimal correct one. queryPromised(query, cancelObj, options) reads options.prepareConnectionQueries, and query() was handing it the bare array — [].prepareConnectionQueries is undefined, so the loop at JDBCDriver.ts:282 ran zero times. Wrapping at the call site rather than widening queryPromised to accept both shapes is the right call: queryPromised is protected with exactly one caller, so there is no compatibility surface to preserve.

I independently confirmed the blast-radius analysis:

Claim	Verified
mysql is the only non-empty prepareConnectionQueries in SupportedDrivers	✅ athena, sparksql, hive all ship []
DatabricksDriver resolves to []	✅ sets dbType: 'databricks' (DatabricksDriver.ts:260), which is absent from SupportedDrivers, and overrides none of query/stream/queryPromised/prepareConnectionQueries
JDBCDriver has no other subclasses	✅ DatabricksDriver is the only one in the monorepo

The behaviour-change call-out in the description is the important part of this PR — MySQL-over-JDBC starts returning UTC timestamps where it returned server-local ones. That belongs in the release notes, and arguably deserves a line in the driver docs too, since anyone who compensated downstream will see values shift silently.

Findings

Medium — the replay is per-call, not per-connection (inline on JDBCDriver.ts:305-309)

These are connection queries: session state that survives for the life of the JDBC connection. Running them at each call site means every query() and every stream() on a pooled connection re-issues the same SET, doubling round-trips on the MySQL query path. The pool create factory (JDBCDriver.ts:142-152) is the natural home — one round-trip per physical connection, and every path including withConnection() gets it for free instead of each having to remember. Since the per-call design has been dead code since the driver's first commit, there is no established behaviour to preserve here; it's cheap to choose now and awkward to revisit once live.

Medium — statement leak, now on the stream path too

executeStatement() never closes the Statement it creates, and the connections are pooled and long-lived, so statements accumulate on each connection for its whole lifetime rather than per query. The PR correctly scopes this out, but it's worth noting the fix moves the stream path from zero leaked statements to one per connection query. Moving the replay into the pool factory (above) would bound this to once per connection rather than compounding it.

Low — a stream test doesn't test what its name says (inline on connection-queries.test.ts:111)

stream()'s primary query goes through conn.createStatement → statement.execute, not the stubbed executeStatement, so executed can only ever hold connection queries. The assertion passes identically if the replay loop is moved after executeQuery(query) — the "before opening the stream" ordering claim is unverified. Pushing the SQL from the stubbed execute into the same array makes it symmetric with the query() tests, which do assert ordering properly.

Low — withConnection() is unused

The out-of-scope note about withConnection() staying asymmetric is accurate but understates it: withConnection has no callers anywhere in this driver, in DatabricksDriver, or in BaseDriver. It's dead code, so the asymmetry is latent rather than live — which also means it's a trap for whoever adds the first caller. Worth either deleting it or, if the replay moves to the pool factory, the question disappears entirely.

Things done well

• The test file follows the established escape-dialect.test.ts mocking pattern rather than inventing a new one, and the Object.create(JDBCDriver.prototype) + stub-pool approach keeps the JVM out of the unit suite.
• Both negative controls (empty-array dbType) and the connection-release-on-failure cases are covered on both paths — the failure test asserting released has exactly one entry is the right shape, since double-release on a pool is its own class of bug.
• Verifying each test fails against the unfixed code is the step most PRs skip, and it's what makes the 5/3 red/green split meaningful.
• The description's honesty about blast radius and out-of-scope issues is exemplary — it made this review much faster.

What I could not verify

yarn tsc and jest require approval in this environment, so I did not compile the package or execute the new suite. The review is static. Note that yarn unit runs jest --verbose dist/test/unit against compiled output, and the jest.mock(..., { virtual: true }) calls rely on TypeScript preserving statement order relative to the emitted require — the same assumption escape-dialect.test.ts already makes, so CI should confirm it.

• [branch](https://github.com/cube-js/cube/tree/igor/core-721-jdbc-connection-queries-never-run-on-the-query-path-mysql)

[claude]

Worth considering whether the replay belongs in the pool's create factory instead of per query/stream.

These are connection queries — session state (SET time_zone) that survives for the life of the JDBC connection. Running them at the call site means every query() and every stream() on a pooled connection re-issues the same SET, so for MySQL-over-JDBC this fix doubles the round-trips on the query path. Doing it once in the pool factory (line 142-152, right after new Connection(...)) gives the same guarantee for one round-trip per physical connection, and would also make withConnection() and any future path correct for free rather than requiring each one to remember.

The counter-argument is that the pool factory can't distinguish "connection reused after a session-resetting failure" — but nothing in this driver resets sessions, and the current per-call design has been dead code since the driver's first commit, so there's no established behaviour to preserve.

Not a blocker for landing the correctness fix, but the per-call placement is a choice that's cheap now and expensive to revisit once it's live.

Fix this →

[claude]

This assertion doesn't actually test the "before opening the stream" part of the test name.

The primary query in stream() goes through conn.createStatement → statement.execute, not through the stubbed executeStatement, so executed can only ever contain connection queries. The assertion would pass identically if the replay loop were placed after await executeQuery(query).

Pushing a marker from the stream's own execution path into the same array makes the ordering claim real:

execute: (sql: string, cb: Function) => {
  built.executed.push(sql);
  cb(null, { /* ... */ });
},

then expect(executed).toEqual([MYSQL_TIME_ZONE, 'SELECT 1']), symmetric with the query() tests above.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.00%. Comparing base (5a47e08) to head (40eaf48).
⚠️ Report is 2 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (5a47e08) and HEAD (40eaf48). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (5a47e08)	HEAD (40eaf48)
cubesql	1	0

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #11456       +/-   ##
===========================================
- Coverage   79.43%   59.00%   -20.43%     
===========================================
  Files         480      223      -257     
  Lines       98778    17891    -80887     
  Branches     3636     3636               
===========================================
- Hits        78460    10556    -67904     
+ Misses      19800     6817    -12983     
  Partials      518      518               

Flag	Coverage Δ
cube-backend	59.00% <ø> (ø)
cubesql	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.