Skip to content

ci: gate the Delta contrib build on symbols, not on libcomet size - #5827

Merged
sunchao merged 1 commit into
apache:mainfrom
andygrove:ci/delta-gate-symbols-not-size
Sep 10, 2026
Merged

ci: gate the Delta contrib build on symbols, not on libcomet size#5827
sunchao merged 1 commit into
apache:mainfrom
andygrove:ci/delta-gate-symbols-not-size

Conversation

@andygrove

Copy link
Copy Markdown
Member

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 1,518,487,584 1,519,692,952 +1,205,368
b5069564b (#5810) 34382116782 1,519,362,160 1,519,296,760 −65,400 (fail)
pingzh-topk-reader-filters 34385681309 1,519,317,344 1,519,252,728 −64,616 (fail)
pingzh-topk-reader-filters 34406327795 1,519,390,608 1,519,258,216 −132,392 (fail)
comet-native-scan-io-observability 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.

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
@github-actions github-actions Bot added build Build environment enhancement New feature or request area:ci CI/CD, GitHub Actions, build tooling labels Sep 10, 2026
@andygrove
andygrove requested a review from sunchao September 10, 2026 02:21
@pingzh

pingzh commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

nice @andygrove thanks for addressing this.

@sunchao
sunchao merged commit 392da2c into apache:main Sep 10, 2026
19 checks passed
@sunchao

sunchao commented Sep 10, 2026

Copy link
Copy Markdown
Member

Merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:ci CI/CD, GitHub Actions, build tooling build Build environment enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Delta contrib build gate fails unrelated PRs: the libcomet size comparison measures build noise

4 participants