PHOENIX-4884 Update INSTR to handle literals and non-literals in either 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. Project: http://git-wip-us.apache.org/repos/asf/phoenix/repo Commit: http://git-wip-us.apache.org/repos/asf/phoenix/commit/e83c6147 Tree: http://git-wip-us.apache.org/repos/asf/phoenix/tree/e83c6147 Diff: http://git-wip-us.apache.org/repos/asf/phoenix/diff/e83c6147 Branch: refs/heads/4.x-cdh5.15 Commit: e83c6147e5696b34d76de7ae16ab2233bda864ae Parents: cb69793 Author: Josh Elser <els...@apache.org> Authored: Fri Aug 31 15:59:47 2018 +0100 Committer: Pedro Boado <pbo...@apache.org> Committed: Wed Oct 17 22:49:38 2018 +0100 ---------------------------------------------------------------------- .../apache/phoenix/end2end/InstrFunctionIT.java | 35 +++++++++ .../expression/function/InstrFunction.java | 78 +++++++++++++------- .../expression/function/InstrFunctionTest.java | 44 +++++++---- 3 files changed, 114 insertions(+), 43 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/phoenix/blob/e83c6147/phoenix-core/src/it/java/org/apache/phoenix/end2end/InstrFunctionIT.java ---------------------------------------------------------------------- diff --git a/phoenix-core/src/it/java/org/apache/phoenix/end2end/InstrFunctionIT.java b/phoenix-core/src/it/java/org/apache/phoenix/end2end/InstrFunctionIT.java index 270b1ec..bc86980 100644 --- a/phoenix-core/src/it/java/org/apache/phoenix/end2end/InstrFunctionIT.java +++ b/phoenix-core/src/it/java/org/apache/phoenix/end2end/InstrFunctionIT.java @@ -131,4 +131,39 @@ public class InstrFunctionIT extends ParallelStatsDisabledIT { 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"); + } } http://git-wip-us.apache.org/repos/asf/phoenix/blob/e83c6147/phoenix-core/src/main/java/org/apache/phoenix/expression/function/InstrFunction.java ---------------------------------------------------------------------- diff --git a/phoenix-core/src/main/java/org/apache/phoenix/expression/function/InstrFunction.java b/phoenix-core/src/main/java/org/apache/phoenix/expression/function/InstrFunction.java index 7a002f8..e6b4c16 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/expression/function/InstrFunction.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/expression/function/InstrFunction.java @@ -30,7 +30,6 @@ import org.apache.phoenix.schema.tuple.Tuple; 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 }), @@ -38,8 +37,9 @@ import org.apache.phoenix.util.ByteUtil; 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 class InstrFunction extends ScalarFunction{ } 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; } http://git-wip-us.apache.org/repos/asf/phoenix/blob/e83c6147/phoenix-core/src/test/java/org/apache/phoenix/expression/function/InstrFunctionTest.java ---------------------------------------------------------------------- diff --git a/phoenix-core/src/test/java/org/apache/phoenix/expression/function/InstrFunctionTest.java b/phoenix-core/src/test/java/org/apache/phoenix/expression/function/InstrFunctionTest.java index 359d772..6fd16ec 100644 --- a/phoenix-core/src/test/java/org/apache/phoenix/expression/function/InstrFunctionTest.java +++ b/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.apache.phoenix.schema.types.PVarchar; 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 class InstrFunctionTest { 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);
