Skip to content
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

Add visitor for converting kernel expressions to engine expressions #358

Closed
zachschuermann opened this issue Sep 25, 2024 · 0 comments · Fixed by #363
Closed

Add visitor for converting kernel expressions to engine expressions #358

zachschuermann opened this issue Sep 25, 2024 · 0 comments · Fixed by #363
Assignees

Comments

@zachschuermann
Copy link
Collaborator

  1. Move the existing schema visitor stuff to its own module (probably schema.rs)
  2. Move the existing expression visitor which does engine_predicate -> kernel_expression into an "expressions" module
  3. Add code for a visitor similar to the schema visitor but for expressions that let's us do kernel_expression -> engine_expression
OussamaSaoudi-db added a commit that referenced this issue Oct 7, 2024
This PR addresses points 1 & 2 in #358 by moving the `EngineSchemaVisitor` to
the `ffi/src/schema.rs` module, and moving the `KernelExpressionVisitor` to the
module `ffi/src/expressions.rs`.
OussamaSaoudi-db added a commit that referenced this issue Oct 28, 2024
…363)

## Rationale for the change
Engines may need to apply expressions that originate in the kernel. This
can include filter predicates such as filters for protocol and metadata
actions in a log file. Another example is logical to physical
transformations that the engine must apply, for instance to materialize
partition columns. This PR introduces a visitor framework for kernel
expressions to be converted to engine expressions so that engines can
apply their own evaluators, predicate pushdowns, and optimizations on
these expressions.

Closes: #358 
## What changes are included in this PR?
5 major changes are made in this PR:
1) A new module `ffi/src/expressions/kernel.rs` is added. This module is
responsible for converting kernel `Expression`s to the engine's
representation of expressions through an ffi-compatible visitor. The
visitor supports binary operations, scalars, struct expressions,
variadic and unary operators, and columns. All the engine has to do is
implement an `EngineExpressionVisitor` to support the conversion.
2) The `EnginePredicate` to kernel `Expression`visitor code is moved to
`ffi/src/expressions/engine.rs`. This is done to keep all ffi expression
code under the `ffi/src/expressions` module.
3) This PR also allows engines to get `SharedExpression`s, which are
handles to the kernel's `Expression` type. These are given back to the
kernel to specify which expression the engine would like to visit.
4) A new ffi example is added to showcase how the
`EngineExpressionVisitor` can be used to construct expressions. This is
a C implementation that can be found in
`ffi/examples/visit-expression/`. The example only prints an expression
that it receives from the kernel and asserts that it matches an expected
output.
5) A new testing feature flag `test-ffi` is added to the `ffi` crate.
This feature flag is used to expose testing functions found in
`ffi/src/test_ffi`. This module is currently used to return a kernel
expression `get_testing_kernel_expression` used in the example c
implementation.
<!-- Mention all the new apis for the visitor. How they work (ex:
Schema) -->
## Are these changes tested?
Two tests are added:
- I introduced a new test to validate the expression visitor functions
correctly and converts data as expected. This simply prints out the
expression tree structure and compares it to an expected result. The
kernel expression can be found in `ffi/src/test_ffi.rs`, and the
expected output is in `ffi/tests/test-expression-visitor/expected.txt`
- There is also a test that checks for memory leaks in the expression
visitor using valgrind.

## Are there any user-facing changes?
This PR introduces a public facing API for constructing expressions and
visiting them. It also adds `delta_kernel_derive` as a public export of
`kernel` crate. This PR does not break existing APIs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants