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

Fix: remove ad-hoc filter parsing for numeric fields #629

Merged
merged 3 commits into from
Jan 26, 2024

Conversation

afagundes
Copy link
Contributor

@afagundes afagundes commented Dec 11, 2023

This PR fixes issue #655

@afagundes afagundes requested a review from a team as a code owner December 11, 2023 15:39
@asimpson
Copy link
Contributor

I'm going to defer to @SpencerTorres on this one since he probably knows the implications on Clickhouse for this change.

@bossinc
Copy link
Collaborator

bossinc commented Jan 17, 2024

This is tricky. It would be best to pass the correct type to CH so it doesn't have to parse strings. (if @SpencerTorres says it's not a big deal, then let's go with the current approach)
As an alternative approach, we could allow filter values not to add quotes if quotes are already in the value. So '123' wouldn't become ''123''. What are your thoughts @afagundes

Copy link
Contributor

@asimpson asimpson left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! 🚀

@asimpson asimpson merged commit abfb99c into grafana:main Jan 26, 2024
14 of 15 checks passed
@afagundes afagundes deleted the adhoc-numeric-varchar-field branch January 26, 2024 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants