edponce commented on a change in pull request #11882:
URL: https://github.com/apache/arrow/pull/11882#discussion_r776864703



##########
File path: cpp/src/arrow/compute/kernels/scalar_compare.cc
##########
@@ -746,6 +787,155 @@ std::shared_ptr<ScalarFunction> 
MakeScalarMinMax(std::string name,
   return func;
 }
 
+template <typename Op>
+void AddIntegerBetween(const std::shared_ptr<DataType>& ty, ScalarFunction* 
func) {
+  auto exec = GeneratePhysicalInteger<ScalarTernaryEqualTypes, BooleanType, 
Op>(*ty);
+  DCHECK_OK(func->AddKernel({ty, ty, ty}, boolean(), std::move(exec)));
+}
+
+template <typename InType, typename Op>
+void AddGenericBetween(const std::shared_ptr<DataType>& ty, ScalarFunction* 
func) {
+  DCHECK_OK(func->AddKernel({ty, ty, ty}, boolean(),
+                            ScalarTernaryEqualTypes<BooleanType, InType, 
Op>::Exec));
+}
+
+template <typename OutType, typename ArgType, typename Op>
+struct BetweenTimestamps : public ScalarTernaryEqualTypes<OutType, ArgType, 
Op> {
+  using Base = ScalarTernaryEqualTypes<OutType, ArgType, Op>;
+
+  static Status Exec(KernelContext* ctx, const ExecBatch& batch, Datum* out) {
+    const auto& var = checked_cast<const TimestampType&>(*batch[0].type());
+    const auto& lhs = checked_cast<const TimestampType&>(*batch[1].type());
+    const auto& rhs = checked_cast<const TimestampType&>(*batch[2].type());
+    if ((var.timezone().empty() != lhs.timezone().empty()) ||
+        (var.timezone().empty() != rhs.timezone().empty()) ||
+        (lhs.timezone().empty() != rhs.timezone().empty())) {
+      return Status::Invalid("Cannot compare timestamps with and without 
timezones.");
+    }
+    return Base::Exec(ctx, batch, out);
+  }
+};
+
+template <typename Op>
+std::shared_ptr<ScalarFunction> MakeBetweenFunction(std::string name,
+                                                    const FunctionDoc* doc) {
+  auto func = std::make_shared<CompareFunction>(name, Arity::Ternary(), doc);
+
+  DCHECK_OK(func->AddKernel(
+      {boolean(), boolean(), boolean()}, boolean(),
+      ScalarTernary<BooleanType, BooleanType, BooleanType, BooleanType, 
Op>::Exec));
+
+  for (const std::shared_ptr<DataType>& ty : IntTypes()) {
+    AddIntegerBetween<Op>(ty, func.get());
+  }
+
+  AddIntegerBetween<Op>(date32(), func.get());
+  AddIntegerBetween<Op>(date64(), func.get());
+
+  AddGenericBetween<FloatType, Op>(float32(), func.get());
+  AddGenericBetween<DoubleType, Op>(float64(), func.get());
+
+  // Add timestamp kernels
+  for (auto unit : TimeUnit::values()) {
+    InputType in_type(match::TimestampTypeUnit(unit));
+    DCHECK_OK(func->AddKernel({in_type, in_type, in_type}, boolean(),
+                              BetweenTimestamps<BooleanType, TimestampType, 
Op>::Exec));
+  }
+
+  // Duration
+  for (auto unit : TimeUnit::values()) {
+    InputType in_type(match::DurationTypeUnit(unit));
+    auto exec =
+        GeneratePhysicalInteger<ScalarTernaryEqualTypes, BooleanType, 
Op>(int64());
+    DCHECK_OK(func->AddKernel({in_type, in_type, in_type}, boolean(), 
std::move(exec)));
+  }
+
+  // Time32 and Time64
+  for (auto unit : {TimeUnit::SECOND, TimeUnit::MILLI}) {
+    InputType in_type(match::Time32TypeUnit(unit));
+    auto exec =
+        GeneratePhysicalInteger<ScalarTernaryEqualTypes, BooleanType, 
Op>(int32());
+    DCHECK_OK(func->AddKernel({in_type, in_type, in_type}, boolean(), 
std::move(exec)));
+  }
+  for (auto unit : {TimeUnit::MICRO, TimeUnit::NANO}) {
+    InputType in_type(match::Time64TypeUnit(unit));
+    auto exec =
+        GeneratePhysicalInteger<ScalarTernaryEqualTypes, BooleanType, 
Op>(int64());
+    DCHECK_OK(func->AddKernel({in_type, in_type, in_type}, boolean(), 
std::move(exec)));
+  }
+
+  for (const std::shared_ptr<DataType>& ty : BaseBinaryTypes()) {
+    auto exec = GenerateVarBinaryBase<ScalarTernaryEqualTypes, BooleanType, 
Op>(*ty);
+    DCHECK_OK(func->AddKernel(
+        {InputType::Array(ty), InputType::Scalar(ty), InputType::Scalar(ty)}, 
boolean(),
+        exec));
+    DCHECK_OK(func->AddKernel(
+        {InputType::Array(ty), InputType::Array(ty), InputType::Array(ty)}, 
boolean(),
+        std::move(exec)));
+  }
+
+  for (const auto id : {Type::DECIMAL128, Type::DECIMAL256}) {
+    auto exec = GenerateDecimal<ScalarTernaryEqualTypes, BooleanType, Op>(id);
+    DCHECK_OK(func->AddKernel({InputType(id), InputType(id), InputType(id)}, 
boolean(),
+                              std::move(exec)));
+  }
+
+  {
+    auto exec = ScalarTernaryEqualTypes<BooleanType, FixedSizeBinaryType, 
Op>::Exec;
+    auto ty = InputType(Type::FIXED_SIZE_BINARY);
+    DCHECK_OK(func->AddKernel({ty, ty, ty}, boolean(), std::move(exec)));
+  }
+
+  return func;
+}
+
+const BetweenOptions* GetDefaultBetweenOptions() {
+  static const auto kBetweenOptions = BetweenOptions::Defaults();
+  return &kBetweenOptions;
+}
+
+const FunctionDoc between_doc{"Check if values are in a range, val betwen a 
and b",
+                              ("A null on either side emits a null comparison 
result.\n"
+                               "options are used to specify if the endpoints 
are\n"
+                               "inclusive, possible values are NEITHER 
(a<val<b),\n"
+                               "LEFT (a<=val<b), RIGHT (a<val<=b), and the 
default\n"
+                               "if not specified BOTH (a<=val<=b)"),
+                              {"val", "a", "b"}};
+
+class BetweenMetaFunction : public MetaFunction {
+ public:
+  BetweenMetaFunction()
+      : MetaFunction("between", Arity::Ternary(), &between_doc,
+                     GetDefaultBetweenOptions()) {}
+
+  Result<Datum> ExecuteImpl(const std::vector<Datum>& args,
+                            const FunctionOptions* options,
+                            ExecContext* ctx) const override {
+    const BetweenOptions& between_options = static_cast<const 
BetweenOptions&>(*options);
+    Datum result;

Review comment:
       I disagree with this approach. It defeats the purpose of having the 
`BetweenOptions` because they are not being consumed by `CallFunction`.
   
   I will take a stab at supporting internal dispatching based on 
`BetweenOptions` and submit a PR to your branch. Up to now this is looking good 
but needs a bit of tweaking.




-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


Reply via email to