Skip to content

fix(mysql): keep unknown prepare placeholders#8150

Merged
killme2008 merged 4 commits into
GreptimeTeam:mainfrom
discord9:fix/mysql-unknown-placeholder-param-count
May 24, 2026
Merged

fix(mysql): keep unknown prepare placeholders#8150
killme2008 merged 4 commits into
GreptimeTeam:mainfrom
discord9:fix/mysql-unknown-placeholder-param-count

Conversation

@discord9
Copy link
Copy Markdown
Contributor

@discord9 discord9 commented May 21, 2026

I hereby agree to the terms of the GreptimeDB CLA.

Refer to a related PR or issue link (optional)

Fixes #8142.

Related history:

What's changed and what's your intention?

This PR fixes MySQL prepared statements whose placeholders are present in the SQL but do not have concrete types inferred by the planner.

Before this change, the MySQL prepare response only advertised placeholders with concrete inferred types. If DataFusion returned None for a placeholder type, prepared_params() skipped it, so the client could see fewer parameters than the SQL actually contains and fail before execution with an argument count mismatch.

The changes are:

  • Count real prepared-statement placeholders from the SQL AST and return one MySQL parameter column per counted placeholder.
  • Use inferred concrete types when available and NULL metadata for unknown placeholder types.
  • Cache SqlPlan::Plan only when all placeholders have concrete inferred types; otherwise keep the statement fallback path.
  • Preserve the original raw SQL in the fallback path instead of relying on AST to_string() round-trips.
  • Replace fallback parameters by collecting placeholder spans from the transformed AST and replacing those original-query ranges from right to left.
  • Guard span replacement by verifying each span maps back to a real ? in the original SQL.
  • Format string/binary parameters in the fallback path with mysql_common::Value::as_sql(false).
  • Add tests for unknown placeholders, mixed known/unknown placeholders, comments/literals, UTF-8 and CRLF span handling, replacement length shifts, NULL, binary bytes, and LIMIT ?.

This does not change public APIs, schemas, or persisted data.

PR Checklist

Please convert it to a draft if some of the following conditions are not met.

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.
  • This PR requires documentation updates.
  • API changes are backward compatible.
  • Schema or data changes are backward compatible.

Tests:

  • cargo fmt --check
  • cargo check -p servers
  • cargo test -p servers --lib mysql::handler::tests
  • cargo test -p servers --features testing --test mod mysql::mysql_server_test::test_query_prepared

@discord9 discord9 requested a review from a team as a code owner May 21, 2026 10:07
@github-actions github-actions Bot added size/S docs-not-required This change does not impact docs. labels May 21, 2026
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request enhances the MySQL handler to correctly support prepared statements with untyped placeholders, ensuring they are properly reported to clients by using a default null datatype when inference fails. It also introduces a SQL string escaping utility for the statement fallback path and includes comprehensive unit and integration tests. Review feedback focuses on optimizing the string escaping logic to reduce allocations and improving variable naming to avoid shadowing and enhance code clarity.

Comment thread src/servers/src/mysql/handler.rs Outdated
Comment thread src/servers/src/mysql/handler.rs
Comment thread src/servers/src/mysql/handler.rs
@discord9 discord9 force-pushed the fix/mysql-unknown-placeholder-param-count branch 3 times, most recently from ffcd3cc to e45b5c6 Compare May 21, 2026 12:21
@github-actions github-actions Bot added size/M and removed size/S labels May 21, 2026
@killme2008 killme2008 requested a review from Copilot May 21, 2026 23:43
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes MySQL prepared-statement behavior so that the server advertises all ? placeholders found in the SQL (even when DataFusion cannot infer a concrete parameter type), preventing client-side “expected N arguments, got M” errors. It also hardens the statement-fallback execution path by replacing placeholders using AST spans and emitting properly escaped SQL literals for bytes/strings.

Changes:

  • Count placeholders from the parsed SQL AST and always return one MySQL parameter column per placeholder, using NULL type metadata when the type is unknown.
  • Avoid caching SqlPlan::Plan unless all placeholders have inferred concrete types; otherwise keep the statement-fallback path and preserve raw SQL.
  • Improve fallback parameter substitution by replacing ? ranges from right-to-left using AST spans, and escape bytes/strings via mysql_common::Value::as_sql(false); add extensive tests.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/servers/tests/mysql/mysql_server_test.rs Adds integration coverage for unknown placeholders, mixed known/unknown placeholders, comments/literals, UTF-8/CRLF spans, binary bytes, NULL, and LIMIT placeholders.
src/servers/src/mysql/helper.rs Adds placeholder counting and placeholder span collection utilities used by prepare/execute.
src/servers/src/mysql/handler.rs Updates prepare/execute logic to advertise unknown placeholders, gate plan caching on full typing, and replace fallback params by span; uses mysql_common for SQL literal escaping.
src/servers/Cargo.toml Adds mysql_common dependency.
Cargo.lock Locks mysql_common version resolution.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/servers/src/mysql/helper.rs Outdated
Comment thread src/servers/src/mysql/handler.rs
Copy link
Copy Markdown
Member

@killme2008 killme2008 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to add some black-box tests to https://github.com/GreptimeTeam/greptimedb-tests

@discord9

Comment thread src/servers/src/mysql/handler.rs
Comment thread src/servers/src/mysql/handler.rs
discord9 added 3 commits May 22, 2026 16:08
Signed-off-by: discord9 <discord9@163.com>
Signed-off-by: discord9 <discord9@163.com>
Signed-off-by: discord9 <discord9@163.com>
@discord9 discord9 force-pushed the fix/mysql-unknown-placeholder-param-count branch from 5d82c3a to 495d914 Compare May 22, 2026 08:42
Signed-off-by: discord9 <discord9@163.com>
Copy link
Copy Markdown
Member

@killme2008 killme2008 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@killme2008 killme2008 added this pull request to the merge queue May 24, 2026
Merged via the queue into GreptimeTeam:main with commit 8c267f3 May 24, 2026
88 of 89 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs-not-required This change does not impact docs. size/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

mysql interface does not expect arguments for LIMIT

3 participants