andygrove opened a new pull request, #5890:
URL: https://github.com/apache/datafusion-comet/pull/5890
## Which issue does this PR close?
Closes #5023.
This is an alternative to #5036, which fixes the same bug by pinning
`[patch.crates-io]` to a
personal GitHub fork. I raised two objections there — that an ASF release
would carry a dependency
fetched from a contributor's personal account, and that `[patch.crates-io]`
is silently ignored by
downstream consumers — and this takes the route I asked about in the second
of them: vendoring the
patched C into our own tree. #5036 should close in favour of this if we
agree on the approach;
either way the credit for the diagnosis and the C patch is @peterxcli's.
## Rationale for this change
`libhdfs` registers a pthread thread-local destructor,
`hdfsThreadDestructor`, that detaches the
current thread from the JVM whenever it finds a cached `JNIEnv`. It does
that regardless of who
attached the thread. Comet attaches each of its Tokio worker threads itself,
with
`AttachCurrentThreadAsDaemon` in `on_thread_start`, and detaches them in
`on_thread_stop`
(`native/core/src/execution/jni_api.rs`). So the ordering on any worker that
has touched HDFS is:
1. Comet attaches the thread.
2. libhdfs calls `AttachCurrentThread`, which succeeds on an
already-attached thread and hands back
the same `JNIEnv`, and caches it in its own TLS.
3. The thread stops. Comet detaches it and the JVM frees the `JNIEnv`.
4. pthread TLS destructors run. `hdfsThreadDestructor` dereferences the
freed `JNIEnv` to call
`GetJavaVM`, reads a null function pointer out of it and jumps to address
zero.
That is `SIGSEGV at pc=0x0000000000000000`, which is exactly the signature
in both symbolized
`hs_err` dumps on #5023 — @peterxcli's macOS one (`lr =
hdfsThreadDestructor+80`, `x8 = 0`) and
@comphead's Linux one (`RIP = 0x0`, `RDI` holding the `JNIEnv`). It is
[HDFS-16021](https://issues.apache.org/jira/browse/HDFS-16021), still open,
and Hadoop trunk still
has the unguarded destructor.
Two things are worth naming because they change how urgent this looks.
First, the suite that arms
it is not the suite that crashes: `ParquetReadFromFakeHadoopFsSuite` is what
routes a read through
libhdfs, but the destructor does not fire until one of those pooled threads
exits, which is
typically minutes later in `CometIcebergNativeSuite`. That is why it reads
as a random `[scans]`
flake. Second, it is not macOS-only despite the issue title — `hdfs-opendal`
is a default feature on
every platform. Scanning every failing CI run back to 2026-09-07 for a
`crash-logs` artifact on a
Linux `[scans]` job, the first is 2026-09-09 and there are none before it,
which puts the Linux
onset right after #5748 landed the explicit `on_thread_stop` detach on
2026-09-08. I can only page
CI history back that far at reasonable cost, so treat that as a strong
correlation with a matching
mechanism rather than proof.
It has taken out at least seven runs of that job since 2026-09-09, on seven
different branches, so
it is costing everyone re-runs rather than just being noise on one PR.
## What changes are included in this PR?
`native/hdfs-sys/` vendors the Apache Hadoop `libhdfs` sources that Comet
actually compiles —
`hdfs_3_3`, POSIX only — with the HDFS-16021 fix applied, and
`native/Cargo.toml` substitutes it for
the crates.io crate with `[patch.crates-io] hdfs-sys = { path = "hdfs-sys"
}`. A path source, so
there is no personal fork, no network dependency at build time, and a source
tarball stays
self-contained.
Three C files differ from Hadoop's originals, each carrying a notice at the
top of the file:
`os/thread_local_storage.h` gains an `attachedByLibhdfs` flag on `struct
ThreadLocalState`;
`jni_helper.c` has `getGlobalJNIEnv` report whether it attached the thread;
and
`os/posix/thread_local_storage.c` guards the detach on that flag and
initialises two fields
`threadLocalStorageCreate` previously left holding `malloc` garbage.
The first and third are the HDFS-16021 patch. The second also adds a
`GetEnv` call before
`AttachCurrentThread`, which is not in the JIRA patch, and that is the part
that actually matters
here: `AttachCurrentThread` returns `JNI_OK` on an already-attached thread,
so without the `GetEnv`
early-out libhdfs would still record itself as the owner of an attachment
Comet made and we would
crash exactly as before. Worth knowing if we ever re-sync with upstream.
`src/lib.rs` and `build.rs` are written for Comet rather than copied — see
the licensing section.
`dev/ci/compute-changes.py` gains a `native/hdfs-sys/**` entry in the eight
Spark-tier filters,
because the vendored C is compiled into `libcomet` but lives outside any
`src/` directory and so
would not have matched `native/**/src/**`.
The same C changes are proposed upstream as
[Xuanwo/hdfs-sys#47](https://github.com/Xuanwo/hdfs-sys/pull/47), open since
July and unreviewed.
`native/hdfs-sys/README.md` records the removal condition.
## Licensing considerations
**We do not need IP clearance.** The [IP Clearance
process](https://incubator.apache.org/ip-clearance/) is for donations of
externally-developed code
to the ASF, where the Foundation takes on ownership and a Software Grant
Agreement is involved.
Vendoring third-party Apache-2.0 code that we redistribute under its
existing license is *bundling*,
governed by
[legal/resolved.html](https://www.apache.org/legal/resolved.html) — ALv2 is
Category A,
always permitted in ASF source and binary releases — and by
[licensing-howto.html](https://infra.apache.org/licensing-howto.html) for
the LICENSE/NOTICE work.
This case is easier than generic Category A, because all 15 vendored C/H
files carry the standard
ASF license header. They are Apache Hadoop's own sources, already ASF-owned,
copied verbatim.
Moving ALv2 ASF code between ASF projects needs no clearance; we just must
not strip the headers,
and we have not.
**We have done this twice before.** #1377 (Feb 2025) brought
[datafusion-contrib/datafusion-objectstore-hdfs](https://github.com/datafusion-contrib/datafusion-objectstore-hdfs)
into `native/hdfs/` and added a NOTICE.txt stanza for it. #2062 (Aug 2025)
then vendored
[datafusion-contrib/fs-hdfs](https://github.com/datafusion-contrib/fs-hdfs)
into `native/fs-hdfs/`,
including `c_src/libhdfs/` — the same Hadoop `hdfs.c`, `jni_helper.c` and
`os/posix/thread_local_storage.c` this PR copies — with a `LICENSE.txt`, a
provenance `README.md`
and a second NOTICE.txt stanza. Both were removed in #4904 when we moved to
the OpenDAL path. This
PR follows that established pattern.
**What is copied, and what deliberately is not.** Only the Hadoop C. The
upstream `hdfs-sys` crate
is 244 files and 3 MB, but thirteen of its fourteen vendored libhdfs trees
are Hadoop versions we
never build; `hdfs_3_3` is 21 files, and dropping the Windows platform layer
leaves 15. We also do
not take the bundled `libdirent` (MIT, Toni Ronkko), which is Windows-only —
we ship no Windows
native artifacts, so nothing in this directory is under any license other
than ALv2.
We also do not copy the crate's *Rust*. That is a deliberate choice:
`hdfs-sys` has no LICENSE file
in its repository or its published crate, and its README links a `./LICENSE`
that 404s, so the only
license grant is the `license = "Apache-2.0"` field in `Cargo.toml`. That is
unremarkable for a Rust
crate but not what I would want to lean on when bundling into an ASF
release. We can avoid the
question entirely, because the only consumer in our graph is `hdrs`, which
uses about two dozen
symbols. So `src/lib.rs` is under 200 lines of `extern "C"` declarations
transcribed from the vendored
`hdfs.h` — which is Hadoop's own header, already in the tree — and
`build.rs` is a `cc::Build`
invocation following the file list in Hadoop's `CMakeLists.txt`. Both are
Comet-authored and carry
ASF headers. The result is that `native/hdfs-sys/` contains no
third-party-authored code at all.
**Attribution.** NOTICE.txt gains a stanza naming Apache Hadoop and pointing
at
`native/hdfs-sys/README.md`, matching the form of the existing Gluten entry
and of the two prior
HDFS stanzas. The three modified files carry the "this file was modified by"
notice that section
4(b) of the license requires. I did *not* add a `LICENSE.txt` inside
`native/hdfs-sys/` the way
#2062 did, because unlike `fs-hdfs3` this directory is not a third-party
crate with its own
licensing — it is ASF code under the same license as the rest of the repo,
and the root LICENSE.txt
covers it. Happy to add one if a reviewer would rather we were
belt-and-braces.
**RAT passes** with `Unapproved: 0, unknown: 0` over 623 files.
I am not a lawyer and this is policy rather than law. The two load-bearing
points — that Category A
bundling is not IP clearance, and that ASF code moving between ASF projects
is unencumbered — are
well established and we have the in-repo precedent above, but this should
have PMC eyes before it
lands, and legal-discuss@ is there if anyone thinks it is borderline. The
thing I would actually
want a second opinion on is not the licensing: it is that we are shipping a
modified copy of another
ASF project's code while HDFS-16021 is still open, so a Hadoop committer's
read of the patch would
be worth having.
## How are these changes tested?
The strongest evidence is that the guard is demonstrably in the shipped
binary. Disassembling
`hdfsThreadDestructor` out of `libcomet.dylib`, the ownership byte is now
tested before the `JNIEnv`
is dereferenced:
```
ldr x20, [x0, #8] ; state->env (now at offset 8, after the new bool)
ldrb w8, [x0] ; state->attachedByLibhdfs
cmp w8, #0
ccmp x20, #0, #4, ne ; attachedByLibhdfs != 0 && env != NULL
b.eq <skip> ; bails out before touching env
ldr x8, [x20] ; *env, the load that used to fault
ldr x8, [x8, #1752] ; GetJavaVM
blr x8 ; the call that used to jump to 0
```
Building the same tree with the `[patch]` commented out produces a
`hdfsThreadDestructor` with no
such guard, so this is not a system `libhdfs` being picked up by accident.
`ParquetReadFromFakeHadoopFsSuite` passes. That is the check that matters
for regressions, because
the `GetEnv` early-out changes control flow on every libhdfs call: if
`GetEnv` misbehaved we would
not crash, we would fail every HDFS read. Running the `[scans]` bucket order
in a single JVM —
`ParquetReadFromFakeHadoopFsSuite` → `ParquetTimestampLtzAsNtzSuite` →
`CometNativeReaderSuite` →
`CometIcebergNativeSuite` — gives 168 tests across 8 suites green, which is
the order and the fork
sharing that the crash needs.
I should be straight about the limit of that last one: I ran the identical
chain against the
unpatched build as a control and it also passed, so it does not
discriminate. The flake is
intermittent, and it fires on Linux/JDK 11 over a much larger bucket than I
ran, so one clean local
macOS run of a subset was never going to reproduce it. Treat the suite runs
as
evidence that nothing is broken, the disassembly as evidence that the fix is
present, and the
mechanism analysis against the two symbolized dumps on #5023 as the argument
that the fix is the
right one. @peterxcli's 100 fresh-JVM runs on #5036 are the closest thing we
have to a positive
result, and I would note there that those ran the arming suite alone rather
than the bucket order,
so they are weaker than they look too.
Compile-time assertions in `src/lib.rs` pin `hdfsFileInfo`'s size, alignment
and every field offset
against the C typedef, since a hand-written binding that got the layout
wrong would be silent memory
corruption rather than a link error. They are `const` assertions rather than
`#[test]`s so they run
on every build; they caught an off-by-one in my own arithmetic while I was
writing them. `cargo
clippy --all-targets --workspace`, `cargo fmt --all --check`, `cargo
machete`, `prettier --check` and
`dev/ci/check-ci-config.py` are all clean.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
