andygrove commented on PR #5459:
URL:
https://github.com/apache/datafusion-comet/pull/5459#issuecomment-5441341311
> **Note on this review:** this was generated by an LLM (Claude Code) at my
request while I worked through a review backlog. I have not verified the
individual findings myself. Please treat everything below as suggestions to
evaluate rather than as authoritative review feedback, and push back on
anything that is wrong or already handled.
Cutting the Iceberg path from 78 minutes is worth doing, and I like that the
split is a Gradle exclusion predicate layered on top of the existing candidate
set rather than an allowlist someone has to maintain. Keeping `@Nested` classes
with their enclosing class and leaving everything on the classpath are the
right details to get right.
Three things I would want addressed.
**`strategy.job-total` will silently drop tests if the matrix ever gains a
dimension**
The workflow passes `-PcometShardCount=${{ strategy.job-total }}`, which
today equals 4 because `shard` is the only matrix key. If anyone later adds a
second dimension, say `jdk: [11, 17]`, `job-total` becomes 8 while
`matrix.shard` still ranges 1 through 4. Classes hashing to shards 5 through 8
would then be excluded from every worker and simply never run, with a green
build. Could this pass `4` literally, or derive the count from the length of
the shard list? A silent coverage hole in the Iceberg suite is exactly the kind
of thing nobody notices for months.
**Nothing verifies that the four real shards cover the real test set**
`check-iceberg-shards.py` is a nice piece of work, but it validates the init
script against a synthetic 10-class fixture in a different job. The actual
Iceberg run uploads four `comet-shards/*.json` manifests as artifacts and then
nothing reads them. The property we care about is "the union of the four real
shards equals what the unsharded run would have selected, with no class
appearing twice," and that is currently unchecked.
Would you add a small follow-up job that depends on the four shards,
downloads the manifests, and asserts the union has no duplicates and matches
the expected class count? That is cheap and it turns the artifact upload from
something you look at during a postmortem into something that fails the build.
Without it, the failure mode of a bad hash change is silently running fewer
tests.
**The runner-minute cost is not stated**
The description quotes 67 minutes of test execution inside a 78-minute job,
so roughly 11 minutes of setup, container pull, and compilation. That 11
minutes is now paid four times per Iceberg version, and the description
explicitly defers the repeated-compilation work. Wall clock clearly improves.
Total consumption on shared ASF runners goes up, and by how much is the number
that matters for whether this is a net win. Could you put the before and after
total runner minutes in the description? If the increase is significant it
might be worth landing the compilation sharing first, or starting with two
shards instead of four.
**One smaller thing**
The comment in `CometIcebergTestShard.owner` records concrete minute figures
from a 1.11 run. Those will go stale and there is nothing that notices when the
buckets drift apart. Since each shard's duration is visible in the Actions UI
anyway, would it be better to say "sized against a 1.11 run in <month>, recheck
if a shard becomes the critical path" rather than baking in numbers that will
silently become wrong?
--
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]
