andygrove opened a new issue, #2377:
URL: https://github.com/apache/datafusion-ballista/issues/2377
**Describe the bug**
`ci/scripts/rust_proto_check.sh` currently passes without checking
`datafusion.proto` at all, and the vendored copy has drifted 471 diff lines
from the DataFusion version we build against.
In DataFusion 55 the logical plan proto moved out of the `datafusion-proto`
crate into a new `datafusion-proto-models` crate:
- 54: `datafusion/proto/proto/datafusion.proto`, shipped in the
`datafusion-proto` crate
- 55: `datafusion/proto-models/proto/datafusion.proto`, shipped in the
`datafusion-proto-models` crate
`dev/update_datafusion_proto.py` still looks for it under `datafusion-proto`:
```python
FILES = {
"datafusion.proto": ("datafusion-proto", "datafusion.proto"),
"datafusion_common.proto": ("datafusion-proto-common",
"datafusion_common.proto"),
}
```
The lookup misses, and the "some releases don't publish the .proto" escape
hatch that was added for pre-54 `datafusion-proto-common` swallows it:
```
note: [email protected] does not ship proto/datafusion.proto; leaving
vendored datafusion.proto unchanged
```
The script then exits 0, so the CI job "Check vendored DataFusion proto is
in sync" is green while checking only `datafusion_common.proto`.
**To Reproduce**
```
$ python3 dev/update_datafusion_proto.py --check
note: [email protected] does not ship proto/datafusion.proto; leaving
vendored datafusion.proto unchanged
$ echo $?
0
```
The real source is present and resolvable. `datafusion-proto-models` is
already in `Cargo.lock`, and the crate does ship the file, so `cargo metadata`
can find it. Comparing the vendored copy against it (after the usual import
rewrite) reports `STALE` with 471 diff lines, including things like `reserved
8; // was bool collect_stat`, the new `RangeRepartition` variant on
`Partitioning`, `repeated string locations = 16` on the scan node, and the
`MERGE_INTO` DML additions.
**Expected behavior**
The check should resolve `datafusion.proto` from `datafusion-proto-models`
and fail on drift, and a missing source file for a crate that is expected to
ship one should be an error rather than a skip.
**Additional context**
There is no build breakage today. The vendored files are build-time stubs
only, with no Rust generated from them, since `ballista/core/build.rs` maps
their packages to the real crates via `extern_path`. `ballista.proto` refers to
`datafusion.Partitioning`, `datafusion.PhysicalPlanNode` and friends only by
type name, so protoc still resolves everything and the generated Rust comes
from `datafusion-proto` itself.
The problem is that the guard described in CLAUDE.md is not currently
guarding, so a future change that does matter would land unnoticed.
Suggested fix:
- Point the `datafusion.proto` entry at `datafusion-proto-models` (with a
fallback to `datafusion-proto` if we want the script to keep working on 54).
- Tighten the skip path so it only applies to a known allowlist, or drop it
now that both crates ship their protos, so a missing file fails loudly.
- Re-vendor and commit the refreshed `datafusion.proto`.
Found while investigating #2376.
--
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]
