refactor: move vacuum command to operations module #1045

roeap · 2022-12-31T11:33:46Z

Description

Moving the vacuum operation into the operations module and adopting IntoFuture for the command builder. This is breaking the APIs for the builder (now with consistent setter names) but we are able to keep the APIs for DeltaTable in rust and python.

In a follow up I would like to move th optimize command as well, This however may require refactoring the PartitionValue since we can only deal with static lifetimes when using IntoFuture, A while back we talked about pulling in ScalarValue from datafusion to optimize that implementation and maybe that's a good opportunitiy to look into that as well.

Related Issue(s)

Documentation

houqp · 2023-01-01T06:57:51Z

I do agree that the builder interface makes the style more consistent and the argument passing less error prone 👍

That said, I think having an extra thin wrapper API on the table struct like below will provide a better developer UX:

        let result = table.vacuum()
            .with_retention_period(Duration::hours(169))
            .with_dry_run(true)
            .await
            .unwrap();

roeap · 2023-01-01T08:43:29Z

wrapper API on the table struct like below will provide a better developer UX

Absoluetely, even more so on the python side of things. This unfortuantely lead me down a little bit of a rabbit hole :D. Essentially again the question on how to most efficiently share the log for commands etc. Right now in the code we just clone that state, but to my understanding this is not the way to go ...

Currently I am exploring a few options without a clear winner yet. However I do want to avoid making the DeltaTableState mutable when being shared, since this to me defeats the point of a snapshot, which is what we need for conflict resolution etc. Also looking forward to @wjones127's work in #1033 getting merged, since this should give us a nice baseline to finaly move to an arrow based state :). I do hope to make some general progress on this in this PR though to prepare for adopting this.

roeap · 2023-01-01T11:26:15Z

rust/src/delta.rs

+        let state = std::mem::take(&mut self.state);
+        let mut plan = VacuumBuilder::new(self.object_store(), state)
+            .with_dry_run(dry_run)
+            .with_enforce_retention_duration(enforce_retention_duration);
+        if let Some(hours) = retention_hours {
+            plan = plan.with_retention_period(Duration::hours(hours as i64));
+        }
+
+        let (table, metrics) = plan.await?;
+        self.state = table.state;


Here I am a little bit concernbed about the error handling. If we fail duing the execution, we are still left with a DeltaTable with an empty state. If users wanted to recover from a failure and keep using the table, they would probably see unexpexted results.

Once we manage the state in arrow, we can likely have near zero-cost clones of the state and mitigate the issue. For now one a way to handle this could be to handle the error internally, and load the previous table state again. But not sure what users wold expect / want to see.

The same applies on the python side.

We could make it immutable and put it behind an Arc, so we can easily clone pointers to it. Not sure how viable that is though.

That would make the mutation operations like merge() and process_action() less efficient.

Can we clone the state for now, and as a follow up let's figure out how to do this more efficiently?

roeap · 2023-01-01T11:29:33Z

rust/src/table_state.rs

@@ -330,6 +345,39 @@ impl DeltaTableState {

        Ok(())
    }
+
+    /// Obtain Add actions for files that match the filter
+    pub fn get_active_add_actions_by_partitions<'a>(


Moving this function to the state was part of an experiment. I did not move it back, since it exclusiveliy works on the state and from what I can see we are going to need functionality as such as we are evolving operations capabilities.

wjones127

This is a nice improvement.

However I do want to avoid making the DeltaTableState mutable when being shared

I'm thinking maybe the builders need to take &mut DeltaTable or &mut DeltaTableState. Then the borrow checker will enforce this. DeltaTableState is clone-able, so that means users can always clone themselves if they don't want to deal with the lifetimes.

rust/src/operations/vacuum.rs

wjones127 · 2023-01-02T18:28:24Z

rust/src/operations/vacuum.rs

+        let expired_tombstones = get_stale_files(&self.snapshot, retention_period, now_millis);
+        let valid_files = self.snapshot.file_paths_iter().collect::<HashSet<Path>>();
+
+        let mut files_to_delete = vec![];


It might be nice to eventually make this an iterator, so we don't have to wait to resolve the entire file list to start deleting.

wjones127 · 2023-01-02T18:48:34Z

rust/src/operations/vacuum.rs

+            .ok_or(DeltaTableError::NoMetadata)?
+            .partition_columns
+            .iter()
+            .any(|partition_column| path_name.starts_with(partition_column)))


I wonder if we need this line. I worry it will accidentally exclude a directory that shouldn't be there. And the partition directories should already be handled by the tombstones, right? Plus, Hive-partitioned directory structure isn't guaranteed.

I feel like we should take this out and see if the _date=2022-07-03/delete_me.parquet test case passes.

Just deleting the line unfortunately led to failing tests, but I did not dive deeper. My general feeling is we have to re-visit the logic anyhow, since we are not yet cleaning up "associated" files. I can look further into this here, but would prefer in a follow up :) - maybe when doing #688.

Okay. Thanks for looking into it.

rust/src/table_state.rs

wjones127 · 2023-01-02T18:56:43Z

rust/src/delta.rs

+        let state = std::mem::take(&mut self.state);
+        let mut plan = VacuumBuilder::new(self.object_store(), state)
+            .with_dry_run(dry_run)
+            .with_enforce_retention_duration(enforce_retention_duration);
+        if let Some(hours) = retention_hours {
+            plan = plan.with_retention_period(Duration::hours(hours as i64));
+        }
+
+        let (table, metrics) = plan.await?;
+        self.state = table.state;


We could make it immutable and put it behind an Arc, so we can easily clone pointers to it. Not sure how viable that is though.

wjones127 · 2023-01-02T19:04:03Z

rust/src/delta.rs

+        let state = std::mem::take(&mut self.state);
+        let mut plan = VacuumBuilder::new(self.object_store(), state)
+            .with_dry_run(dry_run)
+            .with_enforce_retention_duration(enforce_retention_duration);
+        if let Some(hours) = retention_hours {
+            plan = plan.with_retention_period(Duration::hours(hours as i64));
+        }
+
+        let (table, metrics) = plan.await?;
+        self.state = table.state;


That would make the mutation operations like merge() and process_action() less efficient.

rust/src/delta.rs

wjones127 · 2023-01-02T19:15:04Z

rust/src/delta.rs

+        let state = std::mem::take(&mut self.state);
+        let mut plan = VacuumBuilder::new(self.object_store(), state)
+            .with_dry_run(dry_run)
+            .with_enforce_retention_duration(enforce_retention_duration);
+        if let Some(hours) = retention_hours {
+            plan = plan.with_retention_period(Duration::hours(hours as i64));
+        }
+
+        let (table, metrics) = plan.await?;
+        self.state = table.state;


Can we clone the state for now, and as a follow up let's figure out how to do this more efficiently?

wjones127

LGTM. Though if you want, hiding that one function would be good.

Co-authored-by: Will Jones <willjones127@gmail.com>

roeap · 2023-01-05T14:07:25Z

@wjones127 - applied your suggestions! Could you re-approve? :)

# Description Moving the `vacuum` operation into the operations module and adopting `IntoFuture` for the command builder. This is breaking the APIs for the builder (now with consistent setter names) but we are able to keep the APIs for `DeltaTable` in rust and python. In a follow up I would like to move th optimize command as well, This however may require refactoring the `PartitionValue` since we can only deal with `static` lifetimes when using `IntoFuture`, A while back we talked about pulling in `ScalarValue` from datafusion to optimize that implementation and maybe that's a good opportunitiy to look into that as well. # Related Issue(s)  # Documentation  Co-authored-by: Will Jones <willjones127@gmail.com>

refactor: move vacuum command to operations module

5a9b3ae

roeap requested review from houqp, xianwill, wjones127, fvaleye, rtyler and mosyp as code owners December 31, 2022 11:33

github-actions bot added delta-rs-crate rust labels Dec 31, 2022

roeap marked this pull request as draft December 31, 2022 12:01

fix: make vacuum invokable from python

ad21a16

chore: simplify log update function calls

db370d5

github-actions bot added the python label Jan 1, 2023

roeap added 2 commits January 1, 2023 09:25

fix: vacuum op sinvocation

7e61476

fix: add missing import

d16b00f

roeap added 3 commits January 1, 2023 09:49

fix: struct reference in docstring

4c5a38f

fix: build with no features

6c57dc6

refactor: handle raw state in vacuum operation

8657e4f

roeap commented Jan 1, 2023

View reviewed changes

roeap marked this pull request as ready for review January 1, 2023 11:30

wjones127 requested changes Jan 2, 2023

View reviewed changes

chore: PR feedback

4089db9

wjones127 previously approved these changes Jan 5, 2023

View reviewed changes

Apply suggestions from code review

7b14074

Co-authored-by: Will Jones <willjones127@gmail.com>

roeap dismissed wjones127’s stale review via 7b14074 January 5, 2023 13:45

roeap enabled auto-merge (squash) January 5, 2023 13:46

wjones127 approved these changes Jan 5, 2023

View reviewed changes

roeap merged commit 50ea9f5 into delta-io:main Jan 5, 2023

roeap deleted the move-operations branch January 5, 2023 15:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: move vacuum command to operations module #1045

refactor: move vacuum command to operations module #1045

roeap commented Dec 31, 2022 •

edited

Loading

houqp commented Jan 1, 2023 •

edited

Loading

roeap commented Jan 1, 2023 •

edited

Loading

roeap Jan 1, 2023 •

edited

Loading

wjones127 Jan 2, 2023

wjones127 Jan 2, 2023

wjones127 Jan 2, 2023

roeap Jan 1, 2023

wjones127 left a comment

wjones127 Jan 2, 2023

wjones127 Jan 2, 2023

wjones127 Jan 2, 2023

roeap Jan 3, 2023

wjones127 Jan 5, 2023

wjones127 Jan 2, 2023

wjones127 Jan 2, 2023

wjones127 Jan 2, 2023

wjones127 left a comment

roeap commented Jan 5, 2023

refactor: move vacuum command to operations module #1045

refactor: move vacuum command to operations module #1045

Conversation

roeap commented Dec 31, 2022 • edited Loading

Description

Related Issue(s)

Documentation

houqp commented Jan 1, 2023 • edited Loading

roeap commented Jan 1, 2023 • edited Loading

roeap Jan 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjones127 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjones127 left a comment

Choose a reason for hiding this comment

roeap commented Jan 5, 2023

roeap commented Dec 31, 2022 •

edited

Loading

houqp commented Jan 1, 2023 •

edited

Loading

roeap commented Jan 1, 2023 •

edited

Loading

roeap Jan 1, 2023 •

edited

Loading