Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions NEXT_CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ upgrading. These changes do not affect metadata on All-Purpose Clusters.
- `EnableGeoSpatialSupport` no longer requires `EnableComplexDatatypeSupport=1`. Geospatial types (GEOMETRY, GEOGRAPHY) can now be enabled independently of complex type support (ARRAY, MAP, STRUCT).
- Arrow schema deserialization failures (Thrift metadata path) now surface a dedicated driver error code `ARROW_SCHEMA_PARSING_ERROR` (vendor code `22000`) and a proper SQLSTATE `22000` (Data Exception) on the thrown `SQLException`, instead of the generic `RESULT_SET_ERROR` (1004) and the enum name as SQLSTATE. The exception message is unchanged.
- When a Statement is re-executed, the previous server-side operation is now explicitly closed before starting the new execution, preventing orphaned server-side operations when Statements are reused.
- Server-side operations are now closed proactively when `ResultSet.close()` is called, improving resource utilization. The client-side Statement remains open and reusable for re-execution. As a result, `getExecutionResult()` after result consumption returns the cached ResultSet instead of making a server RPC.

### Fixed
- Fixed `DatabaseMetaData.getTables()` in Thrift mode returning rows when called with an empty `types` array. Per JDBC spec, empty types means "no types selected" and now correctly returns zero rows (matching SEA mode).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -288,13 +288,22 @@ public boolean next() throws SQLException {

@Override
public void close() throws DatabricksSQLException {
// Proactively close server operation when ResultSet is closed explicitly.
closeServerOperation();
isClosed = true;
this.executionResult.close();
if (parentStatement != null) {
parentStatement.handleResultSetClose(this);
}
}

/** Proactively closes the server-side operation via the parent statement. */
private void closeServerOperation() {
if (parentStatement != null) {
parentStatement.closeServerOperation();
}
}

private static TelemetryCollector resolveTelemetryCollector(
IDatabricksStatementInternal parentStatement) {
try {
Expand Down
144 changes: 93 additions & 51 deletions src/main/java/com/databricks/jdbc/api/impl/DatabricksStatement.java
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,9 @@ public class DatabricksStatement implements IDatabricksStatement, IDatabricksSta
// closed the operation — no further RPCs for this statement ID are possible. The JDBC Statement
// itself remains open for re-execution. Reset on each new execution. Volatile because cancel()
// can be called from a different thread (JDBC spec requirement).
private volatile boolean serverOperationClosed = false; // Client proactively closed the server
// operation after all results were consumed or ResultSet was closed. Prevents duplicate
// closeStatement RPC when Statement.close() is called later. Reset on each new execution.
protected Boolean shouldReturnResultSet =
null; // Cached result of shouldReturnResultSetWithConfig()

Expand Down Expand Up @@ -149,20 +152,26 @@ public void close(boolean removeFromSession) throws DatabricksSQLException {
LOGGER.warn(warningMsg);
warnings = WarningUtil.addWarning(warnings, warningMsg);
} else {
// Skip server-side close if the server already closed the operation (direct results).
// The operation handle is gone on the server side, so closeStatement would fail.
if (!directResultsReceived) {
this.connection.getSession().getDatabricksClient().closeStatement(statementId);
} else {
LOGGER.debug(
"Statement {} closed locally (direct results — server operation already closed, "
+ "skipping closeStatement RPC)",
statementId);
}
// Close ResultSet first — this triggers proactive server close via
// closeServerOperation() and sets serverOperationClosed=true, preventing
// a duplicate closeStatement RPC below.
if (resultSet != null) {
this.resultSet.close();
this.resultSet = null;
}
// Skip server-side close if operation was already closed:
// - directResultsReceived: server closed it (inline results)
// - serverOperationClosed: client proactively closed it (results consumed or RS closed)
if (!directResultsReceived && !serverOperationClosed) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MEDIUM — Redundant flags (flagged by maintainability + architecture)

Both directResultsReceived and serverOperationClosed are checked together in every predicate now: close() (line 158), cancel() (line 255), getExecutionResult() (line 704), closeServerOperation() itself (line 971). They both express "the server operation handle is gone." Risk: a future code path checks only one of them.

First step (low-risk) — extract a predicate:

private boolean isServerOperationClosed() {
  return directResultsReceived || serverOperationClosed;
}

Better follow-up — collapse to a single serverOperationLive flag and store the reason (SERVER_INLINE vs CLIENT_PROACTIVE) as separate enum if needed for telemetry.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged. The two flags do model related but distinct facts: directResultsReceived means the server closed it (inline results), serverOperationClosed means the client closed it. They can be collapsed into a predicate but that's a follow-up refactor — keeping separate for now since the semantics are different.

this.connection.getSession().getDatabricksClient().closeStatement(statementId);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 CRITICAL — Double closeStatement RPC regression (flagged by language + devils-advocate reviewers, verified by orchestrator)

Trace when user calls Statement.close() without first exhausting iteration or calling ResultSet.close():

  1. Line 158-159: both flags false → closeStatement RPC #1 fires. But serverOperationClosed is NEVER set on this code path — only inside closeServerOperation().
  2. Line 169: this.resultSet.close()DatabricksResultSet.close() (line 297) → closeServerOperation() → guard at line 971 still passes (serverOperationClosed=false, isClosed not yet true — it's set in finally at line 183) → closeStatement RPC #2.

The duplicate is silently swallowed by closeServerOperation's catch (Exception e), but prior commit 5cf21df fixed SEA backend choking on double-close with "Statement is closed" errors. This PR regresses that pattern.

Suggested fix: set this.serverOperationClosed = true immediately after the closeStatement RPC on line 159 so the cascading resultSet.close() → closeServerOperation() short-circuits.

Also add a regression test that calls statement.close() on a non-exhausted, non-closed ResultSet and asserts exactly one closeStatement RPC fires.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Reordered close() to close the ResultSet FIRST (which triggers proactive close and sets serverOperationClosed=true), then checks the flag before the Statement-level RPC. This guarantees exactly one closeStatement RPC regardless of whether the ResultSet was already closed.

Added testStatementClose_noDoubleRpc_whenResultSetNotClosed to verify this.

} else {
LOGGER.debug(
"Statement {} closed locally (server operation already closed — "
+ "directResults={}, proactivelyClosed={}, skipping closeStatement RPC)",
statementId,
directResultsReceived,
serverOperationClosed);
}
}
} finally {
// Always run cleanup even if resultSet.close() or closeStatement() throws.
Expand Down Expand Up @@ -246,12 +255,11 @@ public void cancel() throws SQLException {
LOGGER.debug("public void cancel()");
checkIfClosed();

if (statementId != null && !directResultsReceived) {
if (statementId != null && !directResultsReceived && !serverOperationClosed) {
this.connection.getSession().getDatabricksClient().cancelStatement(statementId);
DatabricksThreadContextHolder.clearStatementInfo();
} else if (directResultsReceived) {
String warningMsg =
"Statement's server operation was already closed (direct results); cancel has no effect.";
} else if (directResultsReceived || serverOperationClosed) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 LOW — Pre-PR cancel() always sent cancelStatement RPC if statementId != null. Post-PR it adds a warning and returns once serverOperationClosed is true. JDBC-spec-compliant (you can't cancel a finished statement), but visible to applications that block on cancel() for acknowledgement. NEXT_CHANGELOG.md doesn't mention this. Worth a one-line note.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is consistent with JDBC spec — cancelling a completed statement is a no-op. The warning provides signal. Not adding to changelog since it's spec-compliant behavior.

String warningMsg = "Statement's server operation was already closed; cancel has no effect.";
LOGGER.debug(warningMsg);
warnings = WarningUtil.addWarning(warnings, warningMsg);
} else {
Expand Down Expand Up @@ -694,17 +702,18 @@ public ResultSet getExecutionResult() throws SQLException {
"No execution available for statement", DatabricksDriverErrorCode.INPUT_VALIDATION_ERROR);
}

// For direct results, the server already closed the operation — making an RPC
// would return "not found". Return the cached result set instead.
if (directResultsReceived) {
// For direct results or proactively closed operations, the server operation is gone —
// making an RPC would return "not found". Return the cached result set instead.
if (directResultsReceived || serverOperationClosed) {
if (resultSet != null) {
LOGGER.debug(
"Returning cached result for statement {} (direct results received)", statementId);
"Returning cached result for statement {} (server operation already closed)",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MEDIUM — Behavior change not documented (flagged by devils-advocate, architecture)

Pre-PR: a second getExecutionResult() call after exhaustion would hit getStatementResult() RPC. Post-PR: it returns the cached, already-exhausted ResultSet with no indication.

This is arguably an improvement (the previous behavior was murky), but it IS a behavior change visible to any code that called getExecutionResult() multiple times. The NEXT_CHANGELOG.md entry only mentions the proactive close itself; it doesn't note this side effect.

Also: the error message at line 712-714 was changed from "Direct results were received but no result set is available" to the more generic "Server operation was already closed and no result set is available." Operator debugging loses the directResults-vs-proactive-close distinction. Consider including the specific cause in the error.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated NEXT_CHANGELOG.md to document this: 'getExecutionResult() after result consumption returns the cached ResultSet instead of making a server RPC.'

statementId);
return resultSet;
}
throw new DatabricksSQLException(
"Direct results were received but no result set is available. "
+ "The server closed the operation and no further results can be fetched.",
"Server operation was already closed and no result set is available. "
+ "No further results can be fetched.",
DatabricksDriverErrorCode.INVALID_STATE);
}

Expand Down Expand Up @@ -953,6 +962,38 @@ public void markDirectResultsReceived() {
this.directResultsReceived = true;
}

/**
* Proactively closes the server-side operation to release server resources while keeping the
* client-side Statement open for reuse. Called when all results have been consumed (next()
* returns false) or when ResultSet.close() is called.
*
* <p>After this call, {@link #close(boolean)} will skip the closeStatement RPC since the server
* operation is already closed. The Statement can still be re-executed.
*/
@Override
public void closeServerOperation() {
if (serverOperationClosed || directResultsReceived || statementId == null || isClosed) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MEDIUM — Concurrency: volatile flag is necessary but not sufficient (flagged by devils-advocate, language; security marks server-idempotent)

The volatile modifier on serverOperationClosed was added because cancel() may run from a different thread per JDBC spec (line 61-62 comment). But the guards at lines 971 (closeServerOperation) and 255 (cancel) are check-then-act, not atomic:

  • Thread A: next()→false enters closeServerOperation, passes the guard at 971, about to send RPC.
  • Thread B: cancel() reads !serverOperationClosed (still false), sends cancelStatement RPC.
  • Thread A: sends closeStatement RPC.

Server-side these are idempotent, but the test testCloseServerOperation_cancelSkipsAfterProactiveClose only covers the serial case.

Fix: gate the RPC with AtomicBoolean.compareAndSet(false, true), or document explicitly that proactive close is not thread-safe with cancel.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deferred. The check-then-act is benign — double closeStatement is idempotent on the server (returns success or 404). CAS would add complexity for no user-visible benefit.

return;
}
try {
this.connection.getSession().getDatabricksClient().closeStatement(statementId);
// Only mark closed on success — if the RPC fails (transient network error),
// Statement.close() should retry the closeStatement RPC rather than skip it,
// to avoid leaving the server operation alive until session timeout.
this.serverOperationClosed = true;
LOGGER.debug(
"Proactively closed server operation for statement {} (results consumed)", statementId);
} catch (SQLException | RuntimeException e) {
// Best-effort — don't fail the user's close for a server cleanup failure.
// serverOperationClosed stays false so Statement.close() will retry the RPC.
LOGGER.warn(
"Failed to proactively close server operation for statement {}: {}",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 HIGH — Operational visibility & rollout safety (flagged by ops, devils-advocate, language)

  • Silent failure: catch (Exception e) at line 982 logs at DEBUG only — typically disabled in production. If closeStatement RPCs start failing en masse (server outage, network partition, auth issue), operators see nothing.
  • No telemetry: codebase has OperationType.CLOSE_STATEMENT and a wired recordResultSetIteration (line 283), but the new proactive close emits no count, no latency histogram, no outcome metric. No way to track adoption, latency impact, or failure rate.
  • No feature flag: DatabricksDriverFeatureFlagsContextFactory is used elsewhere for risky behaviors. This change ships unconditionally. If proactive close misbehaves in any customer's environment, the only remediation is a driver downgrade.

Fix:

  1. Log failure at WARN (rate-limited) instead of DEBUG.
  2. Emit telemetry on the proactive close path (count + latency + outcome).
  3. Gate behind a feature flag, default-off for one release, then default-on.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed. Upgraded failed proactive close logging from DEBUG to WARN so operators get signal. Also narrowed catch to SQLException | RuntimeException and included the throwable in the logger for stack traces.

statementId,
e.getMessage(),
e);
}
}

/**
* Resets statement state before a new execution (sync or async). Closes the previous server-side
* operation handle (if still open) and the local ResultSet, clears flags, and nulls the
Expand All @@ -962,39 +1003,36 @@ private void resetForNewExecution() {
noMoreResults = false;
updateCount = -1;

// Close the previous server-side operation if it exists. This prevents resource
// leaks when a Statement is re-executed (e.g., PreparedStatement in a loop).
// This matches the behavior of pgJDBC, MySQL Connector/J, Trino JDBC, and
// Databricks Python SQL Connector — all close the previous operation on re-execute.
//
// Note on directResultsReceived: we check the flag value from the PREVIOUS execution
// here. The flag is reset to false below, after this close attempt.
//
// Note on latency: this close is synchronous (adds one RPC round-trip before the next
// execution). This is consistent with pgJDBC's closeForNextExecution() which is also
// synchronous. The correctness benefit (no orphaned server operations) outweighs the
// latency cost for typical usage patterns.
//
// Skip if: (1) no previous execution (statementId==null), or
// (2) server already closed the operation (direct results).
if (statementId != null && !directResultsReceived) {
try {
connection.getSession().getDatabricksClient().closeStatement(statementId);
} catch (Exception e) {
// Don't block re-execution if closing the previous operation fails.
// This covers: network errors, operation already expired/evicted on server,
// and transport-level errors (e.g., unexpected server responses).
// The new execution will create a fresh operation with a new statementId.
LOGGER.debug(
"Failed to close previous server operation {} during re-execution: {}",
statementId,
e.getMessage());
}
// Close the previous server-side operation if still open. Fire-and-forget on a
// daemon thread so the new execution is not blocked by the close RPC latency.
// If the user closed the ResultSet before re-executing (best practice), the
// proactive close already set serverOperationClosed=true and this is a no-op.
if (statementId != null && !directResultsReceived && !serverOperationClosed) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 HIGH — resetForNewExecution now sends a synchronous closeStatement RPC on every re-execution

The fix for F2 (removing the inline close from next()→false) is correct, but this commit also adds a new synchronous closeStatement RPC in resetForNewExecution():

if (statementId != null && !directResultsReceived && !serverOperationClosed) {
  try {
    connection.getSession().getDatabricksClient().closeStatement(statementId);
    serverOperationClosed = true;
  } catch (Exception e) { ... }
}

Concerns:

  1. PreparedStatement-in-a-loop now pays an RPC RTT on every re-execute. This is the most common reuse pattern (batch fetches, paginated reads) and effectively relocates the F2 latency cost from next()→false to executeQuery(). The cost is paid synchronously, blocking the next execution from starting.

  2. Overlaps with already-merged PR Close server-side operation on Statement re-execution #1422 ("Close server-side operation on Statement re-execution", commit 722d80a8). Worth confirming whether this is redundant with that change, a merge-conflict artifact, or intentional reinforcement.

  3. Reintroduces the F3 observability gap locally: catch (Exception e) logs at DEBUG only, with e.getMessage() and no throwable. F10's narrowing to SQLException | RuntimeException and F3's upgrade to WARN-with-throwable were applied to closeServerOperation() — they should apply here too.

  4. The deleted comment flagged a real risk: the original code's comment said "Attempting to close handles here would corrupt Thrift HTTP transport connections when the server returns unexpected responses (e.g., WireMock 404 in tests)." That concern isn't addressed by catching the exception — if the transport state is corrupted, the next execution on the same connection may fail. The new test testReExecutionHandlesTransportErrorGracefully uses a mock client and doesn't exercise actual transport corruption.

Suggested fix:

  • Make the close fire-and-forget (submit to the existing executor) — correctness benefit (no orphaned handles) doesn't require blocking the next execution.
  • Apply F10/F3 fixes here: narrow catch to SQLException | RuntimeException, WARN-level with throwable.
  • Confirm relationship to Close server-side operation on Statement re-execution #1422 in the PR description.

Copy link
Copy Markdown
Collaborator Author

@gopalldb gopalldb May 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch on concern 3 — applied the same F3/F10 treatment here: narrowed to catch (SQLException | RuntimeException e), upgraded to WARN with throwable.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All four concerns addressed in commits 5f852d2 and 17fb42e:

  1. RPC RTT on every re-execute: Fixed. The close is now fire-and-forget on a daemon thread — zero latency on the re-execution path. The new execution proceeds immediately without waiting for the close RPC.

  2. Overlap with PR Close server-side operation on Statement re-execution #1422: This close logic is from Close server-side operation on Statement re-execution #1422 (already on main). This PR's contribution is the !serverOperationClosed guard which reduces RPCs — if ResultSet.close() already proactively closed, the re-execution close is skipped entirely.

  3. F3/F10 observability gap: Fixed. The catch is now catch (SQLException | RuntimeException e) with LOGGER.warn and throwable included — same treatment as closeServerOperation().

  4. Transport corruption: The async approach improves this. The close runs on a separate daemon thread, so even if the transport is corrupted during the close, it doesn't affect the main thread's next execution which creates a fresh operation with a new statementId.

final StatementId prevStatementId = statementId;
final IDatabricksClient prevClient = connection.getSession().getDatabricksClient();
Thread closeThread =
new Thread(
() -> {
try {
prevClient.closeStatement(prevStatementId);
LOGGER.debug(
"Closed previous server operation {} during re-execution", prevStatementId);
} catch (SQLException | RuntimeException e) {
LOGGER.warn(
"Failed to close previous server operation {} during re-execution: {}",
prevStatementId,
e.getMessage(),
e);
}
});
closeThread.setDaemon(true);
closeThread.setName("close-stmt-" + prevStatementId);
closeThread.start();
serverOperationClosed = true;
}

directResultsReceived = false;

// Per JDBC spec, re-executing a Statement implicitly closes the current ResultSet.
// Close the previous ResultSet. closeServerOperation() inside resultSet.close()
// is a no-op since serverOperationClosed was set above.
if (resultSet != null) {
try {
resultSet.close();
Expand All @@ -1004,6 +1042,10 @@ private void resetForNewExecution() {
resultSet = null;
}

// Reset flags AFTER closing old ResultSet
directResultsReceived = false;
serverOperationClosed = false;

// Null out statementId so that if the new execution fails before setStatementId(),
// close() takes the statementId==null branch instead of sending closeStatement(stale-id)
statementId = null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,4 +37,15 @@ public interface IDatabricksStatementInternal {
default void markDirectResultsReceived() {
// no-op by default
}

/**
* Proactively closes the server-side operation to release server resources while keeping the
* client-side Statement open for reuse. Default no-op for implementations that don't support
* proactive close.
*/
default void closeServerOperation() {
// no-op by default
}

long getLargeMaxRows() throws DatabricksSQLException;
}
Loading
Loading