refactor(rust/core)!: move the ffi related stuff to the new adbc_ffi package #3381

eitsupi · 2025-08-31T13:10:33Z

Move the adbc_core::ffi module, except for adbc_core::ffi::constants and adbc_core::types::FFI_AdbcStatusCode, to the new adbc_ffi package.

And, also rename the two that were not moved:

Rename adbc_core::ffi::constants to adbc_core::constants
Rename adbc_core::types::FFI_AdbcStatusCode to adbc_core::error::AdbcStatusCode

…to `constants`

eitsupi · 2025-08-31T14:04:20Z

CC @mbrobbel @felipecrv

mbrobbel

Looks good! Just some minor question/suggestions from me.

We should also add this crate to

arrow-adbc/dev/release/post-08-rust.sh

Lines 44 to 46 in dd58077

    
           cargo publish --all-features -p adbc_core 
        
           cargo publish --all-features -p adbc_datafusion 
        
           cargo publish --all-features -p adbc_snowflake

.

rust/core/src/error.rs

rust/core/src/options.rs

rust/driver/datafusion/Cargo.toml

rust/ffi/src/lib.rs

rust/core/src/lib.rs

rust/core/src/error.rs

rust/core/src/options.rs

rust/driver_manager/src/lib.rs

rust/ffi/src/lib.rs

lidavidm · 2025-08-31T23:37:07Z

rust/core/src/error.rs


+use crate::constants;
+
+pub type AdbcStatusCode = u8;


I guess we can't move this to the FFI crate because the TryFrom implementation has to be in this crate?

We could move it and provide a TryInto impl?

As you guessed, I got stuck after my first attempt to move FFI_AdbcStatusCode to adbc_ffi and had to restart by moving it to another module, just like the first commit in this PR (7ff5fb5).

I believe it's better to keep this in adbc_core for the following reasons:

To implement TryFrom / From.

It's referenced from the constants mod.

This status code doesn't include FFI.

Just return u8 on the FFI side. We only need u8 to AdbcStatus Into. Let's not have 2 layers of conversions for an enum.

As I wrote in the comments below, I'm wondering if this should be placed in the constants mod since it's defined in adbc.h.
#3381 (comment)

Fine by me. I just don't want another layer of conversion/indirection.

paleolimbot

I don't have much to add here...I think separating these is great!

rust/core/src/error.rs

eitsupi

Thank you everyone for your quick comments. I'll update later.

eitsupi · 2025-09-01T13:40:36Z

rust/core/src/error.rs


+use crate::constants;
+
+pub type AdbcStatusCode = u8;


As you guessed, I got stuck after my first attempt to move FFI_AdbcStatusCode to adbc_ffi and had to restart by moving it to another module, just like the first commit in this PR (7ff5fb5).

I believe it's better to keep this in adbc_core for the following reasons:

To implement TryFrom / From.

It's referenced from the constants mod.

This status code doesn't include FFI.

rust/driver_manager/src/lib.rs

rust/ffi/src/lib.rs

felipecrv

I'm in favor of merging this PR as it is. It's in a good state and the pending suggestions seem unrelated to the task of extracting the FFI module.

lidavidm

I think this is reasonable as-is. I'll merge later today unless there's objections

The driver exporter has been moved from `adbc_core` to `adbc_ffi` in #3381; however, the README remains unchanged. I found this discrepancy when upgrading the `adbc_core` dependency of [SedonaDB](https://github.com/apache/sedona-db). This out-dated information also appears in https://crates.io/crates/adbc_core and made me frustrated, so I decided to submit a PR containing this very tiny fix.

eitsupi added 11 commits August 31, 2025 07:13

refactor(rust/core)!: rename FFI_AdbcStatusCode to AdbcStatusCode

7ff5fb5

refactor(rust): add the new adbc_ffi package

02bdff2

refactor(rust/driver_manager): depends on adbc_ffi

ac320d5

refactor(rust/driver/snowflake): depends on adbc_ffi

624f404

refactor(rust/driver/datafusion): depends on adbc_ffi

5e98455

refactor(rust/driver/dummy): depends on adbc_ffi

5429117

fix: fix accidentally replaced lines

0c592a9

refactor: remove unused files

4a30192

style: run cargo fmt

d4abf20

refactor(rust/core)!: remove the ffi mod and move ffi::constants …

449519a

…to `constants`

docs(rust): move driver_manager and ffi docs

2bc0fb3

eitsupi marked this pull request as ready for review August 31, 2025 14:03

eitsupi requested a review from wjones127 as a code owner August 31, 2025 14:03

github-actions bot added this to the ADBC Libraries 20 milestone Aug 31, 2025

mbrobbel approved these changes Aug 31, 2025

View reviewed changes

rust/core/src/error.rs Show resolved Hide resolved

rust/core/src/error.rs Outdated Show resolved Hide resolved

rust/core/src/options.rs Outdated Show resolved Hide resolved

rust/driver/datafusion/Cargo.toml Outdated Show resolved Hide resolved

rust/ffi/src/lib.rs Show resolved Hide resolved

felipecrv reviewed Aug 31, 2025

View reviewed changes

lidavidm reviewed Aug 31, 2025

View reviewed changes

lidavidm requested a review from paleolimbot August 31, 2025 23:37

paleolimbot approved these changes Sep 1, 2025

View reviewed changes

rust/core/src/error.rs Show resolved Hide resolved

eitsupi commented Sep 1, 2025

View reviewed changes

eitsupi added 5 commits September 1, 2025 15:24

refactor(rust/core): remove OptionValue::get_type

0c1f0a7

refactor(rust/core): remove unused impl block

88f525c

fix(rust/driver/datafusion): use the dep: syntax for the feature

a5afb8f

docs(rust/core): more informative description

c70a68e

build(release): update rust publish script to publish new packages

c1b1a9c

eitsupi requested a review from kou as a code owner September 1, 2025 15:35

felipecrv approved these changes Sep 2, 2025

View reviewed changes

lidavidm approved these changes Sep 3, 2025

View reviewed changes

lidavidm merged commit dc530d9 into apache:main Sep 3, 2025
11 checks passed

eitsupi deleted the rust-ffi branch September 3, 2025 10:23

Kontinuation mentioned this pull request Sep 29, 2025

docs: Fix macro name for exporting ADBC driver in README #3493

Merged

	cargo publish --all-features -p adbc_core
	cargo publish --all-features -p adbc_datafusion
	cargo publish --all-features -p adbc_snowflake

refactor(rust/core)!: move the ffi related stuff to the new adbc_ffi package #3381

refactor(rust/core)!: move the ffi related stuff to the new adbc_ffi package #3381

Conversation

eitsupi commented Aug 31, 2025 • edited by lidavidm Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eitsupi commented Aug 31, 2025

Uh oh!

mbrobbel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lidavidm Aug 31, 2025

Choose a reason for hiding this comment

Uh oh!

mbrobbel Sep 1, 2025

Choose a reason for hiding this comment

Uh oh!

eitsupi Sep 1, 2025

Choose a reason for hiding this comment

Uh oh!

felipecrv Sep 1, 2025

Choose a reason for hiding this comment

Uh oh!

eitsupi Sep 1, 2025

Choose a reason for hiding this comment

Uh oh!

felipecrv Sep 1, 2025

Choose a reason for hiding this comment

Uh oh!

paleolimbot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eitsupi left a comment

Choose a reason for hiding this comment

Uh oh!

eitsupi Sep 1, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

felipecrv left a comment

Choose a reason for hiding this comment

Uh oh!

lidavidm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

eitsupi commented Aug 31, 2025 •

edited by lidavidm

Loading