Skip to content

Add session version exchange to SQL Exec API client - #1640

Open
aakash-saravanan-db wants to merge 7 commits into
mainfrom
session-version-exchange
Open

Add session version exchange to SQL Exec API client#1640
aakash-saravanan-db wants to merge 7 commits into
mainfrom
session-version-exchange

Conversation

@aakash-saravanan-db

@aakash-saravanan-db aakash-saravanan-db commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Description

  • Request the execution mode used by versioned sessions.
  • Initialize session-local version tracking from the create response when present.
  • Send the highest observed version on execute requests and update it from execute, polling, result, and heartbeat responses.
  • Accept create responses without a version and omit the execute field until a later response supplies one.
  • Record the originating session ID for each executed statement and apply polling updates only when that ID matches the current session.
  • Treat reattached statement handles as having unknown ownership, so polling them cannot advance another connection.

Thrift behavior is unchanged.

Testing

  • Focused ownership, session, SDK client, statement, result-set, and serialization tests: 215 passed
  • Complete unit sweep: 3,608 tests exercised; the socket-dependent suite was rerun with loopback access and all 27 tests passed
  • SQL Exec integration suite: 241 tests passed using the existing unversioned recordings
  • mvn -q spotless:check
  • git diff --check

Signed-off-by: Aakash Saravanan <aakash.saravanan@databricks.com>

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Verdict: 1 Low

Looks solid — the session-version exchange is implemented coherently (session-identity guard, atomic max-accumulation, reattach exclusion, clear-on-close) and is well covered by unit + serialization tests. One Low note about the unconditional execution_mode=FAST on every SEA session. The heartbeat path's parentStatement.shouldTrackSessionVersion() deref is safe (guarded by the null/statementId early-return at DatabricksResultSet:401), and equals/hashCode were correctly kept in sync on the touched model classes.

Signed-off-by: Aakash Saravanan <aakash.saravanan@databricks.com>

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Verdict: 1 Medium

Looks solid overall — well-tested, correctly synchronized version tracking with monotonic max, session-identity guarding, and reattached-statement exclusion. One medium design note: the new execution_mode=FAST / session-version exchange is applied to all SEA sessions with no opt-out gate, which diverges from the repo's usual feature-flag rollout pattern.

Signed-off-by: Aakash Saravanan <aakash.saravanan@databricks.com>

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Verdict: 1 Low

Looks good — a clean, well-tested addition of SQL Exec session-version exchange, and it incidentally fixes a real bug where the poll-loop GET was serializing the full ExecuteStatementRequest as its body. One Low note on thread-visibility of the new originatingSessionId field. Changelog entry and unit/serialization coverage are present.

Comment thread src/main/java/com/databricks/jdbc/api/impl/DatabricksStatement.java Outdated
Signed-off-by: Aakash Saravanan <aakash.saravanan@databricks.com>

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Verdict: 1 Low

Looks good — clean, well-tested session-version exchange. The monotonic version tracking is properly session-scoped (open/close/forceClose clear it, updateSessionVersion guards on matching open session id under a lock), and reattached statement handles correctly carry null ownership so polling can't advance another connection's version. Thrift path and the 1-arg checkStatementAlive delegate unchanged. One low-severity note inline about the unconditional execution_mode=FAST on session creation.

Signed-off-by: Aakash Saravanan <aakash.saravanan@databricks.com>
Signed-off-by: Aakash Saravanan <aakash.saravanan@databricks.com>

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Verdict: 1 Low

Looks good — solid, well-tested session-version exchange with correct monotonic-max/close semantics and a nice poll-request serialization fix. One low design note about the unconditional execution_mode=FAST lacking a kill-switch; may be intentional.

new CreateSessionRequest().setWarehouseId(((Warehouse) warehouse).getWarehouseId());
new CreateSessionRequest()
.setWarehouseId(((Warehouse) warehouse).getWarehouseId())
.setExecutionMode(SessionExecutionMode.FAST);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Low — createSession now sends execution_mode=FAST unconditionally on every SEA session creation, with no connection-parameter opt-out or kill-switch. Unlike other recently added server-facing behaviors in this driver (e.g. EnableThriftNativeMetadata, EnableSeaSyncMetadata, hybrid results), this changes the requested execution semantics for all SEA connections at once. If the FAST/versioned-session path regresses on some warehouse/DBR versions, there is no way to disable it short of a new release. Consider gating this behind a connection flag (even a default-on one) so it can be turned off in the field. If sending FAST unconditionally is a deliberate, coordinated rollout with the server team, this can be dismissed.

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Verdict: 1 Medium

Solid, well-tested change — session-version exchange is correctly synchronized (open/close/update all hold synchronized(this), so no post-close resurrection), the poll loop now serializes a proper GetStatementRequest, and getStatementResult intentionally skips version advancement to match the reattach/unknown-ownership design. One medium concern: FAST execution mode is enabled unconditionally for all SEA sessions with no opt-out, and the changelog documents that this changes async statement-visibility semantics.

new CreateSessionRequest().setWarehouseId(((Warehouse) warehouse).getWarehouseId());
new CreateSessionRequest()
.setWarehouseId(((Warehouse) warehouse).getWarehouseId())
.setExecutionMode(SessionExecutionMode.FAST);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Medium — createSession now unconditionally sets SessionExecutionMode.FAST on every SEA session, with no connection-parameter gate or opt-out. As the changelog itself documents, this silently changes async-execution semantics: on Lakehouse Real-Time, polling an async statement to completion no longer guarantees its session-scoped changes (e.g. SET, USE, temp views) are visible to subsequent statements. Any existing caller relying on executeStatementAsync + poll → run-dependent-statement will break with no way to revert to the prior behavior.

Other new SEA behaviors in this repo are gated behind connection params (e.g. EnableThriftNativeMetadata, EnableSeaSyncMetadata). Consider gating FAST mode behind a connection parameter (defaulting to preserve prior behavior, or at least providing an opt-out) so users hitting the async-visibility regression have a supported escape hatch. If the intent is that non-real-time warehouses ignore this server-side, that assumption is worth confirming and calling out, since the flag is sent to all warehouse types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant