From a7301065d7abacb9a103d3ee4c008de44365a5c6 Mon Sep 17 00:00:00 2001
From: Andras Palinkas <andras.palinkas@elastic.co>
Date: Wed, 11 Nov 2020 17:07:17 -0500
Subject: [PATCH] SQL: Fix the return type in the sign function (#64845)
 (#64968)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Fixes the inconsistency between the type of the object returned by the
`SIGN()/SIGNUM()` SQL functions and the specified `DataType`.

In the Class Sign, DataType is DataTypes.INTEGER. The source code is as
follows：

```
    public DataType dataType() {
        return DataTypes.INTEGER;
    }
```

But In the Class MathProcessor, the source code of SIGN((Object l),
Parameter and return value types are the same. Therefore, when using
double or float parameters to test, there is a little problem, the test
method is like the following curl :

```
curl -XPOST 127.0.0.1:9200/_sql -d "{\"query\":\"select  SIGN(1.0) \"}" \
  -H 'Content-Type: application/json'
```

The result is:

```
{"columns":[{"name":"SIGN(1.0)","type":"integer"}],"rows":[[1.0]]}
```

The result value is `1.0`, but the type is `integer`.

Signed-off-by: mantuliu <240951888@qq.com>
Co-authored-by: Marios Trivyzas <matriv@gmail.com>
(cherry picked from commits aa78301e71f, ced3c1281c7, 40e5b9b)
---
 .../function/scalar/math/MathProcessor.java   | 21 +++------------
 .../scalar/math/MathOperationTests.java       | 27 +++++++++++++++++++
 2 files changed, 31 insertions(+), 17 deletions(-)

diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/math/MathProcessor.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/math/MathProcessor.java
index 07acf3a4314..c5ac5bd4406 100644
--- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/math/MathProcessor.java
+++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/math/MathProcessor.java
@@ -77,26 +77,13 @@ public class MathProcessor implements Processor {
                 Randomness.get().nextDouble(), true),
         SIGN((Object l) -> {
             if (l instanceof Double) {
-                return Math.signum((Double) l);
+                return (int) Math.signum((Double) l);
             }
             if (l instanceof Float) {
-                return Math.signum((Float) l);
+                return (int) Math.signum((Float) l);
             }
 
-            long lo = Long.signum(((Number) l).longValue());
-
-            if (l instanceof Integer) {
-                return DataTypeConverter.safeToInt(lo);
-            }
-            if (l instanceof Short) {
-                return DataTypeConverter.safeToShort(lo);
-            }
-            if (l instanceof Byte) {
-                return DataTypeConverter.safeToByte(lo);
-            }
-
-            //fallback to generic double
-            return lo;
+            return Long.signum(((Number) l).longValue());
         }),
         SIN(Math::sin),
         SINH(Math::sinh),
@@ -188,4 +175,4 @@ public class MathProcessor implements Processor {
     public String toString() {
         return processor.toString();
     }
-}
\ No newline at end of file
+}
diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/function/scalar/math/MathOperationTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/function/scalar/math/MathOperationTests.java
index 3c1c880a8b4..c735c5e666b 100644
--- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/function/scalar/math/MathOperationTests.java
+++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/function/scalar/math/MathOperationTests.java
@@ -10,6 +10,9 @@ import org.elasticsearch.test.ESTestCase;
 import org.elasticsearch.xpack.ql.QlIllegalArgumentException;
 import org.elasticsearch.xpack.sql.expression.function.scalar.math.MathProcessor.MathOperation;
 
+import java.util.Arrays;
+import java.util.List;
+
 public class MathOperationTests extends ESTestCase {
 
     public void testAbsLongMax() {
@@ -35,4 +38,28 @@ public class MathOperationTests extends ESTestCase {
         assertEquals(42f, MathOperation.ABS.apply(-42f));
         assertEquals(42d, MathOperation.ABS.apply(-42d));
     }
+
+    public void testSignIntegerType() {
+        List<Number> negative = Arrays.asList((byte) -42, (short) -42, -42, -42L, -42.0f, -42.0d);
+        List<Number> zero = Arrays.asList((byte) 0, (short) 0, 0, 0L, 0.0f, 0.0d);
+        List<Number> positive = Arrays.asList((byte) 42, (short) 42, 42, 42L, 42.0f, 42.0d);
+
+        for (Number number : negative) {
+            Number result = MathOperation.SIGN.apply(number);
+            assertEquals(Integer.class, result.getClass());
+            assertEquals(-1, result);
+        }
+
+        for (Number number : zero) {
+            Number result = MathOperation.SIGN.apply(number);
+            assertEquals(Integer.class, result.getClass());
+            assertEquals(0, result);
+        }
+
+        for (Number number : positive) {
+            Number result = MathOperation.SIGN.apply(number);
+            assertEquals(Integer.class, result.getClass());
+            assertEquals(1, result);
+        }
+    }
 }