Repository navigation
[test](fe) Test forwarding prepared executions that reuse cached parameter types - #70150
Conversation
…meter types ### What problem does this PR solve? Issue Number: None Related PR: apache#68162, apache#69314 Problem Summary: A client that runs a server-side prepared statement again can leave out the parameter types (new_params_bind_flag = 0) and have the server reuse the cached ones. A follower forwards each COM_STMT_EXECUTE to the master, which prepares the statement again for every forwarded execution and therefore has no cached types. branch-4.1 fixed this in apache#68162 (the part not specific to Lance). On master, apache#69314 fixed it in MysqlConnectProcessor.handleExecute, which rewrites the packet it forwards with a complete type table, and its tests cover cached types, unsigned flags, NULL changes and type updates. Those tests use at most four parameters, so the NULL bitmap is always one byte. They also hand the master the buffer from the follower's context, not the request that FEOpExecutor builds and Thrift carries. This ports the test apache#68162 added, MysqlPreparedStatementForwardingTest, to master's structure. A ten-parameter statement runs five times on a follower connection. Types are sent with the first and the fourth execution and reused otherwise. One execution sets a NULL in each byte of the NULL bitmap, and an unsigned INT later becomes a BIGINT. Each execution is forwarded through FEOpExecutor.buildStmtForwardParams and a Thrift round trip and runs on a new proxy context with untyped placeholders. Every parameter must bind there as it did on the follower, including the unsigned flag, and the client's packet must be read to its end and left unchanged. ### Release note None ### Check List (For Author) - Test: Unit Test - New MysqlPreparedStatementForwardingTest passes, together with MysqlConnectProcessorLongDataTest, FEOpExecutorMysqlProtocolTest, MysqlConnectProcessorCursorFetchTest and MysqlConnectProcessorExecuteCleanupTest. - It fails when handleExecute does not rewrite the forwarded packet for cached types, and when the rewrite keeps only the first byte of the NULL bitmap; MysqlConnectProcessorLongDataTest does not catch the second case. - Behavior changed: No - Does this need documentation: No Co-authored-by: Gabriel <[email protected]> Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
TPC-H: Total hot run time: 28213 ms |
TPC-DS: Total hot run time: 150727 ms |
ClickBench: Total hot run time: 24.17 s |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed PR #70150 at head 06c21ce. The only changed file is the FE unit test fe/fe-core/src/test/java/org/apache/doris/qe/MysqlPreparedStatementForwardingTest.java, so no production behavior, locking, persistence, configuration, compatibility, memory, or error-handling path is changed. The test's protocol setup was traced through MysqlConnectProcessor.handleExecute, FEOpExecutor.buildStmtForwardParams, MysqlProtocolAdapter.fillForwardRequest, Thrift binary serialization, and the proxy-side execute path. It covers cached versus new parameter types, an unsigned LONG to signed LONGLONG change, NULL bits in both bytes of a ten-parameter bitmap, unchanged client packet bytes, and all parameter bindings after the Thrift round trip. Construction-mocked StmtExecutor intentionally keeps this as a parser/binding/forwarding test; the existing FE cleanup tests cover per-execution StatementContext lifecycle separately. No existing P0/P1 inline findings or user focus items apply. Static review only; no build or test commands were run by this reviewer. Round 1 converged: all assigned subagents returned NO_NEW_VALUABLE_FINDINGS and every main risk item was dismissed with evidence.
Review converged after 1 round(s).
FE UT Coverage ReportIncrement line coverage `` 🎉 |
924060929
left a comment
924060929
left a comment
There was a problem hiding this comment.
This test-only PR checks prepared executions forwarded from a follower to a freshly prepared master context, which cannot reuse the client's cached parameter types. Independent static review traced handleExecute, FEOpExecutor.buildStmtForwardParams, MysqlProtocolAdapter.fillForwardRequest, Thrift serialization, and proxy-side parameter binding. The test covers a two-byte NULL bitmap, cached and updated types, unsigned INT to signed BIGINT, packet consumption, and unchanged client bytes. No actionable P0/P1/P2 issues were found in the FE scope. APPROVE.
StmtExecutor is mocked, so this validates binding and forwarding rather than SQL execution. No builds or tests were run during this review. The supplied current-head snapshot reports successful FE UT, compilation and P0 regression, but failed check_coverage_fe; required-check rules and the coverage failure cause were not established. This is code approval, not merge clearance.
Automated FE-focused static review (gpt-6.1-sol, high). No build or tests were run.
What problem does this PR solve?
Issue Number: None
Related PR: #68162, #69314
Problem Summary:
In short. This ports
MysqlPreparedStatementForwardingTestfrom branch-4.1 #68162 to master. Master already has the fix the test guards, through #69314. The port adds what master's own tests do not cover: a NULL bitmap of two bytes, and the forward request thatFEOpExecutorbuilds and Thrift carries. The PR changes tests only.Background. A client that runs a server-side prepared statement again may leave out the parameter types (
new_params_bind_flag= 0) and have the server reuse the ones it cached. A follower forwards every COM_STMT_EXECUTE to the master. The master prepares the statement again for each forwarded execution, so it has no cached types and cannot bind the parameters. #68162 fixed this on branch-4.1, together with the Lance prepared vector search it was written for. Master fixed it independently in #69314: when the client reuses cached types,MysqlConnectProcessor.handleExecutewrites the forwarded packet again, with the NULL bitmap,new_params_bind_flag= 1, every type code including the unsigned flag, and the values. #69314 also has the prepare stage plan with the context of the re-parsed statement, which is the other generic part of #68162. Its tests inMysqlConnectProcessorLongDataTestcover cached types, unsigned flags, changing NULLs, changing types, all-NULL executions and the prepare context.What those tests leave out. They use at most four parameters, so the NULL bitmap always fits in one byte. They also give the master the buffer kept on the follower's context, rather than the request
FEOpExecutor.buildStmtForwardParamsbuilds.What this PR adds. The test runs a ten-parameter statement five times on a follower connection:
After each execution, the test builds the forward request with
FEOpExecutor, passes it through a Thrift round trip and executes it on a new proxy context whose placeholders have no types. It then checks three things: every parameter binds to the same value as on the follower, the unsigned flag carries over, and the client's packet is read to its end and left unchanged. The code is the 4.1 test adapted to master's structure: master'sPrepareCommand, query id and audit setup, as inMysqlConnectProcessorLongDataTest, and expressions compared directly. The 4.1 assertion that the follower keeps the client's buffer was dropped, because master writes a separate buffer for cached types by design.Results.
handleExecuteon masterMysqlConnectProcessorLongDataTest(14)MysqlPreparedStatementForwardingTest(1)No value present)Original author: @Gabriel39
Release note
None
Check List (For Author)
Test
New
MysqlPreparedStatementForwardingTestpasses, together withMysqlConnectProcessorLongDataTest,FEOpExecutorMysqlProtocolTest,MysqlConnectProcessorCursorFetchTestandMysqlConnectProcessorExecuteCleanupTest(28 tests). The two mutations in the table above were run against it. This PR changes no production code.Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)