andygrove commented on PR #5036:
URL:
https://github.com/apache/datafusion-comet/pull/5036#issuecomment-5441441443
> **Note on this review:** this was generated by an LLM (Claude Code) at my
request while I worked through a review backlog. I have not verified the
individual findings myself. Please treat everything below as suggestions to
evaluate rather than as authoritative review feedback, and push back on
anything that is wrong or already handled.
The diagnosis is convincing and the verification is thorough. Detaching a
thread the JVM attached is exactly the ownership bug HDFS-16021 describes, and
100 fresh-JVM runs plus the disassembly check is more evidence than most crash
fixes get.
My concern is entirely about where the fix comes from.
**Pinning to a personal fork**
`[patch.crates-io] hdfs-sys = { git =
"https://github.com/peterxcli/hdfs-sys";, rev = "..." }` makes every Comet
native build, including release builds, fetch source from a contributor's
personal GitHub account. For an ASF project that is a problem beyond the usual
supply-chain discomfort. The release artifacts would carry a dependency that is
not in any registry, is not covered by any release vote, and disappears if the
account or repo goes away.
What is the upstream path here? Some questions that would help:
- Has a PR been opened against `Xuanwo/hdfs-sys`, and is there any signal on
whether it will be merged and released? If there is, linking it here would let
us track it.
- If upstream is unresponsive, is vendoring the patched C source into
Comet's tree an option? That keeps the fix under ASF's own release process and
is auditable in the repo.
- Failing both, could the fork at least live under an org rather than a
personal account, with a comment in `Cargo.toml` pointing at the upstream issue
and stating explicitly that this is temporary?
I would not want to merge this as-is on a release branch, but I also do not
want the crash to stay unfixed. If there is a way to land the fix under Comet's
own control, or to get a release out of upstream, that seems worth a bit more
effort before merging.
**One smaller note**
`[patch.crates-io]` only applies to builds of this workspace. That is fine
today since Comet's native crates are not published, but if
`datafusion-comet-spark-expr` is ever published to crates.io the patch will be
silently ignored for downstream consumers and they will get the broken
`hdfs-sys`. Worth a comment in `Cargo.toml` so that does not surprise someone
later.
--
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]
