andygrove commented on PR #5275:
URL:
https://github.com/apache/datafusion-comet/pull/5275#issuecomment-5195567041
Flagging one ordering risk with moving `load()` out of the static
initializer, since it's subtle and not visible in the diff.
`load()` calls `setArrowProperties()`, which sets
`arrow.enable_unsafe_memory_access` and `arrow.enable_null_check_for_get`.
Arrow reads both in **static initializers** (`BoundsChecking`,
`NullCheckingForGet`), so they only take effect if they're set before the
corresponding Arrow class is initialized. Setting them afterwards is silently
ignored — no warning, no error, just the non-default behavior quietly
persisting.
That was previously guaranteed by `NativeBase`'s static block firing on
first touch of the class. After this PR the properties are set at the first
`new Native()` instead, so correctness now depends on `new Native()` running
before any Arrow vector work on every path, on every executor.
Small repro against arrow-vector 19.0.0 showing the latch is real:
```java
// vectorFirst: touch an IntVector, then set the property
IntVector v = new IntVector("x", allocator);
v.allocateNew(1); v.set(0, 1); v.get(0); v.close();
System.setProperty("arrow.enable_null_check_for_get", "false");
// -> NULL_CHECKING_ENABLED = true (property ignored)
// propFirst: set the property, then touch the vector
// -> NULL_CHECKING_ENABLED = false (property honored)
```
To be clear about scope, two things bound this:
- Comet relocates Arrow to `org.apache.comet.shaded.arrow`, so Spark's own
Arrow usage can't latch Comet's copy. Only Comet code touching Comet's Arrow
matters.
- In the paths I checked the ordering does still hold — e.g.
`CometExecIterator` and `CometBlockStoreShuffleReader` both construct `new
Native()` immediately before `new NativeUtil()`.
So I'm not claiming a live bug. The concern is that a previously structural
guarantee has become an incidental one that nothing enforces or documents: any
future path that touches Arrow before instantiating `Native` would silently
lose these settings, and the symptom would be a quiet performance regression
rather than a failure. Worth either a comment at the `new Native()` sites
noting the ordering is load-bearing, or setting the Arrow properties somewhere
that doesn't depend on native-library load at all — they're pure JVM system
properties and don't actually need the library.
Two smaller notes while I was in here:
- `ensureLoaded()` latches `loadErr` permanently and `setLoaded()` doesn't
clear it, which breaks it as a test reset. `CometSparkSessionExtensionsSuite`
fails on this branch — the `isCometLoaded` test sets `os.name=foo` to force a
load failure, caches the throwable, and the next test (`isCometLoaded requires
CometShuffleManager when shuffle.enabled=true`) then rethrows the stale error
and fails with `was false`. I ran it locally with the native lib built: 7/7 on
`main`, 6/7 here, and 7/7 again after adding `loadErr = null;` to `setLoaded`.
- The description says `Closes #5724`, but that issue doesn't exist — I
think you want #5274, otherwise it won't auto-close.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
