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 json with nil will get error #824

Merged
merged 2 commits into from
Nov 24, 2022
Merged

Fix json with nil will get error #824

merged 2 commits into from
Nov 24, 2022

Conversation

ACV-er
Copy link
Contributor

@ACV-er ACV-er commented Nov 21, 2022

resoved: #823

@CLAassistant
Copy link

CLAassistant commented Nov 21, 2022

CLA assistant check
All committers have signed the CLA.

@gingerwizard
Copy link
Collaborator

Rather than test on the function level - we don't have any other tests like this. Can you do a high level set of inserts/queries which reproduce the issue please? Add to https://github.com/ClickHouse/clickhouse-go/blob/main/tests/json_test.go

@gingerwizard gingerwizard self-requested a review November 21, 2022 09:28
@ACV-er
Copy link
Contributor Author

ACV-er commented Nov 21, 2022

Rather than test on the function level - we don't have any other tests like this. Can you do a high level set of inserts/queries which reproduce the issue please? Add to https://github.com/ClickHouse/clickhouse-go/blob/main/tests/json_test.go

I don't know all the implementations very well, it's a bit of a hassle, I'll give it a try

@ACV-er
Copy link
Contributor Author

ACV-er commented Nov 21, 2022

Rather than test on the function level - we don't have any other tests like this. Can you do a high level set of inserts/queries which reproduce the issue please? Add to https://github.com/ClickHouse/clickhouse-go/blob/main/tests/json_test.go

Is it okay to submit a commit like this?

@gingerwizard
Copy link
Collaborator

OK post some JSON which recreates your issue @ACV-er - I'll write a test.

@ACV-er ACV-er closed this Nov 21, 2022
@ACV-er ACV-er reopened this Nov 21, 2022
@ACV-er
Copy link
Contributor Author

ACV-er commented Nov 22, 2022

What are these errors? It doesn't look like a problem with the code。 @gingerwizard

@gingerwizard
Copy link
Collaborator

@ACV-er our cloud CI tests wont pass for external contributors as you don't have access to cluster secrets. Needs to be fixed. I'll test locally today and merge. thanks again. Will make 2.4.2 - likely end of week.

@gingerwizard gingerwizard merged commit 7bc9573 into ClickHouse:main Nov 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants