PHOENIX-3422 PhoenixQueryBuilder doesn't make value string correctly for char(/varchar) column type.
Signed-off-by: Sergey Soldatov <s...@apache.org> Project: http://git-wip-us.apache.org/repos/asf/phoenix/repo Commit: http://git-wip-us.apache.org/repos/asf/phoenix/commit/a225f5ff Tree: http://git-wip-us.apache.org/repos/asf/phoenix/tree/a225f5ff Diff: http://git-wip-us.apache.org/repos/asf/phoenix/diff/a225f5ff Branch: refs/heads/4.x-HBase-0.98 Commit: a225f5ffe773dde7a7efc1ada1d6dbda9d667cdf Parents: cf70820 Author: Jeongdae Kim <kjd9...@gmail.com> Authored: Fri Oct 28 17:13:23 2016 +0900 Committer: Sergey Soldatov <s...@apache.org> Committed: Wed Nov 2 12:58:46 2016 -0700 ---------------------------------------------------------------------- phoenix-hive/pom.xml | 7 +- .../phoenix/hive/query/PhoenixQueryBuilder.java | 129 ++++++++++--------- .../hive/util/PhoenixStorageHandlerUtil.java | 4 +- .../hive/query/PhoenixQueryBuilderTest.java | 87 +++++++++++++ 4 files changed, 163 insertions(+), 64 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/phoenix/blob/a225f5ff/phoenix-hive/pom.xml ---------------------------------------------------------------------- diff --git a/phoenix-hive/pom.xml b/phoenix-hive/pom.xml index 250db49..c36e737 100644 --- a/phoenix-hive/pom.xml +++ b/phoenix-hive/pom.xml @@ -110,7 +110,12 @@ <artifactId>hadoop-minicluster</artifactId> <scope>test</scope> </dependency> - + <dependency> + <groupId>org.mockito</groupId> + <artifactId>mockito-all</artifactId> + <version>${mockito-all.version}</version> + <scope>test</scope> + </dependency> </dependencies> <build> http://git-wip-us.apache.org/repos/asf/phoenix/blob/a225f5ff/phoenix-hive/src/main/java/org/apache/phoenix/hive/query/PhoenixQueryBuilder.java ---------------------------------------------------------------------- diff --git a/phoenix-hive/src/main/java/org/apache/phoenix/hive/query/PhoenixQueryBuilder.java b/phoenix-hive/src/main/java/org/apache/phoenix/hive/query/PhoenixQueryBuilder.java index 8e3a972..a38814d 100644 --- a/phoenix-hive/src/main/java/org/apache/phoenix/hive/query/PhoenixQueryBuilder.java +++ b/phoenix-hive/src/main/java/org/apache/phoenix/hive/query/PhoenixQueryBuilder.java @@ -19,7 +19,9 @@ package org.apache.phoenix.hive.query; import com.google.common.base.CharMatcher; import com.google.common.base.Joiner; +import com.google.common.base.Predicate; import com.google.common.base.Splitter; +import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import org.apache.commons.lang.StringUtils; import org.apache.commons.logging.Log; @@ -31,12 +33,9 @@ import org.apache.phoenix.hive.ql.index.IndexSearchCondition; import org.apache.phoenix.hive.util.PhoenixStorageHandlerUtil; import org.apache.phoenix.hive.util.PhoenixUtil; +import javax.annotation.Nullable; import java.io.IOException; -import java.util.Arrays; -import java.util.Collections; -import java.util.Iterator; -import java.util.List; -import java.util.Map; +import java.util.*; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -662,17 +661,17 @@ public class PhoenixQueryBuilder { comparisonOp); if (comparisonOp.endsWith("UDFOPEqual")) { // column = 1 - appendCondition(sql, " = ", typeName, constantValues[0]); + sql.append(" = ").append(createConstantString(typeName, constantValues[0])); } else if (comparisonOp.endsWith("UDFOPEqualOrGreaterThan")) { // column >= 1 - appendCondition(sql, " >= ", typeName, constantValues[0]); + sql.append(" >= ").append(createConstantString(typeName, constantValues[0])); } else if (comparisonOp.endsWith("UDFOPGreaterThan")) { // column > 1 - appendCondition(sql, " > ", typeName, constantValues[0]); + sql.append(" > ").append(createConstantString(typeName, constantValues[0])); } else if (comparisonOp.endsWith("UDFOPEqualOrLessThan")) { // column <= 1 - appendCondition(sql, " <= ", typeName, constantValues[0]); + sql.append(" <= ").append(createConstantString(typeName, constantValues[0])); } else if (comparisonOp.endsWith("UDFOPLessThan")) { // column < 1 - appendCondition(sql, " < ", typeName, constantValues[0]); + sql.append(" < ").append(createConstantString(typeName, constantValues[0])); } else if (comparisonOp.endsWith("UDFOPNotEqual")) { // column != 1 - appendCondition(sql, " != ", typeName, constantValues[0]); + sql.append(" != ").append(createConstantString(typeName, constantValues[0])); } else if (comparisonOp.endsWith("GenericUDFBetween")) { appendBetweenCondition(jobConf, sql, condition.isNot(), typeName, constantValues); } else if (comparisonOp.endsWith("GenericUDFIn")) { @@ -687,44 +686,16 @@ public class PhoenixQueryBuilder { return conditionColumnList; } - protected void appendCondition(StringBuilder sql, String comparisonOp, String typeName, - String conditionValue) { - if (serdeConstants.STRING_TYPE_NAME.equals(typeName)) { - sql.append(comparisonOp).append("'").append(conditionValue).append("'"); - } else if (serdeConstants.DATE_TYPE_NAME.equals(typeName)) { - sql.append(comparisonOp).append("to_date('").append(conditionValue).append("')"); - } else if (serdeConstants.TIMESTAMP_TYPE_NAME.equals(typeName)) { - sql.append(comparisonOp).append("to_timestamp('").append(conditionValue).append("')"); - } else { - sql.append(comparisonOp).append(conditionValue); - } - } - protected void appendBetweenCondition(JobConf jobConf, StringBuilder sql, boolean isNot, String typeName, String[] conditionValues) throws IOException { - if (isNot) { - sql.append(" not between "); - } else { - sql.append(" between "); - } - try { - Arrays.sort(PhoenixStorageHandlerUtil.toTypedValues(jobConf, typeName, - conditionValues)); - - if (serdeConstants.STRING_TYPE_NAME.equals(typeName)) { - sql.append("'").append(conditionValues[0]).append("'").append(" and ").append - ("'").append(conditionValues[1]).append("'"); - } else if (serdeConstants.DATE_TYPE_NAME.equals(typeName)) { - sql.append("to_date('").append(conditionValues[0]).append("')").append(" and ") - .append("to_date('").append(conditionValues[1]).append("')"); - } else if (serdeConstants.TIMESTAMP_TYPE_NAME.equals(typeName)) { - sql.append("to_timestamp('").append(conditionValues[0]).append("')").append(" and" + - " ").append("to_timestamp('").append(conditionValues[1]).append("')"); - } else { - sql.append(conditionValues[0]).append(" and ").append(conditionValues[1]); - } + Object[] typedValues = PhoenixStorageHandlerUtil.toTypedValues(jobConf, typeName, conditionValues); + Arrays.sort(typedValues); + + appendIfNot(isNot, sql).append(" between ") + .append(Joiner.on(" and ").join(createConstantString(typeName, typedValues[0]), + createConstantString(typeName, typedValues[1]))); } catch (Exception e) { throw new IOException(e); } @@ -732,29 +703,63 @@ public class PhoenixQueryBuilder { protected void appendInCondition(StringBuilder sql, boolean isNot, String typeName, String[] conditionValues) { - if (isNot) { - sql.append(" not in ("); - } else { - sql.append(" in ("); + List<Object> wrappedConstants = Lists.newArrayListWithCapacity(conditionValues.length); + for (String conditionValue : conditionValues) { + wrappedConstants.add(createConstantString(typeName, conditionValue)); } - for (String conditionValue : conditionValues) { - if (serdeConstants.STRING_TYPE_NAME.equals(typeName)) { - sql.append("'").append(conditionValue).append("'"); - } else if (serdeConstants.DATE_TYPE_NAME.equals(typeName)) { - sql.append("to_date('").append(conditionValue).append("')"); - } else if (serdeConstants.TIMESTAMP_TYPE_NAME.equals(typeName)) { - sql.append("to_timestamp('").append(conditionValue).append("')"); - } else { - sql.append(conditionValue); - } + appendIfNot(isNot, sql) + .append(" in (") + .append(Joiner.on(", ").join(wrappedConstants)) + .append(")"); + } - sql.append(", "); + private StringBuilder appendIfNot(boolean isNot, StringBuilder sb) { + return isNot ? sb.append(" not") : sb; + } + + private static class ConstantStringWrapper { + private List<String> types; + private String prefix; + private String postfix; + + ConstantStringWrapper(String type, String prefix, String postfix) { + this(Lists.newArrayList(type), prefix, postfix); } - sql.delete(sql.length() - 2, sql.length()); + ConstantStringWrapper(List<String> types, String prefix, String postfix) { + this.types = types; + this.prefix = prefix; + this.postfix = postfix; + } - sql.append(")"); + public Object apply(final String typeName, Object value) { + return Iterables.any(types, new Predicate<String>() { + + @Override + public boolean apply(@Nullable String type) { + return typeName.startsWith(type); + } + }) ? prefix + value + postfix : value; + } } + private static final String SINGLE_QUOTATION = "'"; + private static List<ConstantStringWrapper> WRAPPERS = Lists.newArrayList( + new ConstantStringWrapper(Lists.newArrayList( + serdeConstants.STRING_TYPE_NAME, serdeConstants.CHAR_TYPE_NAME, + serdeConstants.VARCHAR_TYPE_NAME, serdeConstants.DATE_TYPE_NAME, + serdeConstants.TIMESTAMP_TYPE_NAME + ), SINGLE_QUOTATION, SINGLE_QUOTATION), + new ConstantStringWrapper(serdeConstants.DATE_TYPE_NAME, "to_date(", ")"), + new ConstantStringWrapper(serdeConstants.TIMESTAMP_TYPE_NAME, "to_timestamp(", ")") + ); + + private Object createConstantString(String typeName, Object value) { + for (ConstantStringWrapper wrapper : WRAPPERS) { + value = wrapper.apply(typeName, value); + } + + return value; + } } http://git-wip-us.apache.org/repos/asf/phoenix/blob/a225f5ff/phoenix-hive/src/main/java/org/apache/phoenix/hive/util/PhoenixStorageHandlerUtil.java ---------------------------------------------------------------------- diff --git a/phoenix-hive/src/main/java/org/apache/phoenix/hive/util/PhoenixStorageHandlerUtil.java b/phoenix-hive/src/main/java/org/apache/phoenix/hive/util/PhoenixStorageHandlerUtil.java index 1313fdb..0dd1134 100644 --- a/phoenix-hive/src/main/java/org/apache/phoenix/hive/util/PhoenixStorageHandlerUtil.java +++ b/phoenix-hive/src/main/java/org/apache/phoenix/hive/util/PhoenixStorageHandlerUtil.java @@ -76,7 +76,9 @@ public class PhoenixStorageHandlerUtil { DateFormat df = null; for (int i = 0, limit = values.length; i < limit; i++) { - if (serdeConstants.STRING_TYPE_NAME.equals(typeName)) { + if (serdeConstants.STRING_TYPE_NAME.equals(typeName) || + typeName.startsWith(serdeConstants.CHAR_TYPE_NAME) || + typeName.startsWith(serdeConstants.VARCHAR_TYPE_NAME)) { results[i] = values[i]; } else if (serdeConstants.INT_TYPE_NAME.equals(typeName)) { results[i] = new Integer(values[i]); http://git-wip-us.apache.org/repos/asf/phoenix/blob/a225f5ff/phoenix-hive/src/test/java/org/apache/phoenix/hive/query/PhoenixQueryBuilderTest.java ---------------------------------------------------------------------- diff --git a/phoenix-hive/src/test/java/org/apache/phoenix/hive/query/PhoenixQueryBuilderTest.java b/phoenix-hive/src/test/java/org/apache/phoenix/hive/query/PhoenixQueryBuilderTest.java new file mode 100644 index 0000000..7f1a7c3 --- /dev/null +++ b/phoenix-hive/src/test/java/org/apache/phoenix/hive/query/PhoenixQueryBuilderTest.java @@ -0,0 +1,87 @@ +package org.apache.phoenix.hive.query; + +import com.google.common.collect.Lists; +import org.apache.commons.lang.ArrayUtils; +import org.apache.hadoop.hive.ql.plan.ExprNodeColumnDesc; +import org.apache.hadoop.hive.ql.plan.ExprNodeConstantDesc; +import org.apache.hadoop.mapred.JobConf; +import org.apache.phoenix.hive.ql.index.IndexSearchCondition; +import org.junit.Test; + +import java.io.IOException; +import java.util.List; + +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; +import static org.junit.Assert.assertEquals; + +public class PhoenixQueryBuilderTest { + private IndexSearchCondition mockedIndexSearchCondition(String comparisionOp, + Object constantValue, + Object[] constantValues, + String columnName, + String typeString, + boolean isNot) { + IndexSearchCondition condition = mock(IndexSearchCondition.class); + when(condition.getComparisonOp()).thenReturn(comparisionOp); + + ExprNodeConstantDesc constantDesc = mock(ExprNodeConstantDesc.class); + when(constantDesc.getValue()).thenReturn(constantValue); + when(condition.getConstantDesc()).thenReturn(constantDesc); + + ExprNodeColumnDesc columnDesc = mock(ExprNodeColumnDesc.class); + when(columnDesc.getColumn()).thenReturn(columnName); + when(columnDesc.getTypeString()).thenReturn(typeString); + when(condition.getColumnDesc()).thenReturn(columnDesc); + + + if (ArrayUtils.isNotEmpty(constantValues)) { + ExprNodeConstantDesc[] constantDescs = new ExprNodeConstantDesc[constantValues.length]; + for (int i = 0; i < constantDescs.length; i++) { + constantDescs[i] = mock(ExprNodeConstantDesc.class); + when(condition.getConstantDesc(i)).thenReturn(constantDescs[i]); + when(constantDescs[i].getValue()).thenReturn(constantValues[i]); + } + when(condition.getConstantDescs()).thenReturn(constantDescs); + } + + when(condition.isNot()).thenReturn(isNot); + + return condition; + } + + @Test + public void testBuildQueryWithCharColumns() throws IOException { + final String tableName = "TEST_TABLE"; + final String COLUMN_CHAR = "Column_Char"; + final String COLUMN_VARCHAR = "Column_VChar"; + final String expectedQueryPrefix = "select /*+ NO_CACHE */ " + COLUMN_CHAR + "," + COLUMN_VARCHAR + + " from TEST_TABLE where "; + + JobConf jobConf = new JobConf(); + List<String> readColumnList = Lists.newArrayList(COLUMN_CHAR, COLUMN_VARCHAR); + List<IndexSearchCondition> searchConditions = Lists.newArrayList( + mockedIndexSearchCondition("GenericUDFOPEqual", "CHAR_VALUE", null, COLUMN_CHAR, "char(10)", false), + mockedIndexSearchCondition("GenericUDFOPEqual", "CHAR_VALUE2", null, COLUMN_VARCHAR, "varchar(10)", false) + ); + + assertEquals(expectedQueryPrefix + "Column_Char = 'CHAR_VALUE' and Column_VChar = 'CHAR_VALUE2'", + PhoenixQueryBuilder.getInstance().buildQuery(jobConf, tableName, readColumnList, searchConditions)); + + searchConditions = Lists.newArrayList( + mockedIndexSearchCondition("GenericUDFIn", null, + new Object[]{"CHAR1", "CHAR2", "CHAR3"}, COLUMN_CHAR, "char(10)", false) + ); + + assertEquals(expectedQueryPrefix + "Column_Char in ('CHAR1', 'CHAR2', 'CHAR3')", + PhoenixQueryBuilder.getInstance().buildQuery(jobConf, tableName, readColumnList, searchConditions)); + + searchConditions = Lists.newArrayList( + mockedIndexSearchCondition("GenericUDFBetween", null, + new Object[]{"CHAR1", "CHAR2"}, COLUMN_CHAR, "char(10)", false) + ); + + assertEquals(expectedQueryPrefix + "Column_Char between 'CHAR1' and 'CHAR2'", + PhoenixQueryBuilder.getInstance().buildQuery(jobConf, tableName, readColumnList, searchConditions)); + } +}
