PHOENIX-4884 Update INSTR to handle literals and non-literals in eith…

…er function argument

INSTR previously only handled arguments of the form non-literal and literal, but the documentation
doesn't clearly state this. We can support all variants though.

phoenix-core/src/it/java/org/apache/phoenix/end2end/InstrFunctionIT.java

@@ -131,4 +131,39 @@ public void testByteInstrDecendingFilter() throws Exception {
         testInstrFilter(conn, queryToExecute,"abcdefghijkl");
     }
 
+    @Test
+    public void testNonLiteralExpression() throws Exception {
+        Connection conn = DriverManager.getConnection(getUrl());
+        String tableName = generateUniqueName();
+        initTable(conn, tableName, "ASC", "asdf", "sdf");
+        // Should be able to use INSTR with a non-literal expression as the 2nd argument
+        String query = "SELECT INSTR(name, substr) FROM " + tableName;
+        testInstr(conn, query, 2);
+    }
+
+    @Test
+    public void testNonLiteralSourceExpression() throws Exception {
+        Connection conn = DriverManager.getConnection(getUrl());
+        String tableName = generateUniqueName();
+        initTable(conn, tableName, "ASC", "asdf", "sdf");
+        // Using the function inside the SELECT will test client-side.
+        String query = "SELECT INSTR('asdf', 'sdf') FROM " + tableName;
+        testInstr(conn, query, 2);
+        query = "SELECT INSTR('asdf', substr) FROM " + tableName;
+        testInstr(conn, query, 2);
+        query = "SELECT INSTR('qwerty', 'sdf') FROM " + tableName;
+        testInstr(conn, query, 0);
+        query = "SELECT INSTR('qwerty', substr) FROM " + tableName;
+        testInstr(conn, query, 0);
+        // Test the built-in function in a where clause to make sure
+        // it works server-side (and not just client-side).
+        query = "SELECT name FROM " + tableName + " WHERE INSTR(name, substr) = 2";
+        testInstrFilter(conn, query, "asdf");
+        query = "SELECT name FROM " + tableName + " WHERE INSTR(name, 'sdf') = 2";
+        testInstrFilter(conn, query, "asdf");
+        query = "SELECT name FROM " + tableName + " WHERE INSTR('asdf', substr) = 2";
+        testInstrFilter(conn, query, "asdf");
+        query = "SELECT name FROM " + tableName + " WHERE INSTR('asdf', 'sdf') = 2";
+        testInstrFilter(conn, query, "asdf");
+    }
 }

phoenix-core/src/main/java/org/apache/phoenix/expression/function/InstrFunction.java

@@ -30,16 +30,16 @@
 import org.apache.phoenix.schema.types.PDataType;
 import org.apache.phoenix.schema.types.PInteger;
 import org.apache.phoenix.schema.types.PVarchar;
-import org.apache.phoenix.util.ByteUtil;
 
 @BuiltInFunction(name=InstrFunction.NAME, args={
         @Argument(allowedTypes={ PVarchar.class }),
         @Argument(allowedTypes={ PVarchar.class })})
 public class InstrFunction extends ScalarFunction{
 
     public static final String NAME = "INSTR";
-
-    private String strToSearch = null;
+
+    private String literalSourceStr = null;
+    private String literalSearchStr = null;
 
     public InstrFunction() { }
 
@@ -49,40 +49,62 @@ public InstrFunction(List<Expression> children) {
     }
 
     private void init() {
-        Expression strToSearchExpression = getChildren().get(1);
-        if (strToSearchExpression instanceof LiteralExpression) {
-            Object strToSearchValue = ((LiteralExpression) strToSearchExpression).getValue();
-            if (strToSearchValue != null) {
-                this.strToSearch = strToSearchValue.toString();
-            }
+        literalSourceStr = maybeExtractLiteralString(getChildren().get(0));
+        literalSearchStr = maybeExtractLiteralString(getChildren().get(1));
+    }
+
+    /**
+     * Extracts the string-representation of {@code expr} only if {@code expr} is a
+     * non-null {@link LiteralExpression}.
+     *
+     * @param expr An Expression.
+     * @return The string value for the expression or null
+     */
+    private String maybeExtractLiteralString(Expression expr) {
+        if (expr instanceof LiteralExpression) {
+            // Whether the value is null or non-null, we can give it back right away
+            return (String) ((LiteralExpression) expr).getValue();
         }
+        return null;
     }
 
 
     @Override
     public boolean evaluate(Tuple tuple, ImmutableBytesWritable ptr) {
-        Expression child = getChildren().get(0);
-
-        if (!child.evaluate(tuple, ptr)) {
-            return false;
-        }
-
-        if (ptr.getLength() == 0) {
-            ptr.set(ByteUtil.EMPTY_BYTE_ARRAY);
-            return true;
+        String sourceStr = literalSourceStr;
+        if (sourceStr == null) {
+            Expression child = getChildren().get(0);
+
+            if (!child.evaluate(tuple, ptr)) {
+                return false;
+            }
+
+            // We need something non-empty to search against
+            if (ptr.getLength() == 0) {
+              return true;
+            }
+
+            sourceStr = (String) PVarchar.INSTANCE.toObject(ptr, child.getSortOrder());
         }
-
-        int position;
-        //Logic for Empty string search
-        if (strToSearch == null){
-            position = 0;
-            ptr.set(PInteger.INSTANCE.toBytes(position));
-            return true;
+
+        String searchStr = literalSearchStr;
+        // A literal was not provided, try to evaluate the expression to a literal
+        if (searchStr == null){
+            Expression child = getChildren().get(1);
+
+            if (!child.evaluate(tuple, ptr)) {
+              return false;
+            }
+
+            // A null (or zero-length) search string
+            if (ptr.getLength() == 0) {
+              return true;
+            }
+
+            searchStr = (String) PVarchar.INSTANCE.toObject(ptr, child.getSortOrder());
         }
-
-        String sourceStr = (String) PVarchar.INSTANCE.toObject(ptr, getChildren().get(0).getSortOrder());
 
-        position = sourceStr.indexOf(strToSearch) + 1;
+        int position = sourceStr.indexOf(searchStr) + 1;
         ptr.set(PInteger.INSTANCE.toBytes(position));
         return true;
     }

phoenix-core/src/test/java/org/apache/phoenix/expression/function/InstrFunctionTest.java

@@ -17,6 +17,8 @@
  */
 package org.apache.phoenix.expression.function;
 
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 
 import java.sql.SQLException;
@@ -32,18 +34,27 @@
 import org.junit.Test;
 
 public class InstrFunctionTest {
+
+    private static Object evaluateExpression(String value, PDataType<?> dataType, String strToSearch, SortOrder order) throws SQLException {
+      Expression inputArg = LiteralExpression.newConstant(value,dataType,order);
+
+      Expression strToSearchExp = LiteralExpression.newConstant(strToSearch,dataType);
+      List<Expression> expressions = Arrays.<Expression>asList(inputArg,strToSearchExp);
+      Expression instrFunction = new InstrFunction(expressions);
+      ImmutableBytesWritable ptr = new ImmutableBytesWritable();
+      instrFunction.evaluate(null,ptr);
+      return instrFunction.getDataType().toObject(ptr);
+    }
 
-    public static void inputExpression(String value, PDataType dataType, String strToSearch,Integer expected, SortOrder order) throws SQLException{
-        Expression inputArg = LiteralExpression.newConstant(value,dataType,order);
-
-        Expression strToSearchExp = LiteralExpression.newConstant(strToSearch,dataType);
-        List<Expression> expressions = Arrays.<Expression>asList(inputArg,strToSearchExp);
-        Expression instrFunction = new InstrFunction(expressions);
-        ImmutableBytesWritable ptr = new ImmutableBytesWritable();
-        instrFunction.evaluate(null,ptr);
-        Integer result = (Integer) instrFunction.getDataType().toObject(ptr);
-        assertTrue(result.compareTo(expected) == 0);
-
+    public static void inputExpression(String value, PDataType<?> dataType, String strToSearch,Integer expected, SortOrder order) throws SQLException {
+        Object obj = evaluateExpression(value, dataType, strToSearch, order);
+        assertNotNull("Result was unexpectedly null", obj);
+        assertTrue(((Integer) obj).compareTo(expected) == 0);
+    }
+
+    public static void inputNullExpression(String value, PDataType<?> dataType, String strToSearch, SortOrder order) throws SQLException {
+        Object obj = evaluateExpression(value, dataType, strToSearch, order);
+        assertNull("Result was unexpectedly non-null", obj);
     }
 
 
@@ -72,10 +83,13 @@ public void testInstrFunction() throws SQLException {
         inputExpression("ABCDE FGHiJKL",PVarchar.INSTANCE, " ", 6, SortOrder.ASC);
 
         inputExpression("ABCDE FGHiJKL",PVarchar.INSTANCE, " ", 6, SortOrder.DESC);
-
-        inputExpression("ABCDE FGHiJKL",PVarchar.INSTANCE, "", 0, SortOrder.ASC);
-
-        inputExpression("ABCDE FGHiJKL",PVarchar.INSTANCE, "", 0, SortOrder.DESC);
+
+        // Phoenix can't represent empty strings, so an empty or null search string should return null
+        // See PHOENIX-4884 for more chatter.
+        inputNullExpression("ABCDE FGHiJKL",PVarchar.INSTANCE, "", SortOrder.ASC);
+        inputNullExpression("ABCDE FGHiJKL",PVarchar.INSTANCE, "", SortOrder.DESC);
+        inputNullExpression("ABCDE FGHiJKL",PVarchar.INSTANCE, null, SortOrder.ASC);
+        inputNullExpression("ABCDE FGHiJKL",PVarchar.INSTANCE, null, SortOrder.DESC);
 
         inputExpression("ABCDEABC",PVarchar.INSTANCE, "ABC", 1, SortOrder.ASC);