Introduce DecimalUtil::adjustSumForOverflow helper method #7334

liujiayi771 · 2023-10-31T02:47:47Z

The sum aggregate function in Spark and Presto uses this logic to adjust the sum
computed via DecimalUtil::addWithOverflow.

netlify · 2023-10-31T02:47:52Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`a20e28e`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/6554309a463bbe0008d36a0d

liujiayi771 · 2023-10-31T02:52:29Z

@mbasmanova Could you help review this PR? #6020 and #5372 will use this common method.

cc @rui-mo

velox/type/DecimalUtil.h

mbasmanova · 2023-10-31T10:39:56Z

@rui-mo Rui, thank you for revewing.
@liujiayi771 Ping me again once Rui's comments are addressed and she is happy with the changes. Thanks.

rui-mo

Thanks for fixing the comments.

velox/functions/lib/aggregates/tests/DecimalAggregateTest.cpp

liujiayi771 · 2023-11-03T02:56:14Z

Hi @mbasmanova. Please continue to take a look.

mbasmanova · 2023-11-03T13:12:52Z

velox/functions/lib/aggregates/SumAggregateBase.h

-
-    DecimalUtil::valueInRange(sum);
-    return sum;
+    auto sum = computeValidSum(accumulator->sum, accumulator->overflow);


Assuming this logic is Spark-specific, can we move it to functions/sparksql?

It is not only Spark that requires this logic, but Presto also needs it.

@liujiayi771 I see. Would you then update PR title and description to explain that this logic is generic and applies to both Presto and Spark. BTW, should there be some changes in the PR for the Presto logic, no?

Presto directly uses the computeFinalValue function in functions/lib/aggregates/SumAggregateBase.h. Therefore, modifying the call in SumAggregateBase.h should be sufficient.

velox/functions/lib/aggregates/DecimalAggregate.h

mbasmanova · 2023-11-03T13:15:03Z

velox/functions/lib/aggregates/DecimalAggregate.h

+ */
+inline static std::optional<int128_t> computeValidSum(
+    int128_t sum,
+    int64_t overflow) {


Given that overflow has only 3 values: 0, 1, -1; perhaps, it would be more readable to use an enum.

overflow can be greater than 1 or less than -1. In the actual calculation, the overflow in the accumulator accumulates continuously, but each time it can only increase by 1 or decrease by 1.

velox/velox/functions/lib/aggregates/DecimalAggregate.h

Lines 327 to 328 in fcb9a37

accumulator->overflow +=

DecimalUtil::addWithOverflow(accumulator->sum, value, accumulator->sum);

I see. The comment talked only about 1 and -1 which confused me. I also noticed that addWithOverflow doesn't have any comments. Would you add documentation to addWithOverflow? This is a tricky topic that is very hard to get right. Let's make sure we explain what we do clearly for future readers.

OK. I will try to add some comments.

velox/functions/lib/aggregates/DecimalAggregate.h

mbasmanova · 2023-11-03T13:18:14Z

velox/functions/lib/aggregates/DecimalAggregate.h

+ * -(1 << 127) + sum.
+ *
+ */
+inline static std::optional<int128_t> computeValidSum(


How about, adjustSumForOverflow or something that doesn't use term 'valid'?

mbasmanova · 2023-11-03T14:39:03Z

velox/functions/lib/aggregates/SumAggregateBase.h

-    return sum;
+    auto sum = DecimalUtil::adjustSumForOverflow(
+        accumulator->sum, accumulator->overflow);
+    VELOX_CHECK(sum.has_value(), "Decimal overflow");


This should be user error, no?

velox/functions/lib/aggregates/SumAggregateBase.h

velox/type/DecimalUtil.h

mbasmanova · 2023-11-03T14:42:33Z

velox/type/DecimalUtil.h

@@ -271,6 +271,37 @@ class DecimalUtil {
    return overflow;
  }

+  /// This method is used to correct the sum result calculated using
+  /// addWithOverflow. In the addWithOverflow method, when summing two int128_t


Let's add comments to addWithOverflow so we can refer to them here instead of repeating. Let's also clarify in comments for addWithOverflow that results should be post-processed using this method (adjustSumForOverflow).

velox/type/DecimalUtil.h

mbasmanova · 2023-11-03T14:48:55Z

velox/type/tests/DecimalTest.cpp

+  SumWithOverflow accumulator;
+  // Int128_t.max + 100 will trigger one upward overflow, but the final sum
+  // result calculated by DecimalUtil::addWithOverflow will be negative.
+  std::vector<int128_t> sumOverflowUpwardOnceInputs = {


this variable name is very long; maybe just 'inputs' or:

add(accumulator, std::numeric_limits<int128_t>::max()); add(accumulator, 100); add(accumulator, std::numeric_limits<int128_t>::min()); EXPECT_EQ(accumulator.result(), 99);

Add add(int128_t) method in SumWithOverflow.

velox/type/tests/DecimalTest.cpp

liujiayi771 · 2023-11-08T11:47:08Z

Hi @mbasmanova. The comments for the function have been revised. Could you help to take a look, thanks.

mbasmanova · 2023-11-08T12:15:17Z

velox/type/tests/DecimalTest.cpp

+  accumulator.add(std::numeric_limits<int128_t>::max());
+  accumulator.add(100);
+  accumulator.add(std::numeric_limits<int128_t>::min());
+  EXPECT_EQ(accumulator.adjustedSum(), 99);


The comment above says "result will be negative" , but the values here is positive (99). This is confusing. Is the comment correct? If so, would it be possible to clarify to avoid confusing future readers?

Here, it refers to the fact that the sum before "adjust" is negative. I will fix the comment more explicit.

mbasmanova · 2023-11-08T12:15:34Z

velox/type/tests/DecimalTest.cpp

+  EXPECT_EQ(accumulator.adjustedSum(), -101);
+
+  accumulator.reset();
+  // These inputs will eventually trigger an overflow, and computeValidSum will


reference to computeValidSum is not valid

liujiayi771 · 2023-11-09T15:49:36Z

@Yuhta This is because there is an issue with the input of my test case. -std::numeric_limits<int128_t>::min() is not valid. I have changed the input to std::numeric_limits<int128_t>::min() + 10, so that -(std::numeric_limits<int128_t>::min() + 10) is valid.

facebook-github-bot · 2023-11-09T15:50:37Z

@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

liujiayi771 · 2023-11-09T15:51:18Z

@Yuhta Can I execute this test locally?

mbasmanova · 2023-11-09T15:51:19Z

This is because there is an issue with the input of my test case.

@liujiayi771 Does that mean the code may still fail in production where we do not control the inputs?

liujiayi771 · 2023-11-09T15:56:10Z

@mbasmanova This related to the algorithm of addWithOverflow. In this case, it is indeed invalid for both -lhs and -rhs when they equal to std::numeric_limits<int128_t>::min(). However, is this scenario common in the code? Every -value has the potential to encounter this issue.

liujiayi771 · 2023-11-09T16:14:50Z

@mbasmanova Here, std::numeric_limits<int128_t>::min() cannot be an input for addWithOverflow because it is only used for decimal agg. The input will be restricted within the range of [kLongDecimalMin, kLongDecimalMax]. Perhaps it is better to construct a case within this range that causes an overflow, although the current test case already verify the functionality of adjustSumForOverflow.

mbasmanova · 2023-11-09T16:18:40Z

@mbasmanova Here, std::numeric_limits<int128_t>::min() cannot be an input for addWithOverflow because it is only used for decimal agg. The input will be restricted within the range of [kLongDecimalMin, kLongDecimalMax].

I see. Can we add a VELOX_DCHECK for the input then?

liujiayi771 · 2023-11-09T16:21:12Z

I see. Can we add a VELOX_DCHECK for the input then?

OK. And I will modify the test case inputs.

liujiayi771 · 2023-11-09T16:31:03Z

@mbasmanova There are still issues present, because the accumulation result of long decimals may exceed the limits of [kLongDecimalMin, kLongDecimalMax]. There are some extreme cases, for example, when the addition of multiple long decimal inputs precisely calculates to int128.min, and the next input to be accumulated is also negative, there is still a possibility of errors occurring.

liujiayi771 · 2023-11-09T16:36:54Z

Can we add these DCHECKs?

VELOX_DCHECK_NE(lhs, std::numeric_limits<int128_t>::min());
VELOX_DCHECK_NE(rhs, std::numeric_limits<int128_t>::min());
overflow = addUnsignedValues(result, -lhs, -rhs, true);

liujiayi771 · 2023-11-10T14:13:59Z

@mbasmanova Hi, I add the DCHECK and change the test inputs.

mbasmanova · 2023-11-10T14:15:19Z

@liujiayi771 CI is red. Please, take a look.

liujiayi771 · 2023-11-10T14:18:16Z

@mbasmanova It is the spark function computeIsAsciiForInputs failed. It is not related to my changes. It seems like the fuzzer test generated a null varchar.

Context: instr(least(null:VARCHAR, lower(substring_index(c0, conv(least(get_json_object(c1, sha1(greatest(c2, pN65R^_lJF&y~@G"H-Rn2n]/1<i|!9v~Is<G-upl39@bJK`)SAAhQc$i'WNZn):VARBINARY))), least(sha1(greatest(c2, pN65R^_lJF&y~@G"H-Rn2n]/1<i|!9v~Is<G-upl39@bJK`)SAAhQc$i'WNZn):VARBINARY)), upper(sha1(greatest(c2, pN65R^_lJF&y~@G"H-Rn2n]/1<i|!9v~Is<G-upl39@bJK`)SAAhQc$i'WNZn):VARBINARY))), substring(c3, 32629329:INTEGER, null:INTEGER))), greatest(shiftleft(c4, c5), c6, c7, round(c8)), c9), round(c8))), least(concat(c10, rpad(sha1(c11), 281289484:INTEGER, least(get_json_object(c1, sha1(greatest(c2, pN65R^_lJF&y~@G"H-Rn2n]/1<i|!9v~Is<G-upl39@bJK`)SAAhQc$i'WNZn):VARBINARY))), least(sha1(greatest(c2, pN65R^_lJF&y~@G"H-Rn2n]/1<i|!9v~Is<G-upl39@bJK`)SAAhQc$i'WNZn):VARBINARY)), upper(sha1(greatest(c2, pN65R^_lJF&y~@G"H-Rn2n]/1<i|!9v~Is<G-upl39@bJK`)SAAhQc$i'WNZn):VARBINARY))), substring(c3, 32629329:INTEGER, null:INTEGER))))), get_json_object(c12, overlay(110111011010111101101110111011100010101000010000010010011100:VARCHAR, null:VARCHAR, greatest(shiftleft(c4, c5), c6, c7, round(c8)), 712476581:INTEGER)), upper(upper(rpad(concat(greatest(c1, c12, c0, null:VARCHAR, c13, ulAA9/W6=q]8D]ILdD6={DLGn_oeY6%GGZpf}B2\yl-]@?kSZ>SVu<P8Y9,PBp&pn1ID0&`']:VARCHAR, null:VARCHAR), c14, 110111011010111101101110111011100010101000010000010010011100:VARCHAR, Y5~'+0/@f;jg:VARCHAR), 85151036:INTEGER, get_json_object(c1, sha1(greatest(c2, pN65R^_lJF&y~@G"H-Rn2n]/1<i|!9v~Is<G-upl39@bJK`)SAAhQc$i'WNZn):VARBINARY)))))), rtrim(substring(g>?R.9[744T+.xdF+X<78g<V_(v&!%laVr;<j;E9G]'`"II<jO?"/:VARCHAR, round(c8), 1375767395:INTEGER)), null:VARCHAR)), c15)
Top-Level Context: Same as context.
Function: computeIsAsciiForInputs
File: ../../velox/expression/Expr.cpp
Line: 1263
Stack trace:

mbasmanova · 2023-11-10T14:20:40Z

It is the spark function computeIsAsciiForInputs failed. It is not related to my changes

Is there an existing GitHub issue about this? I don't think computeIsAsciiForInputs is a Spark function.

If there is no existing issue, please, create one and work with the Gluten team to prioritize fixing.

facebook-github-bot · 2023-11-10T14:21:14Z

@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

liujiayi771 · 2023-11-10T14:25:06Z

If there is no existing issue, please, create one and work with the Gluten team to prioritize fixing.

Alright, the failure is in Spark Fuzzer test, it is not a Spark function. Based on the code modification history, it seems like there have been issues here before. Perhaps Presto's Fuzzer test could also have this error. I will collaborate with the Gluten team to try to resolve it.
#7032
#7009

liujiayi771 · 2023-11-14T14:25:18Z

Hi @mbasmanova. Any issues during the merge process?

mbasmanova · 2023-11-14T14:29:45Z

@liujiayi771 Can't get an internal approval to merge. Will try some more. CC: @Yuhta

mbasmanova · 2023-11-14T14:59:29Z

@liujiayi771 Got an approval, but the diff is too old now. Would you rebase?

liujiayi771 · 2023-11-15T04:38:13Z

@liujiayi771 Got an approval, but the diff is too old now. Would you rebase?

Done. Thanks.

facebook-github-bot · 2023-11-16T07:46:31Z

@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2023-11-16T12:11:53Z

@mbasmanova merged this pull request in 8550a15.

conbench-facebook · 2023-11-16T12:51:34Z

Conbench analyzed the 1 benchmark run on commit 8550a15c.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 31, 2023

liujiayi771 force-pushed the compute-valid-sum branch from f5eca90 to e858a25 Compare October 31, 2023 02:50

rui-mo reviewed Oct 31, 2023

View reviewed changes

velox/type/DecimalUtil.h Outdated Show resolved Hide resolved

velox/type/DecimalUtil.h Outdated Show resolved Hide resolved

liujiayi771 force-pushed the compute-valid-sum branch 3 times, most recently from c6bb4cd to 0cdb26e Compare November 1, 2023 05:01

rui-mo reviewed Nov 3, 2023

View reviewed changes

velox/functions/lib/aggregates/tests/DecimalAggregateTest.cpp Outdated Show resolved Hide resolved

velox/functions/lib/aggregates/tests/DecimalAggregateTest.cpp Outdated Show resolved Hide resolved

velox/functions/lib/aggregates/tests/DecimalAggregateTest.cpp Outdated Show resolved Hide resolved