Support decimal operation precision not loss mode #10383

jinchengchenghh · 2024-07-03T02:01:36Z

Register the decimal functions by setting SparkRegistrationConfig.allowPrecisionLoss to false to enable this feature.
Spark implementation: https://github.com/apache/spark/blob/branch-3.5/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala#L814

netlify · 2024-07-03T02:01:52Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`204cab3`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/66f381fd7d6e7c000929ad26

jinchengchenghh · 2024-07-03T06:29:36Z

Can you help review this PR? Thanks! @rui-mo @PHILO-HE

jinchengchenghh · 2024-07-09T01:44:55Z

hi @rui-mo @PHILO-HE Can I take your time to help review this PR?

rui-mo

Thanks! Added several comments.

rui-mo · 2024-07-05T09:54:56Z

velox/functions/sparksql/DecimalArithmetic.cpp

+}
+
+std::pair<std::string, std::string>
+getNotAllowPrecisionLossDivideResultScale() {


nit: name suggestion: getStrictDivideResultScale, and also document the difference with AllowPrecisionLoss being true.

rui-mo · 2024-07-09T01:58:23Z

velox/core/QueryConfig.h

+  /// rounding the decimal part of the result if an exact representation is not
+  /// possible. Otherwise, NULL is returned in those cases, as previously
+  static constexpr const char* kSparkDecimalOperationsAllowPrecisionLoss =
+      "spark.decimal_operations.allow_precision_loss";


Perhaps document this config and corresponding calculation for each arithmetic operation in https://github.com/facebookincubator/velox/blob/main/velox/docs/functions/spark/decimal.rst#division.

rui-mo · 2024-07-09T01:58:42Z

velox/core/QueryConfig.h

@@ -304,6 +304,13 @@ class QueryConfig {
  /// The current spark partition id.
  static constexpr const char* kSparkPartitionId = "spark.partition_id";

+  /// When true, establishing the result type of an arithmetic operation
+  /// happens according to Hive behavior and SQL ANSI 2011 specification, i.e.


nit: remove 'happens'

rui-mo · 2024-07-09T02:03:10Z

velox/core/QueryConfig.h

+  /// When true, establishing the result type of an arithmetic operation
+  /// happens according to Hive behavior and SQL ANSI 2011 specification, i.e.
+  /// rounding the decimal part of the result if an exact representation is not
+  /// possible. Otherwise, NULL is returned in those cases, as previously


Otherwise, NULL is returned in those cases, as previously

nit: comment suggestion: NULL is returned when the actual result cannot be represented with the calculated decimal type.

rui-mo · 2024-07-09T02:03:40Z

velox/docs/configs.rst

+     - bool
+     - When true, establishing the result type of an arithmetic operation happens according to Hive behavior and SQL ANSI 2011 specification, i.e.
+       rounding the decimal part of the result if an exact representation is not
+       possible. Otherwise, NULL is returned in those cases, as previously


velox/functions/sparksql/DecimalUtil.h

rui-mo · 2024-07-09T02:16:21Z

velox/functions/sparksql/DecimalArithmetic.cpp

+    createDecimalFunction<Divide>);
+
+VELOX_DECLARE_STATEFUL_VECTOR_FUNCTION(
+    udf_decimal_add_not_allow_precision_loss,


This decimal function signature is only used in test, so register it in test scope.
Downstream project can only set the query config to enable this feature.

Looks like the declaration is only used by test code. Can we also test the false behavior via setting the query config?

I have added https://github.com/facebookincubator/velox/pull/10383/files#diff-257bd1947469dfd8494c7e19f535d4a3247b3df086287110e53740c199104073R550

Do we still need the declarations and registrations then? I wonder if it makes sense to register them as special forms if the result type of signature needs to determined at runtime.
https://github.com/facebookincubator/velox/pull/10383/files#diff-257bd1947469dfd8494c7e19f535d4a3247b3df086287110e53740c199104073R27-R37

Yes, it is different, with the registrations , we can test by the function name.
I prefer not, current implementation can cover the feature. We need to do many refactors if switching to special form. But if the community decides to do that, I will help to refactor.

I feel these declarations need to be reconsidered for below reasons.

they are in the runtime code but are only used by tests.

their signatures are generated with allowPrecisionLoss being false, but createDecimalFunction follows the query config to decide if the function allows precision loss. Therefore, its behavior can vary according to config but its signature is fixed.

To solve above issues, maybe we need to register them as special forms. How do you think? Thanks.

And it still has a problem, we need to register the decimal add name from add to decimal_add because the name should be unique in special form, can we register it as add for substrait plan conversion and downstream project plan conversion?

We should also change here https://github.com/facebookincubator/velox/blob/main/velox/expression/ExprCompiler.cpp#L407 to break the special form name is unique, check if we can constructSpecialForm
Can we do that? @mbasmanova @rui-mo

Which way is better?

current implementation;

special form registered as decimal_add, substrait function resolution unique logic;

special form resgistered as add, break special form name unique constraint.

Refactor to current version, remove the most of the signatures, only keep input type as decimal and output type as unknown, construct Expr to test.

Now the function signature requires returnType, but only input type and function name should specify the function, this also aligns with the sql function definition, we query by select hash(col); without return type.
And we don't permit the function with same argument and different return type, so I propose to change returnType in FunctionSignature to be optional, and for decimal functions, we don't need to supply returnType, the signature will be

return {exec::FunctionSignatureBuilder() .integerVariable("a_precision") .integerVariable("a_scale") .integerVariable("b_precision") .integerVariable("b_scale") .argumentType("DECIMAL(a_precision, a_scale)") .argumentType("DECIMAL(b_precision, b_scale)") .build()};

@mbasmanova How about that?

CC @zhouyuan

jinchengchenghh · 2024-07-09T03:40:08Z

Thanks for your comments, I have address all the comments. @rui-mo

PHILO-HE · 2024-07-09T06:28:17Z

velox/docs/functions/spark/decimal.rst

@@ -43,6 +57,8 @@ precision and scale are adjusted.
    precision = 38
    scale = max(38 - (p - s), min(s, 6))

+Caps p ans s at 38 when not allowing precision loss.


Looks a typo: ans

PHILO-HE · 2024-07-09T06:50:31Z

velox/functions/sparksql/tests/DecimalArithmeticTest.cpp

@@ -522,5 +545,50 @@ TEST_F(DecimalArithmeticTest, decimalDivTest) {
      {makeConstant<int128_t>(
          DecimalUtil::kLongDecimalMax, 1, DECIMAL(38, 0))});
 }
+
+TEST_F(DecimalArithmeticTest, notAllowPrecisionLoss) {
+  setDecimalOperationsAllowPrecisionLoss(false);


I note the function name already contains "not_allow_precision_loss", is the above setting still necessary?

Yes, because we construct the class Function by this config https://github.com/facebookincubator/velox/pull/10383/files#diff-4baee9f82f9c347f753972415d345941d2c2a110b608d480b090650171da096bR754

And this function name only exists in test code

PHILO-HE · 2024-07-09T07:03:08Z

velox/core/QueryConfig.h

+  /// When true, establishing the result type of an arithmetic operation
+  /// according to Hive behavior and SQL ANSI 2011 specification, i.e.
+  /// rounding the decimal part of the result if an exact representation is not
+  /// possible. Otherwise, NULL is returned when the actual result cannot be


Can you add some tests for NULL result case?

rui-mo · 2024-07-09T06:05:24Z

velox/core/QueryConfig.h

+  /// possible. Otherwise, NULL is returned when the actual result cannot be
+  /// represented with the calculated decimal type.
+  static constexpr const char* kSparkDecimalOperationsAllowPrecisionLoss =
+      "spark.decimal_operations.allow_precision_loss";


I find this config also controls the behavior of remainder and pmod in Spark. Maybe we need to clarify which operations are supported for now.

rui-mo · 2024-07-10T09:30:18Z

velox/functions/sparksql/DecimalArithmetic.cpp

+    createDecimalFunction<Divide>);
+
+VELOX_DECLARE_STATEFUL_VECTOR_FUNCTION(
+    udf_decimal_add_not_allow_precision_loss,


I feel these declarations need to be reconsidered for below reasons.

they are in the runtime code but are only used by tests.

their signatures are generated with allowPrecisionLoss being false, but createDecimalFunction follows the query config to decide if the function allows precision loss. Therefore, its behavior can vary according to config but its signature is fixed.

To solve above issues, maybe we need to register them as special forms. How do you think? Thanks.

mbasmanova · 2024-07-17T08:48:36Z

velox/functions/sparksql/DecimalArithmetic.cpp

@@ -778,25 +728,40 @@ std::shared_ptr<exec::VectorFunction> createDecimalFunction(
    }
  }
 }
+
+// Find the function according to input types under expression compiler.
+// Mark the return type as unknown, the result type will be computed by Function


This is a hack. Let's not do this.

It seems like function signatures are affected by configuration settings. If that's the case, let's come up with a way to allow access to configs during function registration time.

We do need to be able to resolve function calls without instantiating functions. This is what enables Fuzzer and unit testing.

…ss mode (10383) facebookincubator#10383

FelixYBW · 2024-07-25T04:06:41Z

@jinchengchenghh The PR is used by oap/velox, can you follow up?

…ss mode (10383) facebookincubator#10383

jinchengchenghh · 2024-07-31T05:34:38Z

Address all the comments, can you help review again? @rui-mo

rui-mo

Thanks. Added several comments on the documentation.

rui-mo · 2024-07-31T07:03:42Z

velox/docs/functions/spark/config.rst

+SparkRegistration Configuration properties
+========================
+
+Generic Configuration


allowPrecisionLoss only applies to several functions so it is not a generic configuration. I feel this document might be not needed as the description in struct SparkRegistrationConfig is clear enough.

velox/docs/functions/spark/decimal.rst

rui-mo · 2024-07-31T07:21:45Z

velox/docs/functions/spark/decimal.rst

 ::

    p = p1 - s1 + s2 + max(6, s1 + p2 + 1)
    s = max(6, s1 + p2 + 1)

+When config ``SparkRegistrationConfig.allowPrecisionLoss`` is set to false.


Document suggestion:

If the function is registered with ``allowPrecisionLoss`` being false:

rui-mo · 2024-07-31T07:22:16Z

velox/docs/functions/spark/decimal.rst

+::
+
+    intDig = min(38, p1 - s1 + s2);
+    decDig = min(38, max(6, s1 + p2 + 1));


nit: add one empty line below.

rui-mo · 2024-07-31T07:22:24Z

velox/docs/functions/spark/decimal.rst

+If ``indDig + decDig`` is more than 38:
+::
+    p = 38
+    s = decDig - (intDig + decDig - 38) / 2 - 1


velox/docs/functions/spark/decimal.rst

rui-mo · 2024-07-31T07:30:56Z

velox/docs/functions/spark/decimal.rst

@@ -27,14 +27,28 @@ Multiplication

 Division
 --------
-
+When config ``SparkRegistrationConfig.allowPrecisionLoss`` is set to true as default.


Document suggestion:

If the function is registered with ``allowPrecisionLoss`` being true (default behavior):

rui-mo · 2024-07-31T07:35:22Z

velox/docs/functions/spark/decimal.rst

@@ -27,14 +27,28 @@ Multiplication

 Division
 --------
-
+When config ``SparkRegistrationConfig.allowPrecisionLoss`` is set to true as default.


Since the description at L5 only covers part of case now, would you add some introduction for allowPrecisionLoss being false there?

For add, subtract, multiply, the precision and scale computation is same, only adjust or bounded them.
But for divide, the precision and scale computation is different, so I only add for divide.
And document bounded behavior in the below.

rui-mo · 2024-07-31T08:05:02Z

velox/functions/sparksql/DecimalArithmetic.cpp

-    typename A /* Argument1 */,
-    typename B /* Argument2 */,
-    typename Operation /* Arithmetic operation */>
-class DecimalBaseFunction : public exec::VectorFunction {


This PR also changes vector functions as simple functions. Does it make sense to do the vector-to-simple refactor in a separate PR, and support allowPrecisionLoss in this one?

Do you agree? If you think it is OK, I will only use SparkRegistrationConfig in this PR and revert to vector function. @mbasmanova

Perhaps we don't need to change back to vector function in this PR. After the separate PR lands, we can do a rebase to it.

+1 to @rui-mo 's suggestion

rui-mo · 2024-07-31T08:09:15Z

velox/functions/sparksql/DecimalFunctions.cpp

+      return DecimalUtil::adjustPrecisionScale(precision, scale);
+    } else {
+      auto intDig = std::min(38, aPrecision - aScale + bScale);
+      auto decDig = std::min(38, std::max(6, aScale + bPrecision + 1));


Name suggestion:
intDig -> wholeDigits, decDig -> fractionDigits

rui-mo · 2024-07-31T08:11:02Z

velox/functions/sparksql/DecimalFunctions.cpp

+  }
+}
+
+std::vector<exec::SignatureVariable> getDivideConstraitsAllowPrecisionLoss() {


Constraits -> Constraints, ditto for the others.

…ss mode (10383) facebookincubator#10383

jinchengchenghh · 2024-08-28T05:43:44Z

Do you have any more comments? @rui-mo

rui-mo · 2024-08-28T14:09:55Z

velox/docs/functions/spark/config.rst

@@ -0,0 +1,21 @@
+================================
+SparkRegistration Configuration


Title suggestion:

Spark function registration configurations

rui-mo · 2024-08-28T14:11:05Z

velox/docs/functions/spark/config.rst

+================================
+
+struct SparkRegistrationConfig
+---------------------


nit: make the - of the same length as the letters above it.

velox/docs/functions/spark/config.rst

rui-mo · 2024-08-28T14:15:37Z

velox/functions/sparksql/DecimalArithmetic.cpp

  // If result is representable with long decimal, the result
  // scale is the maximum of 'aScale' and 'bScale'. If not, reduces result scale
  // and caps the result precision at 38.
+  // Caps p and s at 38 when not allowing precision loss.


Do we Caps p and s at 38 when allowPrecisionLoss being true or false?

Not allow precision and scale loss mode just Caps p and s at 38 while another one may reduce the scale.

zhouyuan · 2024-08-28T14:20:57Z

velox/functions/sparksql/RegistrationConfig.h

+  /// possible. Otherwise, NULL is returned when the actual result cannot be
+  /// represented with the calculated decimal type. Now we support add,
+  /// subtract, multiply and divide operations.
+  bool allowPrecisionLoss = true;


it seems try to enabled per-function config? in Spark this should be a session level config and users may modify it

Looks like current implementation only provides fixed-behavior after the functions are registered. We might need query config if we need to make them adjustable at runtime.

rui-mo · 2024-08-29T08:44:28Z

velox/docs/functions/spark/config.rst

+==========================================
+
+struct SparkRegistrationConfig
+------------------------------


Perhaps remove this subtitle as it is more like an implementation stuff.

rui-mo · 2024-08-29T08:49:03Z

velox/docs/functions/spark/config.rst

+     - When true, establishing the result type of an arithmetic operation according to Hive behavior and SQL ANSI 2011 specification, i.e.
+       rounding the decimal part of the result if an exact representation is not
+       possible. Otherwise, NULL is returned when the actual result cannot be represented with the calculated decimal type. Now we support add,
+       subtract, multiply and divide operations.


Doc suggestion:

This configuration is currently in use for decimal add, subtract, multiply and divide operations.

rui-mo · 2024-08-29T08:58:59Z

velox/docs/functions/spark/decimal.rst

@@ -43,6 +46,26 @@ precision and scale are adjusted.
    precision = 38
    scale = max(38 - (p - s), min(s, 6))

+Caps p and s at 38 when not allowing precision loss.


Is it the case as below?

When the precision of result exceeds 38, caps p at 38 and the scale should not be reduced when not allowing precision loss.

rui-mo · 2024-08-29T09:04:35Z

velox/docs/functions/spark/decimal.rst

+::
+
+    wholeDigits = min(38, p1 - s1 + s2);
+    fractionalDigits = min(38, max(6, s1 + p2 + 1));


The Decimal Precision and Scale Adjustment section discusses how to handle the case when the calculated precision exceeds 38, so I assume this part and L66 belong elsewhere (L33 I think). Perhaps only keep the description for precision and scale adjustment at L57.

For decimal addition, subtraction, multiplication, the precision and scale computation logic is same

This can be described in the corresponding location, such as L12 or L20.

rui-mo · 2024-08-29T09:09:20Z

velox/docs/spark_functions.rst

@@ -4,6 +4,8 @@ Spark Functions

 The semantics of Spark functions match Spark 3.5 with ANSI OFF.

+Spark functions can be registered by :doc:`struct SparkRegistrationConfig <functions/spark/config>`.


Doc suggestion:
Spark functions can be registered with :doc:`user specified configurations <functions/spark/config>`.

rui-mo · 2024-08-29T09:15:06Z

velox/functions/sparksql/RegistrationConfig.h

+  /// possible. Otherwise, NULL is returned when the actual result cannot be
+  /// represented with the calculated decimal type. Now we support add,
+  /// subtract, multiply and divide operations.
+  bool allowPrecisionLoss = true;


Looks like current implementation only provides fixed-behavior after the functions are registered. We might need query config if we need to make them adjustable at runtime.

jinchengchenghh · 2024-08-30T02:57:57Z

As this discussion #10467, we cannot set it at runtime. @rui-mo

jinchengchenghh · 2024-09-03T08:06:51Z

Any more comments? Thanks! @rui-mo

jinchengchenghh · 2024-09-10T01:26:35Z

Soft reminder, do you have any more comment? @rui-mo

rui-mo · 2024-09-10T02:28:59Z

Talked with @jinchengchenghh offline, and she will help update this PR to support session-level precision loss mode.

jinchengchenghh · 2024-09-13T03:08:48Z

Updated, thanks! @rui-mo

rui-mo

Thanks. Added several comments on the documentation, and will take a look at the implementation later on.

rui-mo · 2024-09-13T09:28:22Z

velox/docs/functions/spark/decimal.rst

@@ -9,6 +9,8 @@ https://cwiki.apache.org/confluence/download/attachments/27362075/Hive_Decimal_P

 https://msdn.microsoft.com/en-us/library/ms190476.aspx

+For decimal addition, subtraction, multiplication, the precision and scale computation logic is same.


Comment suggestion:

Additionally, the computation of decimal division adapts to the allowing precision loss parameter, while the decimal addition, subtraction, and multiplication do not.

rui-mo · 2024-09-13T09:29:57Z

velox/docs/functions/spark/decimal.rst


 ::

    p = p1 - s1 + s2 + max(6, s1 + p2 + 1)
    s = max(6, s1 + p2 + 1)

+When not allowing precision loss:


When denying precision loss:

rui-mo · 2024-09-13T09:31:33Z

velox/docs/functions/spark/decimal.rst

+
+Decimal Precision and Scale Adjustment
+<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
+
 For above arithmetic operators, when the precision of result exceeds 38,


when the precision of result exceeds 38 and precision loss is allowed,

rui-mo · 2024-09-13T09:31:56Z

velox/docs/functions/spark/decimal.rst

 For above arithmetic operators, when the precision of result exceeds 38,
-caps p at 38 and reduces the scale, in order to prevent the truncation of
+caps p at 38 and reduces the scale when allowing precision loss, in order to prevent the truncation of


remove ' when allowing precision loss'.

rui-mo · 2024-09-13T09:33:16Z

velox/docs/functions/spark/decimal.rst

@@ -43,6 +57,20 @@ precision and scale are adjusted.
    precision = 38
    scale = max(38 - (p - s), min(s, 6))

+Caps p at 38 and the scale should not be reduced when not allowing precision loss.


When precision loss is denied, caps p at 38, and the scale should not be reduced.

rui-mo · 2024-09-13T09:34:17Z

velox/docs/functions/spark/decimal.rst

@@ -43,6 +57,20 @@ precision and scale are adjusted.
    precision = 38
    scale = max(38 - (p - s), min(s, 6))

+Caps p at 38 and the scale should not be reduced when not allowing precision loss.
+For decimal addition, subtraction, multiplication, the adjustment logic is same.


The below formula shows how the precision and scale are adjusted for decimal addition, subtraction, and multiplication.

rui-mo · 2024-09-13T09:36:08Z

velox/docs/functions/spark/decimal.rst

+
+::
+    precision = 38
+    scale = min(38, s)


This part does not render correctly. Perhaps one empty line after L63 is needed.

rui-mo · 2024-09-13T09:37:26Z

velox/docs/functions/spark/decimal.rst

+    precision = 38
+    scale = min(38, s)
+
+But for decimal division, it is different as following:


Decimal division uses a different formula, which is as follows:

rui-mo

Thanks.

rui-mo · 2024-09-14T02:03:36Z

velox/functions/sparksql/DecimalArithmetic.cpp

@@ -253,10 +254,12 @@ struct DecimalAddSubtractBase {
  }

  // Computes the result precision and scale for decimal add and subtract
-  // operations following Hive's formulas.
+  // operations following Hive's formulas when `allowPrecisionLoss` is true.
  // If result is representable with long decimal, the result
  // scale is the maximum of 'aScale' and 'bScale'. If not, reduces result scale


The difference is only on whether 'adjustPrecisionScale' or 'bounded' is called, so we need to move the change on L257 perhaps before 'If not'.

rui-mo · 2024-09-14T02:05:16Z

velox/functions/sparksql/DecimalArithmetic.cpp

@@ -330,8 +337,10 @@ struct DecimalMultiplyFunction {
      B* /*b*/) {
    auto [aPrecision, aScale] = getDecimalPrecisionScale(*inputTypes[0]);
    auto [bPrecision, bScale] = getDecimalPrecisionScale(*inputTypes[1]);
-    auto [rPrecision, rScale] = DecimalUtil::adjustPrecisionScale(
-        aPrecision + bPrecision + 1, aScale + bScale);
+    auto [rPrecision, rScale] = allowPrecisionLoss


nit: if constexpr (allowPrecisionLoss)

velox/functions/sparksql/DecimalArithmetic.cpp

rui-mo · 2024-09-14T02:26:46Z

velox/functions/sparksql/DecimalArithmetic.cpp

+bool registerFunction(
+    const std::vector<std::string>& aliases = {},
+    const std::vector<exec::SignatureVariable>& constraints = {},
+    bool overwrite = true) {


This function seems to be the same as an existing function. Can we reuse it?

velox/velox/functions/Registerer.h

Line 63 in e13c591

bool registerFunction(

No, template bool is different, and using funcClass = typename Func::template udf<exec::VectorExec>; is different, so I add new binder to support template function with variable bool.

template <template <class, bool> typename T, bool allowPrecisionLoss> using ParameterBinder = TempWrapper<T<exec::VectorExec, allowPrecisionLoss>>;

I wonder if we should move it to velox/velox/functions/Registerer.h in a separate PR?
It could allow function with constraints accept template.

It seems compared with the below function, the new one added 'constraints'. It this the only difference? Thanks.

velox/velox/functions/Registerer.h

Lines 45 to 47 in e13c591

bool registerFunction(

const std::vector<std::string>& aliases = {},

bool overwrite = true) {

Yes. I wonder if we can consolidate these APIs.

I agree. Perhaps we can change the API in a separate PR.

Do we need to only keep this one or keep both functions but the latter changes from Func<exec::VectorExec> to typename Func::template udf<exec::VectorExec>, for the backward compatibility, I would prefer keep both. The first could revoke the latter with the argument constraints = {}.

bool registerFunction( const std::vector<std::string>& aliases = {}, const std::vector<exec::SignatureVariable>& constraints = {}, bool overwrite = true) { using funcClass = typename Func::template udf<exec::VectorExec>; }

As the comment indicates, the wrapper for udf struct will be removed finally, so can I do that? @mbasmanova
CC @rui-mo

// New registration function; mostly a copy from the function above, but taking
// the inner "udf" struct directly, instead of the wrapper. We can keep both for
// a while to maintain backwards compatibility, but the idea is to remove the
// one above eventually.

I refactor the register by using

template <typename TExec> using DivideFunctionAllowPrecisionLoss = DecimalDivideFunction<TExec, true>;

like https://github.com/facebookincubator/velox/blob/main/velox/functions/prestosql/GreatestLeast.h#L127

Can you help review again? Thanks! @rui-mo

rui-mo · 2024-09-14T02:30:18Z

velox/functions/sparksql/DecimalArithmetic.cpp

+  registerDecimalBinary<Func, true>(
+      name, makeConstraints(rPrecision, rScale, true));
+  registerDecimalBinary<Func, false>(
+      name + "_not_allow_precision_loss",


The suffix '_not_allow_precision_loss' is used several times. Consider defining a variable for it and renaming as '_deny_precision_loss'.

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 Jul 3, 2024

jinchengchenghh changed the title ~~Support decimal operation not precision loss mode~~ Support decimal operation precision not loss mode Jul 3, 2024

rui-mo reviewed Jul 9, 2024

View reviewed changes

PHILO-HE reviewed Jul 9, 2024

View reviewed changes

rui-mo reviewed Jul 10, 2024

View reviewed changes

jinchengchenghh mentioned this pull request Jul 15, 2024

Change FunctionSignature returnType to std::optional to support the return type varied with config #10467

Closed

mbasmanova reviewed Jul 17, 2024

View reviewed changes

FelixYBW pushed a commit to oap-project/velox that referenced this pull request Jul 25, 2024

[facebookincubator#10383 ] Support decimal operation not precision lo…

e0e8b8d

…ss mode (10383) facebookincubator#10383

zhztheplayer pushed a commit to oap-project/velox that referenced this pull request Jul 25, 2024

[facebookincubator#10383 ] Support decimal operation not precision lo…

dad351f

…ss mode (10383) facebookincubator#10383

zhztheplayer pushed a commit to oap-project/velox that referenced this pull request Jul 25, 2024

[facebookincubator#10383 ] Support decimal operation not precision lo…

1a881f7

…ss mode (10383) facebookincubator#10383

zhztheplayer pushed a commit to oap-project/velox that referenced this pull request Jul 25, 2024

[facebookincubator#10383 ] Support decimal operation not precision lo…

5693c8a

…ss mode (10383) facebookincubator#10383

GlutenPerfBot pushed a commit to oap-project/velox that referenced this pull request Jul 25, 2024

[facebookincubator#10383 ] Support decimal operation not precision lo…

2706b98

…ss mode (10383) facebookincubator#10383

zhztheplayer pushed a commit to oap-project/velox that referenced this pull request Jul 26, 2024

[facebookincubator#10383 ] Support decimal operation not precision lo…

a1d0aac

…ss mode (10383) facebookincubator#10383

zhztheplayer pushed a commit to zhztheplayer/velox that referenced this pull request Jul 27, 2024

[facebookincubator#10383 ] Support decimal operation not precision lo…

3dec6da

…ss mode (10383) facebookincubator#10383

GlutenPerfBot pushed a commit to oap-project/velox that referenced this pull request Jul 29, 2024

[facebookincubator#10383 ] Support decimal operation not precision lo…

ed91b0a

…ss mode (10383) facebookincubator#10383

GlutenPerfBot pushed a commit to oap-project/velox that referenced this pull request Jul 30, 2024

[facebookincubator#10383 ] Support decimal operation not precision lo…

f439cf4

…ss mode (10383) facebookincubator#10383

rui-mo reviewed Jul 31, 2024

View reviewed changes

GlutenPerfBot pushed a commit to oap-project/velox that referenced this pull request Jul 31, 2024

[facebookincubator#10383 ] Support decimal operation not precision lo…

88cf94d

…ss mode (10383) facebookincubator#10383

GlutenPerfBot pushed a commit to oap-project/velox that referenced this pull request Aug 1, 2024

[facebookincubator#10383 ] Support decimal operation not precision lo…

cf20144

…ss mode (10383) facebookincubator#10383

GlutenPerfBot pushed a commit to oap-project/velox that referenced this pull request Aug 2, 2024

[facebookincubator#10383 ] Support decimal operation not precision lo…

ad1d1be

…ss mode (10383) facebookincubator#10383

GlutenPerfBot pushed a commit to oap-project/velox that referenced this pull request Aug 3, 2024

[facebookincubator#10383 ] Support decimal operation not precision lo…

1e33932

…ss mode (10383) facebookincubator#10383

GlutenPerfBot pushed a commit to oap-project/velox that referenced this pull request Aug 4, 2024

[facebookincubator#10383 ] Support decimal operation not precision lo…

ebfda09

…ss mode (10383) facebookincubator#10383

jinchengchenghh force-pushed the precision branch from 76c5795 to 182d32a Compare August 28, 2024 02:33

rui-mo reviewed Aug 28, 2024

View reviewed changes

zhouyuan reviewed Aug 28, 2024

View reviewed changes

rui-mo reviewed Aug 29, 2024

View reviewed changes

jinchengchenghh force-pushed the precision branch 2 times, most recently from 9d14c62 to 0dc20f6 Compare September 11, 2024 06:18

rui-mo reviewed Sep 13, 2024

View reviewed changes

rui-mo reviewed Sep 14, 2024

View reviewed changes

jinchengchenghh force-pushed the precision branch from 726d7b2 to 61f976b Compare September 25, 2024 03:19

jinchengchenghh added 5 commits September 25, 2024 10:06

support decimal arithmetic precision loss mode

6bef471

update comments

5354862

address comments

593e606

refactor the decimal register

61f976b

minor

204cab3

		@@ -0,0 +1,21 @@
		================================
		SparkRegistration Configuration

		@@ -4,6 +4,8 @@ Spark Functions

		The semantics of Spark functions match Spark 3.5 with ANSI OFF.

		Spark functions can be registered by :doc:`struct SparkRegistrationConfig <functions/spark/config>`.

		@@ -9,6 +9,8 @@ https://cwiki.apache.org/confluence/download/attachments/27362075/Hive_Decimal_P

		https://msdn.microsoft.com/en-us/library/ms190476.aspx

		For decimal addition, subtraction, multiplication, the precision and scale computation logic is same.

	bool registerFunction(
	const std::vector<std::string>& aliases = {},
	bool overwrite = true) {

Support decimal operation precision not loss mode #10383

Are you sure you want to change the base?

Support decimal operation precision not loss mode #10383

Conversation

jinchengchenghh commented Jul 3, 2024 • edited Loading

netlify bot commented Jul 3, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

jinchengchenghh commented Jul 3, 2024

jinchengchenghh commented Jul 9, 2024

rui-mo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinchengchenghh Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

rui-mo Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

jinchengchenghh Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinchengchenghh Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinchengchenghh commented Jul 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FelixYBW commented Jul 25, 2024

jinchengchenghh commented Jul 31, 2024

rui-mo left a comment

Choose a reason for hiding this comment

rui-mo Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

rui-mo Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinchengchenghh commented Aug 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinchengchenghh commented Aug 30, 2024

jinchengchenghh commented Sep 3, 2024

jinchengchenghh commented Sep 10, 2024

rui-mo commented Sep 10, 2024

jinchengchenghh commented Sep 13, 2024

rui-mo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinchengchenghh commented Jul 3, 2024 •

edited

Loading

netlify bot commented Jul 3, 2024 •

edited

Loading

jinchengchenghh Jul 9, 2024 •

edited

Loading

rui-mo Jul 9, 2024 •

edited

Loading

jinchengchenghh Jul 10, 2024 •

edited

Loading

jinchengchenghh Jul 11, 2024 •

edited

Loading

rui-mo Jul 31, 2024 •

edited

Loading

rui-mo Jul 31, 2024 •

edited

Loading