Make RowSelection's from_consecutive_ranges public #5848
Conversation
| ranges: I, | ||
| total_rows: usize, | ||
| total_rows_opt: Option<usize>, |
There was a problem hiding this comment.
What is the use-case for not knowing the total number of rows?
There was a problem hiding this comment.
I personally think that making total_rows optional will make it very hard to use this API correctly. Specifically, the `total_rows1 almost always needs to be necessary, otherwise it will not be possible to represent skipping the last rows in the selection
For example, a range of 100..200 for 400 total rows needs to look ike
Skip(100)
Scan(100)
Skip(100). <--- you can't figure this out from the ranges, you need the total row count
There was a problem hiding this comment.
I see, I thought the final skip is optional based on the trim function. I will revert the API change later. I am traveling now, so it might take some days for me to get back with access to a computer.
There was a problem hiding this comment.
I see, I thought the final skip is optional based on the trim function.
It probably didn't help that several test cases in apache/datafusion#10738 didn't do this correctly either. I have fixed them in apache/datafusion#10813
There was a problem hiding this comment.
Thank all for your suggestions. Addressed in the latest commit. Please let me know what you think.
| ranges: I, | ||
| total_rows: usize, | ||
| total_rows_opt: Option<usize>, |
There was a problem hiding this comment.
I personally think that making total_rows optional will make it very hard to use this API correctly. Specifically, the `total_rows1 almost always needs to be necessary, otherwise it will not be possible to represent skipping the last rows in the selection
For example, a range of 100..200 for 400 total rows needs to look ike
Skip(100)
Scan(100)
Skip(100). <--- you can't figure this out from the ranges, you need the total row count
This constructor method should be easier to use.
|
The CI failure seems unrelated to this PR? |
| @@ -81,6 +81,13 @@ impl RowSelector { | |||
| /// | |||
| /// let actual: Vec<RowSelector> = selection.into(); | |||
| /// assert_eq!(actual, expected); | |||
| /// | |||
| /// // you can also create a selection from consecutive ranges | |||
|
🚀 |
Which issue does this PR close?
Closes #5846
Rationale for this change
This constructor method should be easier to use.
What changes are included in this PR?
from_consecutive_rangesfunction publicAre there any user-facing changes?
NO.