morningman opened a new pull request, #67900:
URL: https://github.com/apache/doris/pull/67900
### 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 asked
`ctx.getConnectType()` in a dozen places: five times in `StmtExecutor`, in
`ConnectContext.supportHandleByFe`, in nine insert / transaction `Command`s
(each resetting the MySQL channel), in both coordinators, in the short-circuit
rewrite rule and in `FEOpExecutor`; and the Flight-only `returnResultFromLocal`
flag 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:
```
+---------------------------+
+-----------------------------+
| MySQL client | | Arrow Flight
SQL client |
+-------------+-------------+
+--------------+--------------+
| |
v v
+---------------------------+
+-----------------------------+
| MysqlServer | |
DorisFlightSqlProducer |
| AcceptListener | | every call
goes through |
| ReadListener | |
adapter.runCommand() |
+-------------+-------------+ |
executeQueryStatement: |
| |
adapter.beginRequest() NEW|
v
+--------------+--------------+
+---------------------------+ |
| MysqlConnectProcessor | v
| COM_FIELD_LIST |
+-----------------------------+
| handleExecute: | |
FlightSqlConnectProcessor |
| adapter.beforeStatement NEW |
|
| finalizeCommand() = | |
|
| adapter.finishCommand() | |
|
+-------------+-------------+
+--------------+--------------+
| |
+----------------------------+-----------------------------+
|
v
+-------------------------------------------------------------------------------------------+
| ConnectProcessor.executeQuery -- parse, one StmtExecutor per
statement, audit |
| for each statement: adapter.beforeStatement(ctx)
<-- NEW |
| executor.execute()
|
| adapter.finishStatement(ctx, executor, i, n)
|
| proxyExecute (master side):
MysqlProtocolAdapter.restoreFromForwardRequest <-- NEW |
+---------------------------------------------+---------------------------------------------+
|
v
+-------------------------------------------------------------------------------------------+
| StmtExecutor -- plans and runs one statement, protocol-agnostic
|
|
|
| forwarding a query to the master:
adapter.canReplayForwardedQueryResult() <-- NEW |
| retry after a failed attempt: adapter.canRetryQuery(ctx)
<-- NEW |
| executeAndSendResult: adapter.beforeQuery(ctx), then the
coordinator; |
| relay rows unless
!ctx.isReturnResultFromLocal() |
| FEOpExecutor.buildStmtForwardParams: adapter.fillForwardRequest(ctx,
request) <-- NEW |
|
|
| Commands (insert / txn): no channel reset any more, beforeStatement
did it |
| Coordinator / NereidsCoordinator / QueryProcessor:
ctx.isReturnResultFromLocal() |
| decides receivers vs. Flight endpoints, no ConnectType assertion
|
| LogicalResultSinkToShortCircuitPointQuery:
adapter.supportsShortCircuitPointQuery() NEW|
| no ConnectType branch and no getMysqlChannel() left in qe/** and
nereids/** |
+---------------------------------------------+---------------------------------------------+
|
v
+-------------------------------------------------------------------------------------------+
| ConnectContext -- the session, one per connection
|
| protocolAdapter : ProtocolAdapter getResultSender() =
adapter.resultSender(this)|
| supportHandleByFe() = adapter.supportsFeSideResult() && command !=
COM_STMT_EXECUTE |
| isReturnResultFromLocal() = adapter.returnsResultFromLocal(this)
(setter is gone) |
+---------------------------------------------+---------------------------------------------+
|
v
+-----------------------------------------------+
| <<interface>> qe.protocol.ProtocolAdapter |
| type() remoteHostPortString(ctx) |
| resultSinkType() resultSender(ctx) |
| connectPool(scheduler) closeConnection(ctx) |
| |
| capabilities: |
| supportsSqlCacheReplay() |
| canReplayForwardedQueryResult() <-- NEW |
| supportsFeSideResult() <-- NEW |
| supportsShortCircuitPointQuery() <-- NEW |
| canRetryQuery(ctx) <-- NEW |
| |
| statement lifecycle: |
| beforeStatement(ctx) <-- NEW |
| beforeQuery(ctx) <-- NEW |
| returnsResultFromLocal(ctx) <-- NEW |
| finishStatement(ctx, executor, i, n) |
| afterStatement(ctx) |
| |
| forwarding: |
| fillForwardRequest(ctx, request) <-- NEW |
+-----------------------+-----------------------+
|
+--------------------+----------------------------+
| |
+-----------------------+----------------------+
+-----------------------+----------------------+
| mysql.protocol.MysqlProtocolAdapter | |
arrowflight.protocol.FlightProtocolAdapter |
| capabilities: all true; | | capabilities: all
false (the reasons are |
| canRetryQuery = nothing flushed yet | | documented on each
method) |
| beforeStatement: channel.reset() -- drops | | beforeStatement: the
statement's result is |
| what the previous statement of the | | on this frontend
until beforeQuery says |
| request left unsent | | a backend produces
it; the coordinator |
| beforeQuery: nothing, rows are relayed | | then registers the
endpoints |
| returnsResultFromLocal: true | |
returnsResultFromLocal: that flag |
| fillForwardRequest: capability flags, | | fillForwardRequest:
nothing |
| deprecate-EOF, execute packet + cursor | | beginRequest():
deferred executors, result |
| restoreFromForwardRequest (master side) | | cache, endpoints,
flag (Flight-private) |
| proxyResultPackets() (master side) | |
|
+-----------------------+----------------------+
+-----------------------+----------------------+
| |
v v
+----------------------------------------------+
+----------------------------------------------+
| <<interface>> qe.protocol.ResultSender | |
|
| sendResultSet(rs, fieldInfos, binaryRows) | |
|
| sendFields(names, fieldInfos, types) | |
|
| sendRow(wireRow) | |
|
| reset() -- now only for a retried query | |
|
+----------------------------------------------+
+----------------------------------------------+
| mysql.protocol.MysqlResultSender (unchanged) | |
arrowflight.protocol.FlightResultSender |
| | | sendResultSet no
longer touches the |
| | | result-location flag
(EXPLAIN never marks |
| | | the result as coming
from a backend now) |
+----------------------------------------------+
+----------------------------------------------+
```
**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 of
`ConnectContext.supportHandleByFe()`; `supportsShortCircuitPointQuery()` is the
protocol half of
`LogicalResultSinkToShortCircuitPointQuery.scanMatchShortCircuitCondition`
(#67368); `canRetryQuery(ctx)` is the retry condition of
`handleQueryWithRetry`: 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 by
`ConnectProcessor.executeQuery` before each statement (and by
`MysqlConnectProcessor.handleExecute`): the MySQL adapter resets the channel
there, which is what the query path and the nine `Command`s did each on their
own; the Flight adapter puts the statement's result on this frontend.
`beforeQuery(ctx)` is called at the top of `executeAndSendResult`, before a
coordinator is built: the Flight adapter marks the result as staying on the
backends, and `Coordinator` / `NereidsCoordinator` / `QueryProcessor` keep
reading `ctx.isReturnResultFromLocal()` to register endpoints instead of
receivers (the `checkState(ARROW_FLIGHT_SQL)` assertions go, the decision
itself does not move). Because `beforeQuery` runs only where a coordinator
follows, an `EXPLAIN` -- handled earlier in `handleQueryStmt` -- never marks
its result as coming from a backend, and the `setReturnResultFromLocal(true)`
that `FlightResultSender.sendResultSet` had
to do in #67883 is gone with the setter. The four flips in `StmtExecutor` /
`FlightSqlConnectProcessor` are gone; what a Flight request drops from its
predecessor (deferred executors, result cache, endpoints, the flag) is
`FlightProtocolAdapter.beginRequest()`.
**Forwarding.** `fillForwardRequest(ctx, request)` adds to a
`TMasterOpRequest` what the master needs to know about the client: the MySQL
adapter writes the negotiated capability flags, `CLIENT_DEPRECATE_EOF` and, for
a `COM_STMT_EXECUTE`, the execute packet and the cursor flag (formerly two
blocks in `FEOpExecutor.buildStmtForwardParams`, one of them behind `if
(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`, becomes
`MysqlProtocolAdapter.restoreFromForwardRequest`, and `proxyExecute` reads the
proxy channel's packets through `MysqlProtocolAdapter.proxyResultPackets()`
instead of `StmtExecutor.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 the `ConnectContext.getMysqlChannel()` delegate itself and two lines
of `MysqlConnectProcessor` (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_STATEMENTS` gets 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 the `SELECT` went out together
with the `OK` of the `SET`: a result set terminated by a `0x00` OK 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 what
`MysqlProtocolAdapter.finishStatement` has documented as the intent all along.
The first commit makes `RecordingMysqlChannel` model the send buffer (a reset
drops what was written after the last
flush, the way `MysqlChannel.reset()` clears it) and records `select 1; set @a
= 1` with and without the capability as it is today; the second commit's golden
diff is exactly that: the three packets of `select 1` disappear from
`multi-statement-without-capability-query-then-set`, the `OK` keeps its
sequence id 4. The existing `multi-statement-without-capability` case (`select
1; select 2`) loses the three packets of `select 1` in 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 fails `select 1; select 2` without 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 <!-- At least one of them must be included. -->
- [x] Regression test
- [x] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
`FlightResultGoldenTest` (9 statements) is byte-identical.
`MysqlPacketGoldenTest` gains two cases in the first commit and changes in
exactly one of them in the second, as explained above. New cases in
`MysqlProtocolAdapterTest` (capabilities, `beforeStatement` / `canRetryQuery`
against the recording channel, `fillForwardRequest` ->
`restoreFromForwardRequest` round trip) and `FlightProtocolAdapterTest`
(capabilities, `fillForwardRequest` writes nothing, the result-location
lifecycle across `beforeStatement` / `beforeQuery` / `sendResultSet` /
`beginRequest`); `FEOpExecutorMysqlProtocolTest` and
`DorisFlightSqlProducerTest` follow the moved methods. Local regression (single
FE built from this branch, BE from `f35dd8285aa`): `arrow_flight_sql_p0` 8/8,
`prepared_stmt_p0` 6/6, `load_p0/mysql_load` 7/7, `point_query_p0` 16/16,
`query_p0/cache` 12/12, `query_p0/explain` 9/9, `query_p0/dry_run` 1/1,
`query_p0/system` 13/14, `insert_overwrite_p0` 13/13, `insert_p0/transaction`
16/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`,
is `catalog_meta_cache_statistics` failing on the `MAX_WEIGHT` column #67726
added to the FE side, which the local BE predates -- a version skew of the test
setup, not of this PR.
- Behavior changed:
- [ ] No.
- [x] Yes. <!-- Explain the behavior change -->
A multi-statement request from a MySQL client without
`CLIENT_MULTI_STATEMENTS` whose 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?
- [x] No.
- [ ] Yes. <!-- Add document PR link here. eg:
https://github.com/apache/doris-website/pull/1214 -->
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01WXWFq9NqEnNjsywm5xnmu4
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
