andygrove opened a new issue, #5826:
URL: https://github.com/apache/datafusion-comet/issues/5826
## Describe the bug
The `Delta Contrib Build Gate` job (`dev/verify-contrib-delta-gate.sh`)
fails on pull requests that have nothing to do with Delta. Its last check
asserts that the `--features contrib-delta` `libcomet` is strictly larger than
the default one:
```sh
if [[ "$SIZE_CONTRIB" -le "$SIZE_DEFAULT" ]]; then
red "FAIL: contrib-enabled libcomet (size=$SIZE_CONTRIB) is not larger
than default (size=$SIZE_DEFAULT)"
red " (would indicate contrib was being linked into default build
too)"
exit 1
fi
```
That comparison is a proxy for "contrib did not get linked into the default
build", and the proxy does not have enough signal to work.
`comet-contrib-delta` is currently a 75-line stub (`contrib/delta/native/src`),
while the thing being weighed is a ~1.5 GB unstripped debug cdylib. Byte counts
on a binary that size move by roughly a megabyte in response to source changes
that have nothing to do with Delta, because rustc re-emits DWARF per codegen
unit and a small source edit repartitions them. The two builds also move
independently: the second `cargo build` only recompiles `datafusion-comet` and
relinks, so whatever the repartitioning does to it is not mirrored in the first
measurement.
## Evidence
Sizes reported by the gate itself, in bytes:
| Commit | Run | default | contrib | contrib − default |
| --- | --- | ---: | ---: | ---: |
| `424c31aa7` (main) |
[34369036621](https://github.com/apache/datafusion-comet/actions/runs/34369036621)
| 1,518,487,584 | 1,519,692,952 | +1,205,368 |
| `b5069564b` (#5810) |
[34382116782](https://github.com/apache/datafusion-comet/actions/runs/34382116782)
| 1,519,362,160 | 1,519,296,760 | **−65,400 (fail)** |
| `pingzh-topk-reader-filters` |
[34385681309](https://github.com/apache/datafusion-comet/actions/runs/34385681309)
| 1,519,317,344 | 1,519,252,728 | **−64,616 (fail)** |
| `pingzh-topk-reader-filters` |
[34406327795](https://github.com/apache/datafusion-comet/actions/runs/34406327795)
| 1,519,390,608 | 1,519,258,216 | **−132,392 (fail)** |
| `comet-native-scan-io-observability` |
[34414317975](https://github.com/apache/datafusion-comet/actions/runs/34414317975)
| 1,519,496,304 | 1,519,992,584 | +496,280 (pass, 40% of the usual margin) |
The second row is the clearest case. `424c31aa7` is the exact base commit of
#5810, and the entire native diff between the two is a 20-line
`sort_unstable_by` in the Iceberg writer. Against that base the default lib
grew 875 KB and the contrib-enabled lib shrank 396 KB, which inverted a gap
that had been +1.2 MB one commit earlier. A sort cannot do either of those
things, and whatever it did add would land in both builds, not one.
Running the gate locally on macOS puts a number on what the stub is actually
worth: 107,712 bytes out of a 428 MB dylib. The +1.2 MB seen on quiet branches
is mostly incidental layout, not contrib content, so the margin the check
depends on is not the contrib crate.
The practical effect is that any pull request touching `native/core` can
draw a red X for a Delta leak it did not cause. Three runs across two unrelated
branches have hit it so far.
## Steps to reproduce
Open a pull request that changes anything in `native/core/src`, or re-run
the gate on `b5069564b`. The result is deterministic per commit: two `Delta
Contrib Build Gate` runs on unchanged native sources report byte-identical
sizes, so re-running a failure does not clear it.
## Expected behavior
The gate should fail when Delta surface leaks into a default build, and not
otherwise.
## Additional context
The invariant the size check is standing in for is already asserted
directly, and better, a few lines away: the default `libcomet` must carry zero
symbols matching
`comet_contrib_delta|delta_kernel|deltadvfilter|deltasynthetic`, and the
contrib-enabled one must carry at least one (which is what keeps the first
check from going vacuous if symbol mangling drifts). Dropping the size
assertion and reporting the sizes as diagnostics loses no coverage.
One related gap while in there: both symbol checks are wrapped in `if
command -v nm`, so on an image without `nm` they silently become no-ops and the
size comparison is left as the only enforcement. That is backwards, and it
would leave nothing at all once the size assertion goes. A missing `nm` should
fail loudly, in keeping with the anti-vacuous guards the rest of the script
already applies to `cargo tree` and `help:effective-pom`.
--
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]
