nielspardon opened a new pull request, #12744:
URL: https://github.com/apache/gluten/pull/12744
## What changes were proposed in this pull request?
Fixes #12742.
Both protoc `add_custom_command`s in `cpp/core/CMakeLists.txt` declared
their dependency on a **directory** rather than on the `.proto` files — and on
the wrong directory. The trailing slash passed to `get_filename_component(...
DIRECTORY)` strips the last component, so the declared dependency landed one to
two levels above the actual inputs:
| | path |
|---|---|
| `SUBSTRAIT_PROTO_SRC_DIR` | `.../resources/substrait/proto` |
| `SUBSTRAIT_PROTO_DIR` (declared dependency) | `.../resources/substrait` |
| files protoc actually reads |
`.../resources/substrait/proto/substrait/*.proto` |
Editing a `.proto` in place therefore changed no mtime that the build graph
was watching, so an incremental native build silently skipped protoc and kept
linking stale `*.pb.cc` / `*.pb.h`. The Gluten protos had the identical bug
(`GLUTEN_PROTO_DIR` resolved to `.../org/apache/gluten` while the protos live
in `.../org/apache/gluten/proto/`).
This has gone unnoticed because every `algebra.proto` change so far has been
additive or comment-only, which makes a stale descriptor benign. It stops being
benign as soon as a field is renumbered or removed: the JVM emits at the new
tag while the native library probes the old one, and since protobuf shunts the
mismatched bytes into unknown fields, nothing is raised — no exception, no
fallback to vanilla Spark, just silently wrong behaviour. See the issue for the
concrete case that surfaced this (a `WriteRel.bucket_spec` renumbering
producing **unbucketed** files for a table the metastore records as `CLUSTERED
BY`).
`dev/builddeps-veloxbe.sh` does `rm -rf build`, so CI and the official build
script always do a clean generation — which is precisely why this survived. It
only bites local incremental builds, IDE builds, and any workflow that reuses
`cpp/build`.
### Changes
- `DEPENDS ${SUBSTRAIT_PROTO_DIR}` → `DEPENDS ${SUBSTRAIT_PROTO_FILES}`
- `DEPENDS ${GLUTEN_PROTO_DIR}` → `DEPENDS ${GLUTEN_PROTO_FILES}`
- Dropped the two `get_filename_component` calls, which are now dead
- Added `CONFIGURE_DEPENDS` to both `file(GLOB ...)` calls, so adding or
deleting a `.proto` re-runs CMake instead of requiring a manual re-configure
The globbed file lists were already computed a few lines above each command;
depending on them matches what the ClickHouse backend already does —
`cpp-ch/local-engine/proto/CMakeLists.txt:33` uses `DEPENDS ${protobuf_files}`.
`CONFIGURE_DEPENDS` needs CMake 3.12+; `cpp/core/CMakeLists.txt` requires 3.16,
and `cpp-ch` already uses it.
## How was this patch tested?
`cpp/core` cannot be configured standalone (it needs Velox/Arrow and
`GLUTEN_HOME`), so the proto sections were extracted verbatim into a minimal
Ninja project pointed at the real proto trees, and built both before and after
the change:
| scenario | before | after |
|---|---|---|
| declared dep in `build.ninja` | `.../resources/substrait` (a directory) |
the five `.proto` files |
| `touch algebra.proto` → build | `ninja: no work to do` | `Running
Substrait PROTO compiler` |
| `touch config.proto` → build | `ninja: no work to do` | `Running Gluten
PROTO compiler` |
| add a new `.proto` → build | not generated | `GLOB mismatch!` →
re-configure → generated |
| delete a `.proto` → build | stale output kept | `GLOB mismatch!` →
re-configure |
| no-op build | `no work to do` | `no work to do` (no spurious re-runs) |
The "before" harness reproduces the issue's repro exactly. Regeneration of
`algebra.pb.cc` after the fix was additionally confirmed by mtime.
`python3 dev/check.py format main --fix` reports no formatting changes.
No functional code changes, so no new unit test is applicable — this is a
build-graph correctness fix.
---
This PR was prepared with the assistance of AI.
--
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]
