Fix RowSelection::intersection (#5036) #5041
Conversation
There was a problem hiding this comment.
Thank you @tustvold -- this is very nice.
An invariant of RowSelection is that it alternates select and skip, and does not contain empty RowSelector.
I like how you have added documenting this assumption now. Can you explain where this assumption is used in the code?
cc @Ted-Jiang
| /// A [`RowSelection`] maintains the following invariants: | ||
| /// | ||
| /// * It contains no [`RowSelector`] of 0 rows | ||
| /// * Consecutive [`RowSelector`] alternate whether they skip or select rows |
There was a problem hiding this comment.
| /// * Consecutive [`RowSelector`] alternate whether they skip or select rows | |
| /// * Consecutive [`RowSelector`]s alternate skipping or selecting rows. |
| continue; | ||
| } | ||
|
|
||
| let last = selectors.last_mut().unwrap(); |
There was a problem hiding this comment.
| let last = selectors.last_mut().unwrap(); | |
| // Combine adjacent skip/non-skip | |
| let last = selectors.last_mut().unwrap(); |
| @@ -118,10 +123,13 @@ impl RowSelection { | |||
| let mut last_end = 0; | |||
| for range in ranges { | |||
| let len = range.end - range.start; | |||
| if len == 0 { | |||
There was a problem hiding this comment.
Is this the actual bug fix? It also looks like from_selectors_and_combine didn't remove zero length RowSelectors
There was a problem hiding this comment.
It is part of it, the other part is actually invoking the from_selectors_and_combine logic whilst performing an intersection
| r_iter.next().unwrap(); | ||
| } | ||
| (Some(a), Some(b)) => { |
There was a problem hiding this comment.
I found the switch from l,r to a,b confusing -- can we use one consistently throughout this function? Either would be better than a mix
| } | ||
| res | ||
| } | ||
| }); |
There was a problem hiding this comment.
I reviewed this logic carefully -- it is very cool
From the linked ticket #5036
|
I see -- so another potential fix would be to ignore zero length selections in the reader. I am not saying we should do this, only observing it might be a possiblility |
That would only half fix the problem, as if you have two consecutive skip, you will have the same problem |
* Fix RowSelection::intersection (apache#5036) * Review feedback
* Fix RowSelection::intersection (apache#5036) * Review feedback
* Fix RowSelection::intersection (#5036) * Review feedback Co-authored-by: Raphael Taylor-Davies <[email protected]>
Which issue does this PR close?
Closes #5036
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?