Skip to content

fix multiple use of name param in sqlite#267

Merged
doron050 merged 1 commit intomainfrom
fix-multiple-use-of-name-param
Aug 13, 2025
Merged

fix multiple use of name param in sqlite#267
doron050 merged 1 commit intomainfrom
fix-multiple-use-of-name-param

Conversation

@doron050
Copy link
Collaborator

@doron050 doron050 commented Aug 12, 2025

This pull request refactors the TransformQueryText method in SqliteDriver.cs. The method is responsible for converting sqlc's parameter syntax (? or ?1, ?2, etc.) to the named parameter format (@paramName) used by Microsoft.Data.Sqlite.

Key Changes:

The core of this change is an optimization of how both numbered and positional parameters are handled:

  1. Numbered Parameter Handling (?1, ?2):

    • Before: The method would loop through each parameter, creating a new Regex object on each iteration to replace all instances of a specific numbered placeholder (e.g., ?1). This was inefficient due to repeated regex compilation inside a loop.
    • After: The new implementation is much more performant. It now builds a dictionary mapping parameter numbers to their names and then uses a single Regex.Replace call with a MatchEvaluator. This allows for all numbered placeholders to be replaced in a single pass over the query string.
  2. Positional Parameter Handling (?):

    • Before: The logic for positional parameters also used Regex.Replace inside a loop, replacing one placeholder at a time.
    • After: This has been refactored to use a MatchEvaluator with a parameter enumerator, providing a cleaner and more efficient way to handle sequential replacements.

@doron050 doron050 changed the title ix multiple use of name param in sqlite Fix multiple use of name param in sqlite Aug 12, 2025
@doron050 doron050 changed the title Fix multiple use of name param in sqlite fix multiple use of name param in sqlite Aug 12, 2025
Copy link
Collaborator

@SockworkOrange SockworkOrange left a comment

Choose a reason for hiding this comment

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

Looks good:) Had one comment, but no critical. Can take in a differnt PR

{
var areArgumentsNumbered = new Regex($@"\?\d\b*").IsMatch(query.Text);
// Regex to detect numbered parameters like ?1, ?2
var areArgumentsNumbered = new Regex(@"\?\d+\b").IsMatch(query.Text);
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can extract the regex to a partial static class, like in one of the other drivers. See here for example. We can fix in another PR though

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cool, let's improve on the next PR

@doron050 doron050 merged commit a7ff473 into main Aug 13, 2025
6 checks passed
@github-actions github-actions bot deleted the fix-multiple-use-of-name-param branch October 1, 2025 01:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants