[7/7] Enable insert batching by default - #1634
Conversation
sreekanth-db
changed the title
![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
Clean activation PR — verified the native/legacy routing so enabling both defaults is safe (native prioritized, legacy fallback, no double execution) and the test updates match the new defaults. One low note on the legacy fallback's hardcoded per-row update counts becoming the default behavior. Reminder (already documented by the author): this is intentionally a draft and must not merge until native BINARY parameter support lands on SEA and Thrift.
Scope: reviewed as a stacked increment (07968b8..head); the parent's changes are reviewed on the parent PR.
| "Timeout in seconds for metadata polling operations (e.g. GetTables, GetColumns). 0 means no timeout", | ||
| "300"), | ||
| ENABLE_BATCHED_INSERTS("EnableBatchedInserts", "Enable batched INSERT optimization", "0"), | ||
| ENABLE_BATCHED_INSERTS("EnableBatchedInserts", "Enable batched INSERT optimization", "1"), |
There was a problem hiding this comment.
🔵 Low — Flipping EnableBatchedInserts to 1 by default turns on the legacy client-side multi-row rewrite for the DBR < 18.2 fallback path. Note that this path does not read real per-row update counts from the server — executeBatchedInsert hardcodes allUpdateCounts[i] = 1 for every row (see the loop that sets each chunk's counts). So on the new default, callers that inspect executeBatch() return values will always see 1 per row rather than the actual affected-row count, whereas the previous default (batched inserts off) returned the server's real getUpdateCount() per statement. This is the intended design of the batching feature, but it is a user-visible behavior change worth calling out explicitly in the changelog entry (which currently only describes native batching's parameter/size limits, not the update-count semantics of the legacy fallback).
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>
Validate exact parameter values and array order so replay coverage detects reordered native batches. Signed-off-by: Sreekanth Vadigi <sreekanth.vadigi@databricks.com>
Activate native server-side batching while enabling the legacy rewrite fallback for older compute, with explicit kill switches for both paths. Signed-off-by: Sreekanth Vadigi <sreekanth.vadigi@databricks.com>
Document the DBR version requirement, backend limits, and legacy configuration for binary batches. Signed-off-by: Sreekanth Vadigi <sreekanth.vadigi@databricks.com>
8cb3849 to
f4e3ff2
Compare
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a clean activation PR. Default flips (EnableBatchedInserts/EnableNativeBatching → 1), the supportsBatchUpdates() OR of both strategies, and the fake-service opt-out are all correct and covered by the updated unit tests. One low note: the fixture-preserving opt-out is applied only in the two getValidJDBCConnection helpers, not the M2M/Benchfood URL builders (harmless today since those back only auth/metadata replays).
Scope: reviewed as a stacked increment (2d0f4da..head); the parent's changes are reviewed on the parent PR.
| FakeServiceConfigLoader.shouldUseThriftClient()); | ||
| // Existing replay fixtures characterize the previous batching defaults. Batching-specific | ||
| // tests opt in explicitly so activation does not invalidate unrelated recordings. | ||
| connectionProperties.putIfAbsent( |
There was a problem hiding this comment.
🔵 Low — The batching opt-out (ENABLE_NATIVE_BATCHING=0 / ENABLE_BATCHED_INSERTS=0) is applied only in the two getValidJDBCConnection fake-service paths. The other fake-service URL builders (getFakeServiceM2MUrl, getFakeServiceM2MPrivateKeyCredentialsUrl, getFakeServiceBenchfoodJDBCUrl) build connections that now pick up the new default =1 values. Today those entrypoints back only auth and catalog-metadata replay tests (no PreparedStatement INSERT batches), so no recording is invalidated by this change. But if a future replay test that exercises INSERT batching is added on those URLs, it will silently diverge from its recorded fixture because the guard lives in the helper rather than being centralized. Consider centralizing the batching-default pin so every fake-service connection inherits it, rather than duplicating it in two call sites.
1 participant
🥞 Stacked PR
Use this link to review incremental changes.
Description
PreparedStatementINSERT batches on DBR 18.2 and later.EnableNativeBatching=0andEnableBatchedInserts=0as independent kill switches.BINARYlimitation.Testing
Additional Notes to the Reviewer
BINARYvalues. Binary batches must use legacy client-side batching withEnableNativeBatching=0andsupportManyParameters=1(EnableBatchedInsertsdefaults to1).1for each submitted row because the backend returns a count for the combined multi-row statement rather than separate per-row counts.