Part 3 of expression based transform: Use computed transform #613

nicklan · 2024-12-20T22:43:05Z

What changes are proposed in this pull request?

Use the transform that has been computed rather than using transform_to_logical.

Remove column_mapping_mode from GlobalScanState (it's not needed there anymore)
Remove the old transform_to_logical code
Add a new scan::state::transform_to_logical function that encapsulates the boilerplate of applying the transform expression
Use the new function where needed.

How was this change tested?

Existing tests pass which test this functionality extensively.

codecov · 2024-12-20T22:48:14Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.08%. Comparing base (2240154) to head (d8a2355).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #613   +/-   ##
=======================================
  Coverage   84.08%   84.08%           
=======================================
  Files          77       77           
  Lines       17823    17777   -46     
  Branches    17823    17777   -46     
=======================================
- Hits        14986    14948   -38     
+ Misses       2120     2115    -5     
+ Partials      717      714    -3

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

scovich · 2024-12-30T22:55:49Z

ffi/src/scan.rs

@@ -429,5 +427,12 @@ pub unsafe extern "C" fn visit_scan_data(
        callback,
    };
    // TODO: return ExternResult to caller instead of panicking?
-    visit_scan_files(data, selection_vec, &transforms.transforms, context_wrapper, rust_callback).unwrap();
+    visit_scan_files(


Is this formatting that should have been applied in an earlier PR?

it was applied earlier, it's included in part-2. not quite sure why it's showing up here but it'll go away once earlier PRs merge

scovich · 2024-12-30T22:58:49Z

kernel/src/scan/mod.rs

-                        &all_fields,
-                        have_partition_cols,
-                    );
+                    let logical = if let Some(ref transform) = scan_file.transform {


aside: Is there some way to factor out some of the duplicated logic between this default engine, and the sync engine example above? (I'm guessing this new code just adds to existing duplication, so best addressed in a separate PR)

I've actually moved it to a function: state::transform_to_logical

But I suspect that's not the code you're referencing here. Which duplicate code did you mean?

Hmm, I've forgotten the details now, and I can't see the original code any more (loading the file link for this comment brings up a file without the comment). Maybe it was related to partition value parsing (a quick search turns up several uses of that function, generally falling into two categories). Otherwise... sorry no idea.

scovich · 2024-12-30T23:01:22Z

kernel/src/scan/mod.rs

-    have_partition_cols: bool,
-) -> DeltaResult<Box<dyn EngineData>> {
-    let physical_schema = global_state.physical_schema.clone();
-    if !have_partition_cols && global_state.column_mapping_mode == ColumnMappingMode::None {


IIRC, this code was annoying when I added column mapping support for expression eval. I think this was the only code left that specifically tracked or cared about column mapping mode, because of the new way logical -> physical transforms are performed.

Recommend to audit the caller chain and see what other code can be simplified, now that we don't need column mapping logic here any more.

Good call! I've removed it from GlobalScanState as we don't need it there anymore

scovich · 2024-12-30T23:02:55Z

kernel/tests/read.rs

-            )
-            .unwrap();
+            // to transform the physical data into the correct logical form
+            let logical = if let Some(ref transform) = scan_file.transform {


I think this is (at least) three places that do ~exactly the same thing. Is there a way to factor out a helper method that everyone can use?

Yep, factored out to state::transform_to_logical

…and return it. (#607)  ## What changes are proposed in this pull request?  This is the initial part of moving to using expressions to express transformations when reading data. What this PR does is: - Compute a "static" transform, which is just a set of column expressions that need to be passed directly through without change, or enough metadata for lower levels to fill in a "fixup" expression - The static transform is passed into the iterator that parses each `Add` file - When parsing the `Add` file, if there are needed fix-ups (just partition columns today), the correct expression is created, and inserted into a row indexed map - This map is returned so the caller can find out for a given row what, if any, expression needs to be applied when reading the specified row Follow-up PRs: * #612: Propagate this information through when using `visit_scan_files` * #613: Actually use the data to do transformation and remove `transform_to_logical` entirely * #614: Make this work over ffi and use it * (TODO): Clean up any existing code that's now over complicated in the scan building Each of those are more invasive and end up touching significant code, so I'm staging this as much as possible to make reviews easier.  ## How was this change tested?  Unit tests, and inspection of resultant expressions when run on tables

scovich · 2025-02-05T20:40:32Z

kernel/examples/read-table-multi-threaded/src/main.rs

@@ -12,8 +12,7 @@ use delta_kernel::engine::arrow_data::ArrowEngineData;
 use delta_kernel::engine::default::executor::tokio::TokioBackgroundExecutor;
 use delta_kernel::engine::default::DefaultEngine;
 use delta_kernel::engine::sync::SyncEngine;
-use delta_kernel::scan::state::{DvInfo, GlobalScanState, Stats};
-use delta_kernel::scan::transform_to_logical;
+use delta_kernel::scan::state::{transform_to_logical, DvInfo, GlobalScanState, Stats};


Remove the old transform_to_logical code

??

Ah -- the partition values aware version of transform_to_logical_internal is what disappeared.

scovich · 2025-02-05T20:43:40Z

kernel/src/scan/state.rs

+    if let Some(ref transform) = transform {
+        engine
+            .get_expression_handler()
+            .get_evaluator(
+                physical_schema.clone(),
+                transform.as_ref().clone(), // TODO: Maybe eval should take a ref
+                logical_schema.clone().into(),
+            )
+            .evaluate(physical_data.as_ref())
+    } else {
+        Ok(physical_data)
+    }


nit: possibly cleaner with a match?

Suggested change

if let Some(ref transform) = transform {

engine

.get_expression_handler()

.get_evaluator(

physical_schema.clone(),

transform.as_ref().clone(), // TODO: Maybe eval should take a ref

logical_schema.clone().into(),

)

.evaluate(physical_data.as_ref())

} else {

Ok(physical_data)

}

match transform {

Some(ref transform) => engine

.get_expression_handler()

.get_evaluator(

physical_schema.clone(),

transform.as_ref().clone(), // TODO: Maybe eval should take a ref

logical_schema.clone().into(),

)

.evaluate(physical_data.as_ref()),

None => Ok(physical_data),

}

zachschuermann

just a couple nits, LGTM (and I think these small PRs in parts/sequences are great!)

zachschuermann · 2025-02-05T22:39:34Z

kernel/examples/read-table-multi-threaded/src/main.rs

-    _transform: Option<ExpressionRef>,
-    partition_values: HashMap<String, String>,
+    transform: Option<ExpressionRef>,
+    _: HashMap<String, String>,


should we be able to remove this or does this still prove useful elsewhere? (I know there are a number of spots where we use this callback and pass partition values - curious if we will eventually migrate everything to transform or if this is still actually needed)

We could. My idea was to keep it for now so existing systems could migrate more slowly. Basically, deprecate it

zachschuermann · 2025-02-05T22:44:23Z

kernel/src/scan/state.rs

+        Ok(physical_data)
+    }
+}
+


doc nit: Line 142/143 below might benefit from listing the new transform field

yep, nice catch, added

github-actions bot assigned nicklan Dec 20, 2024

github-actions bot added the breaking-change Change that will require a version bump label Dec 20, 2024

nicklan mentioned this pull request Dec 20, 2024

Part 1, Read transforms via expressions: Just compute the expression and return it. #607

Merged

scovich reviewed Dec 30, 2024

View reviewed changes

nicklan force-pushed the part-3-use-computed-transform branch from 6d72b75 to ec0d671 Compare January 10, 2025 23:55

nicklan force-pushed the part-3-use-computed-transform branch from ec0d671 to 24a45c9 Compare January 23, 2025 00:53

nicklan added 5 commits February 4, 2025 12:00

use transform in execute

f305650

multi-threaded reader uses transform

a7a440b

read.rs uses transform

83c07a6

remove transform_to_logical :)

c1210b1

factor out and make a new transform_to_logical

2d598d8

nicklan force-pushed the part-3-use-computed-transform branch from 24a45c9 to 2d598d8 Compare February 4, 2025 20:01

remove column_mapping_mode from GlobalScanState

ea80224

nicklan marked this pull request as ready for review February 4, 2025 20:17

nicklan requested review from scovich, zachschuermann and OussamaSaoudi-db February 4, 2025 20:17

scovich approved these changes Feb 5, 2025

View reviewed changes

zachschuermann approved these changes Feb 5, 2025

View reviewed changes

nicklan added 2 commits February 5, 2025 15:24

Merge branch 'main' into part-3-use-computed-transform

84c612b

address comments

d8a2355

nicklan merged commit 68f4790 into delta-io:main Feb 6, 2025
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Part 3 of expression based transform: Use computed transform #613

Part 3 of expression based transform: Use computed transform #613

nicklan commented Dec 20, 2024 •

edited

Loading

codecov bot commented Dec 20, 2024 •

edited

Loading

scovich Dec 30, 2024

nicklan Jan 10, 2025

scovich Dec 30, 2024

nicklan Feb 4, 2025

scovich Feb 5, 2025

scovich Dec 30, 2024

nicklan Feb 4, 2025

scovich Dec 30, 2024

nicklan Feb 4, 2025

scovich Feb 5, 2025

scovich Feb 5, 2025

scovich Feb 5, 2025

zachschuermann left a comment

zachschuermann Feb 5, 2025

nicklan Feb 5, 2025

zachschuermann Feb 5, 2025

nicklan Feb 5, 2025

Part 3 of expression based transform: Use computed transform #613

Part 3 of expression based transform: Use computed transform #613

Conversation

nicklan commented Dec 20, 2024 • edited Loading

What changes are proposed in this pull request?

How was this change tested?

codecov bot commented Dec 20, 2024 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachschuermann 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

nicklan commented Dec 20, 2024 •

edited

Loading

codecov bot commented Dec 20, 2024 •

edited

Loading