Skip to content

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

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

Add session version exchange to SQL Exec API client#1640
aakash-saravanan-db wants to merge 4 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.

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