Add visitor for converting kernel expressions to engine expressions #363

OussamaSaoudi-db · 2024-09-27T20:55:10Z

Closes: #358
Depends on: #360

codecov · 2024-09-27T20:58:31Z

Codecov Report

Attention: Patch coverage is 0.36900% with 270 lines in your changes missing coverage. Please review.

Project coverage is 75.66%. Comparing base (6b0df5d) to head (c8f80a3).

Files with missing lines	Patch %	Lines
ffi/src/expressions.rs	0.00%	270 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #363      +/-   ##
==========================================
- Coverage   77.68%   75.66%   -2.03%     
==========================================
  Files          49       49              
  Lines       10084    10354     +270     
  Branches    10084    10354     +270     
==========================================
  Hits         7834     7834              
- Misses       1805     2075     +270     
  Partials      445      445

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

ffi/src/expressions.rs

FFI code for schema and expressions have been moved to their own modules. Add prototype of the visitor to enable kernel expresison -> engine expression

OussamaSaoudi-db · 2024-10-10T23:07:33Z

ffi/examples/read-table/expression_test_results.txt

@@ -0,0 +1,78 @@
+And


@nicklan @zachschuermann How should I go about testing this? I imagine ffi/tests/something is a sensible place to put this file. I need some way to pass this file to the executable, but read_table expects a table input, not this.

Currently I havetest_kernel_expr pointing directly to ./expression_test_results.txt, but that makes it so that you can only run the executable from the read-table directory.

what is test_kernel_expr? maybe a c file you forgot to add?

I think you could have two binaries compiled in the read-table example with adding something like:

add_executable(test_expr test_expr.c) target_compile_definitions(test_expr PUBLIC DEFINE_DEFAULT_ENGINE) target_include_directories(test_expr PUBLIC "${CMAKE_CURRENT_SOURCE_DIR}/../../../target/ffi-headers") target_link_directories(test_expr PUBLIC "${CMAKE_CURRENT_SOURCE_DIR}/../../../target/debug") target_link_libraries(test_expr PUBLIC delta_kernel_ffi) target_compile_options(test_expr PUBLIC)

in the CMakeLists.txt.

That's not ideal, even better would be a new print_expr example. There's not much (if anything) you rely on from read_table.h, so you could just copy over whatever you need into a new header. lmk if you need help with getting that set up

Also note that I removed the expression test in the C code for now since that breaks builds. It currently Works On My Machine ™, but I'd like to make it part of CI if possible.

Just saw your msg. I'll get a second binary setup 👍

OussamaSaoudi-db · 2024-10-10T23:13:29Z

ffi/src/expressions.rs

+/// The caller is responsible for freeing the retured memory, either by calling
+/// [`free_kernel_predicate`], or [`Handle::drop_handle`]
+#[no_mangle]
+pub unsafe extern "C" fn get_kernel_expression() -> Handle<SharedExpression> {


I currently have this C function that returns a big expression for testing. How can I limit visibility while still passing this over to something like read_table?

OussamaSaoudi-db · 2024-10-10T23:14:12Z

ffi/src/expressions.rs

+    /// Visit a `struct` which is constructed from an ordered list of expressions. This declares
+    /// the number of expressions that the struct will be made of. The visitor will populate the
+    /// list of expressions using the [`visit_struct_sub_expr`] method.
+    pub visit_struct: extern "C" fn(data: *mut c_void, len: usize) -> usize,


This is visiting a struct made up of a Vec<Expression>. How's the naming?

I think I'd call it visit_struct_expr to make it clear

ffi/src/expressions.rs

nicklan

flushing some comments

nicklan · 2024-10-10T22:44:35Z

ffi/src/expressions.rs

+/// The caller is responsible for freeing the retured memory, either by calling
+/// [`free_kernel_predicate`], or [`Handle::drop_handle`]
+#[no_mangle]
+pub unsafe extern "C" fn get_kernel_expression() -> Handle<SharedExpression> {


This is really just testing code right? At the very least I'd name it get_testing_kernel_expr and ideally we'd hide it behind a #[cfg(test)] but I'm not sure if we can easily have it then make it into the library. Maybe we make a new module that requires a feature and exposes these kinds of "test" functions?

nicklan · 2024-10-10T22:55:37Z

ffi/src/expressions.rs

+    }
+    fn visit_expression(visitor: &mut EngineExpressionVisitor, expression: &Expression) -> usize {
+        match expression {
+            Expression::Literal(scalar) => visit_scalar(visitor, scalar),


We're just blasting through the Literal expression here without giving the engine any indication that that's what the visited scalar is associated with. If we ever want to use scalars embedded in some other expression that could be an issue.

I think we should probably add a visit_literal(scalar_id: usize). If engines want to to just "ignore" that they can just return scalar_id;. Alternately we could rename all the visit_int type expressions to visit_int_literal to make it more clear. That's maybe easier for the ffi right now, but would be less flexible in the future. thoughts?

Cool, I can add that.

which? i'm not totally clear which is best :)

nicklan · 2024-10-10T22:55:59Z

ffi/src/expressions.rs

+/// to visitor methods
+/// TODO: Add type information in struct field and null. This will likely involve using the schema visitor.
+#[repr(C)]
+pub struct EngineExpressionVisitor {


Uhh, wow, that's a lot of visitors :)

Yessir! Lots of hours and thought, apologies for the big PR lol

nicklan · 2024-10-10T23:14:19Z

ffi/examples/read-table/read_table.c

@@ -3,6 +3,7 @@
 #include <string.h>

 #include "arrow.h"
+#include "expression.h"


don't think you need this

nicklan · 2024-10-10T23:17:25Z

ffi/examples/read-table/expression_test_results.txt

@@ -0,0 +1,78 @@
+And


what is test_kernel_expr? maybe a c file you forgot to add?

I think you could have two binaries compiled in the read-table example with adding something like:

add_executable(test_expr test_expr.c) target_compile_definitions(test_expr PUBLIC DEFINE_DEFAULT_ENGINE) target_include_directories(test_expr PUBLIC "${CMAKE_CURRENT_SOURCE_DIR}/../../../target/ffi-headers") target_link_directories(test_expr PUBLIC "${CMAKE_CURRENT_SOURCE_DIR}/../../../target/debug") target_link_libraries(test_expr PUBLIC delta_kernel_ffi) target_compile_options(test_expr PUBLIC)

in the CMakeLists.txt.

That's not ideal, even better would be a new print_expr example. There's not much (if anything) you rely on from read_table.h, so you could just copy over whatever you need into a new header. lmk if you need help with getting that set up

nicklan · 2024-10-10T23:21:52Z

ffi/examples/read-table/expression.h

+    float float_data;
+    double double_data;
+    bool boolean_data;
+    struct KernelStringSlice string_data;


You don't need this to be a KernelStringSlice, just make it a char*. Get rid of copy_kernel_string() and just allocate a c-string instead, like:

// utility to turn a slice into a char* char* allocate_string(const KernelStringSlice slice) { return strndup(slice.ptr, slice.len); }

nicklan · 2024-10-10T23:22:59Z

ffi/examples/read-table/expression.h

+} ExpressionBuilder;
+struct Struct
+{
+  KernelStringSlice* field_names;


See comment below, just make this a char**.

nicklan

Nice thanks, lots of work!

One major comment, I think it might be better to model this the same we do schemas where each visit_X call gets a sibling_list_id which tells it which list of expressions it should append itself to. Then you don't need all the visit_struct_sub_expr methods, you can just do something like:

fn visit_struct(visitor: &mut EngineExpressionVisitor, exprs: &Vec<Expression>) -> usize {
  let child_list_id = visitor.make_expr_list(exprs.len());
  for expr in exprs {
    visit_expression(visitor, expr, child_list_id);
  }
  visit_struct(visitor.data, child_list_id)
}

There is a subtle difference that visit_schema returns a list id at the end because we know a schema is always a list of struct fields at the top. Here we could be visiting just one expression, but we could just define that the top level always returns a single element list, or figure out how to return the top level expr id directly.

I realize that's a somewhat big change so open to pushback :)

nicklan · 2024-10-10T23:23:42Z

ffi/examples/read-table/expression.h

@@ -0,0 +1,751 @@
+#include "assert.h"


Suggested change

#include "assert.h"

#include <assert.h>

nicklan · 2024-10-10T23:31:29Z

ffi/src/expressions.rs

+        Expr::binary(
+            BinaryOperator::In,
+            Expr::literal(Scalar::Integer(0)),
+            Expr::literal(Scalar::Long(0)),
+        ),
+        Expr::binary(
+            BinaryOperator::Plus,
+            Expr::literal(Scalar::Integer(0)),
+            Expr::literal(Scalar::Long(0)),
+        ),
+        Expr::binary(
+            BinaryOperator::Minus,
+            Expr::literal(Scalar::Integer(0)),
+            Expr::literal(Scalar::Long(0)),
+        ),
+        Expr::binary(
+            BinaryOperator::Equal,
+            Expr::literal(Scalar::Integer(0)),
+            Expr::literal(Scalar::Long(0)),
+        ),
+        Expr::binary(
+            BinaryOperator::NotEqual,
+            Expr::literal(Scalar::Integer(0)),
+            Expr::literal(Scalar::Long(0)),
+        ),
+        Expr::binary(
+            BinaryOperator::NotIn,
+            Expr::literal(Scalar::Integer(0)),
+            Expr::literal(Scalar::Long(0)),
+        ),
+        Expr::binary(
+            BinaryOperator::Divide,
+            Expr::literal(Scalar::Integer(0)),
+            Expr::literal(Scalar::Long(0)),
+        ),
+        Expr::binary(
+            BinaryOperator::Multiply,
+            Expr::literal(Scalar::Integer(0)),
+            Expr::literal(Scalar::Long(0)),
+        ),
+        Expr::binary(
+            BinaryOperator::LessThan,
+            Expr::literal(Scalar::Integer(0)),
+            Expr::literal(Scalar::Long(0)),
+        ),
+        Expr::binary(
+            BinaryOperator::LessThanOrEqual,
+            Expr::literal(Scalar::Integer(0)),
+            Expr::literal(Scalar::Long(0)),
+        ),
+        Expr::binary(
+            BinaryOperator::GreaterThan,
+            Expr::literal(Scalar::Integer(0)),
+            Expr::literal(Scalar::Long(0)),
+        ),
+        Expr::binary(
+            BinaryOperator::GreaterThanOrEqual,
+            Expr::literal(Scalar::Integer(0)),
+            Expr::literal(Scalar::Long(0)),
+        ),
+        Expr::binary(
+            BinaryOperator::Distinct,
+            Expr::literal(Scalar::Integer(0)),
+            Expr::literal(Scalar::Long(0)),
+        ),


Move this to the end and then do something like:

vec.extend( [ BinaryOperator::In, BinaryOperator::Plus, BinaryOperator::Minus, BinaryOperator::Equal, BinaryOperator::NotEqual, BinaryOperator::NotIn, BinaryOperator::Divide, BinaryOperator::Multiply, BinaryOperator::LessThan, BinaryOperator::LessThanOrEqual, BinaryOperator::GreaterThan, BinaryOperator::GreaterThanOrEqual, BinaryOperator::Distinct, ] .iter() .map(|op| { Expr::binary( op, Expr::literal(Scalar::Integer(0)), Expr::literal(Scalar::Long(0)), ) }), )

nicklan · 2024-10-10T23:32:48Z

ffi/src/expressions.rs

+    /// Visit a `struct` which is constructed from an ordered list of expressions. This declares
+    /// the number of expressions that the struct will be made of. The visitor will populate the
+    /// list of expressions using the [`visit_struct_sub_expr`] method.
+    pub visit_struct: extern "C" fn(data: *mut c_void, len: usize) -> usize,


I think I'd call it visit_struct_expr to make it clear

nicklan · 2024-10-10T23:33:25Z

ffi/src/expressions.rs

+            ($visitor.$visitor_fn)($visitor.data $(, $extra_args) *)
+        };
+    }
+    fn visit_array(visitor: &mut EngineExpressionVisitor, array: &ArrayData) -> usize {


are functions defined in an unsafe function themselves unsafe, or are they safe? :)

nicklan · 2024-10-10T23:33:35Z

ffi/src/expressions.rs

+        };
+    }
+    fn visit_array(visitor: &mut EngineExpressionVisitor, array: &ArrayData) -> usize {
+        #[allow(deprecated)]


ffi/src/expressions.rs

nicklan · 2024-10-10T23:34:50Z

ffi/src/expressions.rs

+    }
+    fn visit_expression(visitor: &mut EngineExpressionVisitor, expression: &Expression) -> usize {
+        match expression {
+            Expression::Literal(scalar) => visit_scalar(visitor, scalar),


which? i'm not totally clear which is best :)

nicklan · 2024-10-10T23:38:35Z

ffi/examples/read-table/expression.h

+typedef struct
+{
+  size_t len;
+  ExpressionRef handles[100];


Let's prefer not to call these handles, which has a very specific meaning from the rust handle.rs stuff. expr_refs maybe? Change names below as well.

Also probably best to just make it an ExpressionRef * and realloc it if you need it to grow. Otherwise you have a lurking bug below where >100 expressions will be bad news.

nicklan · 2024-10-10T23:41:03Z

ffi/examples/read-table/expression.h

+  uintptr_t schema_list_id = visit_expression(&predicate, &visitor);
+  return data.handles[schema_list_id];


Suggested change

uintptr_t schema_list_id = visit_expression(&predicate, &visitor);

return data.handles[schema_list_id];

uintptr_t expr_id = visit_expression(&predicate, &visitor);

return data.handles[expr_id];

OussamaSaoudi-db · 2024-10-11T02:01:08Z

Nice thanks, lots of work!

One major comment, I think it might be better to model this the same we do schemas where each visit_X call gets a sibling_list_id which tells it which list of expressions it should append itself to. Then you don't need all the visit_struct_sub_expr methods, you can just do something like:
fn visit_struct(visitor: &mut EngineExpressionVisitor, exprs: &Vec<Expression>) -> usize {
  let child_list_id = visitor.make_expr_list(exprs.len());
  for expr in exprs {
    visit_expression(visitor, expr, child_list_id);
  }
  visit_struct(visitor.data, child_list_id)
}
There is a subtle difference that visit_schema returns a list id at the end because we know a schema is always a list of struct fields at the top. Here we could be visiting just one expression, but we could just define that the top level always returns a single element list, or figure out how to return the top level expr id directly.

I realize that's a somewhat big change so open to pushback :)

Yeah it didn't make sense to me initially to have a one-size top level/one-size array for unary. But this cuts down on functions for visiting complex types, and fewer functions >> avoiding these allocations. I went ahead and implemented sibling_id based. Seems to pass the test, but I need to clean up a bit.

This could also fit nicely with the schema code for visiting structs and nulls with DataType.

scovich reviewed Sep 28, 2024

View reviewed changes

ffi/src/expressions.rs Outdated Show resolved Hide resolved

OussamaSaoudi-db added 9 commits October 2, 2024 14:56

Move ffi code, add kernel expr -> engine expr

6e1bbf1

FFI code for schema and expressions have been moved to their own modules. Add prototype of the visitor to enable kernel expresison -> engine expression

Remove new visitor

b367df1

Add kernel expression to engine expression

1bc700a

Add ffi validation and more visitor features

dc1ed23

initial type expansion

8ce6338

Add new impls for visitor

384056b

Add even more expression visitor stuff

10b6534

Patch expr_struct

e7aa687

Fix ffi test

57fdf77

OussamaSaoudi-db force-pushed the expresions_visitor2 branch from 1814c84 to 57fdf77 Compare October 2, 2024 21:56

OussamaSaoudi-db added 7 commits October 2, 2024 15:05

Fix clippy issues

27736a0

Add support for structs, arrays, null

5ff4396

Improve literal type system

bacc7b2

Remove cache files

318453e

Add Remaining expression visitor functionality

5217dc6

Add deconstructor and make functions consistently named

778c84c

More fixes to expression

07c458b

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

OussamaSaoudi-db added 8 commits October 9, 2024 16:36

Fix all memory leaks

23d1ed2

Merge branch 'main' into expresions_visitor2

ccf47b2

Remove clang cache

c66fb3e

Fix safety, remove unnecessary diffs

45c2a99

Add docs to visit_expression

097fe8b

Add expressions test

4b0ab65

Add docs, improve style

1af2939

Fix spacing

4df19e3

OussamaSaoudi-db changed the title ~~[WIP] Add visitor for converting kernel expressions to engine expressions~~ Add visitor for converting kernel expressions to engine expressions Oct 10, 2024

OussamaSaoudi-db requested a review from nicklan October 10, 2024 22:03

OussamaSaoudi-db requested a review from zachschuermann October 10, 2024 22:03

OussamaSaoudi-db added 4 commits October 10, 2024 15:06

Fix diff, move test to beginning of read_table

283594b

Fix cross-platform printing

57271fb

Merge branch 'main' into expresions_visitor2

71cdf3b

Improve C error handling, remove kernel expression from main

a24419d

OussamaSaoudi-db commented Oct 10, 2024

View reviewed changes

Remove changes from read_table.c

11fc68a

OussamaSaoudi-db marked this pull request as ready for review October 10, 2024 23:10

remove unnecessary changes

c3160ad

OussamaSaoudi-db commented Oct 10, 2024

View reviewed changes

ffi/src/expressions.rs Outdated Show resolved Hide resolved

nicklan reviewed Oct 10, 2024

View reviewed changes

OussamaSaoudi-db requested a review from scovich October 10, 2024 23:18

nicklan reviewed Oct 10, 2024

View reviewed changes

Move testing code into separate executable

295e8e8

nicklan requested changes Oct 11, 2024

View reviewed changes

OussamaSaoudi-db added 2 commits October 10, 2024 17:11

Revamp testing

b069cf1

Move to sibling-list_based approach

69e92f4

Fix free

c8f80a3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add visitor for converting kernel expressions to engine expressions #363

Add visitor for converting kernel expressions to engine expressions #363

OussamaSaoudi-db commented Sep 27, 2024 •

edited

Loading

codecov bot commented Sep 27, 2024 •

edited

Loading

OussamaSaoudi-db Oct 10, 2024

nicklan Oct 10, 2024

OussamaSaoudi-db Oct 10, 2024

OussamaSaoudi-db Oct 10, 2024

OussamaSaoudi-db Oct 10, 2024

OussamaSaoudi-db Oct 10, 2024

nicklan Oct 10, 2024

nicklan left a comment

nicklan Oct 10, 2024

nicklan Oct 10, 2024

OussamaSaoudi-db Oct 10, 2024

nicklan Oct 10, 2024

nicklan Oct 10, 2024

OussamaSaoudi-db Oct 10, 2024

nicklan Oct 10, 2024

nicklan Oct 10, 2024

nicklan Oct 10, 2024

nicklan Oct 10, 2024

nicklan left a comment

nicklan Oct 10, 2024

nicklan Oct 10, 2024

nicklan Oct 10, 2024

nicklan Oct 10, 2024

nicklan Oct 10, 2024

nicklan Oct 10, 2024

nicklan Oct 10, 2024

nicklan Oct 10, 2024

OussamaSaoudi-db commented Oct 11, 2024

		uintptr_t schema_list_id = visit_expression(&predicate, &visitor);
		return data.handles[schema_list_id];

Add visitor for converting kernel expressions to engine expressions #363

Are you sure you want to change the base?

Add visitor for converting kernel expressions to engine expressions #363

Conversation

OussamaSaoudi-db commented Sep 27, 2024 • edited Loading

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

nicklan 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

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

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

OussamaSaoudi-db commented Oct 11, 2024

OussamaSaoudi-db commented Sep 27, 2024 •

edited

Loading

codecov bot commented Sep 27, 2024 •

edited

Loading