Skip to content

FFI and Julia Bindings for incremental scan changes#6

Merged
vustef merged 12 commits intomainfrom
vs-incremental-ffi-and-bindings
Nov 4, 2025
Merged

FFI and Julia Bindings for incremental scan changes#6
vustef merged 12 commits intomainfrom
vs-incremental-ffi-and-bindings

Conversation

@vustef
Copy link
Copy Markdown
Collaborator

@vustef vustef commented Oct 31, 2025

FFI and Julia Bindings for incremental scan changes that were introduced with RelationalAI/iceberg-rust#3.

I refactored the file structures a bit, for both Rust and Julia code. Rust code reuses some common parts through macros, for Julia I didn't bother to do that (mostly afraid of macros and ccall interaction being a rabbit hole with little benefit).
Note that I had to use struct instead of const for ScanRef, since now with additional type, we have method overloads, which if we use ScanRef const aliases actually use same type, and then become overwrites instead of overloads.

There's also a new test data, and new test that exercises positional delete and inserts.

@vustef vustef requested review from gbrgr and mjschleich October 31, 2025 13:41
Copy link
Copy Markdown
Contributor

@gbrgr gbrgr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Vukasin, just posting a first round of comments

Comment thread src/full.jl Outdated
Comment thread src/full.jl
Comment thread test/runtests.jl Outdated
Comment thread test/runtests.jl Outdated
Comment thread test/runtests.jl Outdated
Comment thread test/runtests.jl
}

// Use macros from scan_common for shared functionality
impl_select_columns!(iceberg_select_columns, IcebergScan);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, I think it is fine to use macros. However, in general macros should also be sometimes avoided, so we could go instead with traits for the builder + scan, and a generic ScanBase<B: BuilderTrait, S: ScanTrait> variant, if we want to keep it more extensible.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, agree it's better not to rely on macros in future, thanks for pointing out.

vustef and others added 4 commits November 4, 2025 11:07
Co-authored-by: Gerald Berger <59661379+gbrgr@users.noreply.github.com>
Co-authored-by: Gerald Berger <59661379+gbrgr@users.noreply.github.com>
Co-authored-by: Gerald Berger <59661379+gbrgr@users.noreply.github.com>
@vustef vustef requested a review from gbrgr November 4, 2025 10:13
Copy link
Copy Markdown
Contributor

@gbrgr gbrgr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Vukasin

Comment thread test/runtests.jl
@vustef vustef enabled auto-merge (squash) November 4, 2025 10:42
@vustef vustef merged commit 5628293 into main Nov 4, 2025
2 checks passed
@vustef vustef deleted the vs-incremental-ffi-and-bindings branch November 4, 2025 10:43
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 this pull request may close these issues.

2 participants