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

Refactor GetConverter and handle LowCardinality() type #814

Merged
merged 1 commit into from
May 10, 2024

Conversation

adamyeats
Copy link
Contributor

@adamyeats adamyeats commented Apr 29, 2024

Fixes #805. This PR does a few things:

  • Handles LowCardinality() data type correctly and adds tests
  • Adds a converter for String to handle the recursion in GetConverter correctly (is there a better way to do this? i guess there was a reason why there wasn't a converter there for String already, but adding one does allow us to do some recursiveness here)
  • Modifies defaultConvert to handle strings and add better handling for pointers and dereferencing them
  • Refactors GetConverter to make it a bit nicer (?)
  • Uses reflect.PointerTo instead of reflect.PtrTo, which is now deprecated

@adamyeats adamyeats added type/bug Something isn't working datasource/ClickHouse go Pull requests that update Go code effort/small labels Apr 29, 2024
@adamyeats adamyeats self-assigned this Apr 29, 2024
@adamyeats adamyeats requested a review from a team as a code owner April 29, 2024 17:31
Copy link

github-actions bot commented Apr 29, 2024

Use the following command to run this PR with Docker at http://localhost:3000:

docker run --rm -p 3000:3000 grafana/plugin-builds:0d457e36bd31428776d1f2716df7a73f6aaff5a7pre

@adamyeats adamyeats requested a review from SpencerTorres April 29, 2024 17:44
Copy link
Contributor

@aangelisc aangelisc left a comment

Choose a reason for hiding this comment

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

LGTM! Great stuff @adamyeats 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datasource/ClickHouse effort/small go Pull requests that update Go code type/bug Something isn't working
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Regression in LowCardinality(Nullable(String)) type processing in v4.0.5
2 participants