Skip to content
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

[Feature] Remove log_comment from the cache key #314

Open
yohannjCS opened this issue Mar 3, 2023 · 2 comments
Open

[Feature] Remove log_comment from the cache key #314

yohannjCS opened this issue Mar 3, 2023 · 2 comments

Comments

@yohannjCS
Copy link

Is your feature request related to a problem? Please describe.
I want to have metadata in system.query_log to know more about the queries being executed. For that I'm using the log_comment setting in my queries.
As it is a comment, it does not affect my query computation nor its result.

Describe the solution you'd like
In the cache key (the query), remove the log_comment setting if it exists.

Describe alternatives you've considered
None.

Additional context

@Blokje5
Copy link
Collaborator

Blokje5 commented Mar 3, 2023

I have a draft PR ready to fix this issue with a regex.

As discussed offline, there is also the possibility to allow for setting a custom cache key on the client side. This does introduce a lot of complexity on the client side (the client needs to be aware of the query structure to make sure queries that are the same besides the log_comment are cached properly).

But using a regex might introduce subtle bugs. It only affects the cache key, and I have some test cases to cover it, but it is hard to be exhaustive here. We can make this an optional step in the cache key query parser, and introduce a setting so that it isn't enabled by default.

@mga-chka @sigua-cs @gontarzpawel if you can also take a look. If you have a strong preference for one or the other solution...

@mga-chka
Copy link
Collaborator

mga-chka commented Mar 3, 2023

I didn't looked at the internal discussion but IMHO using a regexp is a bad idea since it will slow down the processing for an edge case. If you look at the code, chproxy already remove the "normal" comments from the hash avoiding regexps so we should at least have a similar mechanism

func skipLeadingComments(q []byte) []byte {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants