ci: gate the Delta contrib build on symbols, not on libcomet size - #5827
Merged
Conversation
The build gate asserted 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". The proxy has no signal: `comet-contrib-delta` is a 75-line stub, and it is being weighed against a ~1.5 GB unstripped debug cdylib whose byte count moves by about a megabyte for 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 builds also move independently, since the second `cargo build` only recompiles `datafusion-comet` and relinks. On apache#5810 the entire native diff against its base commit is a 20-line `sort_unstable_by` in the Iceberg writer. The default lib grew 875 KB and the contrib-enabled lib shrank 396 KB, inverting a +1.2 MB gap and failing the gate. Three runs across two unrelated branches have hit it, and the result is deterministic per commit, so re-running does not clear it. Report the sizes instead of asserting an ordering. The invariant is already measured directly a few lines away -- the default libcomet must carry zero Delta symbols, and the contrib-enabled one at least one, which is what keeps the first check from going vacuous if mangling drifts -- so no coverage is lost. Also make a missing `nm` fail rather than silently skip. Both symbol checks were wrapped in `if command -v nm`, which left the size comparison as the only enforcement on an image without it, and would have left nothing at all once that comparison went. Closes apache#5826
pingz-oai
approved these changes
Sep 10, 2026
Contributor
|
nice @andygrove thanks for addressing this. |
pingzh
approved these changes
Sep 10, 2026
sunchao
approved these changes
Sep 10, 2026
Member
|
Merged, thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #5826.
Rationale for this change
The
Delta Contrib Build Gatejob is failing pull requests that have nothing to do with Delta. Its last check asserts that the--features contrib-deltalibcometis 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-deltais 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 secondcargo buildonly recompilesdatafusion-cometand relinks.Sizes reported by the gate itself, in bytes:
424c31aa7(main)b5069564b(#5810)pingzh-topk-reader-filterspingzh-topk-reader-filterscomet-native-scan-io-observabilityThe second row is the clearest case.
424c31aa7is the exact base commit of #5810, and the whole native diff between the two is a 20-linesort_unstable_byin 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
libcometsizes instead of asserting an ordering between them. The invariant the comparison stood in for is already measured directly a few lines away: the defaultlibcometmust carry zero symbols matchingcomet_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
nmfail rather than silently skip. Both symbol checks were wrapped inif 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 tocargo treeandhelp:effective-pom.Nothing else in the gate changes: the cargo tree, Maven effective-pom, per-Spark
delta-sparkpinning, compiled-class andMETA-INF/serviceschecks are untouched.How are these changes tested?
dev/verify-contrib-delta-gate.shwas run end to end locally on macOS (JDK 17) and passes all five sections: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:
So the check that actually enforces the gate still fires, and it is unchanged by this PR.
shellcheckreports no new warnings (the one pre-existingSC2034forSPARK_DIRis unchanged) andbash -nis clean.