Repository: drill Updated Branches: refs/heads/master e02aa596f -> aaf220ffd
DRILL-4704: Fix incorrect query result when decimal is compared with int value Make sure implicit cast function is provided with the correct precision value for int/bigint input. This version of the fix passes the unit test, but may increase the chance of an overflow error, since the precision is not minimized, and thus it is more likely to exceed that of the destination decimal. see DRILL-4834. Close apache/drill#517 Project: http://git-wip-us.apache.org/repos/asf/drill/repo Commit: http://git-wip-us.apache.org/repos/asf/drill/commit/aaf220ff Tree: http://git-wip-us.apache.org/repos/asf/drill/tree/aaf220ff Diff: http://git-wip-us.apache.org/repos/asf/drill/diff/aaf220ff Branch: refs/heads/master Commit: aaf220ffdddaa329e38237d69b438f4e09d16a87 Parents: e02aa59 Author: Dave Oshinsky <daveoshin...@yahoo.com> Authored: Thu Jun 9 18:08:01 2016 -0400 Committer: Jinfeng Ni <j...@apache.org> Committed: Mon Aug 8 20:44:38 2016 -0700 ---------------------------------------------------------------------- .../exec/expr/ExpressionTreeMaterializer.java | 16 +++- .../exec/store/parquet/TestFixedlenDecimal.java | 82 +++++++++++++++++++ .../resources/parquet/fixedlenDecimal.parquet | Bin 0 -> 11297 bytes .../apache/drill/exec/util/DecimalUtility.java | 2 + 4 files changed, 98 insertions(+), 2 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/drill/blob/aaf220ff/exec/java-exec/src/main/java/org/apache/drill/exec/expr/ExpressionTreeMaterializer.java ---------------------------------------------------------------------- diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/expr/ExpressionTreeMaterializer.java b/exec/java-exec/src/main/java/org/apache/drill/exec/expr/ExpressionTreeMaterializer.java index daac31d..bece93d 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/expr/ExpressionTreeMaterializer.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/expr/ExpressionTreeMaterializer.java @@ -89,6 +89,7 @@ import com.google.common.base.Predicate; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; +import org.apache.drill.exec.util.DecimalUtility; public class ExpressionTreeMaterializer { @@ -267,6 +268,17 @@ public class ExpressionTreeMaterializer { return new BooleanOperator(op.getName(), args, op.getPosition()); } + private int computePrecision(LogicalExpression currentArg) { + int precision = currentArg.getMajorType().getPrecision(); + if (currentArg.getMajorType().getMinorType() == MinorType.INT) { + precision = DecimalUtility.MAX_DIGITS_INT; + } + else if (currentArg.getMajorType().getMinorType() == MinorType.BIGINT) { + precision = DecimalUtility.MAX_DIGITS_BIGINT; + } + return precision; + } + @Override public LogicalExpression visitFunctionCall(FunctionCall call, FunctionLookupContext functionLookupContext) { List<LogicalExpression> args = Lists.newArrayList(); @@ -313,7 +325,7 @@ public class ExpressionTreeMaterializer { if (CoreDecimalUtility.isDecimalType(parmType)) { // We are implicitly promoting a decimal type, set the required scale and precision parmType = MajorType.newBuilder().setMinorType(parmType.getMinorType()).setMode(parmType.getMode()). - setScale(currentArg.getMajorType().getScale()).setPrecision(currentArg.getMajorType().getPrecision()).build(); + setScale(currentArg.getMajorType().getScale()).setPrecision(computePrecision(currentArg)).build(); } argsWithCast.add(addCastExpression(currentArg, parmType, functionLookupContext, errorCollector)); } @@ -339,7 +351,7 @@ public class ExpressionTreeMaterializer { if (CoreDecimalUtility.isDecimalType(parmType)) { // We are implicitly promoting a decimal type, set the required scale and precision parmType = MajorType.newBuilder().setMinorType(parmType.getMinorType()).setMode(parmType.getMode()). - setScale(currentArg.getMajorType().getScale()).setPrecision(currentArg.getMajorType().getPrecision()).build(); + setScale(currentArg.getMajorType().getScale()).setPrecision(computePrecision(currentArg)).build(); } extArgsWithCast.add(addCastExpression(call.args.get(i), parmType, functionLookupContext, errorCollector)); } http://git-wip-us.apache.org/repos/asf/drill/blob/aaf220ff/exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/TestFixedlenDecimal.java ---------------------------------------------------------------------- diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/TestFixedlenDecimal.java b/exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/TestFixedlenDecimal.java new file mode 100644 index 0000000..124a8c8 --- /dev/null +++ b/exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/TestFixedlenDecimal.java @@ -0,0 +1,82 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.drill.exec.store.parquet; + +import org.apache.drill.BaseTestQuery; +import org.apache.drill.exec.planner.physical.PlannerSettings; +import org.apache.drill.exec.proto.UserBitShared; +import org.junit.BeforeClass; +import org.junit.Test; + +public class TestFixedlenDecimal extends BaseTestQuery { + // enable decimal data type + @BeforeClass + public static void enableDecimalDataType() throws Exception { + test(String.format("alter session set `%s` = true", PlannerSettings.ENABLE_DECIMAL_DATA_TYPE_KEY)); + } + + private static final String DATAFILE = "cp.`parquet/fixedlenDecimal.parquet`"; + + @Test + public void testNullCount() throws Exception { + String query = String.format("select count(*) as c from %s where department_id is null", DATAFILE); + testBuilder() + .sqlQuery(query) + .unOrdered() + .baselineColumns("c") + .baselineValues(1L) + .build() + .run(); + } + + @Test + public void testNotNullCount() throws Exception { + String query = String.format("select count(*) as c from %s where department_id is not null", DATAFILE); + testBuilder() + .sqlQuery(query) + .unOrdered() + .baselineColumns("c") + .baselineValues(106L) + .build() + .run(); + } + + @Test + public void testSimpleQueryWithCast() throws Exception { + String query = String.format("select cast(department_id as bigint) as c from %s where cast(employee_id as decimal) = 170", DATAFILE); + testBuilder() + .sqlQuery(query) + .unOrdered() + .baselineColumns("c") + .baselineValues(80L) + .build() + .run(); + } + + @Test + public void testSimpleQueryDrill4704Fix() throws Exception { + String query = String.format("select cast(department_id as bigint) as c from %s where employee_id = 170", DATAFILE); + testBuilder() + .sqlQuery(query) + .unOrdered() + .baselineColumns("c") + .baselineValues(80L) + .build() + .run(); + } +} http://git-wip-us.apache.org/repos/asf/drill/blob/aaf220ff/exec/java-exec/src/test/resources/parquet/fixedlenDecimal.parquet ---------------------------------------------------------------------- diff --git a/exec/java-exec/src/test/resources/parquet/fixedlenDecimal.parquet b/exec/java-exec/src/test/resources/parquet/fixedlenDecimal.parquet new file mode 100644 index 0000000..42b474b Binary files /dev/null and b/exec/java-exec/src/test/resources/parquet/fixedlenDecimal.parquet differ http://git-wip-us.apache.org/repos/asf/drill/blob/aaf220ff/exec/vector/src/main/java/org/apache/drill/exec/util/DecimalUtility.java ---------------------------------------------------------------------- diff --git a/exec/vector/src/main/java/org/apache/drill/exec/util/DecimalUtility.java b/exec/vector/src/main/java/org/apache/drill/exec/util/DecimalUtility.java index e8130ec..a87fe4c 100644 --- a/exec/vector/src/main/java/org/apache/drill/exec/util/DecimalUtility.java +++ b/exec/vector/src/main/java/org/apache/drill/exec/util/DecimalUtility.java @@ -33,6 +33,8 @@ import org.apache.drill.exec.expr.holders.Decimal38SparseHolder; public class DecimalUtility extends CoreDecimalUtility{ public final static int MAX_DIGITS = 9; + public final static int MAX_DIGITS_INT = 10; + public final static int MAX_DIGITS_BIGINT = 19; public final static int DIGITS_BASE = 1000000000; public final static int DIGITS_MAX = 999999999; public final static int INTEGER_SIZE = (Integer.SIZE/8);