[GitHub] [arrow] EpsilonPrime commented on a diff in pull request #33775: GH-33588: [Substrait] Add Substrait→Acero mapping for round operationMajor:

["EpsilonPrime (via GitHub)" <gi...@apache.org>]

EpsilonPrime commented on code in PR #33775:
URL: https://github.com/apache/arrow/pull/33775#discussion_r1083721581


##########
cpp/src/arrow/engine/substrait/extension_set.cc:
##########
@@ -790,6 +796,30 @@ ExtensionIdRegistry::SubstraitCallToArrow DecodeOptionlessUncheckedArithmetic(
   };
 }
 
+ExtensionIdRegistry::SubstraitCallToArrow DecodeBinaryRoundingMode(
+    const std::string& function_name) {
+  return [function_name](const SubstraitCall& call) -> Result<compute::Expression> {
+    ARROW_ASSIGN_OR_RAISE(
+        compute::RoundMode round_mode,
+        ParseOptionOrElse(
+            call, "rounding", kRoundModeParser,
+            {compute::RoundMode::DOWN, compute::RoundMode::UP,
+             compute::RoundMode::TOWARDS_ZERO, compute::RoundMode::TOWARDS_INFINITY,
+             compute::RoundMode::HALF_DOWN, compute::RoundMode::HALF_UP,
+             compute::RoundMode::HALF_TOWARDS_ZERO,
+             compute::RoundMode::HALF_TOWARDS_INFINITY, compute::RoundMode::HALF_TO_EVEN,
+             compute::RoundMode::HALF_TO_ODD},
+            compute::RoundMode::HALF_TOWARDS_INFINITY));

Review Comment:
   HALF_TOWARDS_INFINITY mirrors Ceiling, AWAY_FROM_ZERO mirrors the way humans round.  What rationale is there for HALF_TO_EVEN?  Is it because it's the first option listed in the proposed YAML definition?



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org