andygrove commented on code in PR #23893: URL: https://github.com/apache/datafusion/pull/23893#discussion_r3729945300
########## .ai/skills/audit-datafusion-spark-expression/SKILL.md: ########## @@ -0,0 +1,894 @@ +--- +name: audit-datafusion-spark-expression +description: Audit a datafusion-spark function implementation for correctness against Apache Spark 4.2.0. Studies the Spark source across versions, reviews the Rust implementation and its signature, verifies expected values against a real PySpark, fixes divergences, and captures the rest as issues and disabled tests. +argument-hint: <function-name> +--- + +<!-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, +software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +KIND, either express or implied. See the License for the +specific language governing permissions and limitations +under the License. +--> + +Audit the `datafusion-spark` implementation of the `$ARGUMENTS` function for +correctness against Apache Spark. + +## Baseline Version + +The baseline is **Spark 4.2.0**. Every expected value written into a `.slt` +file asserts Spark 4.2.0 behavior. + +Spark 3.5.8, 4.0.4, and 4.1.3 are also read, but only to detect that behavior +changed between versions. They never set an expectation. There is currently no +mechanism for version-specific expectations in DataFusion's SLT files, which is +tracked by https://github.com/apache/datafusion/issues/23887. + +## Overview + +1. Study the Spark implementation across versions +2. Harvest Spark's own test coverage +3. Review the DataFusion implementation +4. Cross-check Comet and Sail +5. Review existing SLT coverage +6. Gap analysis +7. Establish ground truth with PySpark +8. Apply findings + +Audit one function per invocation. + +## Step 1: Study the Spark Implementation + +Shallow-clone the four version tags. Skip any already present. + +```bash +set -eu +for tag in v3.5.8 v4.0.4 v4.1.3 v4.2.0; do + dir="/tmp/spark-${tag}" + [ -d "$dir" ] || git clone --depth 1 --branch "$tag" \ + https://github.com/apache/spark.git "$dir" +done +``` + +Find the expression class in each version. Spark registers SQL function names +in `FunctionRegistry.scala`, so start there to map `$ARGUMENTS` to its Scala +class name, then locate the class. + +```bash +for tag in v3.5.8 v4.0.4 v4.1.3 v4.2.0; do + dir="/tmp/spark-${tag}" + echo "=== $tag ===" + grep -rn "\"$ARGUMENTS\"" \ + "$dir/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala" +done +``` + +With the class name in hand, read its source in each version: + +```bash +for tag in v3.5.8 v4.0.4 v4.1.3 v4.2.0; do + dir="/tmp/spark-${tag}" + echo "=== $tag ===" + find "$dir/sql/catalyst/src/main/scala" -name "*.scala" \ + | xargs grep -lE "case class <ClassName>[ (\[]" 2>/dev/null +done +``` + +A Scala `case class` name is always followed by a space, a `(`, or a `[`, so +that bracket expression is the portable way to avoid matching a longer class +name that starts with the same prefix. Do not reach for `\b`, which is a GNU +and ugrep extension and is not available in POSIX or BSD `grep`. + +If it is not in catalyst, widen the search to `$dir/sql`. + +For each version, record: + +- `eval` / `nullSafeEval` / `doGenCode` logic +- `inputTypes` and `dataType` +- `nullable`, and how nulls propagate +- ANSI branches (`ansiEnabled`, `failOnError`) +- guards, `require` assertions, thrown exceptions +- any SQLConf the expression reads +- for string functions in 4.0+, whether it routes through + `CollationSupport.X.exec(..., collationId)` or declares + `StringTypeWithCollation` inputs + +Produce a diff summary across 3.5.8 → 4.0.4 → 4.1.3 → 4.2.0. Note new or +removed input types, changed edge-case behavior, new ANSI branches, new +parameters, and collation changes. + +Two changes affect nearly every function and are easy to miss: + +- ANSI mode became the **default** in Spark 4.0. A function whose behavior + differs under ANSI has a different default behavior in 3.5 than in 4.x. +- Collation support landed for many string functions in 4.0. Non-default + collations are a divergence axis that does not exist in 3.5. + +## Step 2: Harvest Spark's Own Test Coverage + +Spark's tests are a ready-made edge-case inventory. Two sources matter. + +Unit tests: + +```bash +dir=/tmp/spark-v4.2.0 +find "$dir/sql" -name "*.scala" -path "*/test/*" \ + | xargs grep -ln "$ARGUMENTS" 2>/dev/null +``` + +Golden files, which carry Spark's own verified query results and are the +highest-value source: + +```bash +for tag in v3.5.8 v4.0.4 v4.1.3 v4.2.0; do + dir="/tmp/spark-${tag}" + echo "=== $tag ===" + grep -rln "$ARGUMENTS" "$dir/sql/core/src/test/resources/sql-tests/results/" 2>/dev/null +done +``` + +**The ANSI and non-ANSI golden directories swapped meaning in Spark 4.0.** Read +the version banner before trusting a path: + +| Version | ANSI-mode results | Non-ANSI results | +| ------- | ------------------------ | --------------------------- | +| 3.5.8 | `results/ansi/X.sql.out` | `results/X.sql.out` | +| 4.x | `results/X.sql.out` | `results/nonansi/X.sql.out` | + +ANSI became the default in 4.0, so the unqualified path flipped from meaning +non-ANSI to meaning ANSI. An auditor who learns the layout on 3.5.8 and then +reads `results/X.sql.out` on 4.2.0 will record exactly inverted expectations, +and they will look plausible. Confirm the mode by checking whether the file +contains error output or `NULL` for a known-invalid input. + +Read every match and extract: + +- input types covered +- edge cases exercised: null, empty, overflow, negative, boundary, special + characters +- ANSI-mode cases +- error cases and their exact messages + +This list is the reference for the gap matrix in Step 6. + +## Step 3: Review the DataFusion Implementation + +```bash +grep -rln "$ARGUMENTS" datafusion/spark/src/function/ --include="*.rs" +``` Review Comment: Good catch, and yes. It is not only Comet: `with_spark_features` layers on top of `with_default_features`, so any name the spark crate does not define falls through to core DataFusion's function of that name, and the `.slt` file under `test_files/spark/` still runs. I confirmed with a scratch file in a spark path that `md5`, `reverse`, and `coalesce` all resolve and pass today, none of which exist in `datafusion/spark`. `date_format` resolves through an alias on core's `to_char`. So an audit that only greps `datafusion/spark/src/function/` reads an empty result as "unimplemented" when the function is live and being tested. Step 3 now widens the grep across `functions`, `functions-nested`, `functions-aggregate`, `functions-window`, and `functions-table` before drawing that conclusion, warns that a hit may sit in a file named after the alias target, and distinguishes the genuinely-unimplemented case (the `.slt` is a fully commented-out porting stub) so the audit stops there instead of reporting a clean pass on nothing. One thing I made explicit rather than adding the crates as equal peers: a core function has non-Spark users, so a divergence there is not fixed by editing core. The fix is a Spark-specific implementation that shadows it, which `datafusion/spark/src/function/math/abs.rs` shadowing `datafusion/functions/src/math/abs.rs` already does. ########## .ai/skills/audit-datafusion-spark-expression/SKILL.md: ########## @@ -0,0 +1,894 @@ +--- +name: audit-datafusion-spark-expression +description: Audit a datafusion-spark function implementation for correctness against Apache Spark 4.2.0. Studies the Spark source across versions, reviews the Rust implementation and its signature, verifies expected values against a real PySpark, fixes divergences, and captures the rest as issues and disabled tests. +argument-hint: <function-name> +--- + +<!-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, +software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +KIND, either express or implied. See the License for the +specific language governing permissions and limitations +under the License. +--> + +Audit the `datafusion-spark` implementation of the `$ARGUMENTS` function for +correctness against Apache Spark. + +## Baseline Version + +The baseline is **Spark 4.2.0**. Every expected value written into a `.slt` +file asserts Spark 4.2.0 behavior. + +Spark 3.5.8, 4.0.4, and 4.1.3 are also read, but only to detect that behavior +changed between versions. They never set an expectation. There is currently no +mechanism for version-specific expectations in DataFusion's SLT files, which is +tracked by https://github.com/apache/datafusion/issues/23887. + +## Overview + +1. Study the Spark implementation across versions +2. Harvest Spark's own test coverage +3. Review the DataFusion implementation +4. Cross-check Comet and Sail +5. Review existing SLT coverage +6. Gap analysis +7. Establish ground truth with PySpark +8. Apply findings + +Audit one function per invocation. + +## Step 1: Study the Spark Implementation + +Shallow-clone the four version tags. Skip any already present. + +```bash +set -eu +for tag in v3.5.8 v4.0.4 v4.1.3 v4.2.0; do + dir="/tmp/spark-${tag}" + [ -d "$dir" ] || git clone --depth 1 --branch "$tag" \ + https://github.com/apache/spark.git "$dir" +done +``` + +Find the expression class in each version. Spark registers SQL function names +in `FunctionRegistry.scala`, so start there to map `$ARGUMENTS` to its Scala +class name, then locate the class. + +```bash +for tag in v3.5.8 v4.0.4 v4.1.3 v4.2.0; do + dir="/tmp/spark-${tag}" + echo "=== $tag ===" + grep -rn "\"$ARGUMENTS\"" \ + "$dir/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala" +done +``` + +With the class name in hand, read its source in each version: + +```bash +for tag in v3.5.8 v4.0.4 v4.1.3 v4.2.0; do + dir="/tmp/spark-${tag}" + echo "=== $tag ===" + find "$dir/sql/catalyst/src/main/scala" -name "*.scala" \ + | xargs grep -lE "case class <ClassName>[ (\[]" 2>/dev/null +done +``` + +A Scala `case class` name is always followed by a space, a `(`, or a `[`, so +that bracket expression is the portable way to avoid matching a longer class +name that starts with the same prefix. Do not reach for `\b`, which is a GNU +and ugrep extension and is not available in POSIX or BSD `grep`. + +If it is not in catalyst, widen the search to `$dir/sql`. + +For each version, record: + +- `eval` / `nullSafeEval` / `doGenCode` logic +- `inputTypes` and `dataType` +- `nullable`, and how nulls propagate +- ANSI branches (`ansiEnabled`, `failOnError`) +- guards, `require` assertions, thrown exceptions +- any SQLConf the expression reads +- for string functions in 4.0+, whether it routes through + `CollationSupport.X.exec(..., collationId)` or declares + `StringTypeWithCollation` inputs + +Produce a diff summary across 3.5.8 → 4.0.4 → 4.1.3 → 4.2.0. Note new or +removed input types, changed edge-case behavior, new ANSI branches, new +parameters, and collation changes. + +Two changes affect nearly every function and are easy to miss: + +- ANSI mode became the **default** in Spark 4.0. A function whose behavior + differs under ANSI has a different default behavior in 3.5 than in 4.x. +- Collation support landed for many string functions in 4.0. Non-default + collations are a divergence axis that does not exist in 3.5. + +## Step 2: Harvest Spark's Own Test Coverage + +Spark's tests are a ready-made edge-case inventory. Two sources matter. + +Unit tests: + +```bash +dir=/tmp/spark-v4.2.0 +find "$dir/sql" -name "*.scala" -path "*/test/*" \ + | xargs grep -ln "$ARGUMENTS" 2>/dev/null +``` + +Golden files, which carry Spark's own verified query results and are the +highest-value source: + +```bash +for tag in v3.5.8 v4.0.4 v4.1.3 v4.2.0; do + dir="/tmp/spark-${tag}" + echo "=== $tag ===" + grep -rln "$ARGUMENTS" "$dir/sql/core/src/test/resources/sql-tests/results/" 2>/dev/null +done +``` + +**The ANSI and non-ANSI golden directories swapped meaning in Spark 4.0.** Read +the version banner before trusting a path: + +| Version | ANSI-mode results | Non-ANSI results | +| ------- | ------------------------ | --------------------------- | +| 3.5.8 | `results/ansi/X.sql.out` | `results/X.sql.out` | +| 4.x | `results/X.sql.out` | `results/nonansi/X.sql.out` | + +ANSI became the default in 4.0, so the unqualified path flipped from meaning +non-ANSI to meaning ANSI. An auditor who learns the layout on 3.5.8 and then +reads `results/X.sql.out` on 4.2.0 will record exactly inverted expectations, +and they will look plausible. Confirm the mode by checking whether the file +contains error output or `NULL` for a known-invalid input. + +Read every match and extract: + +- input types covered +- edge cases exercised: null, empty, overflow, negative, boundary, special + characters +- ANSI-mode cases +- error cases and their exact messages + +This list is the reference for the gap matrix in Step 6. + +## Step 3: Review the DataFusion Implementation + +```bash +grep -rln "$ARGUMENTS" datafusion/spark/src/function/ --include="*.rs" +``` + +The implementation lives at +`datafusion/spark/src/function/<category>/<function>.rs`. **Read every other +file that grep returns as well.** In particular +`datafusion/spark/src/function/<category>/mod.rs` carries the `export_functions!` +registration and the user-facing doc string, and unimplemented-behavior `TODO` +comments are written there too, not only in the function's own file. An audit +that reads only `<function>.rs` will miss them and will leave a doc string that +contradicts the code after a fix. + +Check each of the following. The first two have no Comet analog and are the +most common source of silent divergence. + +**Signature and coercion.** Compare `Signature` (and `coerce_types`, if +implemented) against Spark's `inputTypes`. Two failure directions, both real: + +- DataFusion rejects a type Spark accepts. The user gets a coercion error + where Spark returns a value. +- DataFusion accepts a type Spark rejects. The user gets a value where Spark + raises an analysis error. + +Note that `Signature::exact` is narrow. A Spark function accepting any string +type needs all of `Utf8`, `LargeUtf8`, and `Utf8View` handled, and a function +accepting any integral type needs the full width range. + +**Return nullability.** Compare `return_field_from_args` (or `return_type`) +against Spark's `nullable`. A function that reports non-nullable while Spark +can return NULL produces wrong plans, not merely wrong values: downstream +optimizer rules will fold away null checks that were load-bearing. + +**Null propagation.** Does every input-null combination return what Spark +returns? Check the order in which nullness and validity are tested. Spark +expressions that are null intolerant (a `NullIntolerant` mixin in 3.5, a +`nullIntolerant` override in 4.x) short circuit to NULL *before* validating the +other arguments, so a NULL in one argument suppresses an error the other +argument would otherwise raise, even under ANSI mode. An implementation that +validates first and checks nullness second raises where Spark returns NULL. + +**Arrow kernel semantics are not Java semantics.** A `datafusion-spark` +function is usually a thin arrangement of Arrow kernels, and the kernels do not +share Java's rules. Check each one the implementation leans on: + +- **Comparison kernels order floats by total order**, so `-0.0` compares as + *less than* `0.0` rather than equal to it. Any `eq`/`lt` used to detect a + special value, such as a zero divisor, silently misses `-0.0`. Java compares + them as equal. This is easy to miss because `-0.0` almost never appears in + hand-written test values, so write it in deliberately. +- **Arithmetic kernels are checked, not wrapping.** Java wraps silently on + overflow, so a checked Arrow kernel raises where Spark returns a wrapped + value. Where Spark's own expression is written in `Int` arithmetic, a + wrapping kernel is the faithful choice, not the reckless one. +- **Java promotes `byte` and `short` operands to `int`.** A Spark expression + whose Scala source reads `(r + n)` on `Byte` operands is evaluated at `Int` + width and cannot overflow, while the same source on `Int` operands can. + Reproducing it at `Int8` width wraps where Spark does not. +- **Kernel coverage is type dependent.** Arrow's `rem` raises on a zero divisor + for integers and decimals but returns `NaN` for floats, so an ANSI check that + relies on the kernel to raise silently does nothing on the float path. + +Read the Scala source as arithmetic to be reproduced, not as pseudocode to be +paraphrased. Then test each of these against a live Spark rather than reasoning +about them. + +**`ColumnarValue` shape dispatch.** A two-argument function has four shape +combinations, and each needs its own branch: +`(Scalar, Scalar)`, `(Array, Scalar)`, `(Scalar, Array)`, `(Array, Array)`. +A missing arm falls through to a catch-all `exec_err!` and turns a working +Spark query into a hard error. `(Scalar, Array)` is the one most often +forgotten, because the natural test query puts the column first. Write a query +for every combination and run it. Do not infer coverage from reading the +`match`. + +**How to run an ad-hoc query.** The only in-repo way to execute a +`datafusion-spark` function is to add the query to +`datafusion/sqllogictest/test_files/spark/<category>/$ARGUMENTS.slt` and run + +```bash +cargo test --test sqllogictests -- spark/<category>/$ARGUMENTS +``` + +`datafusion-cli` does **not** register the `datafusion-spark` function library, +so reaching for it fails in a way that looks like the function does not exist: Review Comment: Mechanically yes, `datafusion-cli` has no `datafusion-spark` dependency at all and adding one plus a `register_all` call is a few lines. The catch is that it cannot be on by default: `register_all` overwrites any existing function with the same name, and the names overlap. `abs` is in both and they disagree, since `datafusion-spark`'s wraps on integral overflow when `enable_ansi_mode` is false while core's raises. Turning it on unconditionally would silently change results for every existing CLI user with nothing in the query to indicate a different function ran. So it needs an explicit opt-in, a `--spark` flag or a `SET` option, which is a small feature rather than a one-liner. Filed as https://github.com/apache/datafusion/issues/24146 and linked from this spot in the skill, so the workaround here stays accurate until it lands. -- 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]
