Skip to content

Detect in-band server exceptions on Arrow streaming query paths - #950

Open
Vivek1106-04 wants to merge 1 commit into
ClickHouse:mainfrom
Vivek1106-04:fix/913-arrow-in-band-exceptions
Open

Detect in-band server exceptions on Arrow streaming query paths#950
Vivek1106-04 wants to merge 1 commit into
ClickHouse:mainfrom
Vivek1106-04:fix/913-arrow-in-band-exceptions

Conversation

@Vivek1106-04

Copy link
Copy Markdown

Summary

When a query fails after the HTTP interface has already committed a 200 OK and started streaming rows, ClickHouse appends its exception to the response body and drops the connection. The native read path recovers that error, but the Arrow paths handed the raw bytes straight to pyarrow and never consulted it, so the failure surfaced as a pyarrow parse error or a raw transport error with the real server message lost.

Verified against a live server before writing any code. Both server generations put the error in the last chunk before a transport abort:

  • 26.5: \r\n__exception__\r\n<TAG>\r\nCode: 395. DB::Exception: boom ...\n247 <TAG>\r\n__exception__\r\n
  • 25.8: the bare Code: 395. DB::Exception: boom ... text, no tag header

That chunk is already recovered today. ResponseSource.buffered() swallows the transport error, drains what it has, and yields the trailing chunk; that is exactly why the native path reports a clean error on both versions. The Arrow paths simply never looked at it.

Approach

Per the note on #914, this is a private detector that copies nothing per chunk and checks a bounded tail.

_ExceptionTail holds trailing chunks by reference in a deque capped at 64KB, popping from the left only while the newer chunks can cover the tail on their own. A successful stream pays one deque append per chunk and never copies its payload; the tail is joined once, and only when an error is actually suspected. There is no per-chunk scanning, concat, or slice.

StreamingFileAdapter consults that tail when the stream ends, at both a clean EOF and a transport abort, and raises StreamFailureError. It is the single choke point every Arrow byte already flows through, so one hook covers all five affected methods. No new names are exported.

The extraction itself is shared with the native path rather than reimplemented: the two closures in NativeTransform.parse_response are lifted to module-level extract_stream_error / format_stream_error with identical logic. Both paths now produce the same message and honor show_clickhouse_errors, including scrub.

Judgment calls

The untagged fallback is stricter for Arrow than for native. With show_clickhouse_errors=True the native fallback returns any decodable tail. For a binary Arrow payload that means raising 64KB of null bytes as the "error message" — a unit test caught this. The Arrow path therefore requires the Code: marker before trusting untagged text. Native behavior is unchanged.

Clean EOF trusts only a tagged block. A bare Code: match against binary data at a clean EOF is not evidence of failure, so a healthy stream is never turned into an error there.

Only transport failures are rewritten. The OperationalError / ClientPayloadError check mirrors the native path exactly; anything else keeps its own type and traceback.

Error types moved deliberately. These paths previously leaked urllib3.ProtocolError / aiohttp.ClientPayloadError. They now raise StreamFailureError, matching the native streaming path, which is what the issue asks for. A transport abort with no recoverable message raises StreamFailureError("Stream failed during read (connection closed by server)"), again matching native.

chdb is passed through untouched. It returns a plain file object with no headers and reports its own errors, so the wrap is gated on the response being an HTTP one.

Client.query_arrow was already correct (buffered, wait_end_of_query=1) and is unchanged, as the issue notes.

Verification

Both directions confirmed, per AGENTS.md.

Against a live server, the RED is exactly the five methods the issue lists and nothing else:

FAILED test_arrow_stream_reports_mid_stream_server_error[sync-query_arrow_stream]
FAILED test_arrow_stream_reports_mid_stream_server_error[sync-query_df_arrow_stream]
FAILED test_arrow_stream_reports_mid_stream_server_error[async-query_arrow_stream]
FAILED test_arrow_stream_reports_mid_stream_server_error[async-query_df_arrow_stream]
FAILED test_query_arrow_reports_mid_stream_server_error[async]
5 failed, 9 passed

test_query_arrow_reports_mid_stream_server_error[sync] passes both before and after, confirming the buffered path was already correct, and the success-path guard passes both ways.

  • Verified manually against ClickHouse 25.8 (untagged, the CI floor) and 26.5 (tagged). All five methods return StreamFailureError: Code: 395. DB::Exception: boom ... on both, in sync and async.
  • Full integration suite: 637 passed, 5 failed -> 642 passed, 0 failed, with the identical 23 pre-existing errors before and after. Only the five target tests moved.
  • Full unit suite: 1420 passed, 0 failed.
  • 28 unit tests cover the tail bounds, a marker split across a chunk boundary, tagged and untagged recovery, clean EOF vs abort, the no-false-positive guard, unrelated errors passing through, and the show_clickhouse_errors policy.
  • ruff check and ruff format --check clean. mypy reports "Success: no issues found in 93 source files".

Checklist

  • Unit and integration tests covering the common scenarios were added
  • A human-readable description of the changes was provided to include in CHANGELOG

When a query fails after ClickHouse has committed a 200 and started
streaming rows, the server appends its exception to the response body and
drops the connection. The native read path recovers that error, but the
Arrow paths handed raw bytes to pyarrow and never consulted it, so the
failure surfaced as a pyarrow parse error or a raw transport error with the
server message lost.

StreamingFileAdapter now keeps a bounded tail of the response, held by
reference so a successful stream copies nothing, and consults it when the
stream ends at either a clean EOF or a transport abort. The extraction
itself is shared with the native path so both report the same message and
honor show_clickhouse_errors. Because an Arrow payload is binary, the
untagged fallback requires the "Code: " marker rather than treating any
decodable tail as an error.

Closes ClickHouse#913
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant