Skip to content

Commit

Permalink
refactor: remove too_many_arguments
Browse files Browse the repository at this point in the history
  • Loading branch information
@WenyXu
WenyXu committed Dec 11, 2024
1 parent 0f3d764 commit a13a417
Show file tree
Hide file tree
Showing 9 changed files with 65 additions and 116 deletions.
6 changes: 3 additions & 3 deletions src/mito2/src/read/scan_region.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -422,8 +422,6 @@ impl ScanRegion {
InvertedIndexApplierBuilder::new(
self.access_layer.region_dir().to_string(),
self.access_layer.object_store().clone(),
file_cache,
index_cache,
self.version.metadata.as_ref(),
self.version.metadata.inverted_indexed_column_ids(
self.version
Expand All @@ -434,8 +432,10 @@ impl ScanRegion {
.iter(),
),
self.access_layer.puffin_manager_factory().clone(),
puffin_metadata_cache,
)
.with_file_cache(file_cache)
.with_index_cache(index_cache)
.with_puffin_metadata_cache(puffin_metadata_cache)
.build(&self.request.filters)
.inspect_err(|err| warn!(err; "Failed to build invereted index applier"))
.ok()
Expand Down
38 changes: 24 additions & 14 deletions src/mito2/src/sst/index/inverted_index/applier.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,16 +69,11 @@ pub(crate) struct InvertedIndexApplier {
pub(crate) type InvertedIndexApplierRef = Arc<InvertedIndexApplier>;

impl InvertedIndexApplier {
// TODO(weny): remove this after refactoring.
#[allow(clippy::too_many_arguments)]
/// Creates a new `InvertedIndexApplier`.
pub fn new(
region_dir: String,
region_id: RegionId,
store: ObjectStore,
file_cache: Option<FileCacheRef>,
index_cache: Option<InvertedIndexCacheRef>,
puffin_metadata_cache: Option<PuffinMetadataCacheRef>,
index_applier: Box<dyn IndexApplier>,
puffin_manager_factory: PuffinManagerFactory,
) -> Self {
Expand All @@ -88,14 +83,35 @@ impl InvertedIndexApplier {
region_dir,
region_id,
store,
file_cache,
file_cache: None,
index_applier,
puffin_manager_factory,
inverted_index_cache: index_cache,
puffin_metadata_cache,
inverted_index_cache: None,
puffin_metadata_cache: None,
}
}

/// Sets the file cache.
pub fn with_file_cache(mut self, file_cache: Option<FileCacheRef>) -> Self {
self.file_cache = file_cache;
self
}

/// Sets the index cache.
pub fn with_index_cache(mut self, index_cache: Option<InvertedIndexCacheRef>) -> Self {
self.inverted_index_cache = index_cache;
self
}

/// Sets the puffin metadata cache.
pub fn with_puffin_metadata_cache(
mut self,
puffin_metadata_cache: Option<PuffinMetadataCacheRef>,
) -> Self {
self.puffin_metadata_cache = puffin_metadata_cache;
self
}

/// Applies predicates to the provided SST file id and returns the relevant row group ids
pub async fn apply(&self, file_id: FileId) -> Result<ApplyOutput> {
let _timer = INDEX_APPLY_ELAPSED
Expand Down Expand Up @@ -231,9 +247,6 @@ mod tests {
region_dir.clone(),
RegionId::new(0, 0),
object_store,
None,
None,
None,
Box::new(mock_index_applier),
puffin_manager_factory,
);
Expand Down Expand Up @@ -274,9 +287,6 @@ mod tests {
region_dir.clone(),
RegionId::new(0, 0),
object_store,
None,
None,
None,
Box::new(mock_index_applier),
puffin_manager_factory,
);
Expand Down
57 changes: 36 additions & 21 deletions src/mito2/src/sst/index/inverted_index/applier/builder.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,32 +72,48 @@ pub(crate) struct InvertedIndexApplierBuilder<'a> {
}

impl<'a> InvertedIndexApplierBuilder<'a> {
// TODO(weny): remove this after refactoring.
#[allow(clippy::too_many_arguments)]
/// Creates a new [`InvertedIndexApplierBuilder`].
pub fn new(
region_dir: String,
object_store: ObjectStore,
file_cache: Option<FileCacheRef>,
index_cache: Option<InvertedIndexCacheRef>,
metadata: &'a RegionMetadata,
indexed_column_ids: HashSet<ColumnId>,
puffin_manager_factory: PuffinManagerFactory,
puffin_metadata_cache: Option<PuffinMetadataCacheRef>,
) -> Self {
Self {
region_dir,
object_store,
file_cache,
metadata,
indexed_column_ids,
output: HashMap::default(),
index_cache,
puffin_manager_factory,
puffin_metadata_cache,
file_cache: None,
index_cache: None,
puffin_metadata_cache: None,
}
}

/// Sets the file cache.
pub fn with_file_cache(mut self, file_cache: Option<FileCacheRef>) -> Self {
self.file_cache = file_cache;
self
}

/// Sets the puffin metadata cache.
pub fn with_puffin_metadata_cache(
mut self,
puffin_metadata_cache: Option<PuffinMetadataCacheRef>,
) -> Self {
self.puffin_metadata_cache = puffin_metadata_cache;
self
}

/// Sets the index cache.
pub fn with_index_cache(mut self, index_cache: Option<InvertedIndexCacheRef>) -> Self {
self.index_cache = index_cache;
self
}

/// Consumes the builder to construct an [`InvertedIndexApplier`], optionally returned based on
/// the expressions provided. If no predicates match, returns `None`.
pub fn build(mut self, exprs: &[Expr]) -> Result<Option<InvertedIndexApplier>> {
Expand All @@ -116,16 +132,18 @@ impl<'a> InvertedIndexApplierBuilder<'a> {
.collect();
let applier = PredicatesIndexApplier::try_from(predicates);

Ok(Some(InvertedIndexApplier::new(
self.region_dir,
self.metadata.region_id,
self.object_store,
self.file_cache,
self.index_cache,
self.puffin_metadata_cache,
Box::new(applier.context(BuildIndexApplierSnafu)?),
self.puffin_manager_factory,
)))
Ok(Some(
InvertedIndexApplier::new(
self.region_dir,
self.metadata.region_id,
self.object_store,
Box::new(applier.context(BuildIndexApplierSnafu)?),
self.puffin_manager_factory,
)
.with_file_cache(self.file_cache)
.with_puffin_metadata_cache(self.puffin_metadata_cache)
.with_index_cache(self.index_cache),
))
}

/// Recursively traverses expressions to collect predicates.
Expand Down Expand Up @@ -331,12 +349,9 @@ mod tests {
let mut builder = InvertedIndexApplierBuilder::new(
"test".to_string(),
test_object_store(),
None,
None,
&metadata,
HashSet::from_iter([1, 2, 3]),
facotry,
None,
);

let expr = Expr::BinaryExpr(BinaryExpr {
Expand Down
15 changes: 0 additions & 15 deletions src/mito2/src/sst/index/inverted_index/applier/builder/between.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,12 +75,9 @@ mod tests {
let mut builder = InvertedIndexApplierBuilder::new(
"test".to_string(),
test_object_store(),
None,
None,
&metadata,
HashSet::from_iter([1, 2, 3]),
facotry,
None,
);

let between = Between {
Expand Down Expand Up @@ -119,12 +116,9 @@ mod tests {
let mut builder = InvertedIndexApplierBuilder::new(
"test".to_string(),
test_object_store(),
None,
None,
&metadata,
HashSet::from_iter([1, 2, 3]),
facotry,
None,
);

let between = Between {
Expand All @@ -146,12 +140,9 @@ mod tests {
let mut builder = InvertedIndexApplierBuilder::new(
"test".to_string(),
test_object_store(),
None,
None,
&metadata,
HashSet::from_iter([1, 2, 3]),
facotry,
None,
);

let between = Between {
Expand Down Expand Up @@ -190,12 +181,9 @@ mod tests {
let mut builder = InvertedIndexApplierBuilder::new(
"test".to_string(),
test_object_store(),
None,
None,
&metadata,
HashSet::from_iter([1, 2, 3]),
facotry,
None,
);

let between = Between {
Expand All @@ -218,12 +206,9 @@ mod tests {
let mut builder = InvertedIndexApplierBuilder::new(
"test".to_string(),
test_object_store(),
None,
None,
&metadata,
HashSet::from_iter([1, 2, 3]),
facotry,
None,
);

let between = Between {
Expand Down
12 changes: 0 additions & 12 deletions src/mito2/src/sst/index/inverted_index/applier/builder/comparison.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -231,12 +231,9 @@ mod tests {
let mut builder = InvertedIndexApplierBuilder::new(
"test".to_string(),
test_object_store(),
None,
None,
&metadata,
HashSet::from_iter([1, 2, 3]),
facotry,
None,
);

for ((left, op, right), _) in &cases {
Expand All @@ -261,12 +258,9 @@ mod tests {
let mut builder = InvertedIndexApplierBuilder::new(
"test".to_string(),
test_object_store(),
None,
None,
&metadata,
HashSet::from_iter([1, 2, 3]),
facotry,
None,
);

let res = builder.collect_comparison_expr(&tag_column(), &Operator::Lt, &int64_lit(10));
Expand All @@ -282,12 +276,9 @@ mod tests {
let mut builder = InvertedIndexApplierBuilder::new(
"test".to_string(),
test_object_store(),
None,
None,
&metadata,
HashSet::from_iter([1, 2, 3]),
facotry,
None,
);

builder
Expand Down Expand Up @@ -318,12 +309,9 @@ mod tests {
let mut builder = InvertedIndexApplierBuilder::new(
"test".to_string(),
test_object_store(),
None,
None,
&metadata,
HashSet::from_iter([1, 2, 3]),
facotry,
None,
);

let res = builder.collect_comparison_expr(
Expand Down
Loading

0 comments on commit a13a417

Please sign in to comment.