Repository navigation
build: use a single jni version across the native workspace - #6998
Conversation
The shuffle crate declared jni 0.21 while core, jni-bridge and spark-expr declared 0.22.4, so Cargo.lock held both and every build compiled a second copy of the binding. Shuffle only uses the jint and jlong aliases from jni::sys, which are i32 and i64 in both versions. Declare jni once in [workspace.dependencies] and inherit it in the four crates that use it. The lock drops jni 0.21.1, jni-sys 0.3.1 and the crates only they needed (cesu8, thiserror 1.0.69, windows-sys 0.45.0 and its 0.42.2 dependencies). Nothing else in the lock changes.
sunchao
left a comment
There was a problem hiding this comment.
No introduced P1/P2 issues found within this review.
Reviewed 7faafa0205c8cdafe25df53c4c04d08c280682ee against base e555dba6f5c8c62ceca8c9c6e311a51b8fbfeb70. The PR is open and not a draft. Reviewed all six files in the full PR diff and checked the additional direct base/head differences: the Iceberg changes belong to base-only commit #6962, not this PR. GitHub’s file list agrees with the merge-base diff. The snapshot and live discussion contain no reviews, conversation comments, inline comments, or threads.
Read AGENTS.md and applied review-comet-pr, review-comet-ffi-pr, review-comet-shuffle-pr, and review-comet-expression-pr. Also consulted review-comet-iceberg-write-pr while resolving the base-only differences.
Summary
- Prior state and problem: Shuffle used
jni0.21.1 while core, JNI bridge, and expressions used 0.22.4, retaining duplicate bindings and their dependencies. - Design approach: Declare
jni = "0.22.4"once in workspace dependencies and inherit it in all four consumers. - Correctness: Shuffle’s only direct JNI imports are
jintandjlong. Checked both upstream bindings: these remaini32andi64. Traced the unchanged JVM array-to-row-converter path and compared Spark’sUnsafeRowaddress/size contract across 3.4.3, 3.5.9, 4.0.4, 4.1.3, and 4.2.0. No changed semantics were identified. - Compatibility analysis: Core, JNI bridge, and expressions retain their existing JNI version and features. Latest release branch
branch-1.1has the same pre-change version split. This consolidation introduces no Spark-version-specific API or runtime requirement beyond what the workspace already uses. - Key design decisions: Keep
invocationenabled specifically in core and JNI bridge dev-dependencies; preserve the existing empty default feature set. - Implementation sketch: Five manifest edits centralize the dependency; the lockfile removes 14 packages. Parsed comparison confirms no added packages or changed retained checksums, and only shuffle’s resolved JNI dependency changes.
- Performance: Removing the duplicate dependency graph eliminates redundant build inputs. No runtime allocation, copying, JNI-call, or shuffle-algorithm changes were introduced. Build-time savings were not benchmarked.
- Design: Workspace dependency inheritance is a straightforward fit for keeping these internal consumers aligned. No additional coordination mechanism is needed for this change.
- Abstraction & complexity: The shared declaration replaces repeated version strings without adding a wrapper or runtime abstraction. Existing dependency conventions remain intact.
- Behavioral changes worth calling out: Dependency resolution now produces one
jniand onejni-sysversion. Row representation, shuffle format, expression behavior, and FFI ownership remain unchanged by this PR. - Suggested improvements: No additional change meeting the P1/P2 reporting bar was identified. Existing tests exercise the affected shuffle conversion and JNI boundaries.
Exact-head CI: Comet CI run 38638128258 succeeded. The head has 26 successful checks and 18 skipped checks, with no failures. Inspected Rust logs confirm workspace Clippy passed and 2,442 tests passed, including all 187 shuffle tests; five tests were skipped. Five additional jemalloc tests passed. Linux native build, Spark 4.1 Comet suites, and TPC-H/TPC-DS verification also passed.
Local validation: cargo metadata --offline --locked, all-target dependency-tree checks, normalized lockfile comparison, and diff whitespace checks passed. No local native/JVM test build was run; runtime evidence comes from the inspected CI execution. macOS, upstream Spark SQL/Iceberg suites, and other Spark runtime profiles were not validated by this run. The checkout remains unchanged.
mbutrovich
left a comment
There was a problem hiding this comment.
Seems like a no-brainer to me. Thanks @andygrove!
Which issue does this PR close?
No issue filed.
Rationale for this change
native/shuffle/Cargo.tomldeclaredjni = "0.21", whilecore,jni-bridgeandspark-exprdeclared0.22.4. Cargo treats those as incompatible releases, soCargo.lockheld bothjni0.21.1 and 0.22.4, along withjni-sys0.3.1 and 0.4.1.The shuffle crate only uses
jni::sys::{jint, jlong}, inspark_unsafe/row.rs. Those are aliases fori32andi64in bothjni-sysversions, so nothing needed the old major. The cost was a second compile of the binding on every build (five extra crates on non-Windows targets), and a trap for later:JObjectand the env handle from the two majors are different types, so aJObjecthanded fromjni-bridgeto shuffle would not have type-checked.Each crate repeated its own version string, which is how they drifted. Declaring the version once in the workspace keeps them together.
What changes are included in this PR?
native/Cargo.tomladdsjni = "0.22.4"to[workspace.dependencies], with a comment on why it is shared.shuffle,core,jni-bridgeandspark-exprnow usejni = { workspace = true }. Theinvocationfeature thatcoreandjni-bridgeenable under[dev-dependencies]is kept asjni = { workspace = true, features = ["invocation"] }.native/Cargo.lockloses 14 packages and gains none:jni0.21.1,jni-sys0.3.1,cesu8,thiserrorandthiserror-impl1.0.69,windows-sys0.45.0 and the eight 0.42.2 crates beneath it (windows-targetsand sevenwindows_*). No remaining package changes version. Some dependency entries drop their version suffix because only one version of that crate is left.There is no Rust source change.
How are these changes tested?
No new tests, since behavior does not change.
jintandjlongare the same primitive types under both versions, and the existing tests cover the code that uses them.cargo metadata --offline --lockedpasses. Comparing the name and version of every package inCargo.lockbefore and after shows only the 14 removals.cargo treeshows onejni(0.22.4) and onejni-sys(0.4.1) for all targets.cargo clippy --all-targets --workspace -- -D warnings, the lint command CI runs, is clean on Rust 1.97.0.cargo test -p datafusion-comet-shuffle --libpasses (187 tests).cargo machetereports no unused dependencies.I did not run the
jni-bridgeandcoretests that start a JVM. The workspace entry resolves to the same version and features they had before, and the Rust test job in CI runs them.