audit-datafusion-spark-expression

<!-- 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. Older versions are read rather than ignored because `datafusion-spark` is meant to be reusable by engines that target more than one Spark release. Comet supports Spark 3.4 through 4.0. A function audited against 4.2.0 alone can be correct there and silently wrong for such a consumer, and nothing in the crate records which of the two it is. Finding the divergence is useful even though the test suite cannot yet assert it. ## 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. ## Paths in This Skill Are Hints Every file path below was correct when this skill was written and several will rot. `FunctionRegistry.scala`, the golden-file directories, and `datafusion/common/src/config.rs` all move on someone else's schedule. **A path that does not resolve is a stop condition, not a step to skip.** The failure mode this exists to prevent is a command that quietly matches nothing, an audit that reads the silence as "no ANSI branch" or "Spark does not test this", and a confident report built on an empty grep. A `grep` over a nonexistent directory and a `grep` that genuinely found nothing are the same output. So when a path in this skill does not exist, or a command returns nothing where something was expected: 1. Say so out loud in the audit report. Name the path and the step. 2. Search for where the thing moved to, by content rather than by path. The grep patterns here are more durable than the directories they are pointed at. 3. If it cannot be found, mark every conclusion that depended on that step as unestablished. Do not let an unread source become an absence of findings. The same rule covers a `.slt` file, an implementation file, or a Spark class that is not where this skill says it is. ## 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. **A `.slt` file under `test_files/spark/` does not imply an implementation in `datafusion/spark`.** `SessionStateBuilderSpark::with_spark_features` layers the Spark functions on top of `with_default_features`, so a name the spark crate does not define resolves to DataFusion's own function of that name. `md5`, `coalesce`, and `reverse` are served this way today, and `date_format` resolves through an alias on core's `to_char`. The query under audit still runs, and the code it runs is not in the directory the grep above searched. If that grep comes back empty, widen it before concluding the function is unimplemented: ```bash grep -rln "\"$ARGUMENTS\"" \ datafusion/functions/src datafusion/functions-nested/src \ datafusion/functions-aggregate/src datafusion/functions-window/src \ datafusion/functions-table/src --include="*.rs" ``` A hit may be in a file named after something else, because core registers aliases. `date_format` lives in `to_char.rs`. Read the `aliases` vector, not just the file name. If that grep is also empty, the function is genuinely unimplemented and the `.slt` file is a porting stub with every query commented out. Check with `grep -cE '^(query|statement)' <file>.slt`, which returns zero for a stub. There is no implementation to audit in that case. Say so and stop, rather than reporting a clean audit of nothing. Otherwise audit what the grep found against Spark with the rest of this step's checklist. The findings land differently. A core function has non-Spark users, so Spark's semantics cannot be imposed on it. Where the two agree, record the coverage and leave the core function alone. Where they diverge, the fix is a Spark-specific implementation under `datafusion/spark/src/function/<category>/` that shadows the core one, never an edit to the core function. `datafusion/spark/src/function/math/abs.rs` shadowing `datafusion/functions/src/math/abs.rs` is the in-repo precedent. Say in the audit report which crate the audited code came from, because a reader who assumes `datafusion/spark` will look for a file that does not exist. 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: ``` $ ./target/debug/datafusion-cli -c "SELECT next_day(arrow_cast(95026236,'Date32'),'Mon');" Error during planning: Invalid function 'next_day'. Did you mean 'today'? ``` Giving the CLI an opt-in for the Spark library is tracked by https://github.com/apache/datafusion/issues/24146. Until that lands, treat the error above as a registration gap and not as evidence about the function. For a throwaway probe that should not end up in the committed file, add a temporary `.slt` alongside it, run it by name, and delete it afterwards. To exercise a raw storage value that has no literal syntax, such as a `Date32` past the last date DataFusion can render, wrap it in `arrow_cast`. **Cross the shapes with the NULL cases, and with ANSI mode.** These are not three independent checklists to tick off separately, they are one product to enumerate. The bug this instruction exists to catch is a branch that gets NULL handling right in the row-wise path and wrong in the vectorized one, so the two shapes disagree with each other while each looks correct in isolation. For every shape, ask what happens when the *other* argument is NULL, and when it is NULL in only some rows. A shape that takes an array and a scalar has to make the scalar's validation depend on the array's null mask, because a per-row null intolerant function skips validation on the rows it short circuits. All-NULL, some-NULL, and no-NULL versions of the same array are three different tests. Note that a `(Array, Scalar)` branch returning `ColumnarValue::Scalar` is *not* by itself a bug. In DataFusion a returned `Scalar` means "this value for every row" and the caller broadcasts it to the batch length. Before reporting one, run an N-row query and count the rows that come back. Report it only if the row count is actually wrong, or if the scalar's value depends on per-row input. Otherwise it is at most a readability cleanup, and reporting it as a correctness bug costs the audit credibility. **Overflow and underflow.** Does it return `Err`, wrap, or panic? Spark's answer depends on ANSI mode. A panic is always a bug. **Type dispatch.** Every type the signature admits must have a branch. An `exec_err!` fallthrough for a type the signature accepts is a bug. **ANSI handling.** Compare against `datafusion.execution.enable_ansi_mode` (defined in `datafusion/common/src/config.rs`). The crate has scattered `TODO` comments about unimplemented ANSI paths. Treat every one found in this function as a finding, not as a known-acceptable state. Grep for them across every file Step 3's grep returned, including `mod.rs`: ```bash grep -rn "TODO" datafusion/spark/src/function/<category>/$ARGUMENTS.rs \ datafusion/spark/src/function/<category>/mod.rs ``` Wiring ANSI up is usually mechanical rather than a semantics decision. The flag is already threaded through `ScalarFunctionArgs`, so the implementation reads it directly: ```rust let ScalarFunctionArgs { args, config_options, .. } = args; let ansi_mode = config_options.execution.enable_ansi_mode; ``` `datafusion/spark/src/function/math/abs.rs` and `datafusion/spark/src/function/math/modulus.rs` are the in-repo precedents for both the plumbing and the test layout. DataFusion does not model Spark's error classes or SQLSTATE values, so say that in the audit either way. Whether to match Spark's message text depends on who constructs the error: - **The function constructs it** (an `exec_err!` you write). Match Spark's text exactly, including trailing punctuation, and assert it in the `.slt`. - **An Arrow kernel constructs it** (`rem`, `add`, a cast). You do not control the wording. Assert only that an error is raised, note the divergence in a comment, and file an issue for the message text rather than contorting the implementation to restate an Arrow error in Spark's words. Do not read the `abs.rs` precedent as licence to invent DataFusion-flavoured text for an error you *do* construct. `abs` reports `Int32 overflow on abs(-2147483648)` where Spark 4.2.0 reports `[ARITHMETIC_OVERFLOW] overflow.`, which is a known divergence tracked by https://github.com/apache/datafusion/issues/24145. Follow its ANSI plumbing and test layout, not its wording. `abs` also shows why the "who constructs it" question is worth asking rather than assuming. Its scalar path builds the message in the spark crate, but its array path borrows `make_try_abs_function!` from `datafusion/functions/src/math/abs.rs`, so that half of the text is owned by core DataFusion and shared with core's own `abs`. Wherever a spark function reuses a core macro or kernel, Spark-specific error text is not available without un-sharing the code first. Check where the message is actually built before promising to match Spark's.