Skip to content

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

Merged
SpencerTorres merged 2 commits intomainfrom
fix_json_array
Feb 16, 2025
Merged

fix(json): proper encoding for json array#1043
SpencerTorres merged 2 commits intomainfrom
fix_json_array

Conversation

@SpencerTorres
Copy link
Copy Markdown
Member

@SpencerTorres SpencerTorres commented Feb 16, 2025

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
Copy Markdown
Collaborator

ernado commented Feb 16, 2025

Btw when we can add a e2e test for this?

@SpencerTorres
Copy link
Copy Markdown
Member Author

SpencerTorres commented Feb 16, 2025

@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

@SpencerTorres SpencerTorres mentioned this pull request Feb 17, 2025
Closed
@ernado
Copy link
Copy Markdown
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 SpencerTorres mentioned this pull request Feb 17, 2025
Merged
1 task
@SpencerTorres
Copy link
Copy Markdown
Member Author

SpencerTorres commented Feb 17, 2025

@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

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

Reviewers

@ernado ernado ernado approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@SpencerTorres @ernado