Skip to content
New issue

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

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

Already on GitHub? # to your account

fix(json): proper encoding for json array #1043

Merged
merged 2 commits into from
Feb 16, 2025
Merged

Conversation

SpencerTorres
Copy link
Member

Summary

JSON is one of the few column types that implement a prefix. This change implements the StateEncoder and StateDecoder interfaces for ColJSONStr, enabling proper encoding of the column prefix.

Checklist

  • Unit and integration tests covering the common scenarios were added

@SpencerTorres SpencerTorres merged commit d7f188d into main Feb 16, 2025
19 checks passed
@SpencerTorres SpencerTorres deleted the fix_json_array branch February 16, 2025 18:07
@ernado
Copy link
Collaborator

ernado commented Feb 16, 2025

Btw when we can add a e2e test for this?

@SpencerTorres
Copy link
Member Author

@ernado Do you have an example of an e2e test in this repo? I see some files referencing it, but nothing similar to what we have in clickhouse-go. If there's another column I can copy then I'll add one asap

@ernado
Copy link
Collaborator

ernado commented Feb 17, 2025

Of cousre, something like

ch-go/query_test.go

Lines 420 to 435 in d7f188d

t.Run("SelectStr", func(t *testing.T) {
t.Parallel()
// Select single string row.
var data proto.ColStr
require.NoError(t, Conn(t).Do(ctx, Query{
Body: "SELECT 'foo' AS s",
Result: proto.Results{
{
Name: "s",
Data: &data,
},
},
}))
require.Equal(t, 1, data.Rows())
require.Equal(t, "foo", data.First())
})

Basically, everything that calls github.com/ClickHouse/ch-go/cht.

Also,

ch-go/client_test.go

Lines 50 to 54 in d7f188d

func SkipNoFeature(t *testing.T, client *Client, feature proto.Feature) {
if !client.ServerInfo().Has(feature) {
t.Skipf("Skipping (feature %q not supported)", feature)
}
}

May be helpfull. If we know the protocol version starting from which we can test this JSON field.

@SpencerTorres
Copy link
Member Author

@ernado thanks for the examples! I'll be sure to update these tests in the future as needed. Here's the PR for the e2e tests: #1044

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants