yihua commented on code in PR #834:
URL: https://github.com/apache/hudi-rs/pull/834#discussion_r4393838856
##########
crates/jvm-ffi/Cargo.toml:
##########
@@ -25,7 +25,7 @@ version.workspace = true
edition.workspace = true
license.workspace = true
rust-version.workspace = true
-description = "Plain C ABI over hudi-rs, so a JVM can call it through the
Arrow C Data Interface."
+description = "Plain C ABI over hudi-rs, so a JVM or a C++ engine can call it
through the Arrow C Data Interface."
homepage.workspace = true
Review Comment:
With this PR the main planned consumer is a C++ engine (the DuckDB
extension), so `jvm` in the crate and library name no longer fits. Since the
crate is `publish = false` and the exported symbols are already `hudi_ffi_*`, I
think we should rename it to `hudi-ffi` (lib `hudi_ffi`, `crates/ffi`) before
anything depends on it; the only break is a JNA caller loading
`libhudi_jvm_ffi` by name. @linliu-code, does the Gluten prototype load it by
that name?
##########
crates/jvm-ffi/Cargo.toml:
##########
@@ -41,4 +41,8 @@ ffi-test-hooks = []
[dependencies]
hudi = { path = "../hudi", version = "0.6.0-dev" }
Review Comment:
This takes `hudi` with default features, so a consumer gets `spill-rocksdb`
(RocksDB, bindgen, libclang) with no way to opt out, since it can only drop a
feature this crate exposes. It's worth forwarding it the way `hudi-datafusion`
and `hudi` do: `hudi = { ..., default-features = false }` plus `default =
["spill-rocksdb"]` and `spill-rocksdb = ["hudi/spill-rocksdb"]`, so the DuckDB
extension can make that call itself.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
