Repository navigation
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review conclusion: request changes for one P1 correctness issue. A server-prepared SELECT with a parameterized vector_search property fails if it is prepared locally on a readable follower and a later execution is forwarded to the master: reconstructed PREPARE plans with the executor's unmarked StatementContext before the forwarded parameter packet is decoded. The planner context must be synchronized, and this path needs a forced-forward/proxy regression.
Critical checkpoints: the direct PREPARE/EXECUTE path preserves placeholder identity and rebuilds fresh bindings, authorization, Lance metadata, snapshot state, request values, and the global TopN on every execution. Schema-only PREPARE stops before physical translation, so its deferred vector and dummy values cannot reach a scan. Indexed and flat request construction, limit/offset handling, property validation, plan-copy behavior, cache fencing, and FE/BE request checks were traced without finding another defect. The added JDBC regression covers real server-prepared A/B/A reuse for both search modes, but it does not exercise execution forwarding. The expression AGENTS checkpoint is not implicated because no checkLegalityBeforeTypeCoercion overload was added or changed. No user-specific focus points were provided.
This was a static-only review as required; I did not run builds or tests. The PR author's reported FE tests/checkstyle and the fixture-gated external regression were not independently validated here. Two complete review rounds covered all nine changed files and converged on this single non-duplicate finding, with no unresolved candidates.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review of head 1364feb56e found one blocking correctness issue.
- Prepared-property parsing, placeholder ID retention/copying, schema-only PREPARE deferral, and fresh per-EXECUTE vector/top-k/offset/filter/field-ID/snapshot rebuilding are internally consistent.
- The prior forwarded PREPARE
StatementContextissue is fixed at this head and was treated as an existing-thread duplicate fence. - Repeated forced forwarding still loses Connector/J's cached parameter types: the second same-typed COM_STMT_EXECUTE omits the type table while the master reconstructs an untyped PREPARE, so the A/B/A path fails before binding. See the inline P1.
- The expressions review checkpoint is not directly implicated because no
checkLegalityBeforeTypeCoercionoverload changed. - No additional user focus was provided.
Validation status: static inspection only, as required by the runner contract. I did not run builds or tests. The author reports 107 FE tests and FE Checkstyle passed and the Groovy regression compiled; the external multi-FE Lance regression was not run, and static protocol tracing indicates its second forwarded iteration will fail.
|
run buildall |
|
/review |
|
run buildall |
|
/review |
|
Fixed both JDBC URL concatenations in the Lance prepared-search regression. A leading Verified locally before committing:
The fix does not weaken any assertions or skip forwarding coverage. |
|
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. |
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
FE UT Coverage ReportIncrement line coverage |
d8a1594 to
cab91e1
Compare
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review conclusion: no blocking issues found at head cab91e1. Summary-only approval comment; no inline comments.
Critical checkpoints:
- Prepared vector_search properties are parsed with stable placeholder IDs, restricted to query_vector/top_k/offset/filter, reject dynamic names/duplicates/NULL or non-literal execute values, and are rebound into a fresh property map and VectorSearch/catalog function on every EXECUTE. Retained plans do not reuse a prior Lance vector, metadata snapshot, or catalog-function cache.
- PREPARE defers only query-value-dependent vector encoding, shape/dimension, multi-vector metric, and candidate-budget checks. Table/catalog authorization, metadata/version, schema, field IDs, static options, filter encoding, and top-k/offset invariants remain enforced; PrepareCommandPlanner stops at analysis, so an unset query vector cannot reach physical scanning.
- Repeated COM_STMT_EXECUTE forwarding preserves the follower's cached MySQL type codes and unsigned bits when the packet omits its type table, rebuilds the null bitmap/type table in little-endian form, copies values without mutating the client buffer, and retains zero-parameter forwarding behavior. The existing omitted-type forwarding thread is therefore a duplicate fence and is not resubmitted.
- PrepareCommand now installs the parser-created prepare-stage StatementContext before slot planning and registration. The existing forwarded planner-context thread is fixed at this head and is also a duplicate fence.
- The required expressions AGENTS checkpoint is not implicated: this PR adds no checkLegalityBeforeTypeCoercion overload or child traversal.
- No user-specific review focus points were supplied.
The normal full review and the risk-focused forwarding/context review both returned NO_NEW_VALUABLE_FINDINGS after complete changed-file coverage and missed-issue rechecks. This was static-only review as required by the runner; I did not run builds or tests. The author-reported FE tests/checkstyle and fixture-gated external regression were not independently validated here. The repository contained no discoverable code-review SKILL.md, so the mandated review prompt and checked-in AGENTS guide were followed as the operative instructions.
FE UT Coverage ReportIncrement line coverage |
### What problem does this PR solve?
`vector_search("query_vector"=?, "top_k"=?)` currently converts the
placeholders into the literal string `?`, so server-side prepared Lance
vector queries fail during analysis.
Preserve placeholders for `query_vector`, `top_k`, `offset`, and
`filter`, and bind fresh property values on every EXECUTE. PREPARE
resolves the output schema without requiring a query vector. Table,
column, and search configuration remain constant. Each execution
performs normal analysis and planning and resolves Lance metadata again;
the retained statement never caches a bound vector or snapshot.
This supports both indexed and flat searches and reuses the parsed
statement. Forwarded executions preserve cached parameter types when the
client omits the type table on repeated executions; the forwarding
request includes the types needed by a fresh master-side PREPARE. It
does not introduce an execution-plan cache or bypass the optimizer.
Example with MySQL Connector/J and `useServerPrepStmts=true`:
```java
try (PreparedStatement statement = connection.prepareStatement(
"SELECT id, _distance FROM vector_search("
+ "'table'='catalog.db.items', 'column'='embedding', "
+ "'query_vector'=?, 'top_k'=?, 'offset'=?, 'filter'=?)")) {
for (String vector : vectors) {
statement.setString(1, vector); // JSON array matching the vector column dimension.
statement.setInt(2, 10);
statement.setInt(3, 0);
statement.setString(4, "id > 0");
try (ResultSet rows = statement.executeQuery()) {
// Consume the result before executing the next vector.
}
}
}
```
### Release note
Support server-side prepared parameters for Lance `vector_search` query
vectors, top-k, offsets, and filters.
### Validation
- Proxy PREPARE tests reproduce the stale planner context in both search
modes before the fix; PREPARE now synchronizes the executor with the
parser-created context before parameter decoding.
- Parser and analyzer tests cover parameter retention, quoted question
marks, case-insensitive properties, constant-only property rejection,
repeated execution with different vectors and snapshots, mixed
constants/parameters, and invalid or missing parameters.
- A JDBC regression reuses one `ServerPreparedStatement` with A/B/A
parameter changes and compares results with literal SQL for both indexed
and flat searches. On live non-master FEs, it also prepares locally and
forces repeated executions to the master.
- A protocol test exercises five executions through the follower
decoder, RPC request construction, Thrift serialization, and a fresh
proxy decoder. It covers omitted type tables, unsigned values, NULL
parameters across two bitmap bytes, and later parameter type changes.
Local execution retains the original payload without copying it.
- PREPARE and EXECUTE share single-vector and multi-vector schema
validation. Tests cover schema-only PREPARE, multi-vector binding, and
rejection of invalid dimensions and excessive candidate budgets at
EXECUTE. The four CI FE UT failures caused by unbound-vector validation
were reproduced before the fix.
- Local verification: all 154 tests passed via `run-fe-ut.sh` across
`MysqlPreparedStatementForwardingTest`,
`MysqlConnectProcessorCursorFetchTest`, `FEOpExecutorMysqlProtocolTest`,
`LancePreparedSearchTest`, `LancePreparedStatementTest`,
`LanceVectorQueryTest`, `VectorSearchTableValuedFunctionTest`,
`FullTextSearchTableValuedFunctionTest`, `LanceScanNodeTest`,
`PrepareTest`, and `NereidsParserTest`. FE Checkstyle passed. The full
external JDBC regression was also executed locally using the current PR
FE build, a master FE, an observer FE, one BE, and the MinIO/Lance
fixtures: 1 suite passed, 0 failures, 0 skips. It executed 12 real
server-prepared queries covering direct and forwarded execution, indexed
and flat search, and A/B/A parameter changes. The original script
reproduced the CI String.positive() failure before fixing both JDBC URL
concatenations.
### Check List (For Author)
- Test
- [x] Regression test
- [x] Unit Test
- Behavior changed:
- [x] Yes. Supported `vector_search` property values can be prepared
parameters.
- Does this need documentation?
- [x] Yes. Usage and the supported parameter scope are documented above.
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label
…meter types (#70150) ### What problem does this PR solve? Issue Number: None Related PR: #68162, #69314 Problem Summary: **In short.** This ports `MysqlPreparedStatementForwardingTest` from 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 that `FEOpExecutor` builds 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.handleExecute` writes 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 in `MysqlConnectProcessorLongDataTest` cover 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.buildStmtForwardParams` builds. **What this PR adds.** The test runs a ten-parameter statement five times on a follower connection: - The parameter types are sent with the first and the fourth execution; the other executions reuse the cached ones. - One execution has a NULL parameter in each byte of the NULL bitmap. - An unsigned INT parameter becomes a BIGINT in the fourth execution. 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's `PrepareCommand`, query id and audit setup, as in `MysqlConnectProcessorLongDataTest`, 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.** | `handleExecute` on master | `MysqlConnectProcessorLongDataTest` (14) | `MysqlPreparedStatementForwardingTest` (1) | |---|---|---| | as is | pass | pass | | no rewrite of the forwarded packet for cached types (the bug #68162 fixed) | 3 fail | fails: the master binds untyped placeholders (`No value present`) | | the rewrite keeps only the first byte of the NULL bitmap | pass | fails: the master reads the packet one byte off | Co-authored-by: Gabriel <[email protected]>
What problem does this PR solve?
vector_search("query_vector"=?, "top_k"=?)currently converts the placeholders into the literal string?, so server-side prepared Lance vector queries fail during analysis.Preserve placeholders for
query_vector,top_k,offset, andfilter, and bind fresh property values on every EXECUTE. PREPARE resolves the output schema without requiring a query vector. Table, column, and search configuration remain constant. Each execution performs normal analysis and planning and resolves Lance metadata again; the retained statement never caches a bound vector or snapshot.This supports both indexed and flat searches and reuses the parsed statement. Forwarded executions preserve cached parameter types when the client omits the type table on repeated executions; the forwarding request includes the types needed by a fresh master-side PREPARE. It does not introduce an execution-plan cache or bypass the optimizer.
Example with MySQL Connector/J and
useServerPrepStmts=true:Release note
Support server-side prepared parameters for Lance
vector_searchquery vectors, top-k, offsets, and filters.Validation
ServerPreparedStatementwith A/B/A parameter changes and compares results with literal SQL for both indexed and flat searches. On live non-master FEs, it also prepares locally and forces repeated executions to the master.run-fe-ut.shacrossMysqlPreparedStatementForwardingTest,MysqlConnectProcessorCursorFetchTest,FEOpExecutorMysqlProtocolTest,LancePreparedSearchTest,LancePreparedStatementTest,LanceVectorQueryTest,VectorSearchTableValuedFunctionTest,FullTextSearchTableValuedFunctionTest,LanceScanNodeTest,PrepareTest, andNereidsParserTest. FE Checkstyle passed. The full external JDBC regression was also executed locally using the current PR FE build, a master FE, an observer FE, one BE, and the MinIO/Lance fixtures: 1 suite passed, 0 failures, 0 skips. It executed 12 real server-prepared queries covering direct and forwarded execution, indexed and flat search, and A/B/A parameter changes. The original script reproduced the CI String.positive() failure before fixing both JDBC URL concatenations.Check List (For Author)
vector_searchproperty values can be prepared parameters.Check List (For Reviewer who merge this PR)