[6/7] Add native batch integration coverage - #1633
Conversation
Keep existing batching behavior behind a dedicated executor so native routing can be added without changing the compatibility fallback. Signed-off-by: Sreekanth Vadigi <sreekanth.vadigi@databricks.com>
Introduce a dormant opt-in and deterministic parameter snapshots so both native transports can share the same ordered payload without changing execution behavior. Signed-off-by: Sreekanth Vadigi <sreekanth.vadigi@databricks.com>
Snapshot prepared parameter sets at addBatch time and route eligible batches through an injectable native executor while preserving the legacy production path. Signed-off-by: Sreekanth Vadigi <sreekanth.vadigi@databricks.com>
Accept the immutable parameter map produced by the shared batch model instead of requiring a concrete HashMap. Signed-off-by: Sreekanth Vadigi <sreekanth.vadigi@databricks.com>
Route eligible prepared batches through compute-aware Thrift batchParameters, preserve legacy compatibility fallback, and map repeat-compressed backend results to ordered JDBC update counts. Signed-off-by: Sreekanth Vadigi <sreekanth.vadigi@databricks.com>
Surface post-success count-read errors as RESULT_SET_ERROR and clear committed batches before propagating so retries cannot duplicate inserted rows. Signed-off-by: Sreekanth Vadigi <sreekanth.vadigi@databricks.com>
Submit ordered parameter_sets through the existing SEA execution lifecycle so opt-in prepared batches share the same routing, fallback, and JDBC result handling as Thrift. Signed-off-by: Sreekanth Vadigi <sreekanth.vadigi@databricks.com>
Record real SEA and Thrift responses to guard native parameter batching end to end. Signed-off-by: Sreekanth Vadigi <sreekanth.vadigi@databricks.com>
|
JDBC integration tests triggered ( |
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 1 Medium
Test-only PR (one benign debug log + WireMock replay fixtures + a new native-batch integration test). Cleanup logic correctly mirrors the sibling THRIFT+REPLAY skip pattern. One medium concern: the SEA request-body assertion uses equalToJson(..., true, true) and omits parameter values, so it does not actually verify the ordering it claims to.
Scope: reviewed as a stacked increment (8b0bfbb..head); the parent's changes are reviewed on the parent PR.
Validate exact parameter values and array order so replay coverage detects reordered native batches. Signed-off-by: Sreekanth Vadigi <sreekanth.vadigi@databricks.com>
|
Integration test approval reset. New commits were pushed to this PR. Label(s) A maintainer must re-review and re-add a label to preview tests again. (The real gate runs in the merge queue.) Latest commit: 07968b8 |
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — test-only PR (fake-service replay coverage for native PreparedStatement batching) plus one benign debug log line. The cleanUp() condition correctly mirrors the sibling close-unless-THRIFT+REPLAY convention, and the SEA verify() uses ignoreArrayOrder=false to properly assert ordered parameter_sets. One low-severity robustness note on the Thrift routing workaround in setUp.
Scope: reviewed as a stacked increment (8b0bfbb..head); the parent's changes are reviewed on the parent PR.
|
JDBC integration tests triggered ( |
sreekanth-db
changed the title
1 participant
🥞 Stacked PR
Use this link to review incremental changes.
Description
parameter_setsrequest and both transports return ordered JDBC update counts.Testing
FAKE_SERVICE_TYPE=SQL_EXEC FAKE_SERVICE_TEST_MODE=REPLAY mvn test -pl jdbc-core -Dtest=NativePreparedStatementBatchIntegrationTests -Ddependency-check.skip=trueFAKE_SERVICE_TYPE=THRIFT_SERVER FAKE_SERVICE_TEST_MODE=REPLAY mvn test -pl jdbc-core -Dtest=NativePreparedStatementBatchIntegrationTests -Ddependency-check.skip=truemvn test -pl jdbc-core -Dtest=PreparedStatementBatchExecutorTest,DatabricksSdkClientTest,DatabricksThriftServiceClientTest -Ddependency-check.skip=true(119 tests)isaac review --uncommitted(0 findings)Additional Notes to the Reviewer
Only real recorded native-success responses are included. Compatibility fallback remains covered by unit tests because the available backends support native batching and do not emit the legacy fallback signal.
NO_CHANGELOG=true