andygrove opened a new pull request, #23893: URL: https://github.com/apache/datafusion/pull/23893
## Which issue does this PR close? - Closes #. No issue tracks this. It is tooling, proposed for discussion. Related: https://github.com/apache/datafusion/issues/15914 (the `datafusion-spark` epic) and https://github.com/apache/datafusion/issues/23887 (version-specific SLT expectations, which this skill's cross-version findings link to). ## Rationale for this change The `datafusion-spark` crate reimplements Spark's built-in expressions. Correctness against Spark is the entire point of the crate, but there is no routine process for checking an implementation against the Spark source, and no record of which functions have been checked. The testing situation makes this harder than it looks. Comet, the sibling project, tests by differential execution: it runs each query in both engines and compares, so expected values never need to be written down. DataFusion's `.slt` files hard-code every expected result, with no Spark in the loop. Two consequences follow: 1. Someone has to determine what Spark actually returns before writing the expectation. Reading `nullSafeEval` and predicting the output is how wrong golden values get written, and once written they look like verified behavior. 2. A hard-coded expectation encodes exactly one Spark version, so every file has to be written with one version in mind. Many files under `test_files/spark/` were machine-generated from Sail's gold data and their values were never checked against a running Spark. An existing passing expectation is a claim, not evidence. ## What changes are included in this PR? One file: `.ai/skills/audit-datafusion-spark-expression/SKILL.md`, following the `.ai/skills/` convention already described in the repository `CLAUDE.md`. No production code changes. The skill audits one function per invocation, in eight steps: read the Spark source across 3.5.8, 4.0.4, 4.1.3 and 4.2.0 to detect cross-version divergence; harvest Spark's own unit tests and `sql-tests/results/*.sql.out` golden files; review the Rust implementation's signature and coercion, return nullability, null propagation, overflow, type dispatch and ANSI handling; cross-check Comet and Sail; review existing SLT coverage; build a gap matrix; establish ground truth; then apply findings. Three decisions are worth calling out for review: **Spark 4.2.0 is the declared baseline.** Older versions are read only to detect that behavior changed, never to set an expectation. **Expected values come from a real Spark.** The skill sets up a `pyspark==4.2.0` venv and runs every candidate query through it under both ANSI modes, including expectations that already exist in the file. If PySpark cannot be installed the audit continues from source-derived values, but every such expectation is marked unverified rather than presented as observed. **`--complete` is forbidden for files under `test_files/spark/`.** That mode fills expectations with whatever DataFusion currently returns, which on an audit would silently cement the bug being audited as the golden answer. The skill also establishes a convention for divergences it cannot fix: a commented-out SLT query carrying the correct Spark result and a link to the tracking issue, where uncommenting it is the contract for verifying a fix. That convention is new to this tree and is the piece most worth arguing about. ## Are these changes tested? The skill was run end to end against `next_day`, which is how it was debugged. That run produced https://github.com/apache/datafusion/pull/23892 and issues #23889, #23890 and #23891. Running it surfaced eleven defects in the skill itself, which are fixed here. Two are worth citing as evidence that the exercise was not circular: - The instruction to `unset SPARK_HOME` sat in a different code block from the command that needed it, so it never took effect and the run reproduced the exact `TypeError: 'JavaPackage' object is not callable` failure the text was written to prevent. - The ground-truth harness reported a Python `ValueError` as though it were a Spark error, which would have written a fabricated divergence into the `.slt` as observed fact. Review of the resulting `next_day` work then found a genuine SQL-reachable panic in the audited function that the audit had missed, and the gap that hid it, which is now a rule in the skill: cross the argument shapes with the NULL cases and with both ANSI modes rather than checking each axis alone. The skill carries no automated test. Its verification is that following it produces a correct audit, which is what #23892 demonstrates. The `datafusion-spark` README already points contributors to Comet and Sail when implementing a function. This is the same idea applied to checking 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]
