Skip to content

Fix ambiguous errors when statements are cancelled or connection closed #1291

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.

Sign up for GitHub

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

Merged
gopalldb merged 4 commits into from
Apr 27, 2026
Merged

Fix ambiguous errors when statements are cancelled or connection closed #1291

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8774985
Fix ambiguous error messages when statements are cancelled or connect…
gopalldb Mar 19, 2026
5a5605f
Add tests for cancel detection, revert connection.close cancel
gopalldb Mar 20, 2026
4f9c266
Extract cancelledStatementException helper to deduplicate cancel dete…
gopalldb Mar 20, 2026
c37ed02
Merge main into fix/cancel-error-handling and add changelog
gopalldb Apr 27, 2026
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
- Added support for using SQL SHOW commands for Thrift-mode metadata operations (`getTables`, `getColumns`, `getSchemas`, `getFunctions`, `getPrimaryKeys`, `getImportedKeys`, `getCrossReference`). Enable by setting `UseQueryForMetadata=1`. This aligns Thrift metadata behavior with Statement Execution API (SEA) mode.

### Fixed
- Improved error messages for cancelled statements: operations cancelled via `Statement.cancel()` or closed connections now return SQL state `HY008` (operation cancelled) instead of generic error codes, making it easier for applications to detect and handle cancellations.
- Fixed race condition between chunk download error handling and result set close that could cause invalid state transition warnings (`CHUNK_RELEASED -> DOWNLOAD_FAILED`) during Arrow Cloud Fetch operations in resource-constrained environments.
- Fixed `EnableBatchedInserts` silently falling back to individual execution when table or schema names contain special characters (e.g., hyphens) inside backtick-quoted identifiers. Added a warn log when the fallback occurs.
- Fixed `IntervalConverter` crash (`IllegalArgumentException: Invalid interval metadata`) when INTERVAL columns are returned via CloudFetch. Arrow metadata from CloudFetch uses underscored format (`INTERVAL_YEAR_MONTH`, `INTERVAL_DAY_TIME`) which the driver's regex did not accept.
Expand Down
5 changes: 4 additions & 1 deletion src/main/java/com/databricks/jdbc/api/impl/ExecutionStatus.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,10 @@ class ExecutionStatus implements IExecutionStatus {

public ExecutionStatus(StatementStatus status) {
this.state = getStateFromSdkState(status.getState());
this.errorMessage = status.getError() != null ? status.getError().getMessage() : null;
this.errorMessage =
status.getError() != null && status.getError().getMessage() != null
? status.getError().getMessage()
: (this.state == ExecutionState.ABORTED ? "Statement was cancelled" : null);
this.sqlState = status.getSqlState();
this.sdkStatus = status;
}
Expand Down
4 changes: 4 additions & 0 deletions src/main/java/com/databricks/jdbc/common/DatabricksJdbcConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,10 @@ public final class DatabricksJdbcConstants {
public static final String GCP_GOOGLE_ID_AUTH_TYPE = "google-id";
public static final String DEFAULT_HTTP_EXCEPTION_SQLSTATE = "08000";
public static final String QUERY_EXECUTION_TIMEOUT_SQLSTATE = "57KD0";

/** Standard SQL state for operation cancelled (SQLSTATE HY008). */
public static final String OPERATION_CANCELLED_SQLSTATE = "HY008";

public static final int TEMPORARY_REDIRECT_STATUS_CODE = 307;
public static final String REDACTED_TOKEN = "****";
public static final String QUERY_TAGS = "query_tags";
Expand Down
12 changes: 12 additions & 0 deletions src/main/java/com/databricks/jdbc/dbclient/impl/sqlexec/DatabricksSdkClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.databricks.jdbc.dbclient.impl.sqlexec;

import static com.databricks.jdbc.common.DatabricksJdbcConstants.JSON_HTTP_HEADERS;
import static com.databricks.jdbc.common.DatabricksJdbcConstants.OPERATION_CANCELLED_SQLSTATE;
import static com.databricks.jdbc.common.DatabricksJdbcConstants.QUERY_EXECUTION_TIMEOUT_SQLSTATE;
import static com.databricks.jdbc.common.DatabricksJdbcConstants.TEMPORARY_REDIRECT_STATUS_CODE;
import static com.databricks.jdbc.common.EnvironmentVariables.DEFAULT_RESULT_ROW_LIMIT;
Expand Down Expand Up @@ -709,6 +710,17 @@ void handleFailedExecution(
ExecuteStatementResponse response, String statementId, String statement) throws SQLException {
StatementState statementState = response.getStatus().getState();
ServiceError error = response.getStatus().getError();

// Distinguish cancellation from failure
if (statementState == StatementState.CANCELED) {
String cancelMessage = String.format("Statement [%s] was cancelled", statementId);
LOGGER.info(cancelMessage);
throw new DatabricksSQLException(
Copy link
Copy Markdown
Collaborator

@msrathore-db msrathore-db Apr 27, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

[F3] Cancellation emits ERROR-level telemetry — defeats the PR's "distinguishability" goal — High

This throw (and the symmetric one in DatabricksThriftAccessor.java:859) goes through DatabricksSQLException(reason, sqlState, internalError), which calls:

// DatabricksSQLException.java:52-56
super(reason, sqlState);
logTelemetryEvent(sqlState, reason, false);   // silentExceptions = false (hard-coded)

logTelemetryEvent with silentExceptions=false unconditionally calls exportFailureLog(..., TelemetryLogLevel.ERROR) (DatabricksSQLException.java:73-79). So every user-initiated Statement.cancel() and every Ctrl-C from a BI tool emits an ERROR-level telemetry event with sqlState=HY008.

Cancellations are common in normal BI workloads (Tableau/Looker speculatively cancel; users close tabs); a customer batch-cancelling 10k queries will look like a 10k-error spike to driver-side monitoring. The PR's goal — making cancellations distinguishable from real failures — is undone by sending them through the same ERROR alert path as real failures.

Suggested fix: introduce a 3-arg silentExceptions=true overload (or directly downgrade to INFO/TRACE inside logTelemetryEvent when sqlState == OPERATION_CANCELLED_SQLSTATE), and use it for both cancellation throw sites. Bonus: this also helps F4-style idempotency (concurrent pollers each observing CANCELED would no longer multi-fire ERROR telemetry).

cancelMessage,
OPERATION_CANCELLED_SQLSTATE,
DatabricksDriverErrorCode.EXECUTE_STATEMENT_CANCELLED);
Copy link
Copy Markdown
Collaborator

@msrathore-db msrathore-db Apr 27, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

[F2] EXECUTE_STATEMENT_CANCELLED is silently discarded — getErrorCode() returns 0 — High

The PR's customer-impact section promises:

Customers can now programmatically distinguish cancellations from actual errors by checking: ... Error code: EXECUTE_STATEMENT_CANCELLED

But the 3-arg DatabricksSQLException(String reason, String sqlState, DatabricksDriverErrorCode internalError) constructor (DatabricksSQLException.java:52-56) is:

public DatabricksSQLException(
    String reason, String sqlState, DatabricksDriverErrorCode internalError) {
  super(reason, sqlState);                       // ← internalError NOT passed to vendor code
  logTelemetryEvent(sqlState, reason, false);    // ← internalError NOT logged either
}

The internalError parameter is purely advisory — super(reason, sqlState) does not set SQLException's vendor code, and logTelemetryEvent only takes sqlState/reason. So e.getErrorCode() on a cancellation exception returns 0, and EXECUTE_STATEMENT_CANCELLED never appears anywhere observable to the customer. None of the new tests assert on getErrorCode(), hiding the gap.

Suggested fix: either (a) fix the constructor to use super(reason, sqlState, vendorCode) with a stable mapping for DatabricksDriverErrorCode, then add assertEquals(...) on getErrorCode() in the new tests; or (b) drop the error-code claim from the PR description and rely solely on HY008 + the message string.

}

String errorMessage =
String.format(
"Statement execution failed %s -> %s\n%s.", statementId, statement, statementState);
Expand Down
16 changes: 16 additions & 0 deletions src/main/java/com/databricks/jdbc/dbclient/impl/thrift/DatabricksThriftAccessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package com.databricks.jdbc.dbclient.impl.thrift;

import static com.databricks.jdbc.common.DatabricksJdbcConstants.OPERATION_CANCELLED_SQLSTATE;
import static com.databricks.jdbc.common.DatabricksJdbcConstants.QUERY_EXECUTION_TIMEOUT_SQLSTATE;
import static com.databricks.jdbc.common.EnvironmentVariables.*;
import static com.databricks.jdbc.common.util.DatabricksThriftUtil.*;
Expand Down Expand Up @@ -422,6 +423,9 @@ DatabricksResultSet getStatementResult(
try {
response = getOperationStatus(request, statementId);
TOperationState operationState = response.getOperationState();
if (operationState == TOperationState.CANCELED_STATE) {
throw cancelledStatementException(statementId.toSQLExecStatementId());
Copy link
Copy Markdown
Collaborator

@msrathore-db msrathore-db Apr 27, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

[F4] Asymmetric fix — SEA DatabricksSdkClient.getStatementResult does NOT detect CANCELED — High

This Thrift fix is correct, but the SEA equivalent at DatabricksSdkClient.java:395-423 was not updated:

public DatabricksResultSet getStatementResult(...) throws SQLException {
  ...
  response = apiClient.execute(req, GetStatementResponse.class);
  ...
  return new DatabricksResultSet(
      response.getStatus(),
      typedStatementId,
      response.getResult(),   // ← null when state is CANCELED
      ...);
}

A user who calls executeStatementAsync (SEA), then Statement.cancel(), then polls via getStatementResult gets a DatabricksResultSet wrapping a CANCELED status with null result data — exactly the original bug, on the SEA async code path. The PR's "programmatically distinguish cancellations" promise fails for SEA async users.

Suggested fix: after the apiClient.execute(...) line, add a CANCELED check using the same HY008 / EXECUTE_STATEMENT_CANCELLED contract used in handleFailedExecution at line 698-705. Ideally, factor cancelledStatementException(statementId) into a shared helper (today the construction is duplicated inline in handleFailedExecution and as a private method in DatabricksThriftAccessor).

}
if (operationState == TOperationState.FINISHED_STATE) {
verifySuccessStatus(
response.getStatus(), "getStatementResult", statementId.toSQLExecStatementId());
Expand Down Expand Up @@ -828,6 +832,11 @@ private void checkOperationStatusForErrors(TGetOperationStatusResp statusResp, S
errorMsg, statusResp.isSetSqlState() ? statusResp.getSqlState() : null);
}

if (statusResp.isSetOperationState()
&& statusResp.getOperationState() == TOperationState.CANCELED_STATE) {
throw cancelledStatementException(statementId);
Copy link
Copy Markdown
Collaborator

@msrathore-db msrathore-db Apr 27, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

[F5] New CANCELED branch in checkOperationStatusForErrors is not exercised by any test — High

This branch is on the synchronous polling path — it is reached from pollTillOperationFinished (this file, lines 295 and 315) and is the actual code path for the "real customer" cancellation scenario: query is RUNNING, user calls Statement.cancel(), the next poll returns CANCELED_STATE.

The PR's only Thrift test (testGetStatementResult_cancelled_throwsWithHY008) calls accessor.getStatementResult(...) and exercises the standalone branch at line 407-409 — not this branch. Result: the polling-path CANCELED detection has zero coverage, and a regression in this block (e.g., a future refactor reordering the checks against isErrorStatusCode at line 805) would ship undetected.

Suggested fix: add a test where thriftClient.GetOperationStatus(...) returns RUNNING_STATE then CANCELED_STATE on consecutive calls (precedent: testGetStatementResult_pending already uses multi-call mocks), driving executeStatement and asserting HY008. This single test covers both the polling-loop path and this new branch.

}

if (statusResp.isSetOperationState() && isErrorOperationState(statusResp.getOperationState())) {
String errorMsg =
String.format(
Expand Down Expand Up @@ -863,6 +872,13 @@ private <T extends TBase<T, F>, F extends TFieldIdEnum> boolean hasResultDataInD
return directResults.isSetResultSet() && directResults.isSetResultSetMetadata();
}

private DatabricksSQLException cancelledStatementException(String statementId) {
String msg = String.format("Statement [%s] was cancelled", statementId);
LOGGER.info(msg);
return new DatabricksSQLException(
msg, OPERATION_CANCELLED_SQLSTATE, DatabricksDriverErrorCode.EXECUTE_STATEMENT_CANCELLED);
}

private boolean isErrorStatusCode(TStatus status) {
if (status == null || !status.isSetStatusCode()) {
LOGGER.error("Status code is not set, marking the response as failed");
Expand Down
35 changes: 35 additions & 0 deletions src/test/java/com/databricks/jdbc/api/impl/ExecutionStatusTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package com.databricks.jdbc.api.impl;

import static org.junit.jupiter.api.Assertions.*;

import com.databricks.jdbc.model.core.StatementStatus;
import com.databricks.sdk.service.sql.StatementState;
import org.junit.jupiter.api.Test;

class ExecutionStatusTest {

@Test
void cancelledStateWithNullError_hasMeaningfulMessage() {
StatementStatus cancelledStatus =
new StatementStatus().setState(StatementState.CANCELED).setError(null);
ExecutionStatus status = new ExecutionStatus(cancelledStatus);
assertNotNull(status.getErrorMessage(), "Cancelled state should have non-null error message");
assertTrue(status.getErrorMessage().contains("cancelled"));
}

@Test
void failedStateWithNullError_hasNullMessage() {
StatementStatus failedStatus =
new StatementStatus().setState(StatementState.FAILED).setError(null);
ExecutionStatus status = new ExecutionStatus(failedStatus);
assertNull(status.getErrorMessage(), "Failed state with null error should have null message");
}

@Test
void succeededStateWithNullError_hasNullMessage() {
StatementStatus succeededStatus =
new StatementStatus().setState(StatementState.SUCCEEDED).setError(null);
ExecutionStatus status = new ExecutionStatus(succeededStatus);
assertNull(status.getErrorMessage());
}
}
48 changes: 48 additions & 0 deletions src/test/java/com/databricks/jdbc/dbclient/impl/sqlexec/DatabricksSdkClientTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,54 @@ public void testCancelStatement() throws DatabricksSQLException, IOException {
eq(Void.class));
}

@Test
public void testHandleFailedExecution_CancelledState_ThrowsWithHY008() throws Exception {
IDatabricksConnectionContext connectionContext =
DatabricksConnectionContext.parse(JDBC_URL, new Properties());
DatabricksSdkClient databricksSdkClient =
new DatabricksSdkClient(connectionContext, statementExecutionService, apiClient);

StatementStatus cancelledStatus = new StatementStatus().setState(StatementState.CANCELED);
ExecuteStatementResponse response =
new ExecuteStatementResponse()
.setStatementId(STATEMENT_ID.toSQLExecStatementId())
.setStatus(cancelledStatus);

DatabricksSQLException exception =
assertThrows(
DatabricksSQLException.class,
() ->
databricksSdkClient.handleFailedExecution(
response, STATEMENT_ID.toSQLExecStatementId(), STATEMENT));

assertEquals("HY008", exception.getSQLState());
assertTrue(exception.getMessage().contains("was cancelled"));
}

@Test
public void testHandleFailedExecution_FailedState_ThrowsWithoutHY008() throws Exception {
IDatabricksConnectionContext connectionContext =
DatabricksConnectionContext.parse(JDBC_URL, new Properties());
DatabricksSdkClient databricksSdkClient =
new DatabricksSdkClient(connectionContext, statementExecutionService, apiClient);

StatementStatus failedStatus = new StatementStatus().setState(StatementState.FAILED);
ExecuteStatementResponse response =
new ExecuteStatementResponse()
.setStatementId(STATEMENT_ID.toSQLExecStatementId())
.setStatus(failedStatus);

DatabricksSQLException exception =
assertThrows(
DatabricksSQLException.class,
() ->
databricksSdkClient.handleFailedExecution(
response, STATEMENT_ID.toSQLExecStatementId(), STATEMENT));

assertNotEquals("HY008", exception.getSQLState());
assertTrue(exception.getMessage().contains("execution failed"));
}

@Test
public void testDisposition_arrowAndCloudFetchEnabled_usesExternalLinks() throws Exception {
setupClientMocks(true, false);
Expand Down
23 changes: 23 additions & 0 deletions src/test/java/com/databricks/jdbc/dbclient/impl/thrift/DatabricksThriftAccessorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,6 +449,29 @@ void testGetStatementResult_pending() throws Exception {
assertNull(resultSet.getMetaData());
}

@Test
void testGetStatementResult_cancelled_throwsWithHY008() throws Exception {
when(connectionContext.getDirectResultMode()).thenReturn(false);
accessor = spy(new DatabricksThriftAccessor(connectionContext));
doReturn(thriftClient).when(accessor).getThriftClient();

// Server returns CANCELED_STATE with OK_STATUS and null errorMessage
TGetOperationStatusResp cancelledResp =
new TGetOperationStatusResp()
.setStatus(new TStatus().setStatusCode(TStatusCode.SUCCESS_STATUS))
.setOperationState(TOperationState.CANCELED_STATE);
when(thriftClient.GetOperationStatus(any(TGetOperationStatusReq.class)))
.thenReturn(cancelledResp);

DatabricksSQLException exception =
assertThrows(
DatabricksSQLException.class,
() -> accessor.getStatementResult(tOperationHandle, null, session));

assertEquals("HY008", exception.getSQLState());
assertTrue(exception.getMessage().contains("was cancelled"));
}

@Test
void testListPrimaryKeys() throws TException, SQLException, DatabricksValidationException {
setup(false);
Expand Down
Loading