Add the "throwIfOverflow" flag to methods that may throw an exception due to overflow

[liujiayi771]

Currently, there are many methods in Velox that may encounter overflow during the calculation process. Velox currently defaults to throwing an exception when an overflow occurs. However, in Spark, it returns null when an overflow occurs. To resolve this conflict, the DecimalUtil in Spark has introduced corresponding methods that do not throw exceptions. To fundamentally address this issue, these methods need to have a "throwIfOverflow" flag added to them. Additionally, these functions are usually used in hot paths, so it may be necessary to consider making the flag a template parameter.

velox/velox/common/base/CheckedArithmetic.h

Lines 46 to 54 in fcc49a6

	template <typename T>
	T checkedMultiply(const T& a, const T& b, const char* typeName = "integer") {
	T result;
	bool overflow = __builtin_mul_overflow(a, b, &result);
	if (UNLIKELY(overflow)) {
	VELOX_ARITHMETIC_ERROR("{} overflow: {} * {}", typeName, a, b);
	}
	return result;
	}

velox/velox/functions/sparksql/DecimalUtil.h

Lines 105 to 110 in fcc49a6

	FOLLY_ALWAYS_INLINE static int128_t
	multiply(int128_t a, int128_t b, bool& overflow) {
	int128_t value;
	overflow = __builtin_mul_overflow(a, b, &value);
	return value;
	}

[liujiayi771]

@mbasmanova What do you think, should we add template parameters to these methods or introduce a new method?

[mbasmanova]

@liujiayi771 I'm not sure I understand, but there are version of these functions that do not check for overflow in velox/functions/prestosql/ArithmeticImpl.h

[liujiayi771]

Hi @mbasmanova. Functions in velox/functions/prestosql/ArithmeticImpl.h will produce incorrect results when encountering overflow. Spark needs to be aware of the occurrence of overflow during computation. For example, by using a flag parameter or returning std::nullopt to indicate overflow. In the case of decimal avg, the DecimalUtil::computeAverage method is called, which in turn invokes checkedMultiply. If overflow occurs, an exception will be thrown. Spark need to be able to detect overflow without throwing an exception, in which case it would return a null result. Currently, Spark decimal average requires catching exceptions, which significantly impacts performance and is an anti-pattern in Velox. Alternatively, we could implement a new computeAverage that calls a new non-exception-throwing checkedMultiply. We want to discuss with the community whether to implement new approaches for Spark or modify existing methods to support not throwing exception, using templates or flags.

[mbasmanova]

@liujiayi771 I feel this discussion is a bit too abstract. Let's fix DecimalUtil::computeAverage and whatever else needs fixing. If we start seeing a pattern emerging, we can refactor.