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 incorrect propagation of field's nullability into its inner list #3352

Merged
merged 1 commit into from Sep 18, 2023

Conversation

teh-cmc
Copy link
Member

@teh-cmc teh-cmc commented Sep 18, 2023

The arrow pass of the codegen wrongly attributes the nullability of a field to its inner entity (whether it's a list or fixed-size list), which leads to corrupt datatypes being generated.

This generally doesn't cause any issues in practice because one cannot trust what the datatype says anyway, and nullability is instead checked by directly looking at the bitmap itself at runtime.

But, it turns out that the C++ backend completely ignore this piece of metadata at the moment, and therefore ends up generating datatypes that don't match what the Rust & Python backends generate, leading to broken roundtrips.

Fix the semantic pass so everyone agrees once again.

What

Checklist

@teh-cmc teh-cmc added 🪳 bug Something isn't working 🔨 testing testing and benchmarks 🏹 arrow concerning arrow labels Sep 18, 2023
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great catch, makes sense! extra points for the nice comment

@teh-cmc teh-cmc merged commit 5a5d989 into main Sep 18, 2023
26 of 27 checks passed
@teh-cmc teh-cmc deleted the cmc/codegen_fix_nullability branch September 18, 2023 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏹 arrow concerning arrow 🪳 bug Something isn't working 🔨 testing testing and benchmarks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants