feat: strategized plan compaction #5233

zhangyue19921010 · 2025-11-13T08:28:21Z

codecov-commenter · 2025-11-13T08:56:06Z

Codecov Report

❌ Patch coverage is 96.82540% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.23%. Comparing base (f084677) to head (18eea6d).
⚠️ Report is 14 commits behind head on main.

Files with missing lines	Patch %	Lines
rust/lance/src/dataset/optimize.rs	96.82%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5233      +/-   ##
==========================================
+ Coverage   82.05%   82.23%   +0.18%     
==========================================
  Files         342      344       +2     
  Lines      141516   144881    +3365     
  Branches   141516   144881    +3365     
==========================================
+ Hits       116115   119149    +3034     
- Misses      21561    21800     +239     
- Partials     3840     3932      +92

Flag	Coverage Δ
unittests	`82.23% <96.82%> (+0.18%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

westonpace

A few (mostly minor) suggestions

westonpace · 2025-11-13T21:55:33Z

rust/lance/src/dataset/optimize.rs

+    // get all fragments by default
+    fn get_fragments(&self, dataset: &Dataset, _options: &CompactionOptions) -> Vec<FileFragment> {
+        // get_fragments should be returning fragments in sorted order (by id)
+        // and fragment ids should be unique
+        dataset.get_fragments()
+    }
+
+    // no filter by default
+    async fn filter_fragments(
+        &self,
+        _dataset: &Dataset,
+        fragments: Vec<FileFragment>,
+        _options: &CompactionOptions,
+    ) -> Result<Vec<FileFragment>> {
+        Ok(fragments)
+    }


Do these really need to be trait methods? I think we can probably leave them out and just let individual implementations use them if they want to. It will keep the trait simpler.

westonpace · 2025-11-13T21:56:30Z

rust/lance/src/dataset/optimize.rs

+        Ok(fragments)
+    }
+
+    async fn plan(&self, dataset: &Dataset, options: &CompactionOptions) -> Result<CompactionPlan>;


Let's document this method.

westonpace · 2025-11-13T21:57:55Z

rust/lance/src/dataset/optimize.rs

+        Ok(fragments)
+    }
+
+    async fn plan(&self, dataset: &Dataset, options: &CompactionOptions) -> Result<CompactionPlan>;


Will CompactionOptions be flexible enough for all possible strategies? Should we maybe accept options as a JSON string or a Map<String, String>? This way different strategies can expose their own custom options. That would leave the API a little less defined but it would be more flexible.

Do we even need to take CompactionOptions? Maybe it should be a argument to the constructor of the individual structs. That way each could have their own arguments but also be strongly typed.

Will CompactionOptions be flexible enough for all possible strategies? Should we maybe accept options as a JSON string or a Map<String, String>? This way different strategies can expose their own custom options. That would leave the API a little less defined but it would be more flexible.

Hi @westonpace Thanks a lot for your review. Added Map<String, String> for flexible.

Do we even need to take CompactionOptions? Maybe it should be a argument to the constructor of the individual structs. That way each could have their own arguments but also be strongly typed.

Hi @wjones127 Thanks a lot for your review.

I have tried several ways to eliminate the CompactionOptions parameter in the plan method, but none of them are perfect :( The main contradiction is that during users start planning compaction based on the built planner, they may dynamically adjust the options parameters on certain conditions, such as

https://github.com/lancedb/lance/blob/254a8217ac26666585983aa7ec8c4234f4c3f99f/rust/lance/src/dataset/optimize.rs#L225

If options are passed in when building the planner, then after modifying the options subsequently, it must also be ensured that the options in the planner can be seen. Therefore, we need Arc + mutex and cannot use clone.

On the contrary, it might be simpler and more flexible to pass in the desired options each time the plan method is called here.

If options are passed in when building the planner, then after modifying the options subsequently, it must also be ensured that the options in the planner can be seen. Therefore, we need Arc + mutex and cannot use clone.

I don't understand this. The logic of validate() can live in the planner and be internal.

If I were to rewrite compact_files, I would do:

pub async fn compact_files( dataset: &mut Dataset, mut options: CompactionOptions, remap_options: Option<Arc<dyn IndexRemapperOptions>>, // These will be deprecated later ) -> Result<CompactionMetrics> { info!(target: TRACE_DATASET_EVENTS, event=DATASET_COMPACTING_EVENT, uri = &dataset.uri); // .validate() now happens inside of `from_options` let planner = DefaultCompactionPlanner::from_options(options); compact_files_with_planner(dataset, &planner, remap_options).await }

westonpace · 2025-11-13T21:59:06Z

rust/lance/src/dataset/optimize.rs

 /// Compacts the files in the dataset without reordering them.
 ///
-/// This does a few things:
+/// By default, his does a few things:


Suggested change

/// By default, his does a few things:

/// By default, this does a few things:

westonpace · 2025-11-13T21:59:27Z

rust/lance/src/dataset/optimize.rs

 pub async fn compact_files(
+    dataset: &mut Dataset,
+    options: CompactionOptions,
+    remap_options: Option<Arc<dyn IndexRemapperOptions>>, // These will be deprecated later


zhangyue19921010 · 2025-11-19T02:48:02Z

Hi @westonpace and @wjones127 Thanks a lot for your review. All comments are addressed. PTAL :)

wjones127

Sorry for the delay, I accidentally left my comments pending. I'm still not sure about the design. It seems like it could be simplified further.

wjones127 · 2025-11-17T18:23:48Z

rust/lance/src/dataset/optimize.rs

+    // get all fragments by default
+    fn get_fragments(&self, dataset: &Dataset, _options: &CompactionOptions) -> Vec<FileFragment> {
+        // get_fragments should be returning fragments in sorted order (by id)
+        // and fragment ids should be unique
+        dataset.get_fragments()
+    }


Was already commented on, but do we need this? It seems like individual implementations can just call dataset.get_fragments() and then do whatever filtering they would like.

wjones127 · 2025-11-17T18:39:20Z

rust/lance/src/dataset/optimize.rs

+        Ok(fragments)
+    }
+
+    async fn plan(&self, dataset: &Dataset, options: &CompactionOptions) -> Result<CompactionPlan>;


If options are passed in when building the planner, then after modifying the options subsequently, it must also be ensured that the options in the planner can be seen. Therefore, we need Arc + mutex and cannot use clone.

I don't understand this. The logic of validate() can live in the planner and be internal.

If I were to rewrite compact_files, I would do:

pub async fn compact_files( dataset: &mut Dataset, mut options: CompactionOptions, remap_options: Option<Arc<dyn IndexRemapperOptions>>, // These will be deprecated later ) -> Result<CompactionMetrics> { info!(target: TRACE_DATASET_EVENTS, event=DATASET_COMPACTING_EVENT, uri = &dataset.uri); // .validate() now happens inside of `from_options` let planner = DefaultCompactionPlanner::from_options(options); compact_files_with_planner(dataset, &planner, remap_options).await }

feat: strategized plan compaction

b26776b

github-actions bot added the enhancement New feature or request label Nov 13, 2025

westonpace reviewed Nov 13, 2025

View reviewed changes

code review

18eea6d

Merge branch 'main' into feat_strategized_plan_compaction_v2

9cdedb4

wjones127 reviewed Nov 21, 2025

View reviewed changes

	/// By default, his does a few things:
	/// By default, this does a few things:

feat: strategized plan compaction #5233

Are you sure you want to change the base?

feat: strategized plan compaction #5233

Uh oh!

Conversation

zhangyue19921010 commented Nov 13, 2025

Uh oh!

codecov-commenter commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

westonpace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhangyue19921010 commented Nov 19, 2025

Uh oh!

wjones127 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Nov 13, 2025 •

edited

Loading