apache / apache/datafusion-ballista

Vendored datafusion.proto sync check silently passes without checking datafusion.proto

Closed
#2,377 2 comments 0 reactions 0 assignees View on GitHub
bug
Dominant language
Rust
Stars
2.1k
Forks
320
Avg merge
1d 22h
Merged PRs (30d)
66

Description

**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: datafusion-proto@55.0.0 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: datafusion-proto@55.0.0 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.

Contributor guide

Open the contributing guide

Research direction

Start with dev/update_datafusion_proto.py, especially the FILES mapping and missing-file handling, then inspect ci/scripts/rust_proto_check.sh and the vendored datafusion.proto. Run python3 dev/update_datafusion_proto.py --check to reproduce the silent pass. Done means the check resolves the DataFusion 55 source, reports drift in the vendored file, and fails when an expected proto cannot be found.

Written by the indexing model from the issue text.

Assessment

Tech stack
python, rust, shell
Domain
build-system, ci-cd
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
74/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.