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

Add test_validate feature flag + CI check #1239

Closed
tustvold opened this issue Jan 25, 2022 · 5 comments · Fixed by #1546
Closed

Add test_validate feature flag + CI check #1239

tustvold opened this issue Jan 25, 2022 · 5 comments · Fixed by #1546
Labels
development-process Related to development process of arrow-rs enhancement Any new improvement worthy of a entry in the changelog

Comments

@tustvold
Copy link
Contributor

tustvold commented Jan 25, 2022

Is your feature request related to a problem or challenge? Please describe what you are trying to do.

For better or worse arrow-rs makes use of unsafe in various places. Whilst there are ongoing efforts to reduce the use of unsafe, occasionally bugs will slip through. Both arrow-rs and arrow-datafusion run with MIRI in order to catch many of these, however, MIRI is extremely slow and fiddly to setup which acts as a limiting factor on crowd-sourced test coverage. Additionally it will not catch the arguably more common types of logic bug, e.g. inconsistent null counts, non-consecutive array offsets, etc...

Describe the solution you'd like

Add a test_validate feature flag that when enabled will cause various "unchecked" methods to actually perform validation. The most obvious being ArrayDataBuilder::build_unchecked. This will allow projects to contribute to crowd-sourced nasal demon hunting, without imposing on them all the costs of setting up and running MIRI.

Describe alternatives you've considered

This could also be enabled based on debug_assertions being enabled, but this is a global setting that is on by default in debug builds. A feature flag gives people the ability to opt-out of the validation. FWIW this is what the parquet crate currently does.

@tustvold tustvold added the enhancement Any new improvement worthy of a entry in the changelog label Jan 25, 2022
@alamb alamb changed the title Add test_validate feature flag Add test_validate feature flag + CI check Jan 25, 2022
@alamb
Copy link
Contributor

alamb commented Jan 25, 2022

"nasal demons" for anyone else who is curious http://www.catb.org/jargon/html/N/nasal-demons.html

@alamb
Copy link
Contributor

alamb commented Jan 25, 2022

Maybe force_validate might be a more descriptive name for this flag.

I actually did this (forced ArrayData::new_unchecked to actually check) to manually test the original validation logic

@jhorstmann
Copy link
Contributor

Agree, this sounds useful. Making it independent of debug_assertions is also a good idea and would allow enabling it also in some staging/pre-production environments.

@tustvold
Copy link
Contributor Author

Had a brief stab at adding this, the good news is it doesn't show any new problems, the bad news is it causes a lot of the tests of the validation logic to fail 😆 I need to have a think about how best to handle this...

@alamb
Copy link
Contributor

alamb commented Jan 31, 2022

Had a brief stab at adding this, the good news is it doesn't show any new problems, the bad news is it causes a lot of the tests of the validation logic to fail 😆 I need to have a think about how best to handle this...

Maybe we could just #cfg out the test suite for ArrayData 🤔

@alamb alamb added the development-process Related to development process of arrow-rs label Apr 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development-process Related to development process of arrow-rs enhancement Any new improvement worthy of a entry in the changelog
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants