refactor: Consolidate physical/logical info into StateInfo #1350

DrakeLin · 2025-09-25T01:59:49Z

What changes are proposed in this pull request?

As identified in #1330
the codebase had fragmented schema state management - physical/logical schemas, predicates, and transform specs were scattered across multiple structs (Scan, ScanLogReplayProcessor, etc.) We had

Duplicate storage of the same information
Unnecessary 2-hop conversion: logical_schema → all_fields (Vec) → transform_spec

This PR consolidates all schema-related state into StateInfo as the single source of truth.

StateInfo directly computes TransformSpec without the all_fields vector
Functions pass around one Arc instead of multiple parameters

This change should also somewhat improve performance, as transform specs are now computed once during scan building and not during every scan execution.

This PR only targets scan, CDF is still using the old pattern. We will followup to eliminate ColumnType/all_fields completely.

How was this change tested?

Existing Unit tests + added tests for StateInfo

codecov · 2025-09-26T02:10:28Z

Codecov Report

❌ Patch coverage is 91.44385% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.83%. Comparing base (02dc795) to head (2dcc2cc).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
kernel/src/scan/mod.rs	91.19%	12 Missing and 2 partials ⚠️
kernel/src/scan/log_replay.rs	92.85%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1350      +/-   ##
==========================================
+ Coverage   84.80%   84.83%   +0.02%     
==========================================
  Files         113      113              
  Lines       28642    28735      +93     
  Branches    28642    28735      +93     
==========================================
+ Hits        24289    24376      +87     
- Misses       3196     3203       +7     
+ Partials     1157     1156       -1

☔ 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.

OussamaSaoudi · 2025-09-26T21:54:28Z

kernel/src/scan/log_replay.rs

+        // Extract the physical predicate from StateInfo's PhysicalPredicate enum.
+        // The DataSkippingFilter and partition_filter components expect the predicate
+        // in the format Option<(PredicateRef, SchemaRef)>, so we need to convert from
+        // the enum representation to the tuple format.
+        let physical_predicate = match &state_info.physical_predicate {
+            PhysicalPredicate::Some(predicate, schema) => {
+                // Valid predicate that can be used for data skipping and partition filtering
+                Some((predicate.clone(), schema.clone()))
+            }
+            _ => {
+                // Either PhysicalPredicate::None (no predicate provided) or
+                // PhysicalPredicate::StaticSkipAll (predicate always false).
+                // StaticSkipAll is handled at a higher level, so here we treat both as None.
+                None
+            }
+        };


Why not put this below in scan_metadata_inner with the other PhysicalPredicate cases?

if let PhysicalPredicate::StaticSkipAll = self.state_info.physical_predicate { return Ok(None.into_iter().flatten()); }

So we want to filter it out StaticSkipAll cases in scan_metadata_inner, but if we parse it there we'll have to add an extra param to scan_action_iter. However the extra param contains data already contained in StateInfo

After taking a look, I think it's fine as it is, unless we want to add a variable to StateInfo that says "canStaticallySkip" and have the predicate stored as an option there.

Do you have a better idea?

I like how it is rn :) Just add a debug_assert! that ensures we're not reaching here with a StaticSkipAll case

kernel/src/scan/log_replay.rs

OussamaSaoudi · 2025-09-26T21:59:02Z

I'm loving these changes 🔥 Cleans up kernel's concepts significantly

OussamaSaoudi · 2025-09-29T21:30:59Z

kernel/src/scan/mod.rs

+        let mut last_physical_field: Option<String> = None;
+
+        // Loop over all selected fields and build both the physical schema and transform spec
+        for (index, logical_field) in logical_schema.fields().enumerate() {


Note to reviewers: Hide Whitespace helps

OussamaSaoudi · 2025-09-29T21:34:29Z

kernel/src/scan/log_replay.rs

+        // Extract the physical predicate from StateInfo's PhysicalPredicate enum.
+        // The DataSkippingFilter and partition_filter components expect the predicate
+        // in the format Option<(PredicateRef, SchemaRef)>, so we need to convert from
+        // the enum representation to the tuple format.
+        let physical_predicate = match &state_info.physical_predicate {
+            PhysicalPredicate::Some(predicate, schema) => {
+                // Valid predicate that can be used for data skipping and partition filtering
+                Some((predicate.clone(), schema.clone()))
+            }
+            _ => {
+                // Either PhysicalPredicate::None (no predicate provided) or
+                // PhysicalPredicate::StaticSkipAll (predicate always false).
+                // StaticSkipAll is handled at a higher level, so here we treat both as None.
+                None
+            }
+        };


I like how it is rn :) Just add a debug_assert! that ensures we're not reaching here with a StaticSkipAll case

zachschuermann · 2025-10-01T04:53:03Z

kernel/src/scan/mod.rs

+        if let PhysicalPredicate::StaticSkipAll = self.state_info.physical_predicate {
+            return Ok(None.into_iter().flatten());
+        }
+        let it = scan_action_iter(engine, action_batch_iter, self.state_info.clone());


I thought we discussed pushing this down? Did that not work?

Wasn't successful, the way it's implemented made it so there wasn't an easy way to just return a null iterator.

kernel/src/scan/mod.rs

nicklan · 2025-10-02T22:41:37Z

kernel/src/scan/mod.rs

+        };
+
+        let transform_spec =
+            if !transform_spec.is_empty() || column_mapping_mode != ColumnMappingMode::None {


If we're only generating a transform because of column mapping, that's not really a transform, it's more like a "schema change". I guess we could model that as some kind of Identity expression + a schema, or maybe there's a better way to do it, but regardless, generating a bunch of identity transforms per column is probably silly.

not to be fixed in this PR, but can we make a follow-up issue?

I made an issue, feel free to expand on it #1369

kernel/src/scan/mod.rs

github-actions bot assigned DrakeLin Sep 25, 2025

DrakeLin marked this pull request as draft September 25, 2025 16:49

DrakeLin force-pushed the drake-lin_data/stack/state-info branch 2 times, most recently from dc9e0c6 to 6732472 Compare September 26, 2025 02:05

DrakeLin changed the title ~~State Info refactor~~ refactor: Consolidate physical/logical infor into StateInfo Sep 26, 2025

DrakeLin force-pushed the drake-lin_data/stack/state-info branch from 6732472 to 7fc84e1 Compare September 26, 2025 02:51

DrakeLin marked this pull request as ready for review September 26, 2025 02:51

DrakeLin requested review from OussamaSaoudi, nicklan and zachschuermann September 26, 2025 03:12

OussamaSaoudi reviewed Sep 26, 2025

View reviewed changes

DrakeLin changed the title ~~refactor: Consolidate physical/logical infor into StateInfo~~ refactor: Consolidate physical/logical info into StateInfo Sep 29, 2025

DrakeLin requested a review from OussamaSaoudi September 29, 2025 20:27

OussamaSaoudi removed the request for review from zachschuermann September 29, 2025 21:28

OussamaSaoudi approved these changes Sep 29, 2025

View reviewed changes

zachschuermann self-requested a review September 29, 2025 21:48

stateinfo

ffd596d

zachschuermann reviewed Oct 1, 2025

View reviewed changes

DrakeLin requested a review from zachschuermann October 1, 2025 05:48

nicklan approved these changes Oct 2, 2025

View reviewed changes

edits

d7e99c3

DrakeLin force-pushed the drake-lin_data/stack/state-info branch from c3fa41b to d7e99c3 Compare October 3, 2025 00:17

github-actions bot added the breaking-change Change that require a major version bump label Oct 3, 2025

Merge branch 'main' into drake-lin_data/stack/state-info

2dcc2cc

DrakeLin merged commit 39c440d into delta-io:main Oct 3, 2025
21 checks passed

Uh oh!

refactor: Consolidate physical/logical info into StateInfo #1350

refactor: Consolidate physical/logical info into StateInfo #1350

Conversation

DrakeLin commented Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes are proposed in this pull request?

How was this change tested?

Uh oh!

codecov bot commented Sep 26, 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!

DrakeLin Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

OussamaSaoudi commented Sep 26, 2025

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DrakeLin commented Sep 25, 2025 •

edited

Loading

codecov bot commented Sep 26, 2025 •

edited

Loading

DrakeLin Sep 29, 2025 •

edited

Loading