[fix](protocol) Prevent Connector/J cursor fetch from hanging on empty results - #67520
Merged
CalvinKirs merged 12 commits intoSep 10, 2026
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Member
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review status: REQUEST_CHANGES. I completed the full review on head 53cf2d7d400f508f08c514df3934fbeeba4d3157; the bounded review loop converged after two rounds with two distinct P1 findings.
Critical checkpoint conclusions:
- Goal and proof: The new packet fixes the direct Connector/J 8.0.28 empty-result hang, but the implementation does not complete the stated pre-9.5 compatibility goal for legacy 5.1/6.0 clients (first inline finding), and forwarded zero-placeholder executions can still return malformed non-empty results (second inline finding).
- Scope and clarity: The per-command flag, optional forwarding field, shared metadata helper, and focused tests are otherwise small and localized.
- Concurrency: The new state is connection-local and commands are processed serially on a connection; no new shared mutable state, locking, lock-order, or deadlock concern was found.
- Lifecycle and initialization: The cursor flag survives local/RPC retries and is cleared after command finalization. The new static constants have no cross-initialization dependency. The zero-placeholder forwarding lifecycle remains incomplete as described inline.
- Configuration: No configuration item is added.
- Compatibility: Thrift field 1008 is optional and wire-skippable during rolling FE upgrades. Protocol compatibility is incomplete for the verified legacy Connector/J identities and versions.
- Parallel paths: FE-local and coordinator/cache/short-circuit metadata paths both use the helper exactly once, and proxy packet ordering preserves the compatibility marker before rows and the real final marker afterward. The follower-to-master zero-placeholder path fails to restore binary execute mode.
- Conditional checks: Cursor-bit masking and the deprecated-EOF gate are sound and locally explained. The client-name/version condition is too narrow for affected released drivers.
- Test coverage: The unit test checks the inserted packet shape, and the regression covers direct-master Connector/J 8.0.28 empty and one-row results. Coverage misses legacy driver attributes and a follower-to-master non-empty zero-placeholder result, which correspond to the two findings.
- Test results: The checked-in output matches the suite's two queries, but I did not execute or regenerate tests because this review environment explicitly prohibits builds and test execution.
- Observability: This narrowly gated packet path does not need new metrics or INFO logging; existing connection/packet diagnostics are sufficient.
- Transactions, persistence, and data writes: None are introduced or modified.
- FE/BE variable propagation: The new field is FE-to-FE. Existing
mysql_row_binary_formatpropagation to BE depends on the master's command state and is wrong in the forwarded zero-placeholder case. - Performance: One connection-attribute lookup and one precompiled regex match per result metadata sequence is negligible; no material CPU or memory regression was found.
- Other issues and user focus: No additional user focus was supplied, and no other distinct correctness, error-handling, memory-safety, cloud-mode, or coverage issue survived the final changed-file and candidate sweep.
| return false; | ||
| } | ||
| Map<String, String> connectAttributes = context.getConnectAttributes(); | ||
| if (!MYSQL_CONNECTOR_J.equals(connectAttributes.get("_client_name"))) { |
Contributor
There was a problem hiding this comment.
[P1] Cover the legacy Connector/J cursor consumers
Connector/J 5.1.49 and 6.0.6 have the same cursor-probe behavior this workaround targets: when Doris advertises CLIENT_DEPRECATE_EOF, they consume the packet after column definitions for a cursor request. Those releases send _client_name=MySQL Connector Java (and 5.x/6.x versions), so this predicate never inserts the compatibility terminator. An empty result therefore still consumes its only final OK and waits for another packet; a non-empty result can consume its first binary row as status. Please include the verified legacy identity/version families (or classify by the affected protocol behavior) and add a boundary test.
| if (null != ctx.getPrepareExecuteBuffer()) { | ||
| params.setPrepareExecuteBuffer(ctx.getPrepareExecuteBuffer()); | ||
| } | ||
| params.setCursorFetchRequested(ctx.isCursorFetchRequested()); |
Contributor
There was a problem hiding this comment.
[P1] Preserve binary execute mode without parameters
For a zero-placeholder prepared SELECT, handleExecute never sets prepareExecuteBuffer, so this forwards cursor_fetch_requested=true without any marker that makes the master take its prepared-execute branch. proxyExecute then leaves the fresh context as COM_SLEEP; both coordinator paths set mysql_row_binary_format=false, and non-empty rows are serialized as text even though Connector/J is reading a COM_STMT_EXECUTE binary result. The new regression uses getServerPrepareJdbcUrl, which connects directly to the master, so it misses this path. Please forward execute/binary-result intent independently of parameter bytes (or always carry an empty execute buffer) and cover a follower-to-master non-empty result.
CalvinKirs
force-pushed
the
fix/cursor-fetch-empty-result
branch
from
September 4, 2026 08:24
b745f71 to
12e48c4
Compare
CalvinKirs
changed the title
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review status: capped/incomplete — the third and final convergence round added an accepted scoping constraint, which is included in the P1 old-master finding below; the workflow permits no fourth round.
Request changes. The direct Connector/J classifier and compatibility-terminator sequence are localized and coherent, but the end-to-end goal is not met across the production handshake and mixed-version FE paths.
Critical checkpoints:
- Goal and proof: direct empty/nonempty cursor results and classifier boundaries have tests, but real serializer negotiation, real proxy EOF status, both mixed-version directions, and old-master warning/info are not proved. The already-threaded zero-placeholder forwarding path also remains broken.
- Scope and clarity: production changes are focused on MySQL classification, connection state, packet framing, and FE forwarding. The Arrow Flight DDL smoke test is ancillary and does not validate this fix.
- Concurrency and lifecycle: the new state is connection-thread-owned, reset per command, and copied into fresh per-RPC proxy contexts. No new lock, deadlock, shared-mutation, circular-ownership, or static-initialization risk was found.
- Configuration and compatibility: no configuration is added. Optional Thrift fields are wire-skippable, but their absent-value and runtime-result semantics fail in both rolling-upgrade directions.
- Parallel paths and conditions: direct, new/new, new/old, old/new, coordinator, point-query, cached, FE-local, parameterized, zero-placeholder, legacy/deprecated EOF, affected/standard/unknown, and multi-statement paths were traced. The special gates have the inline defects below.
- Tests and results: the classifier, packet, request, and context tests are internally consistent and the regression output is deterministic, but mocks bypass the production capability and EOF-status flows. I did not run builds or tests because this review environment explicitly prohibits them.
- Observability and performance: the compatibility errors are actionable; no new metric is needed, and the classifier/boolean/packet overhead is negligible.
- Transactions, persistence, writes, and transport: no EditLog, storage format, FE-BE variable, or atomicity change exists. Forwarded DML executes once, but its protocol-visible result is degraded; FE-to-FE transport has the mixed-version defects below.
- Other correctness: warning/status serialization, binary/text selection, packet order, partial errors, and multi-result sequencing were checked. Four distinct inline issues remain. Existing threads
r3930709957andr3930709964were treated as duplicate fences; the former is fixed on this head and the latter was not repeated.
User focus: no additional review focus was provided, so the full PR was reviewed.
| packet = executor.getOutputPacket(); | ||
| if (ctx.getMysqlChannel().clientDeprecatedEOF() | ||
| && !executor.isForwardedClientDeprecatedEofApplied() | ||
| && executor.getProxyStatusCode() == 0) { |
Contributor
There was a problem hiding this comment.
[P1] Classify successful old-master reads without widening this guard
A real forwarded SELECT cannot satisfy this gate: result producers finish with QueryState.setEof(), while proxyExecute assigns status 0 only to OK and maps successful EOF to 1105. The follower therefore replays the unsafe cursor packets; the unit test mocks the impossible combination of query buffers plus status 0. Simply accepting EOF here would also reject safe old-master COM_QUERY and Connector/J 9.5+ results because this predicate never checks cursor intent or the compatibility class. Please recognize real result-set success, scope rejection to requests that need the cursor shim, test through real proxyExecute construction, and ensure a non-final multi-statement sends the local ERR only once.
| } else { | ||
| // An old master has already completed a DDL/DML operation. Rebuild its final OK locally | ||
| // instead of returning an upgrade error that could make the client retry side effects. | ||
| ctx.getState().setOk(executor.getForwardedAffectedRows(), 0, null); |
Contributor
There was a problem hiding this comment.
[P2] Preserve the old master's complete OK result
This rolling-upgrade branch rebuilds a successful DML response with only affectedRows, discarding the warning count and info string already encoded in the master's final OK packet. A normal forwarded INSERT calls OlapInsertExecutor.setReturnInfo(), which reports filteredRows as warnings and includes label/status/txnId in info; through an old master this branch changes those to zero warnings and no message. Please preserve or decode all protocol-visible OK fields (or safely reuse the ordinary OK packet) and cover a response with nonzero warnings and nonempty info.
| if (request.isSetClientDeprecatedEOF() && request.isClientDeprecatedEOF()) { | ||
| ctx.getMysqlChannel().setClientDeprecatedEOF(); | ||
| } | ||
| ctx.setCursorFetchRequested(request.isSetCursorFetchRequested() |
Contributor
There was a problem hiding this comment.
[P1] Handle cursor intent from old forwarding FEs
During a rolling upgrade an old follower cannot set the new optional cursor_fetch_requested field, so this silently records false. It still forwards CLIENT_DEPRECATE_EOF, Connector/J attributes, and, for parameterized statements, the execute buffer; the new master then emits binary rows but omits the compatibility metadata marker. An affected Connector/J cursor SELECT forwarded through that old FE can therefore still consume the final marker and hang. Please treat an absent cursor-intent field as an explicit mixed-version/unknown execute mode and fail safely when the affected combination cannot be disambiguated, with an old-sender/new-master parameterized cursor test.
| } | ||
| } else if (!Strings.isNullOrEmpty(infoMessage)) { | ||
| serializer.writeLenEncodedString(infoMessage); | ||
| } else if (capability.isDeprecatedEOF()) { |
Contributor
There was a problem hiding this comment.
[P2] Read the negotiated EOF capability here
This condition is false in the new unit test, but it remains true for real legacy-EOF connections. MysqlProto.negotiate records the client's bit only in MysqlChannel, then sets the serializer capability to context.getServerCapability(); that default mask always includes CLIENT_DEPRECATE_EOF. ProxyMysqlChannel starts with the same default as well. Consequently an authenticated client that did not negotiate the flag still gets the trailing zero byte this change intends to remove. Please key this from the negotiated/channel capability, and propagate it to proxy serialization, or store the negotiated mask in the serializer, with a handshake-level test.
CalvinKirs
added a commit
to CalvinKirs/incubator-doris
that referenced
this pull request
Sep 7, 2026
### What problem does this PR solve? Related PR: apache#67520 Problem Summary: Cursor queries forwarded to an older master can lose the metadata terminator required by older Connector/J clients. Normalize buffered old-master result boundaries at the follower while retaining complete DML/DDL OK packets. Forward negotiated capabilities and avoid MySQL channel access for Arrow requests. Preserve anonymous older-client cursor behavior and require LOCAL_FILES only for client-side uploads, keeping FE file loads compatible. ### Release note Preserve cursor result boundaries and DML/DDL status information during FE rolling upgrades, and retain Arrow forwarding and server-side file loading. Old followers must be upgraded to preserve cursor intent. ### Check List (For Author) - Test: FE build with Checkstyle; 113 targeted unit tests passed across the complete run and corrected load-test rerun; cursor and prepared SHOW regression; Arrow follower DDL; baseline/candidate real JDBC, Shell, TLS, legacy EOF, file loading and connection-reuse probes. Four broader prepared suites fail identically on baseline/candidate with the available older BE; auth metadata regression and product OIDC E2E remain unverified. See v7 report. - Behavior changed: Yes, preserve forwarded protocol and negotiated capabilities - Does this need documentation: No (compatibility fix; validation report included)
CalvinKirs
added a commit
to CalvinKirs/incubator-doris
that referenced
this pull request
Sep 7, 2026
### What problem does this PR solve? Related PR: apache#67520 Problem Summary: Remove local review and validation documents from the PR as requested. Production code and tests are unchanged. ### Release note None ### Check List (For Author) - Test: No need to test (documentation removal only) - Behavior changed: No - Does this need documentation: No
CalvinKirs
added a commit
to CalvinKirs/incubator-doris
that referenced
this pull request
Sep 7, 2026
### What problem does this PR solve? Related PR: apache#67520 Problem Summary: Merge current master and resolve MysqlOkPacketTest's conflict with the JUnit 5 migration. Retain exact legacy/deprecated-EOF packet assertions and migrate the PR's remaining JUnit 4 imports and assertions so the tests match the upstream test framework. ### Release note None ### Check List (For Author) - Test: Standalone FE Core Checkstyle; source checks for JUnit 4 references, conflict markers and retained packet assertions. No compilation or unit-test rerun, as requested. - Behavior changed: No (conflict resolution and test framework migration) - Does this need documentation: No
Member
Author
|
run buildall |
Member
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Your access token could not be refreshed because your refresh token was already used. Please log out and sign in again. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 16944 ms |
Contributor
TPC-DS: Total hot run time: 81954 ms |
Contributor
ClickBench: Total hot run time: 14.68 s |
Member
Author
Local agent review result — reviewed local commit, not current PR headCompleted a local review using the official
The local commit addresses the previously identified zero-parameter prepared-forwarding issue by preserving the empty execute payload, exercises the actual plain Statement entry, and corrects the two SQL-result assertions identified in the previous review. The updated regression disables JDBC preparation fallback and prepares on a follower before enabling forwarding. Limits: These fixes and this verdict apply to the reviewed local commit, not the current remote PR head. This comment is not an exact-head PASS receipt or a human Apache approval. No build or tests were run during this review. Checkstyle had passed for the local fix, but the new unit/regression tests and strengthened JDBC probe have not been executed; the historical JDBC successes do not prove the newly covered forwarding path. Follower coverage is explicitly skipped when no live non-master FE exists. A separate, pre-existing parameter-type caching issue in repeated parameterized prepared forwarding remains unresolved and is outside this zero-parameter fix. It is not counted as a newly introduced finding. |
Member
Author
|
run buildall |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
CalvinKirs
added a commit
to CalvinKirs/incubator-doris
that referenced
this pull request
Sep 7, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#67520 Problem Summary: A zero-parameter server-prepared statement can be prepared on a readable follower and later forwarded during execution. The follower saved the execute payload only for statements with parameters, so the master treated the request as a text query. Preserve the empty payload as the existing binary execution marker. Add coverage for empty-payload Thrift serialization and the direct-to-forwarded execution transition, and prevent JDBC fallback from hiding missing server-prepared coverage. Exercise the actual plain Statement entry. ### Release note Fix incorrect result encoding when zero-parameter server-prepared queries are forwarded from a follower FE. ### Check List (For Author) - Test: Checkstyle passed with 0 violations; git diff --check passed. Unit and regression tests added but not executed; compilation was excluded by request. - Behavior changed: Yes; preserve binary result encoding for zero-parameter prepared execution forwarded to the master FE. - Does this need documentation: No
CalvinKirs
added a commit
to CalvinKirs/incubator-doris
that referenced
this pull request
Sep 7, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#67520 Problem Summary: Forwarding now reads the negotiated MySQL capability. The delegated credential test constructs a context without a handshake and leaves that capability null, causing an NPE before its credential assertions. Initialize the test context consistently with the protocol forwarding tests. ### Release note None ### Check List (For Author) - Test: Checkstyle passed with 0 violations; git diff --check passed. Unit tests not run locally; buildall requested on the PR after pushing. - Behavior changed: No - Does this need documentation: No
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
hubgeter
previously approved these changes
Sep 9, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#67520, apache#67569 Problem Summary: Merge master into the cursor-fetch fix branch. Resolve the overlapping FEOpExecutor changes by preserving the upstream MySQL connection guard and the negotiated capability propagation needed by cursor forwarding. Initialize negotiated capabilities in the upstream MySQL forwarding tests, which otherwise fail with a null capability after the merge. Preserve Arrow Flight SQL forwarding without accessing a MySQL channel. ### Release note None ### Check List (For Author) - Test: Unit Test / Build - ./build.sh --fe passed with DISABLE_BUILD_UI=ON and FE_MAVEN_THREADS=4. - ./run-fe-ut.sh --run passed all 28 tests in FEOpExecutorMysqlProtocolTest, FEOpExecutorFlightForwardTest, ConnectProcessorFlightForwardOutcomeTest, ConnectProcessorForwardProtocolTest, MysqlConnectProcessorCursorFetchTest, MysqlCursorFetchCompatibilityTest and MysqlResultSetEndPacketTest. - Final fe-core Checkstyle passed with 0 violations; git diff --cached --check passed. - Behavior changed: No (conflict resolution preserves both branches' intended behavior) - Does this need documentation: No
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 16959 ms |
Contributor
TPC-DS: Total hot run time: 81871 ms |
Contributor
ClickBench: Total hot run time: 14.74 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
924060929
approved these changes
Sep 10, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
This was referenced Sep 10, 2026
morningman
added a commit
that referenced
this pull request
Sep 11, 2026
…on PRs from master in merge order (#67533 #67594 #67646 #67669 #67665 #67710 #67775 #67575 #67480) (#67830) Cherry-picked from #67533, #67594, #67646, #67669, #67665, #67710, #67775, #67575, #67480 Batch pick of every merged PR carrying the `incremental-computation` label that `branch-incremental-computation` does not have yet, in the order they landed on master (`git log --first-parent`). One commit per PR, each created with `git cherry-pick -x` so the message ends with `(cherry picked from commit <master sha>)`. | # | Master commit | PR | Title | |---|---|---|---| | 1 | 300d532 | #67533 | [fix](table stream) Preserve table stream offsets during cleanup | | 2 | bfe46ec | #67594 | [fix](row binlog) make time-based incremental reads use a safe fence | | 3 | c1bff0d | #67646 | [fix](ivm) Fail aggregate IVM refresh when the previous refresh txn is not visible yet | | 4 | 4f3abce | #67669 | [fix](ivm) Resolve IVM identity keys by slot identity and materialize unprojected keys | | 5 | 7129a3e | #67665 | [fix](ivm) Propagate and compensate failures of the IVM excluded-trigger-tables ALTER | | 6 | a565aca | #67710 | [fix](regression) Stop MTMV task waits from latching onto the previous task | | 7 | 8565db2 | #67775 | [fix](ivm) Repair the removed rewrite-context constructor call in IvmNormalizeMTMVJoinTest | | 8 | 0ded66a | #67575 | [feature](ivm) Support incremental refresh for array_agg and collect_list aggregates | | 9 | efc929a | #67480 | [fix](table stream) fix table stream TSO boundary semantics | Not included on purpose: - #62606 (IVM feature) is already in the branch's fork point (`efedf10c7e3`). - #67508 already landed on this branch via #67712. ### Cherry-pick notes - All nine picks applied without conflicts. Each pick's diff is identical to its master commit (compared with `index`/`@@` lines stripped). - Every touched file is byte-identical to master at `efc929aa7af` except `Env.java`, `FrontendServiceImpl.java` and `FrontendService.thrift`, whose remaining differences come only from unrelated master commits that are not part of this label (#67708, #66770, #67572, #67520). - #67594 is the safe-read-fence prerequisite of #67480; picking both in master order is what makes #67480 apply cleanly here (it replaces the earlier stand-alone attempt #67828, which had to hand-adapt around the missing fence). - #67775 is needed because #67646 (removes the `IvmRewriteContext` constructor) and #67669 (test that still used it) are both picked. ### Verification - `be/src/exec/scan/olap_scanner.cpp` (the only BE change): syntax-only compile with the Release flags, clean. The new thrift RPC `acquireTimeBasedChangeReadFence` is FE-only; nothing in `be/` or `cloud/` references it. - FE: `run-fe-ut.sh --run` on this branch (regenerates thrift, compiles fe-core main + test) with the 19 test classes touched by the picks: 19 classes, 450 tests, 0 failures, 0 errors, BUILD SUCCESS — `TableStreamManagerCleanupTest` 9, `CloudGlobalTransactionMgrTest` 22, `AlterMTMVTest` 24, `IvmAggArrayAggProcessorTest` 2, `IvmAggCollectListProcessorTest` 1, `IvmAggDeltaHandlerTest` 33, `IvmDeltaRewriterTest` 20, `IvmJoinDeltaHandlerTest` 23, `IvmLinearDeltaHandlerTest` 39, `IvmNormalizeMTMVJoinTest` 44, `PhysicalPlanTranslatorTest` 17, `IvmIncrRefreshMTMVTest` 13, `IvmNormalizeMTMVTest` 52, `CreateMTMVCommandTest` 94, `ExplainTableStreamPlanTest` 23, `OlapScanNodeTest` 12, `TimeBasedChangeVisibleWaiterTest` 7, `TransactionIdGeneratorTest` 1, `TSOTimestampTest` 14. - Regression suites touched parse cleanly (groovy parser check). 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_018m1ARNXtGSWucJTy34uwe1 --------- Co-authored-by: TsukiokaKogane <cby141994@gmail.com> Co-authored-by: Luwei <814383175@qq.com> Co-authored-by: yujun <yujun@selectdb.com>
morningman
added a commit
that referenced
this pull request
Sep 11, 2026
…ead of a Flight subclass (#67835) ### What problem does this PR solve? Issue Number: #67577 -- the tracking issue for the protocol-agnostic session and execution layer. This is the second PR of its Stage 1 (after the golden baseline #67789) and does not close it. The shape after this PR: ``` +---------------------------+ +-----------------------------+ | MySQL client | | Arrow Flight SQL client | +-------------+-------------+ +--------------+--------------+ | | v v +---------------------------+ +-----------------------------+ | MysqlServer | | DorisFlightSqlProducer | | AcceptListener | | every call goes through | | ReadListener | | adapter.runCommand() | | (xnio: one command | | (lock: one command | | at a time) | | at a time) | +-------------+-------------+ +--------------+--------------+ | | v v +---------------------------+ +-----------------------------+ | MysqlConnectProcessor | | FlightSqlConnectProcessor | +-------------+-------------+ +--------------+--------------+ | | +----------------------------+-----------------------------+ | v +-------------------------------------------------------------------------------------------+ | ConnectContext -- the session, one per connection | | | | user, catalog / db, SessionVariable, transaction, prepared statements, | | queryId / stmtId, executor, audit, ... | | | | protocolAdapter : ProtocolAdapter | | bound once, by forMysql() | forMysqlProxy() | forFlight() | new ConnectContext() | | | | getMysqlChannel() getCapability() getFlightSqlChannel() isReturnResultFromLocal() .. | | same signatures as before, now delegate to protocolAdapter | +---------------------------------------------+---------------------------------------------+ | v +-----------------------------------------------+ | <<interface>> qe.protocol.ProtocolAdapter | | | | type() | | remoteHostPortString(ctx) | | resultSinkType() | | connectPool(scheduler) | | afterStatement(ctx) | | closeConnection(ctx) | +-----------------------+-----------------------+ | +--------------------+----------------------------+ | | +-----------------------+----------------------+ +-----------------------+----------------------+ | mysql.protocol.MysqlProtocolAdapter | | service.arrowflight.protocol | | | | .FlightProtocolAdapter | | MysqlChannel: | | | | a socket | | peerIdentity (the bearer token) | | ProxyMysqlChannel (statement forwarded | | FlightSqlChannel (FE-side result cache) | | to the master) | | endpoints of the last query | | DummyMysqlChannel (internal context) | | returnResultFromLocal | | server / negotiated MysqlCapability | | prepared queries | | handshake packet, MysqlSslContext | | deferred executors + idle bound | | COM_STMT_EXECUTE packet, cursor flag | | (#62259, #67503) | | clientConsumesCursorMetadataTerminator() | | per-session command lock: | | accept-query loop | | runCommand() / callCommand() | | (start / suspend / resume / stop) | | | +----------------------------------------------+ +----------------------------------------------+ Not touched by this PR: ConnectProcessor / StmtExecutor / Coordinator keep calling the ConnectContext getters. The result-encoding half (ResultSender) and the capability predicates that replace the remaining ConnectType branches come in the next PRs. ``` **In plain terms.** A client session in the frontend is a `ConnectContext`. Today that one class holds the state of both wire protocols at the same time: the MySQL socket, the capabilities negotiated with the MySQL client, the handshake and SSL state, the prepared-statement packet being executed -- and, next to them, the Arrow Flight SQL result cache, the backend endpoints of the last Flight query, the prepared queries and the deferred coordinators. Which half is real is decided by a subclass, `FlightSqlConnectContext`, that overrides six methods and leaves every other Flight member sitting on the base class, where a MySQL connection carries it as dead weight and a Flight session throws from the MySQL ones. This PR gives each protocol its own object, a `ProtocolAdapter`, and binds a session to exactly one of them when it is created. Nothing a client sees changes: the golden byte-for-byte baseline recorded in #67789 is identical before and after. Problem Summary: `ConnectContext` mixes three things: the session (user, catalog and database, session variables, transaction, prepared statements, the running statement), the MySQL protocol state, and the Arrow Flight SQL protocol state. The next steps of #67577 move the result path of both protocols onto one shared implementation, which needs a place for "what only this protocol knows" that is not the session itself. This PR creates that place and moves the state, without touching the execution layer yet: `StmtExecutor`, `ConnectProcessor` and the coordinators still call the same `ConnectContext` getters, which now delegate. ### What is changed? **`qe/protocol/ProtocolAdapter`** -- the wire-protocol half of a connection: `type()`, the client address for processlist and the audit log, the result sink type the backend must use, the pool the connection is registered in (there is still one per protocol), a per-statement cleanup hook and `closeConnection`. `qe/protocol` holds only the interface; each front end implements it in a `protocol` subpackage of its own package (`mysql/protocol`, `service/arrowflight/protocol`), which is also where the result senders of the next step go. **`mysql/protocol/MysqlProtocolAdapter`** -- owns the `MysqlChannel` (a socket, the `ProxyMysqlChannel` of a forwarded statement on the master, or the `DummyMysqlChannel` of an internal context), the server and negotiated capabilities, the handshake packet, the SSL context, the `COM_STMT_EXECUTE` packet and its cursor flag, and the xnio accept-query loop that `AcceptListener` / `ReadListener` drive. It also owns the decision `StmtExecutor` and `FEOpExecutor` used to compute from `ConnectContext` fields -- whether the Connector/J release on the other end consumes the metadata terminator of a cursor result (#67520) -- as `clientConsumesCursorMetadataTerminator`. **`service/arrowflight/protocol/FlightProtocolAdapter`** -- owns the peer identity (bearer token), the `FlightSqlChannel`, the prepared queries, the endpoints of the last query, `returnResultFromLocal` and the deferred executors of #62259 / #67503, together with their idle bound. `ConnectContext` keeps `checkTimeout` and the idle reaper unchanged; only the list moved. It also serializes the commands of a session. gRPC runs each call of a session on whatever thread it likes and nothing in the Flight transport orders them, while `ConnectContext` is not thread-safe (the existing `DorisFlightSqlProducerTest` spells that out). `runCommand` / `callCommand` take a per-session lock, make the session the thread's current `ConnectContext` for the duration, restore the previous one afterwards, and give up with `UNAVAILABLE` after the session's query timeout if another command is still running. `DorisFlightSqlProducer` runs statement execution, prepared statement creation and close, DoGet of a frontend-side result and the catalog / schema / table metadata requests through it. DoGet of a frontend-side result streams under the lock on purpose: the next statement of the session resets the channel, whose removal listener closes the `VectorSchemaRoot` being streamed. Session teardown (token expiry, `CloseSession`, `KILL`) does not take the lock; that path is reworked when the token becomes the session credential. **`ConnectContext`** -- gets `protocolAdapter` and three factories: `forMysql(StreamConnection)`, `forMysqlProxy(sessionId)` (replaces the `new ConnectContext(null, true, sessionId)` call in `FrontendServiceImpl`) and `forFlight(peerIdentity)` (replaces the subclass in `FlightSessionsManager`). The existing constructors stay as thin wrappers, so the ~115 test files that call `new ConnectContext()` are untouched. Every protocol-specific getter keeps its signature and delegates: `getMysqlChannel()`, `getCapability()`, `getFlightSqlChannel()`, `isReturnResultFromLocal()` and so on. A getter that only makes sense on the other protocol throws `IllegalStateException` naming the actual protocol (the subclass used to throw a `RuntimeException` for `getMysqlChannel()`; the base class used to return `null` / an empty list for the Flight ones, which no caller relied on). `FlightSqlConnectContext` is deleted: its `getClientIP` / `getRemoteHostPortString` / `closeChannel` / `setQueryId` overrides are the adapter's `remoteHostPortString` / `closeConnection` / `connectPool`, and its `kill` override only differed in log text. Removed as dead code while touching the class: `isSend` / `setIsSend` (nothing read them; the real flag lives on `MysqlChannel`), `cloneContext()` (no caller, and it would have to share a channel between two adapters), and the two lines of `resetConnection()` that cleared Flight-only fields (`COM_RESET_CONNECTION` is only sent by MySQL clients). Not in this PR, deliberately: the execution layer still branches on `ConnectType`, and an internal context is still a MySQL context over a `DummyMysqlChannel`, exactly as before. Both go away in the follow-up PRs that introduce the result sender and the capability bits. ### Verification - **Golden baseline of #67789**: `MysqlPacketGoldenTest` (27 cases, byte for byte) and `FlightResultGoldenTest` pass unchanged. Not a byte of the recorded traffic moved. - New unit tests: `FlightProtocolAdapterTest` (commands of one session run one at a time, a waiting command fails with `UNAVAILABLE` after the query timeout, the thread's current context is set and restored, a failing command releases the session, `KILL` unregisters the session from the Flight pool, the trace id lands in the Flight pool) and `MysqlProtocolAdapterTest` (internal and proxy contexts, the cursor-terminator decision and its per-statement reset, the accept-query loop and close going through the channel). - Existing tests adjusted to the factories: the ones that built a `FlightSqlConnectContext`, poked `mysqlChannel` / `connectType` through reflection, or used a plain `new ConnectContext()` as a Flight session (`ShortCircuitPointQueryTest`, `AuditLogWorkloadGroupTest`, `StmtExecutorTest`, `ConnectContextTest`, `MysqlProtoTest`, `ConnectionExceedTest`). 26 test classes around the session, the MySQL channel and the Flight producer: 166 tests, 0 failures. - Regression on a local cluster built from this branch: `arrow_flight_sql_p0` (8 suites, including the forward-to-master, query-release, point-query, SQL cache and `DatabaseMetaData.getColumns` paths) and `prepared_stmt_p0` (cursor fetch and server-side prepare over MySQL). - `checkstyle:check` on fe-core (main and test sources): 0 violations.
morningman
added a commit
that referenced
this pull request
Sep 15, 2026
…on PRs from master in merge order (#67753 #67802 #67814 #67837 #67853 #67876) (#68017) Cherry-picked from #67753, #67802, #67814, #67837, #67853, #67876 Batch pick of every merged PR carrying the `incremental-computation` label that `branch-incremental-computation` does not have yet (no `incremental-computation-picked` label), in the order they landed on master (`git log --first-parent`). One commit per PR, each created with `git cherry-pick -x` so the message ends with `(cherry picked from commit <master sha>)`. Follows the same convention as #67830. | # | Master commit | PR | Title | |---|---|---|---| | 1 | fe39f5b | #67753 | [fix](ivm) Answer FE-computable dry runs on the frontend instead of a placeholder backend | | 2 | f8ed33f | #67802 | [fix](ivm) Refresh the surviving partitions after an IVM baseline rebuild | | 3 | 7bd89a0 | #67814 | [fix](ivm) Stop the incremental delta from reading partitions the MV dropped | | 4 | 3050a9a | #67837 | [fix](ivm) Invalidate the baseline when a column used by the MV is dropped | | 5 | 22c95eb | #67853 | [fix](ivm) Carry the row-binlog hidden columns in the analyzed MTMV schema | | 6 | 3390a7a | #67876 | [test](ivm) Remove unnecessary cloud skips from IVM suites | Not included on purpose: - The 11 labelled PRs that already carry `incremental-computation-picked` (#62606 in the fork point, #67508 via #67712, the nine of #67830). - #67820 is still open on master; this branch already carries its content via #67861. ### Prerequisite check None of the six PRs declares a prerequisite, and none of them needs another master PR for its behavior. The only master commits that touch the same files and are not on this branch are unrelated to incremental computation (#66761 TIMESTAMP_NS, #67545 DLF, #67569 / #67520 / #67835 MySQL-protocol and session refactors, #67186 Hive partition batching, #67787 SQL cache user variables); they were left out, and two picks needed a mechanical adaptation because of that: - **#67753** conflicted in `StmtExecutor.sendMetaData`: master had already extracted the post-metadata EOF into `sendMetadataTerminatorIfNeeded(channel)` (#67520, a Connector/J cursor-fetch fix). The branch keeps its inline EOF block and now sends it on the given `channel` instead of `context.getMysqlChannel()`, which is exactly what the extracted helper does on master. Everything else in the pick is identical to the master commit. - **#67814** applied cleanly but did not compile: the new `MTMVPartitionUtil.generateRelatedBasePartitionIds()` returns an `Optional`, and on master `import java.util.Optional;` came with #67186. The import was added to the pick commit; that is the only difference from the master commit. The other four picks applied without conflicts and are byte-identical to their master commits (diffs compared with `index`/`@@` lines stripped). Both adaptations are recorded in the respective commit messages. ### Drift check against master After the six picks, every touched file is byte-identical to master at `3390a7a721f` except `MTMV.java`, `MTMVTask.java`, `MTMVPartitionUtil.java`, `MTMVPartitionUtilTest.java`, `MTMVTaskTest.java` (differences = #67186 + #67545 + #66761), `CreateTableInfo.java` (= #67787) and `StmtExecutor.java` (= #67520 + #67569 + #66761 + the later session refactors #67835 / #67883 + this branch's #67861). For the first six files, applying those unrelated master commits on top of the branch's versions reproduces master's files exactly; for `StmtExecutor.java`, the diff against master right after #67753 (`fe39f5b6a42`) consists only of #67520 / #67569 / #66761 / #67861 hunks. So nothing IVM-related is missing. The regression framework, plugins and the whole `mtmv_p0/ivm` suite/data directories are identical to master. ### Verification - FE: `run-fe-ut.sh --run` on this branch (regenerates thrift, compiles fe-core main + test) with the 17 test classes touched by the picks or extending the touched `IvmDeltaTestBase`: 17 classes, 404 tests, 0 failures, 0 errors, BUILD SUCCESS (5:19 min) — `MTMVPlanUtilTest` 24, `IvmAggDeltaHandlerTest` 33, `IvmDeltaRewriteHelperTest` 17, `IvmNormalizeMTMVJoinTest` 44, `IvmJoinDeltaHandlerTest` 23, `IvmDeltaRewriteStateTest` 10, `IvmPlanSignatureGeneratorTest` 22, `IvmBaselineRebuildTest` 28, `IvmLinearDeltaHandlerTest` 39, `IvmDeltaRewriterTest` 23, `IvmNormalizeMTMVUnionTest` 10, `MTMVTaskTest` 50, `MTMVPropertyUtilTest` 13, `MTMVPartitionUtilTest` 16, `SchemaChangeHandlerTest` 22, `StmtExecutorInternalQueryTest` 3, `StmtExecutorTest` 27. - FE checkstyle on fe-core: 0 violations. - No BE, cloud or thrift changes in this batch. - All 18 touched groovy files (framework `Suite.groovy`, `plugin_planner.groovy`, 16 suites) parse cleanly (groovy parser check). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: yujun <yujun@selectdb.com>
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
6 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: None
Related PR: #61050, #61062
After
CLIENT_DEPRECATE_EOFis negotiated, affected Connector/J clients usinguseCursorFetch=trueand a positive fetch size can hang when a server-prepared statement returns an empty result. The driver consumes the first terminator after column definitions while checking whether a server cursor was created. With no rows, this consumes the only result-end marker, so the driver waits for a packet that Doris will never send.The reported Connector/J 8.2.0 case is covered by real-driver tests. This PR preserves the final result marker for affected clients and avoids inserting the compatibility marker for identified Connector/J 9.5+ and MariaDB clients. It also preserves this behavior through FE forwarding and fixes related packet/capability regressions.
What is changed?
CURSOR_TYPE_READ_ONLYfrom eachCOM_STMT_EXECUTE, retain it in the request context, and classify driver behavior using connection attributes. For affected cursor clients with deprecated EOF enabled, insert a compatibility ResultSet OK after metadata so the real final marker remains available. Identified modern clients and ordinary non-cursor requests keep the standard sequence.CLIENT_LOCAL_FILESonly for client-sideLOCAL INFILEuploads. FE-side file reads do not require a client-upload capability and remain usable when that bit is absent.Full PR diff against the merged master, at
32d01e5df19:The test total includes 11 automatically generated expected-output lines. Environment configuration and review documents are excluded.
Compatibility boundaries
COM_STMT_FETCHbatching.Release note
Fix affected Connector/J cursor queries hanging on empty prepared results. Preserve negotiated MySQL packet formats, forwarded DML/DDL OK information, Arrow Flight SQL forwarding and FE-side file loading. Upgrade the client-facing follower to preserve cursor intent during an FE rolling upgrade.
Check List (For Author)
prepared_stmt_p0/cursor_fetch_empty_result,prepared_stmt_p0/prepared_show, andarrow_flight_sql_p0/test_ddlthrough a real follower passed. Cursor expected output was generated by the regression runner.mvn checkstyle:check -pl fe-core -Dcheckstyle.skip=falsepassed with 0 violations. Compilation and unit tests were not rerun after the JUnit 5 migration.assertEqualsrather than regressionqtoutput, and missing dedicated tests for errors occurring after partial metadata or rows have been buffered. Those error paths were inspected in source, not fault-injected; the OK/ERR preservation unit test does not cover buffered intermediate errors.Check List (For Reviewer who merge this PR)