andygrove commented on code in PR #23893: URL: https://github.com/apache/datafusion/pull/23893#discussion_r3650834422
########## .ai/skills/audit-datafusion-spark-expression/SKILL.md: ########## @@ -0,0 +1,848 @@ +--- +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 Review Comment: If we want to re-use datafusion-spark expressions in Comet, we'll need them to be compatible across multiple Spark versions -- 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]
