[refactor](session) Replace the execution layer's ConnectType branches with protocol capabilities - #67900
Open
morningman wants to merge 5 commits into
Open
[refactor](session) Replace the execution layer's ConnectType branches with protocol capabilities#67900morningman wants to merge 5 commits into
morningman wants to merge 5 commits into
Conversation
…it ends in a SET RecordingMysqlChannel now models the send buffer: a flush pushes everything written so far and reset() drops what was written after the last flush, as MysqlChannel.reset() clears the buffer. The MySQL golden therefore shows what reaches the client rather than everything the server wrote. The only existing case this changes is multi-statement-without-capability: the packets of `select 1` that the reset at `select 2` throws away no longer appear, and the gap in the sequence ids (the response now starts at seq=4) is exactly what the client sees on the wire. Two cases are added for a request whose last statement is not a query, `select 1; set @A = 1`, with and without CLIENT_MULTI_STATEMENTS. They record the current behavior before the next commit changes where the channel is reset: without the capability the client today receives the result set of `select 1` terminated by the OK packet of the SET, a stream no MySQL client can parse. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WXWFq9NqEnNjsywm5xnmu4
…s with protocol capabilities The last protocol branches outside the two adapters go: StmtExecutor, the coordinators, FEOpExecutor, the short-circuit rewrite rule and nine Commands no longer ask which protocol the connection speaks. ProtocolAdapter gains what they need, one method per use: canReplayForwardedQueryResult() forwarding a query to the master supportsFeSideResult() ConnectContext.supportHandleByFe supportsShortCircuitPointQuery() LogicalResultSinkToShortCircuitPointQuery canRetryQuery(ctx) the retry condition of handleQueryWithRetry beforeStatement(ctx) ConnectProcessor.executeQuery / handleExecute beforeQuery(ctx) executeAndSendResult, before the coordinator returnsResultFromLocal(ctx) behind ConnectContext.isReturnResultFromLocal fillForwardRequest(ctx, request) FEOpExecutor.buildStmtForwardParams The Flight adapter's returnResultFromLocal is no longer flipped from the outside: beforeStatement puts a statement's result on this frontend, beforeQuery hands it to the backends, and the coordinators keep reading ctx.isReturnResultFromLocal() to choose receivers or Flight endpoints. Since beforeQuery is called only where a coordinator is about to be built, an EXPLAIN never marks its result as coming from a backend and FlightResultSender no longer has to undo that. What a Flight request drops from its predecessor is gathered in FlightProtocolAdapter.beginRequest(). The MySQL channel is reset once, when a statement starts, instead of in the query path and in nine insert / transaction Commands. For a client without CLIENT_MULTI_STATEMENTS a request now delivers only its last statement's outcome whatever the last statement is; before, `select 1; set @A = 1` delivered the result set of the SELECT terminated by the OK packet of the SET, which no client can parse (the golden case multi-statement-without-capability-query-then-set shows the difference; the previous commit recorded the old bytes). The query path still resets the sender at every attempt, so a retry starts from an empty buffer. The forward request's MySQL section (capability flags, deprecate-EOF, the COM_STMT_EXECUTE packet and the cursor flag) and its restoration on the master move into MysqlProtocolAdapter as fillForwardRequest / restoreFromForwardRequest, and the master reads the proxy channel's packets through the adapter. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WXWFq9NqEnNjsywm5xnmu4
morningman
requested review from
924060929,
englefly,
morrySnow and
starocean999
as code owners
September 12, 2026 16:27
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
25 tasks
Contributor
TPC-H: Total hot run time: 16780 ms |
Contributor
TPC-DS: Total hot run time: 82582 ms |
Contributor
ClickBench: Total hot run time: 14.8 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
The adapter tests pin what each adapter answers; ProtocolCapabilityWiringTest drives real statements through the processors and the executor to show the answer is what decides: a query forwarded from a follower is refused on a Flight session before any rpc while a MySQL connection goes on to forward; a Flight statement starts on the frontend whatever the previous statement of the request did (ConnectProcessor.executeQuery -> beforeStatement); and the master answers a forwarded query with the packets and terminator the client's capabilities ask for (proxyExecute -> restoreFromForwardRequest, proxyResultPackets), with and without CLIENT_DEPRECATE_EOF. MysqlProtocolAdapterTest checks that the proxy channel's packets come back through proxyResultPackets(), and the MySQL golden gets a request ending in a transaction command, `select 1; begin`, whose command used to reset the channel itself: only the OK is delivered, as before (shape only, the OK carries a label derived from the query id). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WXWFq9NqEnNjsywm5xnmu4
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 16693 ms |
… and Arrow Flight SQL Two more wiring tests and three regression suites for what Stage 1 of the protocol-agnostic session layer moved behind ProtocolAdapter / ResultSender. Unit tests (ProtocolCapabilityWiringTest): - A query whose backend rpc failed is retried under a new query id on a MySQL connection and not on a Flight session (canRetryQuery). The mocked backend gets an injection point, failNextExecPlanFragments(n), that answers the next n exec_plan_fragment rpcs with a TIMEOUT status: the coordinator raises that as the RpcException the executor retries on, without blacklisting the backend. - An internal executor (the IVM dry run) answers on the caller's connection with the caller's capabilities: a caller that did not deprecate EOF gets the EOF after the column definitions although the internal session's defaults would not send one. Regression suites: - arrow_flight_sql_p0/test_arrow_flight_session_lifecycle: one Flight session, many requests. Frontend-side results (SHOW, SET, USE, EXPLAIN, EXPLAIN PLAN PROCESS, DESC, DDL) come back as text, queries come back typed from the backend (a literal too: supportsFeSideResult is false for Flight), the two alternate on one session, session state carries across requests, a request may only return the result of its last statement, and a failed statement leaves the session usable. - query_p0/test_multi_statement_response: what a client receives for a multi-statement request. Connector/J without CLIENT_MULTI_STATEMENTS gets only the last statement's response and stays in step, also when that statement is a SET, an INSERT or a BEGIN. A bare MySQL protocol client in the suite drives the combinations Connector/J cannot negotiate (it asks for CLIENT_MULTI_STATEMENTS only when the server advertises it, and always for CLIENT_DEPRECATE_EOF): with the capability every statement's response is delivered, the intermediate ones flagged SERVER_MORE_RESULTS_EXISTS, with and without the EOF packets. - query_p0/test_mysql_forward_to_master (docker, two FEs): the MySQL side of test_arrow_flight_forward_to_master. A query forwarded from a follower answers with the master's packets, a forwarded SHOW with its rows, a failed one with the master's error, and a server-prepared statement executed with a cursor returns the same rows forwarded as it does locally. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WXWFq9NqEnNjsywm5xnmu4
Contributor
TPC-DS: Total hot run time: 81325 ms |
Contributor
ClickBench: Total hot run time: 14.74 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 16857 ms |
Contributor
TPC-DS: Total hot run time: 82975 ms |
Contributor
ClickBench: Total hot run time: 14.74 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
Local pipeline review — ✅ PASSschema: doris-repo-review/v1
status: PASS
pr: apache/doris#67900
commit: 5181cbac469aa22570ed33fa6f099a165c24816a
base: efe7dec93d048e27b84edb0a3e2506e4cd872b53
reviewed_at: 2026-09-13T18:08+08:00
reviewer: morningman
model: claude-opus-5
effort: max
findings: {blocker: 0, major: 0, minor: 3, nit: 7}
rounds: 1
converged: trueNotes for maintainers
Reviewed locally with the |
… Groovy expression
test_mysql_forward_to_master built the follower's JDBC URL over two lines
with the `+` at the start of the second one. In a bare assignment Groovy
ends the statement at the line break, so the second line became a
statement of its own -- a unary plus on a String -- and the suite died
with
MissingMethodException: No signature of method: java.lang.String.positive()
before reaching the COM_STMT_EXECUTE half. Nothing caught it because the
suite is a docker suite and CI never runs those. Found by running the
suite's body against a local 2-FE cluster, where it now passes end to end
(the forwarded server-side prepared statement included). Trailing `+`
keeps the concatenation inside one expression.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hRBG4Au5xYrRvs5T9y2CH
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 17107 ms |
Contributor
TPC-DS: Total hot run time: 82504 ms |
Contributor
ClickBench: Total hot run time: 14.85 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: #67577
Related PR: #67883 (
ResultSender), #67835 (ProtocolAdapter), #67789 (protocol goldens)Problem Summary:
Third and last refactor step of stage 1 of #67577. After #67835 and #67883 the connection half and the result-encoding half of the two wire protocols live behind
ProtocolAdapter/ResultSender, but the execution layer still askedctx.getConnectType()in a dozen places: five times inStmtExecutor, inConnectContext.supportHandleByFe, in nine insert / transactionCommands (each resetting the MySQL channel), in both coordinators, in the short-circuit rewrite rule and inFEOpExecutor; and the Flight-onlyreturnResultFromLocalflag was flipped from four places outside the adapter. This PR replaces every one of them with a capability or a lifecycle hook on the adapter, one method per use:Capabilities.
canReplayForwardedQueryResult()guards the refusal to forward a query to the master on a Flight session (#67569; the message is unchanged).supportsFeSideResult()is the protocol half ofConnectContext.supportHandleByFe();supportsShortCircuitPointQuery()is the protocol half ofLogicalResultSinkToShortCircuitPointQuery.scanMatchShortCircuitCondition(#67368);canRetryQuery(ctx)is the retry condition ofhandleQueryWithRetry: for MySQL "nothing was flushed to the socket yet", for Flight false (the endpoints a failed attempt registered would have to be withdrawn first; nothing does that, as before). The reason each Flight answer is what it is moves onto the Flight implementation, out of the call sites.Statement lifecycle.
beforeStatement(ctx)is called byConnectProcessor.executeQuerybefore each statement (and byMysqlConnectProcessor.handleExecute): the MySQL adapter resets the channel there, which is what the query path and the nineCommands did each on their own; the Flight adapter puts the statement's result on this frontend.beforeQuery(ctx)is called at the top ofexecuteAndSendResult, before a coordinator is built: the Flight adapter marks the result as staying on the backends, andCoordinator/NereidsCoordinator/QueryProcessorkeep readingctx.isReturnResultFromLocal()to register endpoints instead of receivers (thecheckState(ARROW_FLIGHT_SQL)assertions go, the decision itself does not move). BecausebeforeQueryruns only where a coordinator follows, anEXPLAIN-- handled earlier inhandleQueryStmt-- never marks its result as coming from a backend, and thesetReturnResultFromLocal(true)thatFlightResultSender.sendResultSethad to do in #67883 is gone with the setter. The four flips inStmtExecutor/FlightSqlConnectProcessorare gone; what a Flight request drops from its predecessor (deferred executors, result cache, endpoints, the flag) isFlightProtocolAdapter.beginRequest().Forwarding.
fillForwardRequest(ctx, request)adds to aTMasterOpRequestwhat the master needs to know about the client: the MySQL adapter writes the negotiated capability flags,CLIENT_DEPRECATE_EOFand, for aCOM_STMT_EXECUTE, the execute packet and the cursor flag (formerly two blocks inFEOpExecutor.buildStmtForwardParams, one of them behindif (MYSQL)); the Flight adapter writes nothing, its session consumes the master's status and rows rather than its packets (carryForwardedOutcome). The master's side,ConnectProcessor.restoreForwardedMysqlContext, becomesMysqlProtocolAdapter.restoreFromForwardRequest, andproxyExecutereads the proxy channel's packets throughMysqlProtocolAdapter.proxyResultPackets()instead ofStmtExecutor.getProxyQueryResultBufList()casting the channel.After this PR
grep -rn 'ConnectType\.\|getMysqlChannel()' fe-core/src/main/java/org/apache/doris/{qe,nereids}outside*/protocol/finds only theConnectContext.getMysqlChannel()delegate itself and two lines ofMysqlConnectProcessor(reading the client's packet, the auth-switch handshake), which is MySQL protocol code by definition.One behavior change, on the MySQL side, recorded in the golden. The channel used to be reset at the start of a query and inside the insert / transaction commands, and nowhere else. A client that did not negotiate
CLIENT_MULTI_STATEMENTSgets no intermediate response between the statements of a request, so whatever a query wrote stayed in the send buffer until the next query or insert reset it. When the next statement was neither --select 1; set @a = 1-- the buffered result set of theSELECTwent out together with theOKof theSET: a result set terminated by a0x00OK packet, which no MySQL client parses (it reads the OK as a row and waits for more). With the reset at the start of every statement, such a request delivers only its last statement's outcome, which is whatMysqlProtocolAdapter.finishStatementhas documented as the intent all along. The first commit makesRecordingMysqlChannelmodel the send buffer (a reset drops what was written after the last flush, the wayMysqlChannel.reset()clears it) and recordsselect 1; set @a = 1with and without the capability as it is today; the second commit's golden diff is exactly that: the three packets ofselect 1disappear frommulti-statement-without-capability-query-then-set, theOKkeeps its sequence id 4. The existingmulti-statement-without-capabilitycase (select 1; select 2) loses the three packets ofselect 1in the first commit only, because the recording channel now shows what reaches the client -- its sequence ids, 4 to 7, already were the ones on the wire. That the delivered response does not start at sequence id 1 is pre-existing and not touched here:MysqlChannel.reset()clears the buffer but does not rewind the sequence id, so a client that checks sequence ids (pymysql, libmysqlclient; Connector/J does not by default) already failsselect 1; select 2without the capability with "Packet sequence number wrong - got 5 expected 1", before and after this PR. That deserves its own small fix.Not in this PR: an internal adapter for the no-client context (it is still a MySQL context over a
DummyMysqlChannel, now with nothing in the execution layer keyed on that), and the stage 1 performance baseline.Release note
None
Check List (For Author)
Test
FlightResultGoldenTest(9 statements) is byte-identical.MysqlPacketGoldenTestgains two cases in the first commit and changes in exactly one of them in the second, as explained above. New cases inMysqlProtocolAdapterTest(capabilities,beforeStatement/canRetryQueryagainst the recording channel,fillForwardRequest->restoreFromForwardRequestround trip) andFlightProtocolAdapterTest(capabilities,fillForwardRequestwrites nothing, the result-location lifecycle acrossbeforeStatement/beforeQuery/sendResultSet/beginRequest);FEOpExecutorMysqlProtocolTestandDorisFlightSqlProducerTestfollow the moved methods. The third commit addsProtocolCapabilityWiringTest, which drives real statements through the processors and the executor to show that the adapter's answer is what decides at the call sites the adapter tests cannot reach: a query forwarded from a follower is refused on a Flight session before any rpc while a MySQL connection goes on to forward (executeByNereids), a Flight statement starts on the frontend whatever the previous statement of the request did (executeQuery->beforeStatement), and the master answers a forwarded query with the packets and terminator the client's capabilities ask for, with and withoutCLIENT_DEPRECATE_EOF(proxyExecute->restoreFromForwardRequest/proxyResultPackets); plus a golden case for a request ending in a transaction command (select 1; begin, shape only), whose command used to reset the channel itself. Not unit-tested, as before: the retry loop ofhandleQueryWithRetryitself (its new conditioncanRetryQueryis, on both adapters; the loop needs a coordinator that fails once). Local regression (single FE built from this branch, BE fromf35dd8285aa):arrow_flight_sql_p08/8,prepared_stmt_p06/6,load_p0/mysql_load7/7,point_query_p016/16,query_p0/cache12/12,query_p0/explain9/9,query_p0/dry_run1/1,query_p0/system13/14,insert_overwrite_p013/13,insert_p0/transaction16/16,insert_p0/test_jdbc,unique_with_mow_p0/partial_update/test_partial_update_multi_stmt,mtmv_p0/ivm/test_ivm_refresh_dry_run. The one red,test_query_sys_tables, iscatalog_meta_cache_statisticsfailing on theMAX_WEIGHTcolumn [feature](fe) Add external metadata cache memory governance #67726 added to the FE side, which the local BE predates -- a version skew of the test setup, not of this PR.Behavior changed:
A multi-statement request from a MySQL client without
CLIENT_MULTI_STATEMENTSwhose last statement is not a query (select 1; set @a = 1) now returns only the last statement's response; it used to return the buffered result set of the query followed by that response, a stream no client can parse.Does this need documentation?
🤖 Generated with Claude Code
https://claude.ai/code/session_01WXWFq9NqEnNjsywm5xnmu4