andygrove opened a new pull request, #5827: URL: https://github.com/apache/datafusion-comet/pull/5827
## Which issue does this PR close? Closes #5826. ## Rationale for this change The `Delta Contrib Build Gate` job is failing 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, as a proxy for "contrib did not get linked into the default build too". The proxy does not have enough signal to work: `comet-contrib-delta` is currently a 75-line stub, and it is being weighed against a ~1.5 GB unstripped debug cdylib whose byte count moves 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 edit repartitions them. The two measurements also move independently, since the second `cargo build` only recompiles `datafusion-comet` and relinks. 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 whole 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. A sort cannot do either of those, and whatever it did add would land in both builds rather than 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 leans on is not the contrib crate. The result is deterministic per commit — two runs on unchanged native sources report byte-identical sizes — so a re-run does not clear a failure. ## What changes are included in this PR? - Report the two `libcomet` sizes instead of asserting an ordering between them. The invariant the comparison stood in for is already measured directly 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. No coverage is lost. The comment left in place records the measurement above so the check does not get reintroduced. - Make a missing `nm` fail rather than silently skip. Both symbol checks were wrapped in `if command -v nm`, so on an image without it they degraded to no-ops and the size comparison was left as the only enforcement — backwards, given which of the two is the real measurement, and it would have left nothing at all once the size assertion went. This matches the anti-vacuous guards the script already applies to `cargo tree` and `help:effective-pom`. Nothing else in the gate changes: the cargo tree, Maven effective-pom, per-Spark `delta-spark` pinning, compiled-class and `META-INF/services` checks are untouched. ## How are these changes tested? `dev/verify-contrib-delta-gate.sh` was run end to end locally on macOS (JDK 17) and passes all five sections: ``` ==> libcomet: default build has no Delta symbols OK: default libcomet has 0 Delta symbols (size=427719856 bytes) OK: contrib-enabled libcomet has 1 Delta symbols (size=427827568 bytes, 107712 bytes vs default) ``` That run is also where the 107,712-byte figure above comes from. The one symbol the contrib build carries is `comet_contrib_delta::planner::plan_delta_scan`, so the grep pattern is confirmed to still match what rustc emits, which is what keeps the default-side check honest. The leak case was then exercised directly, by feeding the contrib-enabled dylib to the default-side assertion — the same thing a default build that had linked contrib would produce: ``` FAIL: default libcomet contains 1 Delta-related symbols ``` So the check that actually enforces the gate still fires, and it is unchanged by this PR. `shellcheck` reports no new warnings (the one pre-existing `SC2034` for `SPARK_DIR` is unchanged) and `bash -n` is clean. -- 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]
