Fokko
diff --git a/‎.github/PULL_REQUEST_TEMPLATE.md‎
Lines changed: 13 additions & 0 deletions b/‎.github/PULL_REQUEST_TEMPLATE.md‎
Lines changed: 13 additions & 0 deletions
diff --git a/‎.github/pull_request_template.md‎
Lines changed: 0 additions & 46 deletions b/‎.github/pull_request_template.md‎
Lines changed: 0 additions & 46 deletions
diff --git a/‎CONTRIBUTING.md‎
Lines changed: 28 additions & 1 deletion b/‎CONTRIBUTING.md‎
Lines changed: 28 additions & 1 deletion
diff --git a/‎derive-macros/src/lib.rs‎
Lines changed: 6 additions & 6 deletions b/‎derive-macros/src/lib.rs‎
Lines changed: 6 additions & 6 deletions
diff --git a/‎ffi/examples/common/kernel_utils.c‎
Lines changed: 9 additions & 2 deletions b/‎ffi/examples/common/kernel_utils.c‎
Lines changed: 9 additions & 2 deletions
diff --git a/‎ffi/examples/common/kernel_utils.h‎
Lines changed: 3 additions & 1 deletion b/‎ffi/examples/common/kernel_utils.h‎
Lines changed: 3 additions & 1 deletion
diff --git a/‎ffi/examples/read-table/read_table.c‎
Lines changed: 16 additions & 4 deletions b/‎ffi/examples/read-table/read_table.c‎
Lines changed: 16 additions & 4 deletions
diff --git a/‎ffi/examples/read-table/schema.h‎
Lines changed: 17 additions & 8 deletions b/‎ffi/examples/read-table/schema.h‎
Lines changed: 17 additions & 8 deletions
diff --git a/‎ffi/src/lib.rs‎
Lines changed: 23 additions & 7 deletions b/‎ffi/src/lib.rs‎
Lines changed: 23 additions & 7 deletions
@@ -0,0 +1,13 @@
+## What changes are proposed in this pull request?
+
+<!--
+**Uncomment** this section if there are any changes affecting public APIs. Else, **delete** this section.
+
+### This PR affects the following public APIs
+
+If there are breaking changes, please ensure the `breaking-changes` label gets added by CI, and describe why the changes are needed.
+
+Note that _new_ public APIs are not considered breaking.
+-->
+
+## How was this change tested?
@@ -84,7 +84,34 @@ Our trunk branch is named `main`. Here's the typical workflow:
 
 **Note**: We require two approvals from code owners for any PR to be merged.
 
+## Pull Request Best Practices
+
+#### General Tips
+
+1. When making your first PR, please read our contributor guidelines: https://github.com/delta-incubator/delta-kernel-rs/blob/main/CONTRIBUTING.md
+2. Run `cargo t --all-features --all-targets` to get started testing, and run `cargo fmt`.
+3. Ensure you have added or run the appropriate tests for your PR.
+4. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP] Your PR title ...'.
+5. Be sure to keep the PR description updated to reflect all changes.
+
+#### PR Title Formatting
+
+This project uses conventional commits: https://www.conventionalcommits.org/
+
+Each PR corresponds to a commit on the `main` branch, with the title of the PR (typically) being
+used for the commit message on main. In order to ensure proper formatting in the CHANGELOG please
+ensure your PR title adheres to the conventional commit specification.
+
+Examples:
+- new feature PR: "feat: new API for snapshot.update()"
+- bugfix PR: "fix: correctly apply DV in read-table example"
+
+#### PR Testing
+
+Please make sure to add test cases that check the changes thoroughly including negative and positive cases if possible.
+If it was tested in a way different from regular unit tests, please clarify how you tested, ideally via a reproducible test documented in the PR description.
+
 ## Resources
 
 - [Delta Protocol](https://github.com/delta-io/delta/blob/master/PROTOCOL.md)
-- [Delta Lake Slack](https://go.delta.io/slack) - Join us in the `#delta-kernel` channel
+- [Delta Lake Slack](https://go.delta.io/slack) - Join us in the `#delta-kernel` channel
@@ -218,17 +218,17 @@ pub fn into_engine_data_derive(input: proc_macro::TokenStream) -> proc_macro::To
 
     let expanded = quote! {
         #[automatically_derived]
-        impl crate::IntoEngineData for #struct_name
+        impl delta_kernel::IntoEngineData for #struct_name
         where
-            #(#field_types: Into<crate::expressions::Scalar>),*
+            #(#field_types: Into<delta_kernel::expressions::Scalar>),*
         {
             fn into_engine_data(
                 self,
-                schema: crate::schema::SchemaRef,
-                engine: &dyn crate::Engine)
-            -> crate::DeltaResult<Box<dyn crate::EngineData>> {
+                schema: delta_kernel::schema::SchemaRef,
+                engine: &dyn delta_kernel::Engine)
+            -> delta_kernel::DeltaResult<Box<dyn delta_kernel::EngineData>> {
                 // NB: we `use` here to avoid polluting the caller's namespace
-                use crate::EvaluationHandlerExtension as _;
+                use delta_kernel::EvaluationHandlerExtension as _;
                 let values = [
                     #(self.#field_idents.into()),*
                 ];
 
@@ -61,10 +61,17 @@ void* allocate_string(const KernelStringSlice slice)
 }
 
 // utility function to convert key/val into slices and set them on a builder
-void set_builder_opt(EngineBuilder* engine_builder, char* key, char* val)
+// returns false on failure
+bool set_builder_opt(EngineBuilder* engine_builder, char* key, char* val)
 {
   KernelStringSlice key_slice = { key, strlen(key) };
   KernelStringSlice val_slice = { val, strlen(val) };
-  set_builder_option(engine_builder, key_slice, val_slice);
+  ExternResultbool res = set_builder_option(engine_builder, key_slice, val_slice);
+  if (res.tag != Okbool) {
+    print_error("Failed to set builder option.", (Error*)res.err);
+    free_error((Error*)res.err);
+    return false;
+  }
+  return true;
 }
 
@@ -1,5 +1,6 @@
 #pragma once
 
+#include <stdbool.h>
 #include <delta_kernel_ffi.h>
 
 // This is how we represent our errors. The kernel will ask us to contruct this struct whenever it
@@ -21,4 +22,5 @@ void* allocate_string(const KernelStringSlice slice);
 // error
 EngineError* allocate_error(KernelError etype, const KernelStringSlice msg);
 // utility function to convert key/val into slices and set them on a builder
-void set_builder_opt(EngineBuilder* engine_builder, char* key, char* val);
+// returns false on failure
+bool set_builder_opt(EngineBuilder* engine_builder, char* key, char* val);
@@ -31,7 +31,13 @@ void print_partition_info(struct EngineContext* context, const CStringMap* parti
   for (uintptr_t i = 0; i < context->partition_cols->len; i++) {
     char* col = context->partition_cols->cols[i];
     KernelStringSlice key = { col, strlen(col) };
-    char* partition_val = get_from_string_map(partition_values, key, allocate_string);
+    ExternResultNullableCvoid res = get_from_string_map(partition_values, key, allocate_string, context->engine);
+    if (res.tag != OkNullableCvoid) {
+      print_error("Failed to get from string map.", (Error*)res.err);
+      free_error((Error*)res.err);
+      continue;
+    }
+    char* partition_val = res.ok;
     if (partition_val) {
       print_diag("  partition '%s' here: %s\n", col, partition_val);
       free(partition_val);
@@ -117,7 +123,11 @@ void do_visit_scan_metadata(void* engine_context, HandleSharedScanMetadata scan_
 
   // Ask kernel to iterate each individual file and call us back with extracted metadata
   print_diag("Asking kernel to call us back for each scan row (file to read)\n");
-  visit_scan_metadata(scan_metadata, engine_context, scan_row_callback);
+  ExternResultbool visit_res = visit_scan_metadata(scan_metadata, context->engine, engine_context, scan_row_callback);
+  if (visit_res.tag != Okbool) {
+    print_error("Failed to visit scan metadata.", (Error*)visit_res.err);
+    free_error((Error*)visit_res.err);
+  }
   free_bool_slice(selection_vector);
   free_scan_metadata(scan_metadata);
 }
@@ -276,7 +286,9 @@ int main(int argc, char* argv[])
 
   // an example of using a builder to set options when building an engine
   EngineBuilder* engine_builder = engine_builder_res.ok;
-  set_builder_opt(engine_builder, "aws_region", "us-west-2");
+  if (!set_builder_opt(engine_builder, "aws_region", "us-west-2")) {
+    return -1;
+  }
   // potentially set credentials here
   // set_builder_opt(engine_builder, "aws_access_key_id" , "[redacted]");
   // set_builder_opt(engine_builder, "aws_secret_access_key", "[redacted]");
@@ -306,7 +318,7 @@ int main(int argc, char* argv[])
   uint64_t v = version(snapshot);
   printf("version: %" PRIu64 "\n\n", v);
 
-  CSchema *cschema = get_cschema(snapshot);
+  CSchema *cschema = get_cschema(snapshot, engine);
   print_cschema(cschema);
 
   char* table_root = snapshot_table_root(snapshot, allocate_string);
 
@@ -49,6 +49,7 @@ typedef struct
 {
   int list_count;
   SchemaItemList* lists;
+  SharedExternEngine* engine;
 } SchemaBuilder;
 
 typedef struct
@@ -109,12 +110,18 @@ void print_list(SchemaBuilder* builder, uintptr_t list_id, int indent, int paren
   }
 }
 
-void print_physical_name(const char *name, const CStringMap* metadata)
+void print_physical_name(const char *name, const CStringMap* metadata, SharedExternEngine* engine)
 {
 #ifdef VERBOSE
   char* key_str = "delta.columnMapping.physicalName";
   KernelStringSlice key = { key_str, strlen(key_str) };
-  char* value = get_from_string_map(metadata, key, allocate_string);
+  ExternResultNullableCvoid res = get_from_string_map(metadata, key, allocate_string, engine);
+  if (res.tag != OkNullableCvoid) {
+    printf("Failed to get physical name\n");
+    free_error((Error*)res.err);
+    return;
+  }
+  char* value = res.ok;
   if (value) {
     printf("Physical name of %s is %s\n", name, value);
     free(value);
@@ -124,6 +131,7 @@ void print_physical_name(const char *name, const CStringMap* metadata)
 #else
   (void)name;
   (void)metadata;
+  (void)engine;
 #endif
 }
 
@@ -157,7 +165,7 @@ void visit_struct(
   SchemaBuilder* builder = data;
   char* name_ptr = allocate_string(name);
   PRINT_CHILD_VISIT("struct", name_ptr, sibling_list_id, "Children", child_list_id);
-  print_physical_name(name_ptr, metadata);
+  print_physical_name(name_ptr, metadata, builder->engine);
   SchemaItem* struct_item = add_to_list(&builder->lists[sibling_list_id], name_ptr, "struct", is_nullable);
   struct_item->children = child_list_id;
 }
@@ -172,7 +180,7 @@ void visit_array(
 {
   SchemaBuilder* builder = data;
   char* name_ptr = allocate_string(name);
-  print_physical_name(name_ptr, metadata);
+  print_physical_name(name_ptr, metadata, builder->engine);
   PRINT_CHILD_VISIT("array", name_ptr, sibling_list_id, "Types", child_list_id);
   SchemaItem* array_item = add_to_list(&builder->lists[sibling_list_id], name_ptr, "array", is_nullable);
   array_item->children = child_list_id;
@@ -188,7 +196,7 @@ void visit_map(
 {
   SchemaBuilder* builder = data;
   char* name_ptr = allocate_string(name);
-  print_physical_name(name_ptr, metadata);
+  print_physical_name(name_ptr, metadata, builder->engine);
   PRINT_CHILD_VISIT("map", name_ptr, sibling_list_id, "Types", child_list_id);
   SchemaItem* map_item = add_to_list(&builder->lists[sibling_list_id], name_ptr, "map", is_nullable);
   map_item->children = child_list_id;
@@ -207,7 +215,7 @@ void visit_decimal(
   char* name_ptr = allocate_string(name);
   char* type = malloc(19 * sizeof(char));
   snprintf(type, 19, "decimal(%u)(%d)", precision, scale);
-  print_physical_name(name_ptr, metadata);
+  print_physical_name(name_ptr, metadata, builder->engine);
   PRINT_NO_CHILD_VISIT(type, name_ptr, sibling_list_id);
   add_to_list(&builder->lists[sibling_list_id], name_ptr, type, is_nullable);
 }
@@ -222,7 +230,7 @@ void visit_simple_type(
 {
   SchemaBuilder* builder = data;
   char* name_ptr = allocate_string(name);
-  print_physical_name(name_ptr, metadata);
+  print_physical_name(name_ptr, metadata, builder->engine);
   PRINT_NO_CHILD_VISIT(type, name_ptr, sibling_list_id);
   add_to_list(&builder->lists[sibling_list_id], name_ptr, type, is_nullable);
 }
@@ -273,12 +281,13 @@ void free_cschema(CSchema *schema) {
 }
 
 // Get the schema of the snapshot
-CSchema* get_cschema(SharedSnapshot* snapshot)
+CSchema* get_cschema(SharedSnapshot* snapshot, SharedExternEngine* engine)
 {
   print_diag("Building schema\n");
   SchemaBuilder* builder = malloc(sizeof(SchemaBuilder));
   builder->list_count = 0;
   builder->lists = NULL;
+  builder->engine = engine;
   EngineSchemaVisitor visitor = {
     .data = builder,
     .make_field_list = make_field_list,
 
@@ -2,6 +2,10 @@
 //!
 //! Exposes that an engine needs to call from C/C++ to interface with kernel
 
+#![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic)]
+// we re-allow panics in tests
+#![cfg_attr(test, allow(clippy::unwrap_used, clippy::expect_used, clippy::panic))]
+
 #[cfg(feature = "default-engine-base")]
 use std::collections::HashMap;
 use std::default::Default;
@@ -274,8 +278,10 @@ mod private {
             let len = val.len();
             let boxed = val.into_boxed_slice();
             let leaked_ptr = Box::leak(boxed).as_mut_ptr();
+            // safety: Box::leak always returns a valid, non-null pointer
+            #[allow(clippy::expect_used)]
             let ptr = NonNull::new(leaked_ptr)
-                .expect("This should never be non-null please report this bug.");
+                .expect("This should never be null please report this bug.");
             KernelBoolSlice { ptr, len }
         }
     }
@@ -328,8 +334,10 @@ mod private {
             let len = vec.len();
             let boxed = vec.into_boxed_slice();
             let leaked_ptr = Box::leak(boxed).as_mut_ptr();
+            // safety: Box::leak always returns a valid, non-null pointer
+            #[allow(clippy::expect_used)]
             let ptr = NonNull::new(leaked_ptr)
-                .expect("This should never be non-null please report this bug.");
+                .expect("This should never be null please report this bug.");
             KernelRowIndexArray { ptr, len }
         }
     }
@@ -489,11 +497,19 @@ pub unsafe extern "C" fn set_builder_option(
     builder: &mut EngineBuilder,
     key: KernelStringSlice,
     value: KernelStringSlice,
-) {
-    let key = unsafe { String::try_from_slice(&key) };
-    let value = unsafe { String::try_from_slice(&value) };
-    // TODO: Return ExternalError if key or value is invalid? (builder has an error allocator)
-    builder.set_option(key.unwrap(), value.unwrap());
+) -> ExternResult<bool> {
+    set_builder_option_impl(builder, key, value).into_extern_result(&builder.allocate_fn)
+}
+#[cfg(feature = "default-engine-base")]
+fn set_builder_option_impl(
+    builder: &mut EngineBuilder,
+    key: KernelStringSlice,
+    value: KernelStringSlice,
+) -> DeltaResult<bool> {
+    let key = unsafe { String::try_from_slice(&key) }?;
+    let value = unsafe { String::try_from_slice(&value) }?;
+    builder.set_option(key, value);
+    Ok(true)
 }
 
 /// Consume the builder and return a `default` engine. After calling, the passed pointer is _no
Original file line number	Diff line number	Diff line change
`@@ -61,10 +61,17 @@ void* allocate_string(const KernelStringSlice slice)`
`61`	`61`	`}`
`62`	`62`
`63`	`63`	`// utility function to convert key/val into slices and set them on a builder`
`64`		`-void set_builder_opt(EngineBuilder* engine_builder, char* key, char* val)`
	`64`	`+// returns false on failure`
	`65`	`+bool set_builder_opt(EngineBuilder* engine_builder, char* key, char* val)`
`65`	`66`	`{`
`66`	`67`	`KernelStringSlice key_slice = { key, strlen(key) };`
`67`	`68`	`KernelStringSlice val_slice = { val, strlen(val) };`
`68`		`- set_builder_option(engine_builder, key_slice, val_slice);`
	`69`	`+ ExternResultbool res = set_builder_option(engine_builder, key_slice, val_slice);`
	`70`	`+ if (res.tag != Okbool) {`
	`71`	`+ print_error("Failed to set builder option.", (Error*)res.err);`
	`72`	`+ free_error((Error*)res.err);`
	`73`	`+ return false;`
	`74`	`+ }`
	`75`	`+ return true;`
`69`	`76`	`}`
`70`	`77`