Fix one of ray comment

Author: anlowee

presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpFilterToKqlConverter.java

@@ -50,7 +50,8 @@
 import static com.facebook.presto.common.function.OperatorType.flip;
 import static com.facebook.presto.common.type.BooleanType.BOOLEAN;
 import static com.facebook.presto.plugin.clp.ClpErrorCode.CLP_PUSHDOWN_UNSUPPORTED_EXPRESSION;
-import static com.facebook.presto.plugin.clp.ClpUtils.KqlUtils.escapeKqlSpecialCharsForStringValue;
+import static com.facebook.presto.plugin.clp.ClpUtils.escapeKqlSpecialCharsForStringValue;
+import static com.facebook.presto.plugin.clp.ClpUtils.isNumericType;
 import static com.facebook.presto.spi.relation.SpecialFormExpression.Form.AND;
 import static java.lang.Integer.parseInt;
 import static java.lang.String.format;
@@ -128,9 +129,9 @@ public ClpExpression visitCall(CallExpression node, Void context)
     }
 
     /**
-     * Handles the BETWEEN expression for numeric range, the range must be numeric. If the first
-     * argument is not a variable reference expression or either range boundaries is not constant
-     * expressions it won't translate the expression.
+     * Handles the BETWEEN expression for numeric range, all arguments must be numeric. If the first
+     * argument is not a variable reference expression or either the second or the third is not
+     * constant expressions it won't translate the expression.
      *
      * <p></p>
      * Example: <code>col1 BETWEEN 0 AND 5</code> → <code>col1 >= 0 AND col1 <= 5</code>
@@ -150,6 +151,10 @@ private ClpExpression handleBetween(CallExpression node)
                 || !(node.getArguments().get(2) instanceof ConstantExpression)) {
             return new ClpExpression(node);
         }
+        if (!isNumericType(node.getArguments().get(0).getType())) {
+            // Let the Presto SQL analyzer throw exception
+            return new ClpExpression(node);
+        }
         Optional<String> variableReferencePushDownExpression = node.getArguments().get(0).accept(this, null).getPushDownExpression();
         if (!variableReferencePushDownExpression.isPresent()) {
             return new ClpExpression(node);

presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpUtils.java

@@ -14,21 +14,42 @@
 package com.facebook.presto.plugin.clp;
 
 import com.facebook.airlift.log.Logger;
+import com.facebook.presto.common.type.Type;
+
+import static com.facebook.presto.common.type.BigintType.BIGINT;
+import static com.facebook.presto.common.type.DoubleType.DOUBLE;
 
 public class ClpUtils
 {
-    public static class KqlUtils
+    private static final Logger log = Logger.get(ClpUtils.class);
+
+    /**
+     * Refer to
+     * <a href="https://docs.yscope.com/clp/main/user-guide/reference-json-search-syntax">here
+     * </a> for all special chars in the string value that need to be escaped.
+     *
+     * @param literalString the target string to escape special chars '\', '"', ;?' and '*'
+     * @return the escaped string
+     */
+    public static String escapeKqlSpecialCharsForStringValue(String literalString)
     {
-        private static final Logger log = Logger.get(KqlUtils.class);
+        String escaped = literalString;
+        escaped = escaped.replace("\\", "\\\\");
+        escaped = escaped.replace("\"", "\\\"");
+        escaped = escaped.replace("?", "\\?");
+        escaped = escaped.replace("*", "\\*");
+        return escaped;
+    }
 
-        public static String escapeKqlSpecialCharsForStringValue(String literalString)
-        {
-            String escaped = literalString;
-            escaped = escaped.replace("\\", "\\\\");
-            escaped = escaped.replace("\"", "\\\"");
-            escaped = escaped.replace("?", "\\?");
-            escaped = escaped.replace("*", "\\*");
-            return escaped;
-        }
+    /**
+     * Check if the type is one of the numeric types that CLP will handle. Refer to
+     * {@code ClpSchemaTree} for all types that CLP will handle.
+     *
+     * @param type the type to check
+     * @return is the type numeric or not
+     */
+    public static boolean isNumericType(Type type)
+    {
+        return BIGINT == type || DOUBLE == type;
     }
 }

presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java

@@ -106,14 +106,8 @@ public void testBetweenPushDown()
         testFilter(tryPushDown("0 BETWEEN NULL AND 5", sessionHolder, ImmutableSet.of()), null, "BETWEEN(0, null, 5)", sessionHolder, false);
 
         // Illegal cases
-        assertThrows(SemanticException.class, () -> {
-            testFilter(
-                    tryPushDown("fare BETWEEN 'Apple' AND 'Pear'", sessionHolder, ImmutableSet.of()),
-                    "fare >= 0 AND fare <= 5",
-                    null,
-                    sessionHolder,
-                    true);
-        });
+        assertThrows(SemanticException.class, () -> tryPushDown("fare BETWEEN 'Apple' AND 'Pear'", sessionHolder, ImmutableSet.of()));
+        assertThrows(SemanticException.class, () -> tryPushDown("city.Name BETWEEN 0 AND 5", sessionHolder, ImmutableSet.of()));
     }
 
     @Test
@@ -288,6 +282,12 @@ public void testMetadataSqlGeneration()
                 sessionHolder,
                 "(fare > 0 AND city.Name: \"b*\")",
                 "(\"fare\" > 0)");
+        testFilterWithMetadataSql(
+                tryPushDown("((fare BETWEEN 0 AND 5) AND city.Name like 'b%')",
+                        sessionHolder, ImmutableSet.of("fare")),
+                sessionHolder,
+                "(fare >= 0 AND fare <= 5 AND city.Name: \"b*\")",
+                "(\"fare\" >= 0 AND \"fare\" <= 5)");
         testFilterWithMetadataSql(
                 tryPushDown("(fare > 0 OR city.Name like 'b%')",
                         sessionHolder, ImmutableSet.of("fare")),