Temporaily patch converters to fix support for LowCardinality strings

[adamyeats]

This is a temporary fix for #833. At the moment, the sqlutil package in the Plugin SDK does not correctly handle the LowCardinality data types. I thought we'd fixed this in #814, but it turns out that the GetConverter() function is not actually called by anything within the SDK or our code as expected.

Until we can patch this upstream, I've added two extra converters for now, that can be removed when we can introduce more generic handling of these types.

[github-actions]

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

docker run --rm -p 3000:3000 grafana/plugin-builds:5d28c1db231ecc3019eacae2cb7055dc6f5010d3pre

[SpencerTorres]

I think LowCardinality is mainly used with strings so this should be fine for now. Maybe we can just strip this modifier before calling GetConverter? I don't think it affects the type scanning/reflection once it gets to the client, so if we strip it, it should be effectively the same as String.

[adamyeats]

@SpencerTorres

Maybe we can just strip this modifier before calling GetConverter? I don't think it affects the type scanning/reflection once it gets to the client, so if we strip it, it should be effectively the same as String.

That is more or less how the original fix worked, however the GetConverter() function is not actually called by anything (maybe it's just a test helper function? not sure), so we needed to just add a new converter for the time being. As you mention above, we figured that as only strings are supported by LowCardinality out-of-the-box, that we'd be fine with just covering these cases for now.

Unfortunately, this is blocked on some failing tests in CI at the moment, but the test failures are intermittent and do not seem to occur locally, making this quite a tricky one to debug. We'll have more info ASAP, but for now I'm going to move this back to draft.

[SpencerTorres]

I have thoroughly poked through the plugin and sqlds code, and this is clearly the easiest fix for now. We have users who need LowCardinality(Nullable(String)) so we need to get this merged ASAP.

In the future, I would like to see more flexibility with Converter type in sqlds. Ultimately what we need is a way to strip LowCardinality( + ) from the type string BEFORE it goes to sqlds. This could possibly be done in clickhouse-go, or maybe by making a porxy interface for clickhouse-go's SQL driver, but this is all so complicated. The easiest solution is to add these converters. Perhaps automatically.

It wouldn't hurt to dynamically build the array of converters and add a copy of each converter that is wrapped in LowCardinality. Low Cardinality does not affect clients, only how the data is stored on the server.

A temporary workaround is remove LowCardinality via CAST() in the user's SQL.

TL;DR: This is good, we need this fix ASAP. Not sure what CI issues are failing but let me know if I can help.

[SpencerTorres]

Looks good. Hopefully we can get a better sqlds API for fixing this, but this works for now. Most LowCardinality types are String.

[aangelisc]

LGTM - just one nit in the comments!