andygrove opened a new issue, #6621:
URL: https://github.com/apache/datafusion-comet/issues/6621
Part of #6615.
## What / Why
This issue moves decimal arithmetic coverage to Comet SQL tests under
`sql-tests/expressions/decimal/`. The sources are the decimal tests in
`CometExpressionSuite`, `CometDecimalPromotionSuite`, and the Spark 4.1+
`CometDecimalArithmeticViewSuite`. Static analysis on `main` at 3bc2faa934
found the following:
| Suite | Covered | Convert | Blocked | Split | Keep | Total |
|---|---|---|---|---|---|---|
| `CometExpressionSuite` | 0 | 9 | 2 | 0 | 3 | 14 |
| `CometDecimalPromotionSuite` | 0 | 1 | 0 | 0 | 3 | 4 |
| `CometDecimalArithmeticViewSuite` | 0 | 0 | 0 | 0 | 3 | 3 |
| **Total** | **0** | **10** | **2** | **0** | **9** | **21** |
The decimal fixtures are thin today. `decimal_ops.sql` covers only (10,2)
and (18,6). Nothing covers:
- decimal remainder by zero;
- the decimal(38,x) multiply overflow;
- `spark.sql.decimalOperations.allowPrecisionLoss`;
- decimal overflow flowing through the array and bitwise serdes (#5248).
## Work
Line numbers refer to `CometExpressionSuite` unless marked.
**Existing fixtures to extend**
- `decimal_div.sql`
- L546: `dec % 0.0`.
- L1775: literal `1.00 / 3.00` at (5,2), (18,10), (27,2) and (38,4). The
comment in the Scala test says `checkSparkAnswerAndOperator` verifies the
schema. It doesn't, but a wrong result scale changes the digits, so the answer
check still catches it.
- L3613: a column-divisor `a div b` over the six decimal(38,0) /
decimal(2,2) rows. The ANSI half is already covered by `decimal_div_ansi.sql`.
- `decimal_ops.sql`
- L1750: all-literal `+ - * / %` at (7,3), (18,10), (27,2) and (38,4).
- L1799: decimal(38,18) `11000000000000000000` times itself, which returns
NULL.
- L2439: decimal(33,29) `%` decimal(28,17). The Scala test computes this
but asserts nothing about the answer.
- Both files use `spark_answer_only` for decimal `/`, citing #1526.
`CometDivide` now marks every decimal Compatible, and the Scala tests assert
native execution for (38,4)/(38,4). Try upgrading those queries to `query`.
**New fixtures**
- `decimal_arithmetic_precisions.sql` (L1676)
- Seven precision/scale tables: (5,2), (1,0), (18,10), (18,17), (19,0),
(38,37), and an all-NULL (1,1).
- Ops `+ - / % >= =` against `'1.0'`, plus `± 1.0` and `± 123.456789`
literals.
- The Scala batch sizes 16 and 1024 make no difference with 10 rows, so
leave them out.
- `decimal_multiply_precision_loss.sql` (L2381)
- `-- ConfigMatrix:
spark.sql.decimalOperations.allowPrecisionLoss=true,false` with the test's nine
value pairs.
- Drop the `typeof()` checks. `typeof` has no Comet serde, and Spark's
`TypeOf.doGenCode` fails to compile when the expression isn't folded.
- `decimal_random_precisions.sql` (L2455): curated (p,s) classes instead of
the seed-42 sweep, with all six ops in four column/literal forms, under the
`allowPrecisionLoss` matrix.
- `decimal_overflow_nested.sql` + `_ansi.sql` (`CometDecimalPromotionSuite`
L163)
- Config: codegen dispatch off, plus allowIncompatible for ArrayIntersect,
ArrayExcept and ArrayJoin.
- Table: decimal(38,0) `a`, `b` and decimal(38,6) `c`, `d`, with overflow,
ordinary and NULL rows.
- 24 expressions × {`a * b`, `c / d`}.
- The ANSI file uses `expect_error(NUMERIC_VALUE_OUT_OF_RANGE)`, plus one
plain-query sentinel per expression on the non-overflow row. The sentinels
stand in for the Scala `CometProjectExec` check.
**Blocked on #6617 (error parity, including query context)**
- L568: column/column `a / b` and `a div b` with a zero divisor.
- L1809: the decimal(38,18) multiply overflow. An interim
`expect_error(NUMERIC_VALUE_OUT_OF_RANGE)` can go in now.
- Both tests also compare `getQueryContext.fragment()`, which even
`checkSparkError` doesn't check.
## Stays in Scala
- **L3766, L3785, L3804**: `MakeDecimal` is an internal expression and can
only be built through the DataFrame API.
- **`CometDecimalPromotionSuite` L40, L99, L231**: serde internals. These
check idempotent promotion, `CheckOverflow` protobuf contents, and hand-built
trees with mixed eval modes, plus a `Divide` shape that type coercion never
produces.
- **`CometDecimalArithmeticViewSuite` L38, L69, L108**: serde unit tests on
an `Add` whose stored eval context disagrees with the session conf.
- An end-to-end version of L38 would create a view under one
`allowPrecisionLoss` and query it under another.
- That becomes expressible once fixtures can change configs mid-file
safely (#6618). It would be a good companion test, though the unit tests should
stay.
## 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 |
|---|---|---|---|---|
| `CometDecimalArithmeticViewSuite` | 38 | issue #4124:
DecimalPrecision.promote honours per-expression allowPrecisionLoss | keep | |
| `CometDecimalArithmeticViewSuite` | 69 | issue #5075:
DecimalPrecision.promote honours per-expression eval mode | keep | |
| `CometDecimalArithmeticViewSuite` | 108 | plain CometScalarFunction
rejects Spark 4.1+ Add with evalContext | keep | |
| `CometDecimalPromotionSuite` | 40 | issue #5190: decimal promotion is
idempotent during recursive serialization | keep | |
| `CometDecimalPromotionSuite` | 99 | issue #5248: nested decimal children
and aggregate roots retain overflow wrappers | keep | |
| `CometDecimalPromotionSuite` | 163 | issue #5248: decimal overflow values
under recursive serdes, ANSI=$ansi (instances: ANSI=false, ANSI=true) | convert
| decimal/decimal_overflow_nested.sql (new) +
decimal/decimal_overflow_nested_ansi.sql (new) |
| `CometDecimalPromotionSuite` | 231 | decimal Divide with a non-decimal
operand is unsupported | keep | |
| `CometExpressionSuite` | 546 | decimals divide by zero | convert |
decimal/decimal_div.sql |
| `CometExpressionSuite` | 568 | ANSI decimal divide by zero raises a Spark
error | blocked: #6617 | decimal/decimal_div_ansi.sql |
| `CometExpressionSuite` | 1676 | decimals arithmetic and comparison |
convert | decimal/decimal_arithmetic_precisions.sql (new) |
| `CometExpressionSuite` | 1750 | scalar decimal arithmetic operations |
convert | decimal/decimal_ops.sql |
| `CometExpressionSuite` | 1775 | decimal division result type matches Spark
| convert | decimal/decimal_div.sql |
| `CometExpressionSuite` | 1799 | scalar decimal overflow - legacy mode
produces null | convert | decimal/decimal_ops.sql |
| `CometExpressionSuite` | 1809 | scalar decimal overflow - ANSI mode throws
ArithmeticException | blocked: #6617 | decimal/decimal_ops_ansi.sql (new) |
| `CometExpressionSuite` | 2381 | Decimal binary ops multiply is aligned to
Spark | convert | decimal/decimal_multiply_precision_loss.sql (new) |
| `CometExpressionSuite` | 2439 | Decimal modulus with Decimal256
intermediate type | convert | decimal/decimal_ops.sql |
| `CometExpressionSuite` | 2455 | Decimal random number tests | convert |
decimal/decimal_random_precisions.sql (new) |
| `CometExpressionSuite` | 3613 | test integral divide overflow for decimal
| convert | decimal/decimal_div.sql |
| `CometExpressionSuite` | 3766 | make decimal using DataFrame API - integer
| keep | |
| `CometExpressionSuite` | 3785 | make decimal using DataFrame API - long |
keep | |
| `CometExpressionSuite` | 3804 | make decimal using DataFrame API -
overflow throws when nullOnOverflow=false | keep | |
</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]
