Thrift: NPE on metadata-less empty FetchResults batch with hasMoreRows=true (should skip + refetch) (#1567)

[peco-engineer-bot]

Summary

Automated fix for #1567 — Thrift: NPE on metadata-less empty FetchResults batch with hasMoreRows=true (should skip + refetch).

Fixed the NPE by making DatabricksThriftAccessor.executeFetchRequest (the shared initial-fetch path for both execute() and getStatementResult()) loop: when a FetchResults response has null result-set metadata but hasMoreRows=true, it is treated as a legitimate intermediate empty batch, skipped, and FetchResults is re-issued until metadata/rows arrive — matching the reference C#/ADBC drivers. Verified via the two provided tests (now green) plus the full 57-test DatabricksThriftAccessorTest class. The bug is only reproducible by injecting the crafted metadata-less empty-batch response (per the issue, done through the databricks-driver-test mitmproxy harness / RESULTFETCH-009/010 spec cases); a real warehouse does not emit this sequence on demand, so the deterministic mocked-client tests are the faithful reproduction.

Root cause & plan

Root cause: On the Thrift result path, the INITIAL TFetchResultsResp that seeds a DatabricksResultSet is assumed to always carry result-set metadata. When the server returns an empty row set with hasMoreRows=true and getResultSetMetadata()==null (a legitimate intermediate empty batch), the driver dereferences the null metadata rather than re-fetching, throwing NPE. Dereference sites reached from the initial response: ExecutionResultFactory.getResultSet (resultsResp.getResultSetMetadata().isSetIsStagingOperation()), ExecutionResultFactory.getResultHandler (getResultSetMetadata().getResultFormat()), and the DatabricksResultSet Thrift constructor (getResultSetMetadata().getResultFormat() in the switch, and passing metadata to DatabricksResultSetMetaData). Subsequent-batch loops (LazyThriftResult, AbstractRemoteChunkProvider, convertColumnarToRowBased, streaming) already tolerate empty batches because they only read getResults()/hasMoreRows, never metadata. The correct behavior per the issue and the C#/ADBC reference drivers is to skip a metadata-less empty batch and issue the next FetchResults until metadata arrives (or hasMoreRows=false). The fix belongs in DatabricksThriftAccessor where the initial fetch response is produced (the execute post-poll fetch branch and getStatementResult).
["jdbc-core/src/main/java/com/databricks/jdbc/dbclient/impl/thrift/DatabricksThriftAccessor.java", "jdbc-core/src/test/java/com/databricks/jdbc/dbclient/impl/thrift/DatabricksThriftAccessorTest.java"]
Planned coverage:

• In DatabricksThriftAccessorTest, drive accessor.execute with a mocked TCLIService.Client whose first FetchResults returns an empty row set with hasMoreRows=true and null resultSetMetadata, and the next FetchResults returns the real batch (metadata + rows). Before the fix this throws NullPointerException; after the fix the empty batch is skipped, a follow-up FetchResults is issued, and a DatabricksResultSet is returned (no NPE). (Single metadata-less empty batch with hasMoreRows=true (RESULTFETCH-009))
• Same setup but the mock returns TWO consecutive metadata-less empty batches (each hasMoreRows=true) before the real batch. Asserts the driver keeps issuing FetchResults across multiple empty intermediate batches until metadata/rows arrive, surfacing the result set without NPE. (Multiple consecutive metadata-less empty batches (RESULTFETCH-010))

Files changed

• src/test/java/com/databricks/jdbc/dbclient/impl/thrift/DatabricksThriftAccessorTest.java
• src/main/java/com/databricks/jdbc/dbclient/impl/thrift/DatabricksThriftAccessor.java

Test plan

• com.databricks.jdbc.dbclient.impl.thrift.DatabricksThriftAccessorTest#testExecute_skipsMetadataLessEmptyBatch — fails (red) against the original code, passes (green) after the fix
• com.databricks.jdbc.dbclient.impl.thrift.DatabricksThriftAccessorTest#testExecute_skipsMultipleConsecutiveMetadataLessEmptyBatches — fails (red) against the original code, passes (green) after the fix

NO_CHANGELOG=true

🤖 Generated by engineer-bot (bug-fix flow) — review before merge.

[peco-review-bot]

Verdict: 1 Medium · 1 Low

Targeted fix that loops FetchResults to skip metadata-less empty batches — correct in spirit and well-tested for the reported case. One medium concern: the new while (true) loop is unbounded and, unlike every other server-wait loop in this class, has no timeout guard, so a misbehaving server sequence hangs the thread forever. One low note on a residual NPE path when a metadata-less batch has hasMoreRows=false.

Other findings

• 🟡 Medium — The new while (true) re-fetch loop has no iteration bound and no timeout guard. If the server keeps returning metadata-less empty batches with hasMoreRows=true (e.g. a buggy/looping backend, or an offset that never advances), the driver spins indefinitely, issuing FetchResults RPCs forever with no way to abort — the calling thread hangs and never surfaces an exception.

Every other loop in this class that waits on the server is bounded by a TimeoutHandler.checkTimeout() (see the polling loop at pollTillOperationFinished, line 341, and the metadata poll at line 794). This fetch loop is the one exception. Consider bounding the number of consecutive empty-batch skips, or wiring in the statement's query timeout via a TimeoutHandler, so a pathological server sequence fails cleanly instead of hanging.

• 🔵 Low — The skip condition is metadata == null && hasMoreRows==true. A metadata-less batch with hasMoreRows==false (a final empty batch with no metadata) still falls through to return response, and the initial-fetch consumers described in the PR (ExecutionResultFactory.getResultSet/getResultHandler, the DatabricksResultSet Thrift constructor) will then dereference the null metadata and throw the same NPE this PR is fixing. If the server can emit such a terminal metadata-less batch, this gap remains uncovered by both the fix and the two new tests. Worth confirming that case cannot occur, or handling it explicitly.