From cf02306ea168e1e7a4d3579278bb499a104f968b Mon Sep 17 00:00:00 2001
From: Kevin Wilfong <kevinwilfong@meta.com>
Date: Mon, 9 Dec 2024 09:55:52 -0800
Subject: [PATCH] fix: Only register inequality comparisons for orderable types
 (#11784)

Summary:

Inequalities only work with types that are orderable pretty much by definition.

For maps in particular we allow users to call map1 < map2 but it will always throw because the map
type does not support inequalities unless null is treated as a value (which it is not in GenericView's
compare function which is what gets invoked).

This will address https://github.com/facebookincubator/velox/issues/11549 where inequalities on
maps can be generated in fuzzers and result in exceptions.

Reviewed By: xiaoxmeng

Differential Revision: D66909744
---
 .../ComparisonFunctionsRegistration.cpp        | 10 ++++++----
 .../prestosql/tests/ComparisonsTest.cpp        | 18 ------------------
 2 files changed, 6 insertions(+), 22 deletions(-)

diff --git a/velox/functions/prestosql/registration/ComparisonFunctionsRegistration.cpp b/velox/functions/prestosql/registration/ComparisonFunctionsRegistration.cpp
index 2870a3ff787f..f167d0e3e330 100644
--- a/velox/functions/prestosql/registration/ComparisonFunctionsRegistration.cpp
+++ b/velox/functions/prestosql/registration/ComparisonFunctionsRegistration.cpp
@@ -52,20 +52,22 @@ void registerComparisonFunctions(const std::string& prefix) {
 
   registerNonSimdizableScalar<LtFunction, bool>({prefix + "lt"});
   VELOX_REGISTER_VECTOR_FUNCTION(udf_simd_comparison_lt, prefix + "lt");
-  registerFunction<LtFunction, bool, Generic<T1>, Generic<T1>>({prefix + "lt"});
+  registerFunction<LtFunction, bool, Orderable<T1>, Orderable<T1>>(
+      {prefix + "lt"});
 
   registerNonSimdizableScalar<GtFunction, bool>({prefix + "gt"});
   VELOX_REGISTER_VECTOR_FUNCTION(udf_simd_comparison_gt, prefix + "gt");
-  registerFunction<GtFunction, bool, Generic<T1>, Generic<T1>>({prefix + "gt"});
+  registerFunction<GtFunction, bool, Orderable<T1>, Orderable<T1>>(
+      {prefix + "gt"});
 
   registerNonSimdizableScalar<LteFunction, bool>({prefix + "lte"});
   VELOX_REGISTER_VECTOR_FUNCTION(udf_simd_comparison_lte, prefix + "lte");
-  registerFunction<LteFunction, bool, Generic<T1>, Generic<T1>>(
+  registerFunction<LteFunction, bool, Orderable<T1>, Orderable<T1>>(
       {prefix + "lte"});
 
   registerNonSimdizableScalar<GteFunction, bool>({prefix + "gte"});
   VELOX_REGISTER_VECTOR_FUNCTION(udf_simd_comparison_gte, prefix + "gte");
-  registerFunction<GteFunction, bool, Generic<T1>, Generic<T1>>(
+  registerFunction<GteFunction, bool, Orderable<T1>, Orderable<T1>>(
       {prefix + "gte"});
 
   registerFunction<DistinctFromFunction, bool, Generic<T1>, Generic<T1>>(
diff --git a/velox/functions/prestosql/tests/ComparisonsTest.cpp b/velox/functions/prestosql/tests/ComparisonsTest.cpp
index 707a61d108fa..01d5cab90f6e 100644
--- a/velox/functions/prestosql/tests/ComparisonsTest.cpp
+++ b/velox/functions/prestosql/tests/ComparisonsTest.cpp
@@ -724,24 +724,6 @@ TEST_F(ComparisonsTest, gtLtArray) {
   testCompareArray<double>({NaN}, {NaN, 3.0}, expectedResults);
 }
 
-TEST_F(ComparisonsTest, gtLtMap) {
-  // Comparing maps is not supported for any types.
-  auto expectedException = "Map is not orderable type";
-
-  std::optional<std::vector<std::pair<int64_t, std::optional<int64_t>>>> map1 =
-      {{{1, 2}, {3, 4}}};
-  std::optional<std::vector<std::pair<int64_t, std::optional<int64_t>>>> map2 =
-      {{{1, 2}, {3, 4}}};
-  testCompareMap<int64_t>(map1, map2, expectedException);
-
-  static const auto NaN = std::numeric_limits<double>::quiet_NaN();
-  std::optional<std::vector<std::pair<double, std::optional<double>>>> map3 = {
-      {{1.0, 2.0}, {NaN, 4.0}}};
-  std::optional<std::vector<std::pair<double, std::optional<double>>>> map4 = {
-      {{1.0, 2.0}, {3, 4.0}}};
-  testCompareMap<double>(map3, map4, expectedException);
-}
-
 TEST_F(ComparisonsTest, distinctFrom) {
   auto input = makeRowVector({
       makeNullableFlatVector<int64_t>({3, 1, 1, std::nullopt, std::nullopt}),