PHOENIX-3512 PhoenixStorageHandler makes erroneous query string when handling between clauses with date constants.
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/03c2bc2c Tree: http://git-wip-us.apache.org/repos/asf/phoenix/tree/03c2bc2c Diff: http://git-wip-us.apache.org/repos/asf/phoenix/diff/03c2bc2c Branch: refs/heads/4.x-HBase-1.1 Commit: 03c2bc2cd5c7d6a3134b9508b42315a36e4f2f78 Parents: bc09078 Author: Jeongdae Kim <kjd9...@gmail.com> Authored: Tue Feb 21 14:58:28 2017 +0900 Committer: Sergey Soldatov <s...@apache.org> Committed: Tue Feb 21 22:05:08 2017 -0800 ---------------------------------------------------------------------- .../phoenix/hive/query/PhoenixQueryBuilder.java | 287 +++++++++++-------- .../hive/query/PhoenixQueryBuilderTest.java | 87 +++++- 2 files changed, 240 insertions(+), 134 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/phoenix/blob/03c2bc2c/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 d1e74d9..ebc5fc0 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 @@ -18,6 +18,7 @@ package org.apache.phoenix.hive.query; import com.google.common.base.CharMatcher; +import com.google.common.base.Function; import com.google.common.base.Joiner; import com.google.common.base.Predicate; import com.google.common.base.Splitter; @@ -35,6 +36,7 @@ import javax.annotation.Nullable; import org.apache.commons.lang.StringUtils; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.apache.hadoop.hive.ql.plan.ExprNodeConstantDesc; import org.apache.hadoop.hive.serde.serdeConstants; import org.apache.hadoop.hive.serde2.typeinfo.TypeInfo; import org.apache.hadoop.mapred.JobConf; @@ -42,6 +44,7 @@ import org.apache.phoenix.hive.constants.PhoenixStorageHandlerConstants; import org.apache.phoenix.hive.ql.index.IndexSearchCondition; import org.apache.phoenix.hive.util.PhoenixStorageHandlerUtil; import org.apache.phoenix.hive.util.PhoenixUtil; +import org.apache.phoenix.util.StringUtil; /** * Query builder. Produces a query depending on the colummn list and conditions @@ -87,7 +90,7 @@ public class PhoenixQueryBuilder { readColumnList, String whereClause, String queryTemplate, String hints, Map<String, TypeInfo> columnTypeMap) throws IOException { StringBuilder sql = new StringBuilder(); - List<String> conditionColumnList = buildWhereClause(jobConf, sql, whereClause,columnTypeMap); + List<String> conditionColumnList = buildWhereClause(jobConf, sql, whereClause, columnTypeMap); if (conditionColumnList.size() > 0) { addConditionColumnToReadColumn(readColumnList, conditionColumnList); @@ -110,25 +113,25 @@ public class PhoenixQueryBuilder { private String makeQueryString(JobConf jobConf, String tableName, List<String> readColumnList, List<IndexSearchCondition> searchConditions, String queryTemplate, String hints) throws IOException { - StringBuilder sql = new StringBuilder(); - List<String> conditionColumnList = buildWhereClause(jobConf, sql, searchConditions); + StringBuilder query = new StringBuilder(); + List<String> conditionColumnList = buildWhereClause(query, searchConditions); if (conditionColumnList.size() > 0) { addConditionColumnToReadColumn(readColumnList, conditionColumnList); - sql.insert(0, queryTemplate.replace("$HINT$", hints).replace("$COLUMN_LIST$", + query.insert(0, queryTemplate.replace("$HINT$", hints).replace("$COLUMN_LIST$", getSelectColumns(jobConf, tableName, readColumnList)).replace("$TABLE_NAME$", tableName)); } else { - sql.append(queryTemplate.replace("$HINT$", hints).replace("$COLUMN_LIST$", + query.append(queryTemplate.replace("$HINT$", hints).replace("$COLUMN_LIST$", getSelectColumns(jobConf, tableName, readColumnList)).replace("$TABLE_NAME$", tableName)); } if (LOG.isInfoEnabled()) { - LOG.info("Input query : " + sql.toString()); + LOG.info("Input query : " + query.toString()); } - return sql.toString(); + return query.toString(); } private String getSelectColumns(JobConf jobConf, String tableName, List<String> @@ -614,155 +617,189 @@ public class PhoenixQueryBuilder { return itsMine; } - protected List<String> buildWhereClause(JobConf jobConf, StringBuilder sql, - List<IndexSearchCondition> searchConditions) throws - IOException { - if (searchConditions == null || searchConditions.size() == 0) { + protected List<String> buildWhereClause(StringBuilder sql, + List<IndexSearchCondition> conditions) + throws IOException { + if (conditions == null || conditions.size() == 0) { return Collections.emptyList(); } - List<String> conditionColumnList = Lists.newArrayList(); + List<String> columns = Lists.newArrayList(); sql.append(" where "); - boolean firstCondition = true; - for (IndexSearchCondition condition : searchConditions) { - String comparisonOp = condition.getComparisonOp(); + Iterator<IndexSearchCondition> iter = conditions.iterator(); + appendExpression(sql, iter.next(), columns); + while (iter.hasNext()) { + sql.append(" and "); + appendExpression(sql, iter.next(), columns); + } - if (comparisonOp.endsWith("GenericUDFBetween") || comparisonOp.endsWith - ("GenericUDFIn")) { - if (condition.getConstantDescs() == null) { - continue; - } - } else if (comparisonOp.endsWith("GenericUDFOPNull") || comparisonOp.endsWith - ("GenericUDFOPNotNull")) { - // keep going - } else if (comparisonOp.endsWith("GenericUDFOPEqual")) { - // keep going - } else { - if (condition.getConstantDesc().getValue() == null) { - continue; - } - } + return columns; + } - if (!firstCondition) { - sql.append(" and "); - } else { - firstCondition = false; - } + private void appendExpression(StringBuilder sql, IndexSearchCondition condition, + List<String> columns) { + Expression expr = findExpression(condition); + if (expr != null) { + sql.append(expr.buildExpressionStringFrom(condition)); + columns.add(condition.getColumnDesc().getColumn()); + } + } - String columnName = condition.getColumnDesc().getColumn(); - String typeName = condition.getColumnDesc().getTypeString(); + private Expression findExpression(final IndexSearchCondition condition) { + return Iterables.tryFind(Arrays.asList(Expression.values()), new Predicate<Expression>() { + @Override + public boolean apply(@Nullable Expression expr) { + return expr.isFor(condition); + } + }).orNull(); + } - if (LOG.isDebugEnabled()) { - LOG.debug(columnName + " has condition: " + condition); + private static final Joiner JOINER_COMMA = Joiner.on(", "); + private static final Joiner JOINER_AND = Joiner.on(" and "); + private static final Joiner JOINER_SPACE = Joiner.on(" "); + + private enum Expression { + EQUAL("UDFOPEqual", "="), + GREATER_THAN_OR_EQUAL_TO("UDFOPEqualOrGreaterThan", ">="), + GREATER_THAN("UDFOPGreaterThan", ">"), + LESS_THAN_OR_EQUAL_TO("UDFOPEqualOrLessThan", "<="), + LESS_THAN("UDFOPLessThan", "<"), + NOT_EQUAL("UDFOPNotEqual", "!="), + BETWEEN("GenericUDFBetween", "between", JOINER_AND, true) { + public boolean checkCondition(IndexSearchCondition condition) { + return condition.getConstantDescs() != null; + } + }, + IN("GenericUDFIn", "in", JOINER_COMMA, true) { + public boolean checkCondition(IndexSearchCondition condition) { + return condition.getConstantDescs() != null; } - conditionColumnList.add(columnName); - sql.append(columnName); - - String[] constantValues = PhoenixStorageHandlerUtil.getConstantValues(condition, - comparisonOp); - - if (comparisonOp.endsWith("UDFOPEqual")) { // column = 1 - sql.append(" = ").append(createConstantString(typeName, constantValues[0])); - } else if (comparisonOp.endsWith("UDFOPEqualOrGreaterThan")) { // column >= 1 - sql.append(" >= ").append(createConstantString(typeName, constantValues[0])); - } else if (comparisonOp.endsWith("UDFOPGreaterThan")) { // column > 1 - sql.append(" > ").append(createConstantString(typeName, constantValues[0])); - } else if (comparisonOp.endsWith("UDFOPEqualOrLessThan")) { // column <= 1 - sql.append(" <= ").append(createConstantString(typeName, constantValues[0])); - } else if (comparisonOp.endsWith("UDFOPLessThan")) { // column < 1 - sql.append(" < ").append(createConstantString(typeName, constantValues[0])); - } else if (comparisonOp.endsWith("UDFOPNotEqual")) { // column != 1 - sql.append(" != ").append(createConstantString(typeName, constantValues[0])); - } else if (comparisonOp.endsWith("GenericUDFBetween")) { - appendBetweenCondition(jobConf, sql, condition.isNot(), typeName, constantValues); - } else if (comparisonOp.endsWith("GenericUDFIn")) { - appendInCondition(sql, condition.isNot(), typeName, constantValues); - } else if (comparisonOp.endsWith("GenericUDFOPNull")) { - sql.append(" is null "); - } else if (comparisonOp.endsWith("GenericUDFOPNotNull")) { - sql.append(" is not null "); + public String createConstants(final String typeName, ExprNodeConstantDesc[] desc) { + return "(" + super.createConstants(typeName, desc) + ")"; } - } + }, + IS_NULL("GenericUDFOPNull", "is null") { + public boolean checkCondition(IndexSearchCondition condition) { + return true; + } + }, + IS_NOT_NULL("GenericUDFOPNotNull", "is not null") { + public boolean checkCondition(IndexSearchCondition condition) { + return true; + } + }; - return conditionColumnList; - } + private final String hiveCompOp; + private final String sqlCompOp; + private final Joiner joiner; + private final boolean supportNotOperator; - protected void appendBetweenCondition(JobConf jobConf, StringBuilder sql, boolean isNot, - String typeName, String[] conditionValues) throws - IOException { - try { - Object[] typedValues = PhoenixStorageHandlerUtil.toTypedValues(jobConf, typeName, conditionValues); - Arrays.sort(typedValues); + Expression(String hiveCompOp, String sqlCompOp) { + this(hiveCompOp, sqlCompOp, null); + } - 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); + Expression(String hiveCompOp, String sqlCompOp, Joiner joiner) { + this(hiveCompOp, sqlCompOp, joiner, false); } - } - protected void appendInCondition(StringBuilder sql, boolean isNot, String typeName, String[] - conditionValues) { - List<Object> wrappedConstants = Lists.newArrayListWithCapacity(conditionValues.length); - for (String conditionValue : conditionValues) { - wrappedConstants.add(createConstantString(typeName, conditionValue)); + Expression(String hiveCompOp, String sqlCompOp, Joiner joiner, boolean supportNotOp) { + this.hiveCompOp = hiveCompOp; + this.sqlCompOp = sqlCompOp; + this.joiner = joiner; + this.supportNotOperator = supportNotOp; } - appendIfNot(isNot, sql) - .append(" in (") - .append(Joiner.on(", ").join(wrappedConstants)) - .append(")"); - } + public boolean checkCondition(IndexSearchCondition condition) { + return condition.getConstantDesc().getValue() != null; + } - private StringBuilder appendIfNot(boolean isNot, StringBuilder sb) { - return isNot ? sb.append(" not") : sb; - } + public boolean isFor(IndexSearchCondition condition) { + return condition.getComparisonOp().endsWith(hiveCompOp) && checkCondition(condition); + } - private static class ConstantStringWrapper { - private List<String> types; - private String prefix; - private String postfix; + public String buildExpressionStringFrom(IndexSearchCondition condition) { + final String type = condition.getColumnDesc().getTypeString(); + return JOINER_SPACE.join( + condition.getColumnDesc().getColumn(), + getSqlCompOpString(condition), + joiner != null ? createConstants(type, condition.getConstantDescs()) : + createConstant(type, condition.getConstantDesc())); + } - ConstantStringWrapper(String type, String prefix, String postfix) { - this(Lists.newArrayList(type), prefix, postfix); + public String getSqlCompOpString(IndexSearchCondition condition) { + return supportNotOperator ? + (condition.isNot() ? "not " : "") + sqlCompOp : sqlCompOp; } - ConstantStringWrapper(List<String> types, String prefix, String postfix) { - this.types = types; - this.prefix = prefix; - this.postfix = postfix; + public String createConstant(String typeName, ExprNodeConstantDesc constantDesc) { + if (constantDesc == null) { + return StringUtil.EMPTY_STRING; + } + + return createConstantString(typeName, String.valueOf(constantDesc.getValue())); } - public Object apply(final String typeName, Object value) { - return Iterables.any(types, new Predicate<String>() { + public String createConstants(final String typeName, ExprNodeConstantDesc[] constantDesc) { + if (constantDesc == null) { + return StringUtil.EMPTY_STRING; + } - @Override - public boolean apply(@Nullable String type) { - return typeName.startsWith(type); - } - }) ? prefix + value + postfix : value; + return joiner.join(Iterables.transform(Arrays.asList(constantDesc), + new Function<ExprNodeConstantDesc, String>() { + @Nullable + @Override + public String apply(@Nullable ExprNodeConstantDesc desc) { + return createConstantString(typeName, String.valueOf(desc.getValue())); + } + } + )); } - } - 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 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); + } + + ConstantStringWrapper(List<String> types, String prefix, String postfix) { + this.types = types; + this.prefix = prefix; + this.postfix = postfix; + } + + public String apply(final String typeName, String value) { + return Iterables.any(types, new Predicate<String>() { - private Object createConstantString(String typeName, Object value) { - for (ConstantStringWrapper wrapper : WRAPPERS) { - value = wrapper.apply(typeName, value); + @Override + public boolean apply(@Nullable String type) { + return typeName.startsWith(type); + } + }) ? prefix + value + postfix : value; + } } - return 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 String createConstantString(String typeName, String value) { + for (ConstantStringWrapper wrapper : WRAPPERS) { + value = wrapper.apply(typeName, value); + } + + return value; + } } } http://git-wip-us.apache.org/repos/asf/phoenix/blob/03c2bc2c/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 index 920e8cf..1dc6e25 100644 --- 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 @@ -33,6 +33,9 @@ import static org.mockito.Mockito.when; import static org.junit.Assert.assertEquals; public class PhoenixQueryBuilderTest { + private static final PhoenixQueryBuilder BUILDER = PhoenixQueryBuilder.getInstance(); + private static final String TABLE_NAME = "TEST_TABLE"; + private IndexSearchCondition mockedIndexSearchCondition(String comparisionOp, Object constantValue, Object[] constantValues, @@ -42,9 +45,11 @@ public class PhoenixQueryBuilderTest { 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); + if (constantValue != null) { + 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); @@ -69,7 +74,6 @@ public class PhoenixQueryBuilderTest { @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 + @@ -78,12 +82,12 @@ public class PhoenixQueryBuilderTest { 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) + 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)); + BUILDER.buildQuery(jobConf, TABLE_NAME, readColumnList, searchConditions)); searchConditions = Lists.newArrayList( mockedIndexSearchCondition("GenericUDFIn", null, @@ -91,7 +95,15 @@ public class PhoenixQueryBuilderTest { ); assertEquals(expectedQueryPrefix + "Column_Char in ('CHAR1', 'CHAR2', 'CHAR3')", - PhoenixQueryBuilder.getInstance().buildQuery(jobConf, tableName, readColumnList, searchConditions)); + BUILDER.buildQuery(jobConf, TABLE_NAME, readColumnList, searchConditions)); + + searchConditions = Lists.newArrayList( + mockedIndexSearchCondition("GenericUDFIn", null, + new Object[]{"CHAR1", "CHAR2", "CHAR3"}, COLUMN_CHAR, "char(10)", true) + ); + + assertEquals(expectedQueryPrefix + "Column_Char not in ('CHAR1', 'CHAR2', 'CHAR3')", + BUILDER.buildQuery(jobConf, TABLE_NAME, readColumnList, searchConditions)); searchConditions = Lists.newArrayList( mockedIndexSearchCondition("GenericUDFBetween", null, @@ -99,6 +111,63 @@ public class PhoenixQueryBuilderTest { ); assertEquals(expectedQueryPrefix + "Column_Char between 'CHAR1' and 'CHAR2'", - PhoenixQueryBuilder.getInstance().buildQuery(jobConf, tableName, readColumnList, searchConditions)); + BUILDER.buildQuery(jobConf, TABLE_NAME, readColumnList, searchConditions)); + + searchConditions = Lists.newArrayList( + mockedIndexSearchCondition("GenericUDFBetween", null, + new Object[]{"CHAR1", "CHAR2"}, COLUMN_CHAR, "char(10)", true) + ); + + assertEquals(expectedQueryPrefix + "Column_Char not between 'CHAR1' and 'CHAR2'", + BUILDER.buildQuery(jobConf, TABLE_NAME, readColumnList, searchConditions)); + } + + @Test + public void testBuildBetweenQueryWithDateColumns() throws IOException { + final String COLUMN_DATE = "Column_Date"; + final String tableName = "TEST_TABLE"; + final String expectedQueryPrefix = "select /*+ NO_CACHE */ " + COLUMN_DATE + + " from " + tableName + " where "; + + JobConf jobConf = new JobConf(); + List<String> readColumnList = Lists.newArrayList(COLUMN_DATE); + + List<IndexSearchCondition> searchConditions = Lists.newArrayList( + mockedIndexSearchCondition("GenericUDFBetween", null, + new Object[]{"1992-01-02", "1992-02-02"}, COLUMN_DATE, "date", false) + ); + + assertEquals(expectedQueryPrefix + + COLUMN_DATE + " between to_date('1992-01-02') and to_date('1992-02-02')", + BUILDER.buildQuery(jobConf, TABLE_NAME, readColumnList, searchConditions)); + + searchConditions = Lists.newArrayList( + mockedIndexSearchCondition("GenericUDFBetween", null, + new Object[]{"1992-01-02", "1992-02-02"}, COLUMN_DATE, "date", true) + ); + + assertEquals(expectedQueryPrefix + + COLUMN_DATE + " not between to_date('1992-01-02') and to_date('1992-02-02')", + BUILDER.buildQuery(jobConf, TABLE_NAME, readColumnList, searchConditions)); + } + + @Test + public void testBuildQueryWithNotNull() throws IOException { + final String COLUMN_DATE = "Column_Date"; + final String tableName = "TEST_TABLE"; + final String expectedQueryPrefix = "select /*+ NO_CACHE */ " + COLUMN_DATE + + " from " + tableName + " where "; + + JobConf jobConf = new JobConf(); + List<String> readColumnList = Lists.newArrayList(COLUMN_DATE); + + List<IndexSearchCondition> searchConditions = Lists.newArrayList( + mockedIndexSearchCondition("GenericUDFOPNotNull", null, + null, COLUMN_DATE, "date", true) + ); + + assertEquals(expectedQueryPrefix + + COLUMN_DATE + " is not null ", + BUILDER.buildQuery(jobConf, TABLE_NAME, readColumnList, searchConditions)); } }
