[4/7] Implement native Thrift parameter batching - #1625
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>
|
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 Low
Looks good — clean, well-factored opt-in native Thrift batch path with solid unit coverage (routing, fallback, repeat expansion, ordinal mapping). Ordinal 0-based remap, unsetParameters/unsetResultRowLimit, the exact-42P02 legacy fallback, and 1-based column-index checks in getBatchUpdateCounts all check out. One low-severity note about EXECUTE_FAILED being reported when a successful batch returns an unexpected result shape.
Scope: reviewed as a stacked increment (7d19fed..head); the parent's changes are reviewed on the parent PR.
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>
|
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: 29c3268 |
There was a problem hiding this comment.
Verdict: 1 Low
Solid, well-tested increment — native Thrift batch execution is opt-in, gated correctly, and the result-count extraction (with repeat-column expansion, maxRows bypass, and cardinality validation) is defensive and covered by unit tests. No correctness or resource-lifecycle bugs found. One low-severity robustness note on the fallback detection heuristic, plus a nit on the exception message.
Scope: reviewed as a stacked increment (7d19fed..head); the parent's changes are reviewed on the parent PR.
sreekanth-db
changed the title
1 participant
🥞 Stacked PR
Use this link to review incremental changes.
Description
Add opt-in native
PreparedStatementbatch execution over Thrift.TExecuteStatementReq.batchParametersin one request.42P02 [UNBOUND_SQL_PARAMETER]errors.BatchUpdateExceptionwith backend SQLState, vendor code, and cause.Native batching remains disabled by default through
EnableNativeBatching=0.Testing
Additional Notes to the Reviewer
Real backend validation showed successful batch results may compress identical counts using the
repeatcolumn; the driver expands these rows back to one JDBC count per submitted parameter set.NO_CHANGELOG=true