-
Notifications
You must be signed in to change notification settings - Fork 39
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 #363
Merged
OussamaSaoudi-db
merged 87 commits into
delta-incubator:main
from
OussamaSaoudi-db:expresions_visitor2
Oct 28, 2024
Merged
Changes from all commits
Commits
Show all changes
87 commits
Select commit
Hold shift + click to select a range
6e1bbf1
Move ffi code, add kernel expr -> engine expr
OussamaSaoudi-db b367df1
Remove new visitor
OussamaSaoudi-db 1bc700a
Add kernel expression to engine expression
OussamaSaoudi-db dc1ed23
Add ffi validation and more visitor features
OussamaSaoudi-db 8ce6338
initial type expansion
OussamaSaoudi-db 384056b
Add new impls for visitor
OussamaSaoudi-db 10b6534
Add even more expression visitor stuff
OussamaSaoudi-db e7aa687
Patch expr_struct
OussamaSaoudi-db 57fdf77
Fix ffi test
OussamaSaoudi-db 27736a0
Fix clippy issues
OussamaSaoudi-db 5ff4396
Add support for structs, arrays, null
OussamaSaoudi-db bacc7b2
Improve literal type system
OussamaSaoudi-db 318453e
Remove cache files
OussamaSaoudi-db 5217dc6
Add Remaining expression visitor functionality
OussamaSaoudi-db 778c84c
Add deconstructor and make functions consistently named
OussamaSaoudi-db 07c458b
More fixes to expression
OussamaSaoudi-db 23d1ed2
Fix all memory leaks
OussamaSaoudi-db ccf47b2
Merge branch 'main' into expresions_visitor2
OussamaSaoudi-db c66fb3e
Remove clang cache
OussamaSaoudi-db 45c2a99
Fix safety, remove unnecessary diffs
OussamaSaoudi-db 097fe8b
Add docs to visit_expression
OussamaSaoudi-db 4b0ab65
Add expressions test
OussamaSaoudi-db 1af2939
Add docs, improve style
OussamaSaoudi-db 4df19e3
Fix spacing
OussamaSaoudi-db 283594b
Fix diff, move test to beginning of read_table
OussamaSaoudi-db 57271fb
Fix cross-platform printing
OussamaSaoudi-db 71cdf3b
Merge branch 'main' into expresions_visitor2
OussamaSaoudi-db a24419d
Improve C error handling, remove kernel expression from main
OussamaSaoudi-db 11fc68a
Remove changes from read_table.c
OussamaSaoudi-db c3160ad
remove unnecessary changes
OussamaSaoudi-db 295e8e8
Move testing code into separate executable
OussamaSaoudi-db b069cf1
Revamp testing
OussamaSaoudi-db 69e92f4
Move to sibling-list_based approach
OussamaSaoudi-db c8f80a3
Fix free
OussamaSaoudi-db 006adfb
fix visit_struct naming
OussamaSaoudi-db 30e0596
Fix formtting and style in test expr
OussamaSaoudi-db aef2928
Remove KernelStringSlice from c side code
OussamaSaoudi-db 3cfeb2a
Move to using allocate_string
OussamaSaoudi-db f2d4cf4
Remove read_table dependency in expresison
OussamaSaoudi-db 15e52eb
Fix tests, change enum naming to be clearer
OussamaSaoudi-db 595280f
Use system header for assert
OussamaSaoudi-db 7e7c847
rename literals
OussamaSaoudi-db a6ddd40
fix memory leaks
OussamaSaoudi-db 24dbce4
Revert read_table
OussamaSaoudi-db 9019da1
Improve naming
OussamaSaoudi-db 406cfe7
Put in documentation
OussamaSaoudi-db cfb1ed7
Change arg order
OussamaSaoudi-db fe339d1
Remove unnecessary change
OussamaSaoudi-db ba3153a
Fix grammar
OussamaSaoudi-db 1b8575c
Move test function to separate module
OussamaSaoudi-db 6cfe39f
Add feature flag to test
OussamaSaoudi-db bd2332c
Make sibling id second arg always
OussamaSaoudi-db 046147d
Test binary data
OussamaSaoudi-db a2d0dbf
move abort to assert
OussamaSaoudi-db d46b4e3
Small cosmetic changes
OussamaSaoudi-db 75a84a3
Address PR comments
OussamaSaoudi-db d8250ac
Remove unneccessary changes
OussamaSaoudi-db 748c45a
better naming
OussamaSaoudi-db 8f1b2eb
Improve doc comment
OussamaSaoudi-db d67d7d1
Spacing
OussamaSaoudi-db 05e95dc
Remove leaks test
OussamaSaoudi-db 22b8896
Rename print_tree, take out implementation detail of lists from examples
OussamaSaoudi-db 539d9c6
Address PR comments
OussamaSaoudi-db 456d727
More pr addressing
OussamaSaoudi-db 5c1a2f8
Simplify Variadic and Unary
OussamaSaoudi-db ffc3d51
Fix build error
OussamaSaoudi-db 0c88b31
fix build
OussamaSaoudi-db 9a73536
Fix kernel issue
OussamaSaoudi-db 3683a85
Follow PR recommendations
OussamaSaoudi-db 3f37450
Fix typo
OussamaSaoudi-db 6abdd78
Merge branch 'main' into expresions_visitor2
OussamaSaoudi-db 03c1440
Fix rebase issue
OussamaSaoudi-db bdd28d7
Rename the expressions modules
OussamaSaoudi-db 14a5c15
Apply review comments
OussamaSaoudi-db c22e311
Address comments
OussamaSaoudi-db 49f6198
Try windows without MSVC
OussamaSaoudi-db 633a4ba
Fix wording
OussamaSaoudi-db 4faff29
Add dev visibility
OussamaSaoudi-db 984b615
Flushing changes
OussamaSaoudi-db 8530b8d
Address pr comments
OussamaSaoudi-db 306c982
Merge branch 'main' into expresions_visitor2
OussamaSaoudi-db bbac1bb
Fix rebase issue
OussamaSaoudi-db 00d0354
hopefully fix linux leak test
OussamaSaoudi-db d282063
Remove failing test
OussamaSaoudi-db d0218fd
Renaming
OussamaSaoudi-db 60049ab
Small nit
OussamaSaoudi-db abcf590
Merge branch 'main' into expresions_visitor2
OussamaSaoudi-db File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,3 +47,4 @@ default-engine = [ | |
] | ||
sync-engine = ["delta_kernel/sync-engine"] | ||
developer-visibility = [] | ||
test-ffi = [] |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
cmake_minimum_required(VERSION 3.12) | ||
project(visit_expressions) | ||
|
||
add_executable(visit_expression visit_expression.c) | ||
target_compile_definitions(visit_expression PUBLIC DEFINE_DEFAULT_ENGINE) | ||
target_include_directories(visit_expression PUBLIC "${CMAKE_CURRENT_SOURCE_DIR}/../../../target/ffi-headers") | ||
target_link_directories(visit_expression PUBLIC "${CMAKE_CURRENT_SOURCE_DIR}/../../../target/debug") | ||
target_link_libraries(visit_expression PUBLIC delta_kernel_ffi) | ||
target_compile_options(visit_expression PUBLIC) | ||
|
||
target_compile_options(visit_expression PRIVATE -Wall -Wextra -Wpedantic -Werror -Wno-strict-prototypes) | ||
|
||
# Get info on the OS and platform | ||
if (${CMAKE_SYSTEM_NAME} MATCHES "Darwin") | ||
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. @zachschuermann just wanted to have a second look. Slightly yucky, but it's the way I found to target linux for valgrind. Note that macos does not support valgrind. |
||
set(MACOSX TRUE) | ||
endif() | ||
if(UNIX AND NOT APPLE) | ||
set(LINUX TRUE) | ||
endif() | ||
|
||
# Add the kernel expresion -> engine expression test | ||
include(CTest) | ||
set(ExprTestRunner "../../../tests/test-expression-visitor/run_test.sh") | ||
set(ExprExpectedPath "../../../tests/test-expression-visitor/expected.txt") | ||
add_test(NAME test_expression_visitor COMMAND ${ExprTestRunner} ${ExprExpectedPath}) | ||
if(LINUX) | ||
add_test(NAME test_expression_visitor_leaks COMMAND valgrind | ||
--error-exitcode=1 | ||
--tool=memcheck | ||
--leak-check=full | ||
--errors-for-leak-kinds=definite | ||
--show-leak-kinds=definite ./visit_expression) | ||
endif() |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
does this run in parallel with the first (low prio)?
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.
Nope, this is sequential.