Improve UnionArray logical_nulls tests #6781
Conversation
| assert_eq!(expected, array.logical_nulls().unwrap().into_inner()); | ||
| assert_eq!( | ||
| expected, | ||
| array.mask_sparse_all_with_nulls_skip_one(array.fields_logical_nulls()) |
There was a problem hiding this comment.
What is this mask_sparse_all_with_nulls_skip_one function? This formulation is somewhat confusing to me (without delving deeply into the code). Is there any way to make this more "obvious" 🤔
There was a problem hiding this comment.
Each strategy for computing the logical_nulls are outlined here. The code determines a cost estimate to decide which strategy to use. Then it calls a helper function per each strategy => and that did the computation.
With the way the tests were written before, it always got the same cost estimate on certain architecture. For example, tests running on ARM would always get the same cost estimate and therefore always pick the same strategy (UnionArray::gather_nulls helper function). So even though this test is named test_sparse_union_logical_nulls_mask_all_nulls_skip_one, it was not actually testing that strategy and helper function.
With this change, the tests are directly calling each helper function.
In the example above, the test named test_sparse_union_logical_nulls_mask_all_nulls_skip_one has been changed to directly calling the helper method UnionArray::mask_sparse_all_with_nulls_skip_one.
|
Just came back from FTO. I'll pick up this review tmrw. Thanks @gstvg ! |
There was a problem hiding this comment.
Very nice. ❤️
The possible strategies are all tested:
- dense => gather nulls
- sparse => all options of the SparseStrategy are tested
I didn't bother doing a branch coverage analysis, since this is already a big gain. Thank you!
Which issue does this PR close?
Follow-up on #6303
Rationale for this change
As noted by @wiedld on #6303 (comment), the existing tests coverage is quite fragile relative to the target architecture and enabled CPU features, leaving most logic untested on ARM, for example.
What changes are included in this PR?
Instead of calling only the public
Array::logical_nulls, tests of specific strategies also calls the correspondent private method directly. To do so, part oflogical_nullsmethod, which builds the arguments to be passed to specific implementations, is made into a new private methodUnionArray::fields_logical_nulls, and then used by bothlogical_nullsand the tests.Are there any user-facing changes?
No, just tests