refactor: use EagerSnapshot in datafusion module #3796

roeap · 2025-10-01T11:54:49Z

Description

While migrating to kernel log replay we took on a lot of tech dept, that we no need to clean up :).

One reason for bloat and is the similar nature of DeltaTableState, EagerSnapshot, and Snapshot. In this PR we reduce the API surface that use DeltaTableState in favour of using EagerSnapshot. While we still require some pathfinding, the most likely candidate to consolidate is using EagerSnapshot and getting rid of the others.

Almost all operations are migrated, except Vacuum which would have required too much changes to business logic and will be migrated later.

Related Issue(s)

related #3733

codecov · 2025-10-01T11:57:31Z

Codecov Report

❌ Patch coverage is 89.38907% with 33 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.06%. Comparing base (46dcf9c) to head (8ed3325).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
python/src/lib.rs	0.00%	7 Missing ⚠️
crates/core/src/table/state.rs	71.42%	2 Missing and 2 partials ⚠️
crates/core/src/delta_datafusion/table_provider.rs	89.65%	1 Missing and 2 partials ⚠️
crates/core/src/operations/mod.rs	82.35%	3 Missing ⚠️
crates/core/src/operations/restore.rs	92.59%	0 Missing and 2 partials ⚠️
crates/aws/src/logstore/dynamodb_logstore.rs	0.00%	1 Missing ⚠️
crates/core/src/delta_datafusion/schema_adapter.rs	66.66%	1 Missing ⚠️
crates/core/src/kernel/snapshot/iterators.rs	0.00%	1 Missing ⚠️
crates/core/src/kernel/snapshot/log_data.rs	66.66%	1 Missing ⚠️
crates/core/src/kernel/snapshot/mod.rs	92.85%	1 Missing ⚠️
... and 9 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3796      +/-   ##
==========================================
- Coverage   76.09%   76.06%   -0.03%     
==========================================
  Files         145      145              
  Lines       45200    45273      +73     
  Branches    45200    45273      +73     
==========================================
+ Hits        34397    34439      +42     
- Misses       9107     9144      +37     
+ Partials     1696     1690       -6

☔ 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.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

roeap · 2025-10-01T11:58:05Z

crates/core/src/kernel/snapshot/mod.rs

+    #[deprecated(since = "0.30.0", note = "Use `files` with kernel predicate instead.")]
+    pub fn file_views_by_partitions(
+        &self,
+        log_store: &dyn LogStore,
+        filters: &[PartitionFilter],
+    ) -> BoxStream<'_, DeltaResult<LogicalFileView>> {
+        if filters.is_empty() {
+            return self.files(log_store, None);
+        }
+        let predicate = match to_kernel_predicate(filters, self.snapshot.schema()) {
+            Ok(predicate) => Arc::new(predicate),
+            Err(err) => return Box::pin(futures::stream::once(async { Err(err) })),
+        };
+        self.files(log_store, Some(predicate))
+    }


this is a migration helper. We should move the conversion of PartitionFilter to predicate either to the python crate, or even better translate DNF directly to kernel predicates. Getting rid of this completely would have required a much larger change.

roeap · 2025-10-01T12:01:30Z

crates/core/src/delta_datafusion/table_provider.rs

-        let context = SessionContext::new();
        let df_schema = logical_schema.clone().to_dfschema()?;

        let logical_filter = self
            .filter
            .clone()
-            .map(|expr| simplify_expr(&context, &df_schema, expr));
+            .map(|expr| simplify_expr(self.session, &df_schema, expr));


This is a drive-by fix and a good example of what @rtyler raised w.r.t. LogStores. We were creating a new session while also tracking a session on the operation. i.e. using inconsistent datafusion sessions in the same operation.

Signed-off-by: Robert Pack <[email protected]>

crates/core/src/table/state.rs

rtyler · 2025-10-01T12:39:19Z

crates/benchmarks/src/bin/merge.rs


    let provider = DeltaTableProvider::try_new(
-        table.snapshot()?.clone(),
+        table.snapshot()?.snapshot().clone(),


rtyler · 2025-10-01T12:47:25Z

crates/core/src/kernel/snapshot/mod.rs

        futures::stream::iter(iter).boxed()
    }

+    #[deprecated(since = "0.30.0", note = "Use `files` with kernel predicate instead.")]


😆 what version number is this?

was hoping the net one we release 😆

rtyler · 2025-10-01T12:48:13Z

crates/core/src/logstore/mod.rs

    }

    #[deprecated(
        since = "0.1.0",


well ... i feel like we knew this was coming early on.

rtyler · 2025-10-01T12:51:42Z

crates/core/src/operations/cdc.rs

        table.load().await.expect("Failed to reload table");
-        let result = should_write_cdc(table.snapshot().unwrap()).expect("Failed to use table");
+        let result =
+            should_write_cdc(&table.snapshot().unwrap().snapshot).expect("Failed to use table");


When we were hacking yesterday this kind of accessing into the snapshot field of the EagerSnapshot is something we should probably be removing.

Is it possible to put a #[cfg(test)] around that to keep its isolation into tests?

I was able to get rid of some more call sites of this bit not all. its pub(crate) for now and should go away as we consolidate.

Co-authored-by: Ion Koutsouris <[email protected]> Signed-off-by: Robert Pack <[email protected]>

Signed-off-by: Robert Pack <[email protected]>

roeap · 2025-10-01T13:48:47Z

@rtyler @ion-elgreco - addressed your feedback and would appreciate some new stamps :).

ion-elgreco

☑️

roeap requested review from hntd187, ion-elgreco and rtyler as code owners October 1, 2025 11:54

github-actions bot added binding/python Issues for the Python package binding/rust Issues for the Rust crate labels Oct 1, 2025

roeap force-pushed the refactor/use-eager branch from 38d7807 to 928471e Compare October 1, 2025 11:55

roeap commented Oct 1, 2025

View reviewed changes

refactor: use EagerSnapshot in detafusiuon module

1fa9932

Signed-off-by: Robert Pack <[email protected]>

roeap force-pushed the refactor/use-eager branch from 928471e to 1fa9932 Compare October 1, 2025 12:02

chore: fix typos

c0b8935

Signed-off-by: Robert Pack <[email protected]>

roeap force-pushed the refactor/use-eager branch from 674ea63 to c0b8935 Compare October 1, 2025 12:07

ion-elgreco previously approved these changes Oct 1, 2025

View reviewed changes

crates/core/src/table/state.rs Outdated Show resolved Hide resolved

ion-elgreco changed the title ~~refactor: use EagerSnapshot in detafusion module~~ refactor: use EagerSnapshot in datafusion module Oct 1, 2025

roeap dismissed ion-elgreco’s stale review via 0cc7d26 October 1, 2025 12:31

rtyler reviewed Oct 1, 2025

View reviewed changes

rtyler previously approved these changes Oct 1, 2025

View reviewed changes

roeap and others added 3 commits October 1, 2025 15:41

fix: typo

0e55480

Co-authored-by: Ion Koutsouris <[email protected]> Signed-off-by: Robert Pack <[email protected]>

Merge branch 'main' into refactor/use-eager

9e67e33

chore: PR feedback

b18ebf6

Signed-off-by: Robert Pack <[email protected]>

roeap dismissed rtyler’s stale review via b18ebf6 October 1, 2025 13:47

roeap force-pushed the refactor/use-eager branch from 0cc7d26 to b18ebf6 Compare October 1, 2025 13:47

roeap enabled auto-merge (squash) October 1, 2025 13:53

Merge branch 'main' into refactor/use-eager

8ed3325

ion-elgreco approved these changes Oct 1, 2025

View reviewed changes

roeap merged commit 18f949e into delta-io:main Oct 1, 2025
28 of 29 checks passed

roeap deleted the refactor/use-eager branch October 2, 2025 07:54

Uh oh!

refactor: use EagerSnapshot in datafusion module #3796

refactor: use EagerSnapshot in datafusion module #3796

Uh oh!

Conversation

roeap commented Oct 1, 2025

Description

Related Issue(s)

Uh oh!

codecov bot commented Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

roeap commented Oct 1, 2025

Uh oh!

ion-elgreco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Oct 1, 2025 •

edited

Loading