Skip to content

feat(csharp): httpPath-based protocol auto-detection (PECO-3055) [SEA] - #467

Open
eric-wang-1990 wants to merge 1 commit into
mainfrom
feat/csharp/PECO-3055-protocol-auto-detect
Open

feat(csharp): httpPath-based protocol auto-detection (PECO-3055) [SEA]#467
eric-wang-1990 wants to merge 1 commit into
mainfrom
feat/csharp/PECO-3055-protocol-auto-detect

Conversation

@eric-wang-1990

Copy link
Copy Markdown
Collaborator

What's Changed

Matches JDBC's compute-type-based protocol selection (DatabricksConnectionContext.getClientTypeFromContext) so the C# driver defaults to SEA for warehouse paths and forces Thrift for GP-cluster paths when adbc.databricks.protocol is not set explicitly. Adds the SEA → Thrift fallback on HTTP 307 from createSession, mirroring JDBC's DatabricksSession.open() handling of DatabricksTemporaryRedirectException.

Resolution order in new DatabricksDatabase.ResolveProtocol:

  1. Explicit adbc.databricks.protocol value wins (case-insensitive).
  2. Else inspect httpPath:
    • matches .*/o/.+/.+"thrift" (GP cluster forced)
    • matches .*/warehouses/.+ or .*/endpoints/.+"rest" (SEA default)
  3. Else fall back to "thrift" (preserves pre-3055 behavior for callers that omit httpPath, e.g. some integration-test fixtures).

The 307 fallback is gated on auto-detected protocol — explicit "rest" surfaces the 307 to the caller so the SEA-not-available signal is not silently swallowed.

Why

PECO-3055 — M3 explicit deliverable for the SEA-as-default rollout. Without this, the C# driver always defaults to Thrift even on warehouse paths, which is inconsistent with JDBC.

Red → Green proof

Ran against the live pecotesting warehouse (/sql/1.0/warehouses/2f03dd43e35e2aa0).

Before fix (WarehousePath_DefaultsToSeaProtocol_WhenProtocolNotSpecified):

AdbcDrivers.Databricks.Tests.ProtocolAutoDetectionE2ETests.WarehousePath_DefaultsToSeaProtocol_WhenProtocolNotSpecified [FAIL]
  Error Message:
   Assert.IsType() Failure: Value is not the exact type
Expected: typeof(AdbcDrivers.Databricks.StatementExecution.StatementExecutionConnection)
Actual:   typeof(AdbcDrivers.Databricks.DatabricksConnection)

Failed!  - Failed: 1, Passed: 3, Skipped: 0, Total: 4

After fix:

Passed!  - Failed: 0, Passed: 4, Skipped: 0, Total: 4, Duration: 1 s

Full DatabricksConnectionTest regression run (the largest adjacent class):

Passed!  - Failed: 0, Passed: 86, Skipped: 6, Total: 92, Duration: 1 m 1 s

The 6 skips are tests that were previously skipped via TestConfiguration.Protocol == "rest" and now correctly skip via the new DatabricksTestEnvironment.IsResolvedProtocolRest helper (same semantics, just correct for the new default).

Files touched

  • csharp/src/DatabricksDatabase.cs — new ResolveProtocol, IsTemporaryRedirectError, OpenThriftConnection, and GetHttpPath helpers; Connect() uses them.
  • csharp/test/E2E/ProtocolAutoDetectionE2ETests.cs (new) — four E2E tests covering warehouse-default-SEA, explicit-thrift-override, explicit-rest, GP-cluster-forces-thrift.
  • csharp/test/E2E/DatabricksTestEnvironment.cs — adds IsResolvedProtocolRest static helper.
  • 21 existing E2E test files — Skip.If(TestConfiguration.Protocol == "rest", ...) predicates replaced with Skip.If(DatabricksTestEnvironment.IsResolvedProtocolRest(TestConfiguration), ...) so they still skip when the resolved (not just explicit) protocol is SEA. Mechanical change, no test semantics altered.

Manual verification

  • New ProtocolAutoDetectionE2ETests passes against pecotesting warehouse (4/4)
  • DatabricksConnectionTest adjacent regression run passes (86 pass / 6 correctly skipped)
  • DriverTests smoke run passes (34 pass / 1 skipped)
  • Source builds clean on netstandard2.0 and net8.0 (no warnings, no errors)
  • Test project builds clean on net8.0
  • pre-commit run — sandbox-network restricted, blocked on installing setuptools from PyPI. Ruff/codespell hooks are Python-only and do not touch the C# files in this PR.

PECO-3055


This PR was created with GitHub MCP.

Match JDBC's compute-type-based protocol selection
(DatabricksConnectionContext.getClientTypeFromContext) so the C# driver
defaults to SEA for warehouse paths and forces Thrift for GP-cluster paths
when adbc.databricks.protocol is not set explicitly.

Resolution order (DatabricksDatabase.ResolveProtocol):
  1. Explicit adbc.databricks.protocol value wins (case-insensitive).
  2. Else inspect httpPath:
     - matches .*/o/.+/.+    -> "thrift" (GP cluster forced)
     - matches .*/warehouses/.+ or .*/endpoints/.+ -> "rest" (SEA default)
  3. Else fall back to "thrift" (preserves pre-3055 behavior for callers
     that omit httpPath, e.g. some integration-test fixtures).

Also adds the SEA -> Thrift fallback on HTTP 307 from createSession,
mirroring JDBC's DatabricksSession.open() handling of
DatabricksTemporaryRedirectException. The fallback is gated on auto-
detected protocol — explicit "rest" surfaces the 307 to the caller so
the SEA-not-available signal is not silently swallowed.

Test changes:
- New csharp/test/E2E/ProtocolAutoDetectionE2ETests.cs covers all three
  resolution paths (warehouse-default-SEA, explicit-thrift-override,
  explicit-rest, GP-cluster-forces-thrift). Red proof captured against
  the pecotesting warehouse: IsType<StatementExecutionConnection> failed
  with "Actual: typeof(DatabricksConnection)". Green after fix.
- DatabricksTestEnvironment.IsResolvedProtocolRest helper centralizes
  the "is this run effectively SEA?" check for existing Skip.If guards
  that previously compared TestConfiguration.Protocol to the literal
  "rest" (which silently misses the auto-detected case). 19 existing
  tests' Skip.If predicates are updated to use it; same skip semantics
  as before, just correct for the new default.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant