Skip to content

Commit db7ad68

Browse files
fix: Address PR review comments for streaming prefetch feature
- Add null validation for required parameters in ThriftStreamingProvider - Wrap batchFetcher.close() and batch.release() in try-catch blocks - Add timeout to waitForBatchCreation to prevent indefinite waiting - Add logging for error conditions in StreamingInlineArrowResult - Add logging for error conditions in InlineArrowResponseProcessor - Extract timeout constant in ExecutionResultFactory - Update NEXT_CHANGELOG.md to document streaming prefetch feature Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
1 parent ffbcd51 commit db7ad68

5 files changed

Lines changed: 67 additions & 5 deletions

File tree

NEXT_CHANGELOG.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,9 @@
33
## [Unreleased]
44

55
### Added
6+
- Added streaming prefetch mode for Thrift inline results (columnar and Arrow) with background batch prefetching and configurable sliding window for improved throughput.
7+
- Added `EnableInlineStreaming` connection parameter to enable/disable streaming mode (default: enabled).
8+
- Added `ThriftMaxBatchesInMemory` connection parameter to control the sliding window size for streaming (default: 3).
69

710
### Updated
811
- Implemented lazy loading for inline Arrow results, fetching arrow batches on demand instead of all at once. This improves memory usage and initial response time for large result sets when using the Thrift protocol with Arrow format.

src/main/java/com/databricks/jdbc/api/impl/ExecutionResultFactory.java

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,9 @@ class ExecutionResultFactory {
2626
private static final JdbcLogger LOGGER =
2727
JdbcLoggerFactory.getLogger(ExecutionResultFactory.class);
2828

29+
/** Default timeout in seconds for waiting for a batch to be ready in streaming mode. */
30+
private static final int DEFAULT_STREAMING_BATCH_TIMEOUT_SECONDS = 300;
31+
2932
static IExecutionResult getResultSet(
3033
ResultData data,
3134
ResultManifest manifest,
@@ -129,7 +132,11 @@ private static IExecutionResult createThriftColumnarResult(
129132
LOGGER.info("Using StreamingColumnarResult for improved throughput (default)");
130133
int maxBatchesInMemory = connectionContext.getThriftMaxBatchesInMemory();
131134
return new StreamingColumnarResult(
132-
resultsResp, parentStatement, session, maxBatchesInMemory, 300); // 5 minute timeout
135+
resultsResp,
136+
parentStatement,
137+
session,
138+
maxBatchesInMemory,
139+
DEFAULT_STREAMING_BATCH_TIMEOUT_SECONDS);
133140
} else {
134141
LOGGER.info("Using LazyThriftResult (streaming explicitly disabled)");
135142
return new LazyThriftResult(resultsResp, parentStatement, session);
@@ -153,7 +160,11 @@ private static IExecutionResult createInlineArrowResult(
153160
LOGGER.info("Using StreamingInlineArrowResult for improved throughput (default)");
154161
int maxBatchesInMemory = connectionContext.getThriftMaxBatchesInMemory();
155162
return new StreamingInlineArrowResult(
156-
resultsResp, parentStatement, session, maxBatchesInMemory, 300); // 5 minute timeout
163+
resultsResp,
164+
parentStatement,
165+
session,
166+
maxBatchesInMemory,
167+
DEFAULT_STREAMING_BATCH_TIMEOUT_SECONDS);
157168
} else {
158169
LOGGER.info("Using LazyThriftInlineArrowResult (streaming explicitly disabled)");
159170
return new LazyThriftInlineArrowResult(resultsResp, parentStatement, session);

src/main/java/com/databricks/jdbc/api/impl/arrow/StreamingInlineArrowResult.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,18 +167,23 @@ public Object getObject(int columnIndex) throws DatabricksSQLException {
167167
/** Validates state before getting an object. */
168168
private void validateGetObjectState(int columnIndex) throws DatabricksSQLException {
169169
if (isClosed) {
170+
LOGGER.debug("[STREAMING] Attempted to access closed result");
170171
throw new DatabricksSQLException(
171172
"Result is closed", DatabricksDriverErrorCode.STATEMENT_CLOSED);
172173
}
173174
if (globalRowIndex == -1) {
175+
LOGGER.debug("[STREAMING] Attempted to access data before first row");
174176
throw new DatabricksSQLException(
175177
"Cursor is before first row", DatabricksDriverErrorCode.INVALID_STATE);
176178
}
177179
if (currentChunkIterator == null) {
180+
LOGGER.debug("[STREAMING] No current chunk available at row {}", globalRowIndex);
178181
throw new DatabricksSQLException(
179182
"No current chunk available", DatabricksDriverErrorCode.INVALID_STATE);
180183
}
181184
if (columnIndex < 0 || columnIndex >= columnInfos.size()) {
185+
LOGGER.debug(
186+
"[STREAMING] Column index {} out of bounds (0-{})", columnIndex, columnInfos.size() - 1);
182187
throw new DatabricksSQLException(
183188
"Column index out of bounds: " + columnIndex, DatabricksDriverErrorCode.INVALID_STATE);
184189
}

src/main/java/com/databricks/jdbc/api/impl/streaming/InlineArrowResponseProcessor.java

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,7 @@ public StreamingBatch<ArrowResultChunk> processResponse(
101101
return batch;
102102

103103
} catch (DatabricksParsingException e) {
104+
LOGGER.error("Failed to process inline Arrow batch {}: {}", batchIndex, e.getMessage(), e);
104105
batch.setError(e);
105106
throw new DatabricksSQLException(
106107
"Failed to process Arrow data", e, DatabricksDriverErrorCode.INLINE_CHUNK_PARSING_ERROR);
@@ -141,6 +142,7 @@ private ByteArrayInputStream createArrowByteStream(TFetchResultsResp response)
141142

142143
return new ByteArrayInputStream(baos.toByteArray());
143144
} catch (DatabricksSQLException | IOException e) {
145+
LOGGER.error("Failed to create Arrow byte stream: {}", e.getMessage(), e);
144146
throw new DatabricksParsingException(
145147
"Failed to create Arrow byte stream: " + e.getMessage(),
146148
e,
@@ -204,13 +206,17 @@ private Schema hiveSchemaToArrowSchema(TTableSchema hiveSchema)
204206
throws DatabricksParsingException {
205207
List<Field> fields = new ArrayList<>();
206208
if (hiveSchema == null) {
209+
LOGGER.debug("Hive schema is null, returning empty Arrow schema");
207210
return new Schema(fields);
208211
}
209212
try {
213+
LOGGER.debug(
214+
"Converting Hive schema to Arrow schema with {} columns", hiveSchema.getColumnsSize());
210215
for (TColumnDesc columnDesc : hiveSchema.getColumns()) {
211216
fields.add(getArrowField(columnDesc));
212217
}
213218
} catch (SQLException e) {
219+
LOGGER.error("Failed to convert Hive schema to Arrow: {}", e.getMessage(), e);
214220
throw new DatabricksParsingException(
215221
"Failed to convert Hive schema to Arrow: " + e.getMessage(),
216222
e,

src/main/java/com/databricks/jdbc/api/impl/streaming/ThriftStreamingProvider.java

Lines changed: 40 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,17 @@ private ThriftStreamingProvider(
174174
int timeoutSeconds)
175175
throws DatabricksSQLException {
176176

177+
// Validate required parameters
178+
if (initialResponse == null) {
179+
throw new IllegalArgumentException("initialResponse cannot be null");
180+
}
181+
if (fetcher == null) {
182+
throw new IllegalArgumentException("fetcher cannot be null");
183+
}
184+
if (processor == null) {
185+
throw new IllegalArgumentException("processor cannot be null");
186+
}
187+
177188
this.batchFetcher = fetcher;
178189
this.processor = processor;
179190
this.maxBatchesInMemory = Math.max(2, maxBatchesInMemory);
@@ -355,13 +366,22 @@ public void close() {
355366
}
356367

357368
// Release all batches using type-safe release action
369+
// Continue releasing subsequent batches even if one fails
358370
for (StreamingBatch<T> batch : batches.values()) {
359-
batch.release();
371+
try {
372+
batch.release();
373+
} catch (Exception e) {
374+
LOGGER.warn("Error releasing batch during close: {}", e.getMessage(), e);
375+
}
360376
}
361377
batches.clear();
362378

363379
if (batchFetcher != null) {
364-
batchFetcher.close();
380+
try {
381+
batchFetcher.close();
382+
} catch (Exception e) {
383+
LOGGER.warn("Error closing batchFetcher: {}", e.getMessage(), e);
384+
}
365385
}
366386
}
367387

@@ -459,6 +479,9 @@ private void releaseBatch(long batchIndex) {
459479
private void waitForBatchCreation(long batchIndex) throws DatabricksSQLException {
460480
prefetchLock.lock();
461481
try {
482+
long waitStartTime = System.currentTimeMillis();
483+
long timeoutMillis = batchReadyTimeoutSeconds * 1000L;
484+
462485
while (!closed && !batches.containsKey(batchIndex)) {
463486
checkPrefetchError();
464487
if (endOfStreamReached && batchIndex > highestFetchedBatchIndex.get()) {
@@ -470,8 +493,22 @@ private void waitForBatchCreation(long batchIndex) throws DatabricksSQLException
470493
+ ")",
471494
DatabricksDriverErrorCode.CHUNK_READY_ERROR);
472495
}
496+
497+
// Check for timeout
498+
long elapsedMillis = System.currentTimeMillis() - waitStartTime;
499+
if (elapsedMillis >= timeoutMillis) {
500+
throw new DatabricksSQLException(
501+
"Timeout waiting for batch "
502+
+ batchIndex
503+
+ " to be created (timeout: "
504+
+ batchReadyTimeoutSeconds
505+
+ "s)",
506+
DatabricksDriverErrorCode.CHUNK_READY_ERROR);
507+
}
508+
473509
try {
474-
batchAvailable.await();
510+
long remainingMillis = timeoutMillis - elapsedMillis;
511+
batchAvailable.await(remainingMillis, java.util.concurrent.TimeUnit.MILLISECONDS);
475512
} catch (InterruptedException e) {
476513
Thread.currentThread().interrupt();
477514
throw new DatabricksSQLException(

0 commit comments

Comments
 (0)