andygrove opened a new issue, #6620:
URL: https://github.com/apache/datafusion-comet/issues/6620
Part of #6615.
## What / Why
This issue moves math and arithmetic coverage to Comet SQL tests under
`sql-tests/expressions/math/` (plus `misc/width_bucket.sql`). The sources are
`CometMathExpressionSuite`, the math and arithmetic tests in
`CometExpressionSuite`, and a note on `CometFuzzMathSuite`. Decimal arithmetic
has its own issue. Static analysis on `main` at 3bc2faa934 found the following:
| Suite | Covered | Convert | Blocked | Split | Keep | Total |
|---|---|---|---|---|---|---|
| `CometExpressionSuite` | 6 | 19 | 5 | 0 | 1 | 31 |
| `CometMathExpressionSuite` | 1 | 6 | 0 | 0 | 0 | 7 |
| `CometFuzzMathSuite` | 0 | 0 | 0 | 0 | 2 | 2 |
| **Total** | **7** | **25** | **5** | **0** | **3** | **40** |
Problems with the existing fixtures that this issue has to fix:
- **NaN results are never compared.** Every per-function fixture (`acos`,
`asin`, `atan`, `atan2`, `cos`, `cosh`, `cot`, `exp`, `expm1`, `log`, `log10`,
`log2`, `sin`, `sinh`, `sqrt`, `tan`, `tanh`) runs its NaN-producing inputs
only under `query tolerance=`. Examples are NaN input, `sqrt(-1.0)`, `acos(2)`
and `sin(Infinity)`. That check skips any row where either side is NaN and
accepts +Infinity against -Infinity (#6616). The Scala math tests share that
helper and that blind spot.
- Keep `tolerance=` only for finite, non-zero results.
- Move every input whose result is NaN, ±Infinity or a signed zero into a
plain `query`.
- **Missing edge values.** No fixture has Double Min, Max or MinPositive
values. `abs.sql` has no integral MIN, which is the non-ANSI wrap case where
`abs(MIN) = MIN`, and no -0.0.
- **The `try_*` arithmetic functions have no fixture.** The Scala `try_add`,
`try_subtract` and `try_multiply` tests are all-literal, so Spark
constant-folds them and they never reached Comet. In a fixture they test the
TRY kernels for the first time.
## Work
Line numbers refer to `CometExpressionSuite` unless marked
`CometMathExpressionSuite`.
**Existing fixtures to extend**
- `arithmetic.sql`
- L1663 (divide by zero, ANSI off): double divisor columns holding `0.0`
and `double('-0.0')` under `/` and `%`, int `/` and `%` by a literal `0`, and
double `% 0.0`.
- L2217: negation of tinyint, smallint, bigint, float, double and
decimal(38,18). Only int is covered today.
- L2228: `-f` and `-d` over NaN, ±Inf, ±0.0, MinPositive, Max, Min and
NULL, as a plain query.
- L2259: `fa % fb` and `da % db`. There is no non-ANSI float remainder in
any fixture today.
- L3490: non-ANSI `a div b` with a zero divisor. The ANSI half is already
in `arithmetic_ansi.sql`.
- L2793 (non-ANSI half): `SELECT a, -a` over Max and Min of int, bigint,
smallint and tinyint.
- `abs.sql` (`CometMathExpressionSuite` L34, L45)
- Add tinyint, smallint and decimal(10,2) columns, the MIN of every
integral type, float/double ±Infinity, NaN and Min/Max.
- Add `float('-0.0')` and `double('-0.0')`.
- `abs_ansi.sql` / `abs_ansi_spark42.sql` (`CometMathExpressionSuite` L56)
- Add the non-throwing half that the Scala test compares: a plain query
over float, double, decimal(10,2) and non-MIN integral values.
- The Scala regex `.+[ARITHMETIC_OVERFLOW].+overflow...` uses a character
class, not the literal error class, so it is weaker than it reads.
- `round.sql` / `round_ansi.sql` (L3521, L3556)
- An int `2147483647` input at scales -1000, -100, -20, -19, -10, -1, 0,
1, 10, 100 and 1000. In ANSI mode, scale -1 is
`expect_error(ARITHMETIC_OVERFLOW)`.
- Long Max and Min inputs at scales -18, -38 and -39.
- `ceil.sql` / `floor.sql` (L1983)
- A bigint input and a decimal(21,1) column.
- decimal(20,0) and decimal(38,18) casts of ±0.5, ±1.2 and ±0.0.
- decimal(5,2) values 123.45, 125.00 and -129.99.
- `signum.sql` (L1975): Double Min, Max and MinPositive.
- `expm1.sql` (L1979): `double('-0.0')`, NaN and ±Inf in a plain query, plus
Max, Min and MinPositive.
- The 16 per-function fixtures (L1880): add Double Min, Max and MinPositive
to each, plus whichever of -0.0, ±Inf and ±1 it lacks. Restructure the queries
as described above.
- `misc/width_bucket.sql` (`CometMathExpressionSuite` L97, L110, L127)
- A Parquet table with value, min, max and bucket-count columns.
- Rows: the four from L97; value = max with normal and reversed bounds;
min = max; zero buckets; NaN as value, as min and as max.
- No version gate is needed; see the notes below.
**New fixtures**
- `try_arithmetic.sql` (L803, L819, L835, L851, L870)
- `try_add`, `try_subtract`, `try_multiply` and `try_divide` over int and
bigint boundary operands, as columns and as literals, plus NULL.
- `try_divide` also needs a zero divisor and a decimal overflow.
- `remainder.sql` + `remainder_ansi.sql` (L2634)
- The int/float/decimal(18,4) remainder table and the ten-expression `%` /
`mod()` projection, including `Int.MinValue % -1`.
- For ANSI, use version-paired files (`MinSparkVersion: 4.1` expecting
`REMAINDER_BY_ZERO`, `MaxSparkVersion: 4.0` expecting `DIVIDE_BY_ZERO`), or
`expect_error(BY_ZERO)`.
- `unary_minus_ansi.sql` (`MinSparkVersion: 4.0`, `MaxSparkVersion: 4.1`)
and `unary_minus_ansi_spark3.sql` (`MaxSparkVersion: 3.5`) (L2793)
- Short and byte negation overflow, plus scalar `-(cast(<max|min> as
<type>))`.
- On 3.x, Spark says "caused overflow" while Comet says "short/byte
overflow", so the only common substring is `overflow`.
- `deep_arithmetic_chains.sql` (L3884)
- 200-term `+` and `*` chains. They must not overflow the protobuf
recursion limit.
- SQL's left recursion yields left-deep trees, as the Scala test builds.
**Blocked on #6617 (error parity)**
- **L3413, L3429, L3444 (ANSI add, subtract, multiply).** These need
`MaxSparkVersion: 4.1` files (#4967).
- `arithmetic_ansi.sql` already has `expect_error(ARITHMETIC_OVERFLOW)`
for int and long.
- That substring also matches `BINARY_ARITHMETIC_OVERFLOW`, so it cannot
catch the error-class divergence these tests guard against.
- **L3460 (int `/` by zero) and L3475 (float `/` by zero).** The float case
isn't covered even weakly: the fixture's float section tests only `%`. Add an
interim `expect_error(DIVIDE_BY_ZERO)` now.
## Stays in Scala
- **L3596**: a negative-scale decimal built from a DataFrame cast. SQL
cannot declare negative-scale decimals.
- **`CometFuzzMathSuite`** (2 tests × 5 ops × 3 variants): the fuzz layer
for mixed int/bigint and decimal arithmetic with extreme values. Keep it. If it
is ever dropped, `arithmetic.sql` needs a curated table of Int/Long extremes
first.
## Notes on the Scala tests
- **The three `CometMathExpressionSuite` width_bucket tests are vacuous.**
- Their views come from `createDataFrame(Seq(...))`, a `LocalRelation`,
and `ConvertToLocalRelation` is active in `CometTestBase`. So Spark folds the
projection in both runs.
- Their `assume(isSpark35Plus)` is stale too: `width_bucket.sql` runs on
3.4.
- **`try_integral_divide overflow cases` (L870)** calls only `try_divide`,
and its -128 and -32768 are INT literals.
- **Stale comments and configs in the fixtures.**
- `arithmetic_ansi.sql` says its all-literal cases are folded before
reaching Comet. They are not, because the suite excludes ConstantFolding.
- `tan.sql`'s `Tan.allowIncompatible=true` does nothing, because Tan is a
plain `CometScalarFunction` and always Compatible.
## Done when
- Every test in the table below is accounted for. Either a fixture covers it
(the same inputs, configs and assertion strength, or stronger), the fixture
that already covers it is cited, or it stays in Scala for the reason given
above.
- Each converted Scala test is deleted in the PR that adds its coverage. A
suite left empty is deleted and removed from the suite lists in
`.github/workflows/pr_build_linux.yml` and `pr_build_macos.yml`.
- The new and changed fixtures pass on every Spark profile. Apply
`run-all-spark-profiles`, because pull requests run only the default profile.
<details><summary>Every Scala test in scope, with its verdict and target
fixture</summary>
| Suite | Line | Test | Verdict | Target |
|---|---|---|---|---|
| `CometExpressionSuite` | 619 | Integral Division Overflow Handling Matches
Spark Behavior | covered | math/arithmetic.sql |
| `CometExpressionSuite` | 803 | try_add | convert | math/try_arithmetic.sql
(new) |
| `CometExpressionSuite` | 819 | try_subtract | convert |
math/try_arithmetic.sql (new) |
| `CometExpressionSuite` | 835 | try_multiply | convert |
math/try_arithmetic.sql (new) |
| `CometExpressionSuite` | 851 | try_divide | convert |
math/try_arithmetic.sql (new) |
| `CometExpressionSuite` | 870 | try_integral_divide overflow cases |
convert | math/try_arithmetic.sql (new) |
| `CometExpressionSuite` | 885 | dictionary arithmetic | covered |
math/arithmetic.sql |
| `CometExpressionSuite` | 894 | dictionary arithmetic with scalar | covered
| math/arithmetic.sql |
| `CometExpressionSuite` | 1653 | add overflow (ANSI disable) | covered |
math/arithmetic.sql |
| `CometExpressionSuite` | 1663 | divide by zero (ANSI disable) | convert |
math/arithmetic.sql |
| `CometExpressionSuite` | 1880 | various math scalar functions | convert |
math/{acos,asin,atan,cos,cosh,exp,log,log10,log2,sin,sinh,sqrt,tan,tanh,cot,atan2}.sql
|
| `CometExpressionSuite` | 1975 | signum | convert | math/signum.sql |
| `CometExpressionSuite` | 1979 | expm1 | convert | math/expm1.sql |
| `CometExpressionSuite` | 1983 | ceil and floor | convert | math/ceil.sql +
math/floor.sql |
| `CometExpressionSuite` | 2204 | negative | covered | math/arithmetic.sql |
| `CometExpressionSuite` | 2217 | unary minus across numeric types | convert
| math/arithmetic.sql |
| `CometExpressionSuite` | 2228 | unary minus on float/double special values
and nulls | convert | math/arithmetic.sql |
| `CometExpressionSuite` | 2259 | basic arithmetic | convert |
math/arithmetic.sql |
| `CometExpressionSuite` | 2634 | remainder function | convert |
math/remainder.sql (new) + math/remainder_ansi.sql (new; or extend
math/arithmetic_ansi.sql) |
| `CometExpressionSuite` | 2793 | unary negative integer overflow test |
convert | math/arithmetic.sql + math/unary_minus_ansi.sql (new: MinSparkVersion
4.0, MaxSparkVersion 4.1) + math/unary_minus_ansi_spark3.sql (new:
MaxSparkVersion 3.5) |
| `CometExpressionSuite` | 2923 | isnan | covered | math/isnan.sql |
| `CometExpressionSuite` | 3413 | ANSI support for add | blocked: #6617 |
math/arithmetic_ansi.sql |
| `CometExpressionSuite` | 3429 | ANSI support for subtract | blocked: #6617
| math/arithmetic_ansi.sql |
| `CometExpressionSuite` | 3444 | ANSI support for multiply | blocked: #6617
| math/arithmetic_ansi.sql |
| `CometExpressionSuite` | 3460 | ANSI support for divide (division by zero)
| blocked: #6617 | math/arithmetic_ansi.sql |
| `CometExpressionSuite` | 3475 | ANSI support for divide (division by zero)
float division | blocked: #6617 | math/arithmetic_ansi.sql |
| `CometExpressionSuite` | 3490 | ANSI support for integral divide (division
by zero) | convert | math/arithmetic.sql |
| `CometExpressionSuite` | 3521 | ANSI support for round function | convert
| math/round.sql + math/round_ansi.sql |
| `CometExpressionSuite` | 3556 | ANSI support for round function with
negative scale overflow on long | convert | math/round.sql +
math/round_ansi.sql |
| `CometExpressionSuite` | 3596 | round on negative-scale decimal | keep | |
| `CometExpressionSuite` | 3884 | deep integral Add/Multiply chains do not
overflow the protobuf recursion limit | convert |
math/deep_arithmetic_chains.sql (new) |
| `CometFuzzMathSuite` | 35 | integer math: $op (instances: + - * / div,
each x3 shuffle/nativeC2R variants = 15 runtime tests) | keep | |
| `CometFuzzMathSuite` | 57 | decimal math: $op (instances: + - * / div,
each x3 shuffle/nativeC2R variants = 15 runtime tests) | keep | |
| `CometMathExpressionSuite` | 34 | abs | convert | math/abs.sql |
| `CometMathExpressionSuite` | 45 | abs - negative zero | convert |
math/abs.sql |
| `CometMathExpressionSuite` | 56 | abs (ANSI mode) | convert |
math/abs_ansi.sql + math/abs_ansi_spark42.sql |
| `CometMathExpressionSuite` | 97 | width_bucket | convert |
misc/width_bucket.sql |
| `CometMathExpressionSuite` | 110 | width_bucket - edge cases | convert |
misc/width_bucket.sql |
| `CometMathExpressionSuite` | 127 | width_bucket - NaN values | convert |
misc/width_bucket.sql |
| `CometMathExpressionSuite` | 139 | width_bucket - with range data |
covered | misc/width_bucket.sql |
</details>
--
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]
