perf: Native Flight SQL prepared statements, statement caching, and transport tuning (v0.7.0) - #50
Conversation
…ransport tuning (v0.7.0) Replace the ADBC-based parameterized query path with Arrow Flight SQL prepared statements running on the SDK's tuned gRPC channels, and fix the performance issues found in a full SDK audit: - queryWithParams now shares the primary channels: inherits dns:/// re-resolution, HTTP/2 keep-alive, and (new for this path) mTLS. Removes adbc-driver-flight-sql and adbc-core. - Prepared statement cache keyed by SQL (default 64, 0 disables): repeated queries skip Create/ClosePreparedStatement round trips (5 -> 3 RPCs per call), with transparent re-prepare on stale handles. - New SDK-local FlightInfoReader consumes all endpoints of a result. - withChannelCount(n): round-robin connection pool; query hot path is now lock-free (volatile channel snapshot). - withQueryTimeout(d): deadline for planning/prepare/bind RPCs; refreshDataset gains a 60s request timeout. - Retry backoff fixed from 1-2ms fibonacci to exponential + jitter (~250ms..10s); UNAUTHENTICATED triggers one automatic re-handshake. - Fixed reset() vs queryWithParams NPE race via per-attempt snapshots. - Param bind roots sized for 1 row (was ~48KB per string param/query); user agent cached; HttpClient lazy; Windows os.version fix. - Netty aligned: netty-all 4.2.12 (unsupported under grpc-netty 1.79) replaced with netty-bom 4.1.130.Final + grpc-bom 1.79.0 + Linux epoll natives; BouncyCastle now an explicit dependency. Testing: new in-process mock Flight SQL server with per-RPC counters and failure injection; suites for both query paths, statement cache, retries/backoff, timeouts, token-expiry recovery, channel pool, multi-endpoint reads, refresh HTTP; PerfBenchmarkTest asserts the round-trip contract and prints latency percentiles. 257 tests, 0 failures; SpotBugs clean.
There was a problem hiding this comment.
Pull request overview
This PR delivers a v0.7.0 performance-focused overhaul of the Java SDK by moving queryWithParams off ADBC onto native Arrow Flight SQL prepared statements, adding statement caching, and introducing transport tuning (channel pooling, query timeouts, improved retry/backoff, auto re-auth). It also aligns Netty/gRPC dependencies, improves allocation behavior for parameter binding, and adds extensive local/in-process test coverage plus release notes.
Changes:
- Replace ADBC-based parameterized queries with native Flight SQL prepared statements + bounded statement cache and a new
FlightInfoReaderthat consumes all endpoints. - Add transport/resilience features: multi-channel pool, query planning/prepare/bind timeout, exponential backoff with jitter, and automatic re-handshake on
UNAUTHENTICATED. - Align dependency stack (Netty/gRPC BOMs, epoll natives, explicit BouncyCastle) and expand test suite with an in-process mock Flight SQL server.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/ai/spice/TpchIntegrationTest.java | Cache/probe TPC-H availability once per class; avoid per-test retry backoff. |
| src/test/java/ai/spice/TestFlightSqlServer.java | New in-process Flight SQL server with RPC counters and failure injection for deterministic tests. |
| src/test/java/ai/spice/StatementCacheTest.java | New tests validating prepared statement cache behavior, invalidation, and concurrency. |
| src/test/java/ai/spice/SpiceClientBuilderTest.java | New builder validation tests for channel count, query timeout, and statement cache size. |
| src/test/java/ai/spice/ResilienceTest.java | New tests for retry/backoff, timeout, auto re-auth, channel pool, and reset/query race regression. |
| src/test/java/ai/spice/ResetTest.java | Update local-runtime tests to disable retries for faster no-server paths; adjust docs/comments post-ADBC removal. |
| src/test/java/ai/spice/RefreshHttpTest.java | New tests for refreshDataset via a local HTTP server (headers/body/status handling). |
| src/test/java/ai/spice/PerfBenchmarkTest.java | New in-process micro-benchmarks asserting deterministic RPC-count performance contracts. |
| src/test/java/ai/spice/ParamRootTest.java | New tests ensuring parameter roots are right-sized and type-mapping is correct. |
| src/test/java/ai/spice/ParameterizedQueryTest.java | Speed up local runtime parameterized-query tests by disabling retries on no-server paths. |
| src/test/java/ai/spice/LocalFlightServerTest.java | New end-to-end tests for both query paths against the in-process Flight SQL server. |
| src/test/java/ai/spice/FlightQueryTest.java | Disable retries for faster skip/no-server path under real backoff behavior. |
| src/test/java/ai/spice/FlightInfoReaderTest.java | New unit tests for multi-endpoint reader edge cases and bytesRead() accumulation. |
| src/main/java/ai/spice/Version.java | Bump SDK version constant to 0.7.0. |
| src/main/java/ai/spice/SpiceClientBuilder.java | Add builder options for channel count, query timeout, and prepared statement cache size with validation. |
| src/main/java/ai/spice/SpiceClient.java | Core refactor: channel pool, prepared-statement execution + cache, new retry/backoff, lazy HTTP client, auth recovery, allocation fixes. |
| src/main/java/ai/spice/FlightInfoReader.java | New ArrowReader that consumes all endpoints of a FlightInfo using unload/load zero-copy transfer. |
| src/main/java/ai/spice/Config.java | Compute user agent once per process and fix Windows OS version formatting. |
| README.md | Document new performance tuning options, backoff behavior, and prepared-statement parameterized queries. |
| pom.xml | Align Netty/gRPC via BOMs; remove ADBC + netty-all; add epoll natives and explicit BouncyCastle. |
| docs/release_notes/v0.7.0.md | New release notes for v0.7.0 covering features, deps, and testing. |
| docs/parameterized_queries.md | Update docs to reflect statement reuse + shared channel pool (ADBC removal). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…tion Review comments (github-code-quality): - LocalFlightServerTest: widen to long before multiplication in row-count assertion - TestFlightSqlServer: parse endpoint indexes defensively, converting malformed ticket handles into INVALID_ARGUMENT instead of an uncaught NumberFormatException (both getStream methods, shared helper) HikariCP interop: - New HikariCpCompatTest: HikariCP-pooled connections to Spice through the Arrow Flight SQL JDBC driver — plain and prepared statements, connection reuse, 4-thread concurrency, and authenticated pools using the same handshake credentials as the native client - TestFlightSqlServer now advertises dataset + parameter schemas in CreatePreparedStatement results (JDBC/Avatica clients treat an empty dataset schema as an UPDATE and route execution to DoPut) - README: connection pooling guidance (native SpiceClient needs no pool; HikariCP via Flight SQL JDBC for JDBC ecosystems); release notes updated - Test-scope deps: HikariCP 5.1.0, flight-sql-jdbc-core 19.0.0 262 tests, 0 failures; SpotBugs clean.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/main/java/ai/spice/SpiceClient.java:1546
- queryInternal() assumes FlightInfo always contains at least one endpoint and unconditionally reads endpoints.get(0). If a server returns a valid FlightInfo with zero endpoints (e.g., empty result or misconfigured producer), this will throw IndexOutOfBoundsException instead of surfacing a meaningful Flight error.
FlightInfo flightInfo = channel.client.execute(sql, channel.callOptions);
List<FlightEndpoint> endpoints = flightInfo.getEndpoints();
if (endpoints.size() > 1) {
logger.warn("Server returned {} endpoints; query() consumes only the first. "
+ "Use queryWithParams() (ArrowReader) to consume all endpoints.", endpoints.size());
}
Ticket ticket = endpoints.get(0).getTicket();
return channel.client.getStream(ticket, channel.streamOptions);
…eai/spice-java into perf/flight-sql-prepared-statements
…nt ticket independence - queryInternal now surfaces a clear INTERNAL Flight error when the server returns a FlightInfo with zero endpoints instead of IndexOutOfBoundsException - Expanded the statement-cache comment explaining why returning a statement before the DoGet completes is safe (tickets are self-contained; the previous implementation closed statements outright with readers open)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/main/java/ai/spice/SpiceClient.java:1545
queryInternalassumesflightInfo.getEndpoints()is non-empty and unconditionally accessesendpoints.get(0). Some Flight servers can legally return aFlightInfowith zero endpoints (e.g., for empty/DDL-like results), which would throwIndexOutOfBoundsExceptioninstead of surfacing a protocol error.
try {
…line CI caught a race in ResilienceTest.testConcurrentResetAndQueryWithParams: reset() called FlightClient.close(), which closes the client's buffer allocator immediately — an in-flight query on the old channel then hit 'Attempting operation on allocator when allocator is closed' (not a FlightRuntimeException, so not retryable) and leaked its in-flight buffer. reset()/auth-rebuild now retire old channels: the gRPC transport is shut down gracefully (in-flight RPCs and open result streams complete; new RPCs fail with retryable UNAVAILABLE) while the Flight client and its allocator stay open until the transport reports terminated. Retired channels are swept on subsequent resets and force-closed in close(). Tightened the concurrent reset test interleaving (8 resets/15ms, 40 queries/5ms) and verified 20 consecutive green runs locally.
catch (Exception e) { ...; throw e; } compiled via Java 7 precise rethrow
(the try block can only throw IOException or unchecked exceptions), but it
read as a compile error to reviewers. catch (IOException | RuntimeException)
is equivalent and self-evidently correct.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/main/java/ai/spice/SpiceClient.java:1553
- refreshDataset(...) does not guard against being called after the client is closed. Other public operations (e.g., query/queryWithParams/reset) throw IllegalStateException when closed, so this method should do the same to avoid surprising behavior (e.g., silently creating an HttpClient and issuing requests after close()).
HttpRequest.Builder builder = HttpRequest.newBuilder()
.uri(new URI(String.format("%s/v1/datasets/%s/acceleration/refresh", this.httpAddress, dataset)))
.timeout(HTTP_REQUEST_TIMEOUT)
.header("Content-Type", "application/json")
.header("X-Spice-User-Agent", Config.getUserAgent());
src/main/java/ai/spice/SpiceClient.java:1441
- runtimeStatus() can still be invoked after SpiceClient.close(), which is inconsistent with query/queryWithParams/reset behavior and can yield confusing failures. Consider failing fast with IllegalStateException when the client is closed.
This issue also appears on line 1549 of the same file.
logger.debug("Fetching runtime status");
try {
HttpRequest request = buildProbeRequest(this.httpAddress, "/v1/status", this.apiKey);
HttpResponse<String> response = httpClient().send(request,
…ardening (#51) * docs: v0.7.0 release preparation — release notes, upgrade guide, QA hardening - Release notes now cover the full v0.6.0..v0.7.0 delta: mTLS client certificates (#46) and health/readiness/status checks (#48) were never released and join the perf overhaul (#50); dependency table corrected against what v0.6.0 actually shipped (ADBC 0.22.0) - New upgrade guide: docs/upgrade_guides/v0.6.0-to-v0.7.0.md (behavior changes, dependency-tree impacts, new capabilities) - README installation snippets bumped to 0.7.0 - Integration-test hardening found during live-runtime QA: - testRefreshWithOptionsSpiceOSS was rerun-unsafe: its refresh_sql LIMIT persists in the accelerated table across runs and refreshes are async. Now self-heals the precondition, polls instead of fixed sleep, and restores the dataset afterward - availability probes use one retry instead of zero (resilient gating, still fast when no runtime is present) QA: 284 tests green twice consecutively against a live spiced quickstart (taxi_trips, 2.9M rows) plus mock-only runs; jar/sources/javadoc build clean. * fix: address Copilot review — test resource management and comment accuracy - FlightQueryTest: all four tests now close SpiceClient (and FlightStream) via try-with-resources - testRefreshWithOptionsSpiceOSS: shrink/verify runs in try/finally with a guaranteed restore that waits for the async refresh to land; restore failures are logged, never masking the primary test failure - TpchIntegrationTest: probe comment updated to match withMaxRetries(1) Validated against a live spiced quickstart: FlightQueryTest green twice consecutively with the dataset restored to full row count after each run; full suite 284/284 + SpotBugs clean.
Summary
Performance overhaul of the SDK (v0.7.0), fixing every finding from a full performance audit of allocations, connection pooling, and connection management. Public API is unchanged.
Native Flight SQL prepared statements — ADBC removed
queryWithParamspreviously ran on a second, untuned connection managed by the ADBC driver: DNS resolved once (never re-resolved behind load balancers), no HTTP/2 keep-alive, no mTLS, and every call paid ~5 sequential round trips (CreatePreparedStatement→DoPut→GetFlightInfo→DoGet→ClosePreparedStatement) with the statement re-prepared every time.It is now implemented directly on the same
FlightSqlClientasquery():dns:///re-resolution, keep-alive, and (new for this path) mTLS/custom CA.withPreparedStatementCacheSize, default 64,0disables) — repeated SQL reuses the server-side statement, cutting 5 round trips to 3. Stale handles (e.g. server restart) transparently re-prepare. Statements are never shared mid-flight and are invalidated onreset()/rebuild/close.FlightInfoReader(mirrors ADBC's zero-copy unload/load reader) that consumes all endpoints of a partitioned result.adbc-driver-flight-sqlandadbc-core.Transport & resilience
withChannelCount(n)— round-robin gRPC connection pool for highly concurrent workloads (single HTTP/2 connection is capped by serverMAX_CONCURRENT_STREAMSand one TCP pipe). The query hot path is now lock-free (volatile channel snapshot; previously every query synchronized on the client).withQueryTimeout(d)— deadline for query planning / statement prepare / parameter bind RPCs (intentionally not applied toDoGetso long result streams aren't killed).refreshDatasetgains a 60s request timeout.fibonacciWait()waited 1–2ms between attempts, so all retries burned out in <5ms during any real outage. Now exponential with jitter (~250ms, 500ms, 1s, … capped 10s).UNAUTHENTICATED(expired handshake bearer on long-lived clients) triggers a single guarded re-handshake and the retryer re-attempts; previously required manualreset().reset()concurrent withqueryWithParamscould NPE (per-attempt snapshots, matching the plain-query path).Dependency alignment
netty-all 4.2.12.Finalforced every Netty module to the 4.2 line whilegrpc-netty 1.79.0declares 4.1.130.Final — an unsupported combination — and dragged unused codecs (HTTP/3, MQTT, SMTP, Redis, …) into consumers' classpaths. Replaced withnetty-bom 4.1.130.Final+grpc-bom 1.79.0imports plus Linux epoll natives. This also surfaced that BouncyCastle (mTLS PEM parsing) was an undeclared transitive via ADBC — now explicit (bcprov/bcpkix1.80).Allocation fixes
(long, string, double)).Config.getUserAgent()computed once instead of per request (also fixes the reported Windows os.version).HttpClientcreated lazily — clients that never callrefreshDatasetno longer spawn a selector thread.Testing
New in-process mock Flight SQL server (
TestFlightSqlServer) with per-RPC counters and failure injection (transient errors, handle invalidation, delays, bearer-token expiry) — the SDK's behavior is now fully covered locally; previously almost every test skipped without a runtime.LocalFlightServerTest— end-to-end both query paths, parameter type round-trips, multi-endpoint reads, error contractsStatementCacheTest— reuse (asserts exactly 1 server-side prepare across N queries), disable, stale-handle recovery, reset invalidation, 8-thread concurrency with leak checkResilienceTest— backoff timing, retry exhaustion, non-retryable fail-fast, query timeout, token-expiry auto-recovery, channel pool, concurrent reset+queryWithParams (regression test for the NPE race)FlightInfoReaderTest,ParamRootTest,RefreshHttpTestPerfBenchmarkTest— prints latency percentiles and asserts the deterministic round-trip contract (cached: 0 prepares per query; uncached: 1 prepare + 1 close per query)On loopback the cached path is ~17% faster at p50; on a real network each cached query saves 2 full round trips.
Compatibility notes
query,queryWithParams,refreshDataset,reset,close, all existing builder methods and constructors).queryWithParamsfailure causes are nowFlightRuntimeExceptioninstead ofAdbcException(both were wrapped inExecutionException).withPreparedStatementCacheSize(0)restores the previous prepare-per-query behavior.Release notes:
docs/release_notes/v0.7.0.md