comphead commented on PR #5974:
URL:
https://github.com/apache/datafusion-comet/pull/5974#issuecomment-5700726343
Thanks for exercising it rather than just reading it. All six points were
sound and are fixed in f34a1ae. I reproduced each one first, and two of them
turned out slightly different from the description, noted below.
**The `shards` guard.** Confirmed exactly as you describe, and it is the
worst of the three:
```
$ shards="" ; ... while [ "$i" -le "$shards" ] ...
t.sh: line 6: [: : integer expression expected
TARGETS: extensions runtime
exit=0
```
One stderr line, all four core shards dropped, exit 0. `shards`, `scala` and
the `DEDICATED_JVM_SBT_TESTS` gate now all `die`. One refinement on severity:
an empty `scala` yields `:iceberg-spark:iceberg-spark-4.1_:test`, which Gradle
rejects as an unknown task, so that one fails loudly but unhelpfully rather
than quietly. The gate is silent like `shards`, since its value is legitimately
empty for every version except the gated one, so it is guarded on the line
being present and parsing rather than on being non-empty.
**The edited-diff wedge.** Reproduced: `patch does not apply` plus `new.txt:
already exists in working directory`. The applied diff is now recorded beside
the tree, and a changed diff reverse-applies the recorded copy before applying
the new one. That handles files the diff added, and leaves `target/` alone so
Zinc recompiles only what moved. Verified across three runs (apply, no-op,
edited) with no duplicated hunks. A tree patched before the record existed is
adopted rather than failed, and if the revert itself fails the error names the
directory to delete.
**`MAVEN_REPO_LOCAL`.** You are right that I invented it. Demonstrated:
```
$ MAVEN_REPO_LOCAL=/tmp/definitely-not-here ./mvnw -q -N help:evaluate \
-Dexpression=settings.localRepository -DforceStdout
/Users/ovoievodin/.m2/repository
```
Now resolved through `help:evaluate` as you suggested, cached for the run,
and it dies rather than falling back if the answer is not an absolute path.
**`--print-config` plus a preflight assertion.** Added, and wired into
`check-ci-config.py` as `check_local_ci_config`. It runs `bash -n` on the
script, then compares `--print-config` against the same sources parsed
independently in Python.
One thing worth flagging, because it changed the design: comparing two
parsers is not sufficient on its own. My first version passed a requoted
`spark-full: "4.1.3"` cleanly, because *both* parsers stripped only single
quotes and so agreed on the broken value `"4.1.3"`. There are now shape
assertions on top, and both parsers accept either quote style. Mutation results:
| mutation | before | now |
| --- | --- | --- |
| requote to `"4.1.3"` (legitimate YAML) | passed, script broken | passes,
script works |
| reindent a `with:` block | caught | caught |
| `spark-full: 'nope'` | passed | caught |
| syntax error in the script | not checked | caught |
`--print-config` touches no toolchain, no network and no Maven repository,
so it is safe in preflight. On shellcheck: I have kept the script clean under
`shellcheck -x` throughout, but you are right that nothing enforces it. `bash
-n` is what I added since it needs no new tooling in the preflight image. Happy
to add shellcheck to preflight as a follow-up if you want it enforced.
**Both doc points.** `SKIP_PREPARE=1` now says it skips the Comet install
and must not be used after changing Comet. The purge caveat now says the sweep
covers the whole local Maven repository rather than Comet and Spark artifacts,
and that it is a shared cache other projects will re-download from. Both also
added to `AGENTS.md`, since that is what tells agents to run this unattended.
**The description.** Fixed, along with a stale reference to cloning Spark:
it is fetched as a tag archive now, while Iceberg still needs a real clone
because its build derives the version from the `apache-iceberg-*` tag through
`com.palantir.git-version`.
Still true and unchanged from the description: no full green suite run end
to end. The furthest I have got locally is the `catalyst` row, and that one
does not exercise Comet at all, since `sql/catalyst` has no Comet dependency
and the diff's test changes are 87 files in `sql/core`, 5 in `sql/hive` and
none in catalyst. A `sql_core-*` run is what would actually prove the wiring,
and I have not completed one.
--
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]
