fix: Greatest/least custom comparison; add Time/TimeWithTimezone support (facebookincubator#15666)

Summary:
Pull Request resolved: facebookincubator#15666

The greatest/least functions were extracting raw values (int64_t, int128_t) early and comparing them with standard operators, bypassing custom comparison logic. This caused incorrect results for types like IPAddress:

- IPAddress: Used signed int128_t comparison instead of unsigned byte-by-byte comparison (memcmp), causing addresses with high bit set (e.g., ffff::1) to compare as less than addresses starting with 0x00

The fix compares them directly using their operator< and operator>, which trigger the custom compare() methods. The raw value is extracted only from the winning element at the end and hence fixes [facebookincubator#15620](facebookincubator#15620)

Additionally, this diff adds support for Time and TimeWithTimezone types in greatest/least functions (registration + tests), which also benefit from the custom comparison fix.

Reviewed By: kgpai

Differential Revision: D87961025

fbshipit-source-id: 789ea8fd7bf1ff0cd91ff7de36721aa0158cc82e

Author: pratikpugalia

velox/functions/prestosql/GreatestLeast.h

@@ -16,6 +16,7 @@
 #pragma once
 
 #include <cmath>
+#include <optional>
 #include "velox/expression/ComplexViewTypes.h"
 #include "velox/functions/Macros.h"
 
@@ -41,23 +42,32 @@ struct ExtremeValueFunction {
       out_type<T>& result,
       const arg_type<T>& firstElement,
       const arg_type<Variadic<T>>& remainingElement) {
-    auto currentValue = extractValue(firstElement);
+    // Track the winner: nullopt means firstElement, a value means index in
+    // remainingElement
+    std::optional<vector_size_t> winnerIdx;
 
-    for (auto element : remainingElement) {
-      auto candidateValue = extractValue(element.value());
+    for (size_t i = 0; i < remainingElement.size(); ++i) {
+      const auto& cur = remainingElement[i].value();
+      const auto& best = winnerIdx.has_value()
+          ? remainingElement[*winnerIdx].value()
+          : firstElement;
 
       if constexpr (isLeast) {
-        if (smallerThan(candidateValue, currentValue)) {
-          currentValue = candidateValue;
+        if (smallerThan(cur, best)) {
+          winnerIdx = i;
         }
       } else {
-        if (greaterThan(candidateValue, currentValue)) {
-          currentValue = candidateValue;
+        if (greaterThan(cur, best)) {
+          winnerIdx = i;
         }
       }
     }
 
-    result = currentValue;
+    if (!winnerIdx.has_value()) {
+      result = extractValue(firstElement);
+    } else {
+      result = extractValue(remainingElement[*winnerIdx].value());
+    }
   }
 
  private:

velox/functions/prestosql/registration/GeneralFunctionsRegistration.cpp

@@ -23,6 +23,7 @@
 #include "velox/functions/prestosql/InPredicate.h"
 #include "velox/functions/prestosql/Reduce.h"
 #include "velox/functions/prestosql/types/IPAddressType.h"
+#include "velox/functions/prestosql/types/TimeWithTimezoneType.h"
 #include "velox/functions/prestosql/types/TimestampWithTimeZoneType.h"
 
 namespace facebook::velox::functions {
@@ -60,6 +61,8 @@ void registerAllGreatestLeastFunctions(const std::string& prefix) {
   registerGreatestLeastFunction<Timestamp>(prefix);
   registerGreatestLeastFunction<TimestampWithTimezone>(prefix);
   registerGreatestLeastFunction<IPAddress>(prefix);
+  registerGreatestLeastFunction<Time>(prefix);
+  registerGreatestLeastFunction<TimeWithTimezone>(prefix);
 }
 } // namespace
 

velox/functions/prestosql/tests/GreatestLeastTest.cpp

@@ -19,6 +19,8 @@
 #include <optional>
 #include "velox/common/base/tests/GTestUtils.h"
 #include "velox/functions/prestosql/tests/utils/FunctionBaseTest.h"
+#include "velox/functions/prestosql/types/TimeWithTimezoneType.h"
+#include "velox/type/Time.h"
 
 using namespace facebook::velox;
 
@@ -286,6 +288,104 @@ TEST_F(GreatestLeastTest, leastDate) {
       DATE());
 }
 
+TEST_F(GreatestLeastTest, greatestLeastTime) {
+  using namespace facebook::velox::util;
+
+  // Helper to parse TIME from string
+  const auto parseTime = [](const std::string& timeStr) -> int64_t {
+    auto result = fromTimeString(timeStr.c_str(), timeStr.size());
+    if (result.hasError()) {
+      throw std::runtime_error("Parse error: " + result.error().message());
+    }
+    return result.value();
+  };
+
+  // Test greatest
+  runTest<int64_t>(
+      "greatest(c0, c1, c2)",
+      {
+          {parseTime("10:00:00"),
+           parseTime("00:00:00"),
+           parseTime("12:00:00.001")}, // c0
+          {parseTime("12:30:00"),
+           parseTime("12:00:00"),
+           parseTime("12:00:00.500")}, // c1
+          {parseTime("15:30:45.100"),
+           parseTime("23:59:59.999"),
+           parseTime("12:00:00.999")}, // c2
+      },
+      {parseTime("15:30:45.100"),
+       parseTime("23:59:59.999"),
+       parseTime("12:00:00.999")},
+      std::nullopt,
+      TIME(),
+      TIME());
+
+  // Test least
+  runTest<int64_t>(
+      "least(c0, c1, c2)",
+      {
+          {parseTime("10:00:00"),
+           parseTime("00:00:00"),
+           parseTime("12:00:00.001")}, // c0
+          {parseTime("12:30:00"),
+           parseTime("12:00:00"),
+           parseTime("12:00:00.500")}, // c1
+          {parseTime("15:30:45.100"),
+           parseTime("23:59:59.999"),
+           parseTime("12:00:00.999")}, // c2
+      },
+      {parseTime("10:00:00"), parseTime("00:00:00"), parseTime("12:00:00.001")},
+      std::nullopt,
+      TIME(),
+      TIME());
+}
+
+TEST_F(GreatestLeastTest, greatestLeastTimeWithTimezone) {
+  using namespace facebook::velox::util;
+
+  // Helper to parse TIME WITH TIME ZONE from string
+  const auto parseTimeWithTz = [](const std::string& timeStr) -> int64_t {
+    auto result = fromTimeWithTimezoneString(timeStr.c_str(), timeStr.size());
+    if (result.hasError()) {
+      throw std::runtime_error("Parse error: " + result.error().message());
+    }
+    return result.value();
+  };
+
+  // Test greatest
+  runTest<int64_t>(
+      "greatest(c0, c1, c2)",
+      {
+          {parseTimeWithTz("10:00:00+00:00"),
+           parseTimeWithTz("20:00:00-08:00")}, // c0
+          {parseTimeWithTz("10:00:00-08:00"),
+           parseTimeWithTz("23:59:59.999+00:00")}, // c1
+          {parseTimeWithTz("10:00:00+08:00"),
+           parseTimeWithTz("00:00:00+08:00")}, // c2
+      },
+      {parseTimeWithTz("10:00:00-08:00"), parseTimeWithTz("20:00:00-08:00")},
+      std::nullopt,
+      TIME_WITH_TIME_ZONE(),
+      TIME_WITH_TIME_ZONE());
+
+  // Test least
+  runTest<int64_t>(
+      "least(c0, c1, c2)",
+      {
+          {parseTimeWithTz("10:00:00+00:00"),
+           parseTimeWithTz("00:00:00-08:00")}, // c0
+          {parseTimeWithTz("10:00:00-08:00"),
+           parseTimeWithTz("23:59:59.999+00:00")}, // c1
+          {parseTimeWithTz("10:00:00+08:00"),
+           parseTimeWithTz("00:00:00+08:00")}, // c2
+      },
+      {parseTimeWithTz("10:00:00+08:00"), parseTimeWithTz("00:00:00+08:00")},
+      std::nullopt,
+      TIME_WITH_TIME_ZONE(),
+      TIME_WITH_TIME_ZONE());
+}
+
 TEST_F(GreatestLeastTest, greatestLeastIpAddress) {
   auto greatest = [&](const std::optional<std::string>& a,
                       const std::optional<std::string>& b,
@@ -324,6 +424,14 @@ TEST_F(GreatestLeastTest, greatestLeastIpAddress) {
       "255.255.255.255",
       "2001:0db8:0000:0000:0000:ff00:0042:832");
   EXPECT_FALSE(leastValueWithNulls.has_value());
+
+  // Case where raw comparison of values will give
+  // different result than comparison of IPAddress values byte by byte.
+  auto greatValue = greatest("ffff::1", "1::1", "1::2");
+  EXPECT_EQ("ffff::1", greatValue.value());
+
+  auto lessValue = least("ffff::1", "1::1", "ffff::2");
+  EXPECT_EQ("1::1", lessValue.value());
 }
 
 TEST_F(GreatestLeastTest, stringBuffersMoved) {