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

Throw exception on type error in JsonDeserializer #8291

Merged
merged 5 commits into from
Aug 15, 2023

Conversation

Maxxen
Copy link
Member

@Maxxen Maxxen commented Jul 18, 2023

Found by @Wang-Ji20 in #6919

@carlopi
Copy link
Contributor

carlopi commented Jul 18, 2023

As mentioned live, there is an attribute [[noreturn]] that could be used to decorate the function, basically to add a consistency check / clarify expectations to whoever passes by.

Looking around the codebase it's already in use in a couple of places, so I guess it has to be supported.

@Mytherin
Copy link
Collaborator

Thanks for the PR! LGTM - but it seems this breaks one of the existing tests.

@Wang-Ji20
Copy link
Contributor

That's caused by the type check during json deserialization.
seems when the number is positive, 3 in this case, the number is recognized as unsigned number, rather than signed integer. so the test case failed.

@Maxxen
Copy link
Member Author

Maxxen commented Jul 20, 2023

Ill continue with this once I pick up the serialization work again next week.

@github-actions github-actions bot marked this pull request as draft August 15, 2023 10:11
@Maxxen Maxxen marked this pull request as ready for review August 15, 2023 10:25
@Maxxen
Copy link
Member Author

Maxxen commented Aug 15, 2023

@taniabogatsch This also now fixes the ugly (void)field_id in the JsonDeserializer

@taniabogatsch
Copy link
Contributor

Ah, that's a neat solution!

@hannes hannes merged commit e212894 into duckdb:master Aug 15, 2023
53 checks passed
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.

None yet

6 participants