Move intersect_row_selections from datafusion to arrow-rs.#3047
Move intersect_row_selections from datafusion to arrow-rs.#3047tustvold merged 2 commits intoapache:masterfrom
intersect_row_selections from datafusion to arrow-rs.#3047Conversation
There was a problem hiding this comment.
I recommend we put this function in parquet/src/arrow/arrow_reader/selection.rs
that way it is logically grouped with similar functionality as well as is in the same module as the test
There was a problem hiding this comment.
@alamb I used to put it in selection.rs refactor avoid pub mod,
but it need pub the selection mod avoid method not used, it this ok?🤔 or any method
There was a problem hiding this comment.
Perhaps you could extend
pub use selection::{RowSelection, RowSelector};to
pub use selection::{intersect_row_selections, RowSelection, RowSelector};So it is exposed?
There was a problem hiding this comment.
Or perhaps we could rework this method as a member function on RowSelection? i.e. fn intersection(&self, other: &RowSelection) -> RowSelection ?
There was a problem hiding this comment.
Or perhaps we could rework this method as a member function on RowSelection? i.e.
fn intersection(&self, other: &RowSelection) -> RowSelection?
This is a good idea , but it may call from(selectors: Vec<RowSelector>) more times🤔
There was a problem hiding this comment.
Perhaps you could extend
pub use selection::{RowSelection, RowSelector};to
pub use selection::{intersect_row_selections, RowSelection, RowSelector};So it is exposed?
@alamb 😂 yes, thanks notice its ok only one method in one private mod
There was a problem hiding this comment.
I don't have a strong opinion
Ted-Jiang
force-pushed
the
ISSUE_3003
branch
from
7627627 to
8a2b540
Compare
There was a problem hiding this comment.
Looks good to me -- any concerns @tustvold ?
Maybe as a follow on we could make a wrapper around Vec to add such functions
/// a list of RowSelector
pub struct RowSelections {
selections: Vec<RowSelector>
}
impl RowSelections {
fn intersect(other: &Self) -> Self {
...
}
}🤔
|
Benchmark runs are scheduled for baseline = 4dd7fea and contender = 9f14683. 9f14683 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
4 participants
Which issue does this PR close?
Closes #3003.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?