Repository navigation
feat(tests): add decimal arithmetic cases - #1213
alexandrefimov wants to merge 8 commits into
Conversation
`functions_arithmetic_decimal` defines add, subtract, multiply and divide over decimals, and the cases under those names cover only the integer and floating-point overloads. The precision and scale formula had no case behind it. Each function gets ordinary operands, where the result type follows from the formula and the arithmetic is exact, and a pair at scale 10, where the formula gives up scale to keep precision within 38. Add and multiply also get an operand pair whose exact result is outside the returned type under any treatment of scale, with `overflow:ERROR`. Coverage moves from 278 to 282 of 531 function variants.
Null operands exercise the MIRROR nullability the four implementations declare, which is the one property of a declared result this repository checks. Subtract and divide gain the overflow case add and multiply already had, so all four state what happens when the exact result is outside the type the formula returns.
|
The |
| # basic: Sums of small values, with the result type given by the precision and scale formula | ||
| add(1.00::dec<10, 2>, 2.0::dec<5, 1>) = 3.00::dec<11, 2> | ||
| add(-1.00::dec<10, 2>, 2.0::dec<5, 1>) = 1.00::dec<11, 2> | ||
| add(0.00::dec<10, 2>, 0.0::dec<5, 1>) = 0.00::dec<11, 2> |
There was a problem hiding this comment.
Before we encode these semantics... we should figure out what engine(s) this is intended to model.
There was a problem hiding this comment.
I explored around and the best I could find was spark. I executed the following with docker:
SELECT a + b AS result, typeof(a + b) AS result_type
FROM VALUES
(CAST(1.00 AS DECIMAL(10,2)), CAST(2.0 AS DECIMAL(5,1))),
(CAST(-1.00 AS DECIMAL(10,2)), CAST(2.0 AS DECIMAL(5,1))),
(CAST(0.00 AS DECIMAL(10,2)), CAST(0.0 AS DECIMAL(5,1)))
AS t(a, b);and got:
3.00 decimal(11,2)
1.00 decimal(11,2)
0.00 decimal(11,2)
which is a good sign.
I executed the equivalent query in datafusion:
SELECT a + b AS result, arrow_typeof(a + b) AS result_type
FROM (VALUES
(CAST(1.00 AS DECIMAL(10,2)), CAST(2.0 AS DECIMAL(5,1))),
(CAST(-1.00 AS DECIMAL(10,2)), CAST(2.0 AS DECIMAL(5,1))),
(CAST(0.00 AS DECIMAL(10,2)), CAST(0.0 AS DECIMAL(5,1)))
) AS t(a, b);and also got:
+--------+-------------------+
| result | result_type |
+--------+-------------------+
| 3.00 | Decimal128(11, 2) |
| 1.00 | Decimal128(11, 2) |
| 0.00 | Decimal128(11, 2) |
+--------+-------------------+
So they both agree there which is good.
There was a problem hiding this comment.
The reference is Gandiva's DecimalTypeUtil — #151 names it, and that PR is the only change to these return expressions since the file was written. Hive implements the same rules, and the comment on Spark's adjustPrecisionScale says it is based on Hive's.
I transcribed Gandiva, Hive 4.0.1 and Spark 4.0.1 beside the YAML and compared them over every operand type pair type_classes.md allows (779 × 779 = 606841): add, subtract and multiply derive the same type on every one. divide does not; more on that in the thread below.
| add(0.00::dec<10, 2>, 0.0::dec<5, 1>) = 0.00::dec<11, 2> | ||
|
|
||
| # scale_reduction: Two operands at scale 10 need 39 digits, so the formula keeps precision 38 and drops the scale to 9 | ||
| add(1.0000000000::dec<38, 10>, 2.0000000000::dec<38, 10>) = 3.000000000::dec<38, 9> |
There was a problem hiding this comment.
However in this case, they dont' agree.
Spark
Query
SELECT
CAST('1.0000000000' AS DECIMAL(38,10))
+ CAST('2.0000000000' AS DECIMAL(38,10)) AS result,
typeof(
CAST('1.0000000000' AS DECIMAL(38,10))
+ CAST('2.0000000000' AS DECIMAL(38,10))
) AS result_type;Result
3.000000000 decimal(38,9)
Datafusion
Query
SELECT
CAST('1.0000000000' AS DECIMAL(38,10))
+ CAST('2.0000000000' AS DECIMAL(38,10)) AS result,
arrow_typeof(
CAST('1.0000000000' AS DECIMAL(38,10))
+ CAST('2.0000000000' AS DECIMAL(38,10))
) AS result_type;Result
3.0000000000 Decimal128(38, 10)
Outcome
Notice that the scale is different in both.
IMO, a function could belong inside of the substrait core functions provided it is supported by >1 reasonably supported engines. If we can't find a few engines where they agree on the semantics, I would almost opt to remove the functions rather than codify a very engine-specific implementation.
If we are going to update these tests to lay claim to the intended semantics, can we also update the function's documentation to clarify why they have these semantics in the first place?
There was a problem hiding this comment.
I asked codex to investigate via docker and see if there are any others that support these semantics, and it found that both hive and spark look good, but Trino, DataFusion, MySQL, and ClickHouse all differ.
There was a problem hiding this comment.
Trino splits from Spark and Hive on this line the same way DataFusion does, and MySQL and ClickHouse are not on either side of it. Result types for dec(10,2) and dec(5,1) as operands, and for the pair on this line:
| add | multiply | divide | this line | |
|---|---|---|---|---|
| the YAML | dec(11,2) |
dec(16,3) |
dec(21,8) |
dec(38,9) |
| Spark 4.0.1, Hive 4.0.1 | dec(11,2) |
dec(16,3) |
dec(17,8) |
dec(38,9) |
| Trino 483 | dec(11,2) |
dec(15,3) |
dec(17,8) |
dec(38,10) |
| MySQL 8.4.11 | dec(11,2) |
dec(15,3) |
dec(15,6) |
dec(39,10) |
| ClickHouse 26.8.6.5 | Decimal(18,2) |
Decimal(18,3) |
Decimal(18,2) |
Decimal(38,10) |
MySQL does not cap precision at 38, and ClickHouse widens a result to the next Decimal size — its Decimal(18,2) is Decimal64 — so those two rows are a different decimal type, not a different rule. Trino's decimal type is Substrait's, and it splits from Spark and Hive where DataFusion does: above 38 it keeps the scale instead of taking the surplus out of it, and it drops the +1 on multiply, as MySQL does. That rule above 38 comes from Gandiva's DecimalTypeUtil, which #151 names as the reference for these expressions.
So this line does codify one side, and merging it turns a difference into a failing case for Trino and DataFusion. I would rather a test file not decide that: the return expression is already released and these cases only write down what it says, so moving to the other side is a change to the YAML. If you would rather this PR not assert it at all, say so and I will drop the scale_reduction cases too.
divide is not that — there the YAML matches none of them. Its precision uses P2 where Gandiva, Hive, Spark and Trino use S2: on dec(10,2) / dec(9,0) the formula gives dec(29,12) and Spark, Hive and Trino all give dec(20,12). #1216 is open on exactly that: a formula to fix rather than a function to drop. I will take divide.test out of this PR.
On the documentation: opened #1223 — description on add, subtract and multiply, since that is what the generated function page prints. divide waits on #1216 and modulus never reaches the reduction, so both are left out; the alternative of one paragraph in the prose docs is in that PR's description.
There was a problem hiding this comment.
Are you using these substrait functions with one of these engines? Just trying to understand context better 🙂 Thanks!
There was a problem hiding this comment.
Not me, no. But Spark (via substrait-java), Isthmus and DuckDB all write add:dec_dec from functions_arithmetic_decimal for decimal +, each declaring its own result type: here dec(38,9) from Spark, dec(38,10) from the other two. So removing the functions would break those plans, and of the three, this line matches only Spark. I'd drop the scale_reduction cases, if you agree. Run and versions: probe/decimal-rules.
There was a problem hiding this comment.
If substrait core spec is following SQL 99, the tests should be based on that. @vbarua
There was a problem hiding this comment.
SQL:1999 section 6.26 leaves result precision implementation-defined. The 38-digit cap and scale reduction are Substrait's choices. PR 1223 now makes this distinction explicit. These tests cover the existing return expressions.
The divide return expression writes P2 where Gandiva, which substrait-io#151 names as the reference these rules were fixed against, and Hive, Spark and Trino all write S2: dec(10,2) / dec(5,1) is dec(21,8) here and dec(17,8) in those three. Cases would settle that by assertion; substrait-io#1216 is open on it instead.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. ℹ️ Recent review info
Summary
Merge Risk:
 |
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title uses conventional commit syntax and clearly describes the addition of decimal arithmetic test cases. |
| Description check | |
The description explains the tested operations, coverage, omitted divide cases, and the related discrepancy. No breaking change is indicated, so the BREAKING CHANGE section is not required. |
| Docstring Coverage | |
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Review of substrait-io/substrait#1213 asks which engine the functions_arithmetic_decimal return expressions model, whether more than one agrees with them, and reports that Trino, MySQL and ClickHouse do not. probe/decimal-rules answers all three: rules.py transcribes the Substrait, Gandiva, Hive 4.0.1, Spark 4.0.1 and arrow-rs rules, each from its own source so that agreement is computed rather than shared, and compares them over all 606841 decimal operand type pairs; engines.py runs that pull request's cases on Spark, Hive, Trino, MySQL and ClickHouse, printing the type the formula derives beside the one the engine derived. add, subtract, multiply and modulus derive the same type as Gandiva, Hive and default Spark on every pair. divide writes P2 where all three write S2, and Trino answers S2 as well, so that row is the formula standing apart from its own reference rather than an operation each engine defines its own way. It is substrait-io/substrait#1216 seen from the other side. MySQL and ClickHouse differ about the type rather than the rule: MySQL has no bound at 38, and ClickHouse's precision is a storage width. Trino's decimal is Substrait's, and it differs twice - p1 + p2 for multiply, and keeping the scale above 38 where Spark and Hive borrow from it. That last one is the split the review found between Spark and DataFusion, and it runs two against two rather than one engine against a Spark setting.
Review of substrait-io/substrait#1213 asks which engine the functions_arithmetic_decimal return expressions model, whether more than one agrees with them, and reports that Trino, MySQL and ClickHouse do not. probe/decimal-rules answers all three: rules.py transcribes the Substrait, Gandiva, Hive 4.0.1, Spark 4.0.1 and arrow-rs rules, each from its own source so that agreement is computed rather than shared, and compares them over all 606841 decimal operand type pairs; engines.py runs that pull request's cases on Spark, Hive, Trino, MySQL and ClickHouse, printing the type the formula derives beside the one the engine derived. add, subtract, multiply and modulus derive the same type as Gandiva, Hive and default Spark on every pair. divide writes P2 where all three write S2, and Trino answers S2 as well, so that row is the formula standing apart from its own reference rather than an operation each engine defines its own way. It is substrait-io/substrait#1216 seen from the other side. MySQL and ClickHouse differ about the type rather than the rule: MySQL has no bound at 38, and ClickHouse's precision is a storage width. Trino's decimal is Substrait's, and it differs twice - p1 + p2 for multiply, and keeping the scale above 38 where Spark and Hive borrow from it. That last one is the split the review found between Spark and DataFusion, and it runs two against two rather than one engine against a Spark setting.
…tic-cases # Conflicts: # tests/baseline.json
functions_arithmetic_decimaldefines add, subtract, multiply and divide over decimals, and the cases under those names cover only the integer and floating-point overloads. The precision and scale formula had no case behind it.add,subtractandmultiplyeach get ordinary operands, where the result type follows from the formula and the arithmetic is exact, and a pair at scale 10, where the formula gives up scale to keep precision within 38. Each also gets an operand pair whose exact result is outside the returned type under any treatment of scale, withoverflow:ERROR, and null cases for one nullable operand and for both.divideis left out. Its precision writesP2where Gandiva, which #151 names as the reference these expressions were fixed against, and Hive, Spark and Trino all writeS2, sodec(10,2) / dec(5,1)isdec(21,8)here anddec(17,8)in those three. Cases would settle that by assertion; #1216 is open on it instead.Coverage moves from 282 to 285 of 531 function variants.
This change is