-
Notifications
You must be signed in to change notification settings - Fork 58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Part 2: propagate transform in visit_scan_files #612
base: main
Are you sure you want to change the base?
Changes from all commits
2a0257a
b6eb3e0
5d98eb1
8831535
d0f8e67
5843f85
de5fd07
779a662
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,7 @@ use delta_kernel::scan::state::{visit_scan_files, DvInfo, GlobalScanState}; | |
use delta_kernel::scan::{Scan, ScanData}; | ||
use delta_kernel::schema::Schema; | ||
use delta_kernel::snapshot::Snapshot; | ||
use delta_kernel::{DeltaResult, Error}; | ||
use delta_kernel::{DeltaResult, Error, ExpressionRef}; | ||
use delta_kernel_ffi_macros::handle_descriptor; | ||
use tracing::debug; | ||
use url::Url; | ||
|
@@ -211,6 +211,7 @@ pub unsafe extern "C" fn kernel_scan_data_next( | |
engine_context: NullableCvoid, | ||
engine_data: Handle<ExclusiveEngineData>, | ||
selection_vector: KernelBoolSlice, | ||
transforms: &CTransforms, | ||
), | ||
) -> ExternResult<bool> { | ||
let data = unsafe { data.as_ref() }; | ||
|
@@ -224,15 +225,17 @@ fn kernel_scan_data_next_impl( | |
engine_context: NullableCvoid, | ||
engine_data: Handle<ExclusiveEngineData>, | ||
selection_vector: KernelBoolSlice, | ||
transforms: &CTransforms, | ||
), | ||
) -> DeltaResult<bool> { | ||
let mut data = data | ||
.data | ||
.lock() | ||
.map_err(|_| Error::generic("poisoned mutex"))?; | ||
if let Some((data, sel_vec, _transforms)) = data.next().transpose()? { | ||
if let Some((data, sel_vec, transforms)) = data.next().transpose()? { | ||
let bool_slice = KernelBoolSlice::from(sel_vec); | ||
(engine_visitor)(engine_context, data.into(), bool_slice); | ||
let transform_map = CTransforms { transforms }; | ||
(engine_visitor)(engine_context, data.into(), bool_slice, &transform_map); | ||
Ok(true) | ||
} else { | ||
Ok(false) | ||
|
@@ -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( | ||
map: &CStringMap, | ||
key: KernelStringSlice, | ||
allocate_fn: AllocateStringFn, | ||
|
@@ -293,6 +296,10 @@ pub unsafe extern "C" fn get_from_map( | |
.and_then(|v| allocate_fn(kernel_string_slice!(v))) | ||
} | ||
|
||
pub struct CTransforms { | ||
transforms: Vec<Option<ExpressionRef>>, | ||
} | ||
|
||
/// Get a selection vector out of a [`DvInfo`] struct | ||
/// | ||
/// # Safety | ||
|
@@ -355,6 +362,7 @@ fn rust_callback( | |
size: i64, | ||
kernel_stats: Option<delta_kernel::scan::state::Stats>, | ||
dv_info: DvInfo, | ||
_transform: Option<ExpressionRef>, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any particular reason not to update the callback in this PR as well, so we can pass this on to the engine? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, mostly I was trying to keep it separated. This PR was focused on getting |
||
partition_values: HashMap<String, String>, | ||
) { | ||
let partition_map = CStringMap { | ||
|
@@ -388,6 +396,7 @@ struct ContextWrapper { | |
pub unsafe extern "C" fn visit_scan_data( | ||
data: Handle<ExclusiveEngineData>, | ||
selection_vec: KernelBoolSlice, | ||
transforms: &CTransforms, | ||
engine_context: NullableCvoid, | ||
callback: CScanCallback, | ||
) { | ||
|
@@ -398,5 +407,12 @@ pub unsafe extern "C" fn visit_scan_data( | |
callback, | ||
}; | ||
// TODO: return ExternResult to caller instead of panicking? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. seems pretty easy to address this todo? maybe we can go ahead and do that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That will require changes in the |
||
visit_scan_files(data, selection_vec, context_wrapper, rust_callback).unwrap(); | ||
visit_scan_files( | ||
data, | ||
selection_vec, | ||
&transforms.transforms, | ||
context_wrapper, | ||
rust_callback, | ||
) | ||
.unwrap(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a reasonable name change, but why this PR?
(also -- do we anticipate exposing other map types through FFI in the future?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)