feat(csharp): honor cloudfetch.lz4.enabled on SEA path (PECO-3056)#465
Open
eric-wang-1990 wants to merge 8 commits into
Open
feat(csharp): honor cloudfetch.lz4.enabled on SEA path (PECO-3056)#465eric-wang-1990 wants to merge 8 commits into
eric-wang-1990 wants to merge 8 commits into
Conversation
…SEA path (PECO-3056) [SEA] The SEA path previously silently ignored adbc.databricks.cloudfetch.enabled and adbc.databricks.cloudfetch.lz4.enabled — both are JDBC-compatible connection-level params that the Thrift path honors via the canDownloadResult and canDecompressLZ4Result flags. Wire them through StatementExecutionConnection: - cloudfetch.lz4.enabled=false clears result_compression on the wire, mirroring JDBC's CompressionCodec.NONE branch in DatabricksSdkClient. Default (true) preserves any explicit adbc.databricks.rest.result_compression value. - cloudfetch.enabled=false: JDBC pairs this with disposition=INLINE + format=JSON_ARRAY because the SEA server rejects ARROW_STREAM with pure INLINE. The C# driver does not yet ship a JSON_ARRAY reader, so this combination now throws AdbcStatusCode.NotImplemented at connect time with a clear message pointing to the workaround (use protocol=thrift, or drop the param and use the default INLINE_OR_EXTERNAL_LINKS hybrid disposition). This is the honest signal: the param is no longer silently ignored. PECO-3056 is the foundation for four dependents (PECO-3061, 3063, 3067, 3068) that wire individual cloudfetch sub-params on SEA. Those operate when cloudfetch.enabled=true (the default), so this NotImplemented branch does not block them. Reyden caveat: Reyden does not currently generate external links and coerces any disposition to INLINE server-side; the driver request still carries whatever the user asked for. Test seam: StatementExecutionStatement.LastExecuteRequest (internal) captures the request the driver last handed to the SEA client. E2E tests assert on disposition / result_compression there rather than intercepting HTTP. CloudFetchE2ETest: the existing TestCloudFetch theory had cases with useCloudFetch=false + protocol=rest that were accidentally validating the silent-ignore bug as if it were intended behavior. Those cases now Skip.If(...) with a justification pointing to this ticket. PECO-3056
…sable path cloudfetch.enabled=false on SEA requires disposition=INLINE + format=JSON_ARRAY, which the C# driver cannot yet serve (no JSON_ARRAY reader). Rather than throwing NotImplemented and breaking existing users, defer that path and silently ignore the param — consistent with the pre-PR behavior. The lz4.enabled wiring is complete and is the only behavioral change in this PR. - Remove NotImplemented throw for cloudfetch.enabled=false on SEA - Remove CloudFetchEnabledFalse_ThrowsNotImplementedOnSea test - Revert CloudFetchE2ETest Skip.If that was added to paper over the throw - Update DatabricksParameters XML doc to reflect deferred status Co-authored-by: Isaac
eric-wang-1990
commented
May 27, 2026
| /// JDBC pairs <c>cloudfetch.enabled=false</c> with | ||
| /// <c>disposition=INLINE</c> + <c>format=JSON_ARRAY</c>, but the | ||
| /// C# SEA driver does not yet ship a JSON_ARRAY reader. Support | ||
| /// is deferred until a JSON_ARRAY reader lands. |
Collaborator
Author
There was a problem hiding this comment.
We are waiting the Arrow_stream inline result is ready from server
Collaborator
Author
There was a problem hiding this comment.
✅ Done — updated the doc to say the blocker is server-side (waiting for ARROW_STREAM + INLINE disposition support from the server), not a missing JSON_ARRAY reader.
This comment was generated with GitHub MCP.
Remove stale references to the NotImplemented cloudfetch.enabled=false behavior and drop the unused System import. Co-authored-by: Isaac
Co-authored-by: Isaac
…h doc - StatementExecutionConnection: derive result_compression from CanDecompressLz4 only (LZ4_FRAME when true, null when false). The driver has no gzip decoder so there is no other valid non-null value; passing through a user-supplied ResultCompression param was misleading. - DatabricksParameters.UseCloudFetch doc: correct the SEA note to say the blocker is server-side (ARROW_STREAM + INLINE disposition not yet supported), not a missing JSON_ARRAY reader. - SeaCloudFetchParamsE2ETests: rename Lz4EnabledTrue test to Lz4EnabledTrue_UsesLz4Compression and adjust assertion accordingly. Co-authored-by: Isaac
eric-wang-1990
changed the title
…ry results Replace wire-level assertions on LastExecuteRequest with end-to-end behavior checks: run a real query and assert the correct row count. If result_compression is set incorrectly the server rejects the request or returns data the driver cannot decode, so a successful round-trip is sufficient proof. Co-authored-by: Isaac
…comment A behavioral "query succeeds" test cannot verify what compression value the driver sent — the query succeeds regardless. The wire-level seam is the only way to assert driver intent without full HTTP interception. Added a comment to StatementExecutionStatement explaining why the seam exists and to the test class explaining why a behavioral substitute is insufficient. Co-authored-by: Isaac
…-sea-cloudfetch-enabled # Conflicts: # csharp/src/StatementExecution/StatementExecutionConnection.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What's Changed
Wires
adbc.databricks.cloudfetch.lz4.enabledthroughStatementExecutionConnectionon the SEA path. Previously this param was silently ignored on SEA.cloudfetch.lz4.enabled=falseclearsresult_compressionon the wire (null → server treats as no compression). Mirrors JDBC'sCompressionCodec.NONEbranch inDatabricksSdkClient.cloudfetch.lz4.enabled=true(default) setsresult_compression=LZ4_FRAME. The driver only implements LZ4 decompression; there is no other valid non-null value.cloudfetch.enabled=falseis not wired on SEA. Disabling CloudFetch on SEA requires the server to supportARROW_STREAMresults withINLINEdisposition. That server-side capability is not yet available, so the param is accepted but silently ignored on SEA for now. Support will be added in a follow-on once the server side lands.Test seam:
StatementExecutionStatement.LastExecuteRequest(internal) captures the request the driver last handed to the SEA client. E2E tests assert onresult_compressionthere rather than intercepting HTTP.Why
PECO-3056 — https://databricks.atlassian.net/browse/PECO-3056
The M3 remaining-parameter-gap audit confirmed
cloudfetch.lz4.enabledwas ignored on SEA. This is the foundation for dependent CloudFetch sub-param tickets (PECO-3061, 3063, 3067, 3068), all of which operate withcloudfetch.enabled=true(the default).Red → Green proof
E2E tests run against the live
pecotestingwarehouse (DATABRICKS_TEST_CONFIG_FILEset).Before fix (test seam added, no production logic — params still silently ignored):
After fix:
Safety run —
StatementExecutionDriverE2ETests(11 tests, CREATE/INSERT/UPDATE/DELETE/SELECT round-trips on SEA):Files touched
csharp/src/DatabricksParameters.cs— XML doc clarifying SEA-path semantics forUseCloudFetch/CanDecompressLz4.csharp/src/StatementExecution/StatementExecutionConnection.cs— deriveresult_compressionfromCanDecompressLz4only (LZ4_FRAMEor null).csharp/src/StatementExecution/StatementExecutionStatement.cs— internalLastExecuteRequesttest seam.csharp/test/E2E/StatementExecution/SeaCloudFetchParamsE2ETests.cs— 3 E2E tests covering default disposition, lz4-off, and lz4-on.Manual verification
dotnet buildgreen onnetstandard2.0andnet8.0pecotestingwarehouseStatementExecutionDriverE2ETests(full class) passes — no regressionPECO-3056