Skip to content

fix: avoid placeholder detection in SQL comments #1573

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tosolveit
Copy link
Contributor

Summary

This pull request fixes issue #1537, where parameter placeholders such as ? were incorrectly detected inside -- single-line SQL comments. This led to false-positive errors like:

clickhouse [bind]: mixed named, numeric or positional parameters

when using $1 placeholders together with comments.

What this patch does

  • Adds a stripLineComments helper in bind.go that removes -- comments before applying placeholder-detection regexes.
  • Ensures that ? or $1 inside comments are not interpreted as actual parameter placeholders.
  • Adds a regression test under tests/issues/1537_test.go to verify the fix and prevent future regressions.

Limitations

This patch only addresses -- style comments. It does not currently handle:

  • /* ... */ block comments
  • 'string literals' containing ? or $1

These could be addressed in future improvements if needed.


Fixes #1537

Checklist

  • Unit and integration tests covering the common scenarios were added

@SpencerTorres
Copy link
Member

Hello! Thanks for the PR.

Instead of spending CPU removing comments it would be nice if we could recommend users simply don't include comments on queries with bound parameters.

This is a good fix but perhaps in a future version we can be more strict about this, maybe even show warnings in a debug mode.

At a certain point we would need a full tokenizer to truly parse out the parameters while handling edge cases, much easier to just enforce a style for these programmatic queries.

@SpencerTorres
Copy link
Member

I wonder if any queries would be broken by merging this. It might be safer to tell users to remove comments like this one.

I need to follow up in the original issue, but is there any valid case to have comments like this in the query?

@tosolveit
Copy link
Contributor Author

tosolveit commented Jun 7, 2025

Hi,
I see your point, there are many cases that needs to be handled and yes I agree an elegant solution might better, I approached pragramatically.

I also understand that you don't want to touch to the query not to introduce some risks, you also mentioned about adding comments, one possibility is that the user might have many SQL files and maybe comments are already added, It would be weird to say delete your comments when submitting queries in my opinion.

I am very new to the project and I'm totally fine if we skip to merge this and get back to this later.

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.

Question mark in comment section recognized as query param
2 participants