Bugfix for reduce-by-segment with differing input and output types #1987
Conversation
For differing key types, we cannot store an input key into a variable that is the output key type. This is important for discard iterators in the output where we are met with a compile-time error if we attempt to do so. Signed-off-by: Matthew Michel <[email protected]>
mmichel11
changed the title
Signed-off-by: Matthew Michel <[email protected]>
| auto end_keys_in = oneapi::dpl::make_zip_iterator(d_keys1 + n, d_keys2 + n); | ||
| auto begin_vals_in = oneapi::dpl::make_zip_iterator(d_values1, d_values2); | ||
| auto begin_keys_out = oneapi::dpl::make_zip_iterator(d_output_keys, oneapi::dpl::discard_iterator()); | ||
| auto begin_vals_out = oneapi::dpl::make_zip_iterator(d_output_values1, d_output_values2); |
There was a problem hiding this comment.
Does it make sense to include a discard iterator in the values as well? I think right now we are just hitting a compilation issue just with the key side of the issue, not the value side, right?
There was a problem hiding this comment.
Yes, it makes sense to add this for completeness as the same issue would occur with a discard iterator in the output values. I have added it.
Signed-off-by: Matthew Michel <[email protected]>
| const auto& __next_key = get<2>(__tup); | ||
| const auto& __current_key = get<3>(__tup); | ||
| const auto& __current_value = get<1>(get<0>(__tup)); |
There was a problem hiding this comment.
I'm not sure if there is any advantage, but we could get the type with std::tuple_element_t<I, _Tup> rather than using auto.
Effectively I think we end up with the same code, without much more information about the types were using. Just mentioning it to see if you find reason to use it.
There was a problem hiding this comment.
I think auto is a bit cleaner here since it is clear which tuple element type the variable will be from the rhs of the equal sign. Also, __current_value will become
const std::tuple_element_t<1, std::tuple_element_t<0, _Tup>>& __current_value = get<1>(get<0>(__tup));
which is more difficult to follow in my opinion.
There was a problem hiding this comment.
I think I agree. The only thing I stopped to think about is if somehow with the lvalue ref auto, we are possibly being less specific than we intend to be, but I definitely agree that what you have now is much cleaner.
I'm good with how you have it, I think.
For differing input and output key / value types, we cannot store an input element into a variable that is the output type. This is especially important for discard iterators in the output where we are met with a compile-time error if we attempt to store an input into this type as it can hold no value.
A test case has been added which combines zip and discard iterators. This catches the compilation issue before the fix.