Closed JustusAdam closed 5 years ago
This looks like a good change! I think there is a difference here in how the two code segments would work if group_by
is not in sorted order, but I think the old code was also just broken for that use-case, so this is probably fine :) Some minor nits above, but apart from that :+1:
Yeah, it seems the old code would omit out-of-order columns, while this one should always emit a filtered row in the same order as the group_by
.
Not really sure if either is better in that case. The old one would most likely fail faster (which might be a desirable property).
Perhaps I could add a debug_assert!
checking that the columns are sorted? That might make it more reliable.
I just find this version to me much easier to read.
I agree! Yeah, that seems like a reasonable assertion to add. Happy to merge after that.
We should also probably fix Travis again :sweat_smile:
Apparently travis actually worked there for once.... Maybe don't make the compilation process silent (should that be the case)? Travis likes you to do lots of output while building 😄
I think I'd prefer if you just asserted that each column is numerically greater than the previous one. Just keep last_col: usize
and check + re-assign on each iteration. You shouldn't need is_sorted
.
Oh, @ms705 already merged it.. :sweat_smile: I'd prefer not to have conditional features, as it makes it even harder to get off of nightly.
... Actually no 😄 , but I stumbled upon this section when trying to understand the code and I think this version is more readable. It doesn't break any tests and, according to taster doesn't appear to cause slowdown.