Part 2: propagate transform in visit_scan_files #612

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

Propagate the computed transforms from #607 through calls to visit_scan_files.

How was this change tested?

codecov · 2024-12-20T22:43:03Z

Codecov Report

Attention: Patch coverage is 60.41667% with 19 lines in your changes missing coverage. Please review.

Project coverage is 84.02%. Comparing base (d999b5c) to head (779a662).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
ffi/src/scan.rs	0.00%	16 Missing ⚠️
kernel/src/scan/mod.rs	88.46%	0 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #612      +/-   ##
==========================================
+ Coverage   83.92%   84.02%   +0.09%     
==========================================
  Files          75       76       +1     
  Lines       17277    17565     +288     
  Branches    17277    17565     +288     
==========================================
+ Hits        14500    14759     +259     
- Misses       2078     2091      +13     
- Partials      699      715      +16

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

scovich

Just double checking -- this PR makes almost no functional changes (other than printing the transforms)? It's mostly wiring to prepare for the next PR?

scovich · 2024-12-30T22:44:44Z

ffi/src/scan.rs

+}
+
+// #[no_mangle]
+// /// allow probing into a CStringMap. If the specified key is in the map, kernel will call


Suggested change

// /// allow probing into a CStringMap. If the specified key is in the map, kernel will call

// /// allow probing into a CTransformMap. If the specified key is in the map, kernel will call

(tho probably this whole chunk of code should just be moved to the later FFI PR?)

nicklan · 2025-01-10T01:38:35Z

Just double checking -- this PR makes almost no functional changes (other than printing the transforms)? It's mostly wiring to prepare for the next PR?

Yep, that's right. Just keeping things in logical chunks as much as possible. Hopefully that made the reviews easier

…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

OussamaSaoudi · 2025-01-23T23:59:05Z

kernel/src/scan/state.rs

@@ -213,7 +220,10 @@ impl<T> RowVisitor for ScanFileVisitor<'_, T> {
 mod tests {
    use std::collections::HashMap;

-    use crate::scan::test_utils::{add_batch_simple, run_with_validate_callback};
+    use crate::{


nit: maybe flatten these imports?

OussamaSaoudi · 2025-01-24T00:08:50Z

kernel/src/scan/mod.rs

+    if row < transforms.len() {
+        transforms[row].clone()
+    } else {
+        None
+    }


Suggested change

if row < transforms.len() {

transforms[row].clone()

} else {

None

}

transforms.get(row).cloned().flatten()

It looks like this only has a single call site. Now that it has reduced to a one-liner, should we just inline the code directly where it's used? Or do we anticipate other uses coming soon?

Note it's pub. My thinking was, much like the issues with deletion vectors being shorter than the actual number of rows, here we have a case where it's easy to mess up if the transform vec is shorter than the data, so let's have a nice interface that makes it easy. Given the one liner maybe we don't need it though. thoughts?

OussamaSaoudi · 2025-01-24T00:14:36Z

ffi/src/scan.rs

@@ -398,5 +407,12 @@ pub unsafe extern "C" fn visit_scan_data(
        callback,
    };
    // TODO: return ExternResult to caller instead of panicking?


seems pretty easy to address this todo? maybe we can go ahead and do that

That will require changes in the c code, which I was trying to avoid mostly here. I added a reminder for myself here for when I do the final fixup that requires lots of C changes

OussamaSaoudi · 2025-01-24T00:15:35Z

kernel/src/scan/state.rs

@@ -138,12 +141,14 @@ pub type ScanCallback<T> = fn(
 pub fn visit_scan_files<T>(
    data: &dyn EngineData,
    selection_vector: &[bool],
+    transforms: &Vec<Option<ExpressionRef>>,


any reason for &Vec<Option<ExpressionRef>> instead of &[Option<ExpresisonRef>]?

nope, good catch. usually clippy gets these, but it didn't this time :)

scovich · 2025-01-24T15:00:54Z

ffi/src/scan.rs

@@ -355,6 +362,7 @@ fn rust_callback(
    size: i64,
    kernel_stats: Option<delta_kernel::scan::state::Stats>,
    dv_info: DvInfo,
+    _transform: Option<ExpressionRef>,


Any particular reason not to update the callback in this PR as well, so we can pass this on to the engine?

Well, mostly I was trying to keep it separated. This PR was focused on getting visit_scan_files to pass things along. If I update the callback here I have to update all the c code too so it brings a bunch into this PR and I thought it was better to keep all the c changes to part 4

scovich · 2025-01-24T15:02:09Z

ffi/src/scan.rs

@@ -281,7 +284,7 @@ pub struct CStringMap {
 /// # Safety
 ///
 /// The engine is responsible for providing a valid [`CStringMap`] pointer and [`KernelStringSlice`]
-pub unsafe extern "C" fn get_from_map(
+pub unsafe extern "C" fn get_from_string_map(


Seems like a reasonable name change, but why this PR?
(also -- do we anticipate exposing other map types through FFI in the future?)

I think mostly I just noticed it while possibly having a "transform map" (which we no longer will have), and thought it was a good change. I can move it to another PR, but it does seem to make more sense this way. TBD on other types, but I imagine eventually we'll find something :)

scovich · 2025-01-24T15:04:54Z

kernel/src/scan/mod.rs

+    if row < transforms.len() {
+        transforms[row].clone()
+    } else {
+        None
+    }


It looks like this only has a single call site. Now that it has reduced to a one-liner, should we just inline the code directly where it's used? Or do we anticipate other uses coming soon?

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-2-propogate-transform-in-visit-scan-files branch from ff5d5fe to e88db7e Compare January 10, 2025 01:23

zachschuermann changed the title ~~Part 2: propogate transform in visit_scan_files~~ Part 2: propagate transform in visit_scan_files Jan 15, 2025

nicklan force-pushed the part-2-propogate-transform-in-visit-scan-files branch from da29cc7 to 18b29db Compare January 23, 2025 00:43

nicklan marked this pull request as ready for review January 23, 2025 00:44

nicklan requested review from scovich and zachschuermann January 23, 2025 00:44

nicklan added 7 commits January 23, 2025 13:48

checkpoint

2a0257a

pass through transforms in kernel

b6eb3e0

pass through in ffi (missing final bit)

5d98eb1

fmt cleanup

8831535

fixups for Vec not Map

d0f8e67

fix c code

5843f85

remove commented out code

de5fd07

nicklan force-pushed the part-2-propogate-transform-in-visit-scan-files branch from 18b29db to de5fd07 Compare January 23, 2025 21:48

OussamaSaoudi reviewed Jan 24, 2025

View reviewed changes

scovich reviewed Jan 24, 2025

View reviewed changes

address comments

779a662

nicklan requested review from OussamaSaoudi and scovich January 25, 2025 01:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Part 2: propagate transform in visit_scan_files #612

Part 2: propagate transform in visit_scan_files #612

nicklan commented Dec 20, 2024 •

edited

Loading

codecov bot commented Dec 20, 2024 •

edited

Loading

scovich left a comment

scovich Dec 30, 2024

nicklan commented Jan 10, 2025

OussamaSaoudi Jan 23, 2025

OussamaSaoudi Jan 24, 2025

scovich Jan 24, 2025

nicklan Jan 25, 2025

OussamaSaoudi Jan 24, 2025

nicklan Jan 25, 2025

OussamaSaoudi Jan 24, 2025

nicklan Jan 25, 2025

scovich Jan 24, 2025

nicklan Jan 25, 2025

scovich Jan 24, 2025

nicklan Jan 25, 2025

scovich Jan 24, 2025

	// /// allow probing into a CStringMap. If the specified key is in the map, kernel will call
	// /// allow probing into a CTransformMap. If the specified key is in the map, kernel will call

Part 2: propagate transform in visit_scan_files #612

Are you sure you want to change the base?

Part 2: propagate transform in visit_scan_files #612

Conversation

nicklan commented Dec 20, 2024 • edited Loading

How was this change tested?

codecov bot commented Dec 20, 2024 • edited Loading

Codecov Report

scovich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicklan commented Jan 10, 2025

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

Choose a reason for hiding this comment

nicklan commented Dec 20, 2024 •

edited

Loading

codecov bot commented Dec 20, 2024 •

edited

Loading