[GitHub] drill pull request #1242: DRILL-6361: Revised typeOf() function versions
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1242#discussion_r185464800 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/StringFunctionHelpers.java --- @@ -202,6 +204,127 @@ public static String toStringFromUTF16(int start, int end, DrillBuf buffer) { return new String(buf, Charsets.UTF_16); } + /** + * Convert a non-nullable VarChar input to a Java string, assuming UTF-8 encoding. + * + * @param input the VarChar holder for the input parameter + * @return Java String that holds the value + */ + + public static String varCharToString(VarCharHolder input) { +return org.apache.drill.exec.expr.fn.impl.StringFunctionHelpers.getStringFromVarCharHolder(input); + } + + /** + * Convert a nullable VarChar input to a Java string, assuming UTF-8 encoding. + * + * @param input the VarChar holder for the input parameter + * @return Java String that holds the value, or null if the input + * is null + */ + + public static String varCharToString(NullableVarCharHolder input) { +if (input.isSet == 0) { + return null; +} +return org.apache.drill.exec.expr.fn.impl.StringFunctionHelpers.getStringFromVarCharHolder(input); + } + + /** + * Convert a non-nullable VarBinary to a Java string, assuming that the + * VarBinary is actually a UTF-8 encoded string, as is often the case + * with HBase. + * + * @param input the input VarBinary parameter + * @return a Java string + */ + + public static String varBinaryToString(VarBinaryHolder input) { +int len = input.end - input.start; +byte buf[] = new byte[len]; +input.buffer.getBytes(input.start, buf); +return new String(buf, Charsets.UTF_8); + } + + /** + * Convert a nullable VarBinary to a Java string, assuming that the + * VarBinary is actually a UTF-8 encoded string, as is often the case + * with HBase. + * + * @param input the input VarBinary parameter + * @return a Java string, or null if the parameter was NULL + */ + + public static String varBinaryToString(NullableVarBinaryHolder input) { +if (input.isSet == 0) { + return null; +} +int len = input.end - input.start; +byte buf[] = new byte[len]; +input.buffer.getBytes(input.start, buf); +return new String(buf, Charsets.UTF_8); + } + + /** + * Convert a Java string to a Drill non-nullable output. Encodes the + * Java string into a set of UTF-8 bytes. Resizes the working + * buffer if needed. For this reason, you must assign the result of + * this function to your @Inject DrillBuf. That is: + * + * {@literal @}Output VarCharHolder output; + * {@literal @}Inject DrillBuf outputBuf; + * ... + * String result = ... + * outputBuf = varCharOutput(result, outputBuf, output); + * + * + * @param result the (non-null) string value to return + * @param outputBuf the output buffer identified by the + * {@literal @}Inject annotation + * @param output the non-nullable VarChar holder for the function output + * identified by the {@literal @}Output annotation + * @return the (possibly new) output buffer + */ + + public static DrillBuf varCharOutput(String result, DrillBuf outputBuf, VarCharHolder output) { +byte outBytes[] = result.toString().getBytes(com.google.common.base.Charsets.UTF_8); --- End diff -- Please remove `toString()` call: `result.toString().getBytes` -> `result.getBytes` ---
[GitHub] drill pull request #1242: DRILL-6361: Revised typeOf() function versions
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1242#discussion_r185458392 --- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/expr/fn/impl/TestTypeFns.java --- @@ -0,0 +1,196 @@ +/* + * 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.expr.fn.impl; + +import static org.junit.Assert.assertEquals; + +import org.apache.drill.exec.rpc.RpcException; +import org.apache.drill.test.ClusterFixture; +import org.apache.drill.test.ClusterFixtureBuilder; +import org.apache.drill.test.ClusterTest; +import org.junit.BeforeClass; +import org.junit.Test; + +public class TestTypeFns extends ClusterTest { + + @BeforeClass + public static void setup() throws Exception { +// Use the following three lines if you add a function +// to avoid the need for a full Drill build. +ClusterFixtureBuilder builder = ClusterFixture.builder(dirTestWatcher) +.configProperty("drill.classpath.scanning.cache.enabled", false); +startCluster(builder); + +// Use the following line if a full Drill build has been +// done since adding new functions. +// startCluster(ClusterFixture.builder(dirTestWatcher).maxParallelization(1)); + } + + @Test + public void testTypeOf() throws RpcException { +// SMALLINT not supported in CAST +//doTypeOfTest("SMALLINT"); +doTypeOfTest("INT"); +doTypeOfTest("BIGINT"); +doTypeOfTest("VARCHAR"); +doTypeOfTest("FLOAT", "FLOAT4"); +doTypeOfTest("DOUBLE", "FLOAT8"); +doTypeOfTestSpecial("a", "true", "BIT"); +doTypeOfTestSpecial("a", "CURRENT_DATE", "DATE"); +doTypeOfTestSpecial("a", "CURRENT_TIME", "TIME"); +doTypeOfTestSpecial("a", "CURRENT_TIMESTAMP", "TIMESTAMP"); +doTypeOfTestSpecial("a", "AGE(CURRENT_TIMESTAMP)", "INTERVAL"); +doTypeOfTestSpecial("BINARY_STRING(a)", "'\\xde\\xad\\xbe\\xef'", "VARBINARY"); +try { + client.alterSession("planner.enable_decimal_data_type", true); --- End diff -- Please replace `planner.enable_decimal_data_type` with `PlannerSettings.ENABLE_DECIMAL_DATA_TYPE_KEY` ---
[GitHub] drill pull request #1242: DRILL-6361: Revised typeOf() function versions
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1242#discussion_r185435469 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/UnionFunctions.java --- @@ -146,16 +139,131 @@ private static int getTypeValue(MinorType type) { @Inject DrillBuf buf; +@Override public void setup() {} +@Override public void eval() { - byte[] type; + String typeName; if (input.isSet()) { - type = input.getType().getMinorType().name().getBytes(); +typeName = input.getType().getMinorType().name(); } else { -type = org.apache.drill.common.types.TypeProtos.MinorType.NULL.name().getBytes(); +typeName = org.apache.drill.common.types.TypeProtos.MinorType.NULL.name(); } + byte[] type = typeName.getBytes(); + buf = buf.reallocIfNeeded(type.length); + buf.setBytes(0, type); + out.buffer = buf; + out.start = 0; + out.end = type.length; +} + } + + @FunctionTemplate(name = "sqlTypeOf", + scope = FunctionTemplate.FunctionScope.SIMPLE, + nulls = NullHandling.INTERNAL) + public static class GetSqlType implements DrillSimpleFunc { + +@Param +FieldReader input; +@Output +VarCharHolder out; +@Inject +DrillBuf buf; + +@Override +public void setup() {} + +@Override +public void eval() { + + org.apache.drill.common.types.TypeProtos.MajorType type = input.getType(); + + // Note: extendType is a static function because the byte code magic + // for UDFS can't handle switch statements. + + String typeName = + org.apache.drill.exec.expr.fn.impl.UnionFunctions.extendType( + type, + org.apache.drill.common.types.Types.getBaseSqlTypeName(type)); + + org.apache.drill.exec.expr.fn.impl.StringFunctionHelpers.varCharOutput( --- End diff -- I think passing a holder into the method may prevent scalar replacement for this holder. ---
[GitHub] drill issue #1224: DRILL-6321: Customize Drill's conformance. Allow support ...
Github user vvysotskyi commented on the issue: https://github.com/apache/drill/pull/1224 @parthchandra, thanks for the explanation. @chunhui-shi, thanks for making changes, +1 ---
[GitHub] drill issue #1224: DRILL-6321: Customize Drill's conformance. Allow support ...
Github user vvysotskyi commented on the issue: https://github.com/apache/drill/pull/1224 As I understand from DRILL-1921, cross join was prevented due to the `CannotPlanException` exception at the planning stage. Can we get the same problem using `APPLY`? If yes, should be discussed the possibility of adding some limitations for `APPLY`, for example, deny usage for the case when a filter is absent in the query etc. ---
[GitHub] drill pull request #1230: DRILL-6345: DRILL Query fails on Function LOG10
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1230#discussion_r184144882 --- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/TestNewMathFunctions.java --- @@ -132,4 +148,200 @@ public void testIsNumeric() throws Throwable{ final Object [] expected = new Object[] {1, 1, 1, 0}; runTest(expected, "functions/testIsNumericFunction.json"); } + + @Test + public void testLog10WithDouble() throws Throwable { +String json = "{" + + "\"positive_infinity\" : Infinity," + + "\"negative_infinity\" : -Infinity," + + "\"nan\" : NaN," + + "\"num1\": 0.0," + + "\"num2\": 0.1," + + "\"num3\": 1.0," + + "\"num4\": 1.5," + + "\"num5\": -1.5," + + "\"num6\": 10.0" + + "}"; +String query = "select " + +"log10(positive_infinity) as pos_inf, " + +"log10(negative_infinity) as neg_inf, " + +"log10(nan) as nan, " + +"log10(num1) as num1, " + +"log10(num2) as num2, " + +"log10(num3) as num3, " + +"log10(num4) as num4, " + +"log10(num5) as num5, " + +"log10(num6) as num6 " + +"" + +"from dfs.`data.json`"; +File file = new File(dirTestWatcher.getRootDir(), "data.json"); +try { + FileUtils.writeStringToFile(file, json); + test("alter session set `%s` = true", ExecConstants.JSON_READ_NUMBERS_AS_DOUBLE); + test("alter session set `%s` = true", ExecConstants.JSON_READER_NAN_INF_NUMBERS); + testBuilder() + .sqlQuery(query) + .ordered() + .baselineColumns("pos_inf", "neg_inf", "nan", "num1", "num2", "num3", "num4", "num5", "num6") + .baselineValues(Double.POSITIVE_INFINITY, Double.NaN, Double.NaN, Double.NEGATIVE_INFINITY, + -1.0d, 0d, 0.17609125905568124d, Double.NaN, 1.0d) + .build() + .run(); +} finally { + test("alter session set `%s` = false", ExecConstants.JSON_READ_NUMBERS_AS_DOUBLE); + test("alter session set `%s` = false", ExecConstants.JSON_READER_NAN_INF_NUMBERS); + FileUtils.deleteQuietly(file); +} + } + + @Test + public void testLog10WithFloat() throws Throwable { +String json = "{" + +"\"positive_infinity\" : Infinity," + +"\"negative_infinity\" : -Infinity," + +"\"nan\" : NaN," + +"\"num1\": 0.0," + +"\"num2\": 0.1," + +"\"num3\": 1.0," + +"\"num4\": 1.5," + +"\"num5\": -1.5," + +"\"num6\": 10.0" + +"}"; +String query = "select " + +"log10(cast(positive_infinity as float)) as pos_inf, " + +"log10(cast(negative_infinity as float)) as neg_inf, " + +"log10(cast(nan as float)) as nan, " + +"log10(cast(num1 as float)) as num1, " + +"log10(cast(num2 as float)) as num2, " + +"log10(cast(num3 as float)) as num3, " + +"log10(cast(num4 as float)) as num4, " + +"log10(cast(num5 as float)) as num5, " + +"log10(cast(num6 as float)) as num6 " + +"" + +"from dfs.`data.json`"; +File file = new File(dirTestWatcher.getRootDir(), "data.json"); +try { + FileUtils.writeStringToFile(file, json); + test("alter session set `%s` = true", ExecConstants.JSON_READER_NAN_INF_NUMBERS); + testBuilder() + .sqlQuery(query) + .ordered() + .baselineColumns("pos_inf", "neg_inf", "nan", "num1", "num2", "num3", "num4", "num5", "num6") +
[GitHub] drill pull request #1230: DRILL-6345: DRILL Query fails on Function LOG10
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1230#discussion_r184106411 --- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/TestNewMathFunctions.java --- @@ -132,4 +148,200 @@ public void testIsNumeric() throws Throwable{ final Object [] expected = new Object[] {1, 1, 1, 0}; runTest(expected, "functions/testIsNumericFunction.json"); } + + @Test + public void testLog10WithDouble() throws Throwable { +String json = "{" + + "\"positive_infinity\" : Infinity," + + "\"negative_infinity\" : -Infinity," + + "\"nan\" : NaN," + + "\"num1\": 0.0," + + "\"num2\": 0.1," + + "\"num3\": 1.0," + + "\"num4\": 1.5," + + "\"num5\": -1.5," + + "\"num6\": 10.0" + + "}"; +String query = "select " + +"log10(positive_infinity) as pos_inf, " + +"log10(negative_infinity) as neg_inf, " + +"log10(nan) as nan, " + +"log10(num1) as num1, " + +"log10(num2) as num2, " + +"log10(num3) as num3, " + +"log10(num4) as num4, " + +"log10(num5) as num5, " + +"log10(num6) as num6 " + +"" + --- End diff -- Please remove empty string here and in other places. ---
[GitHub] drill pull request #1230: DRILL-6345: DRILL Query fails on Function LOG10
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1230#discussion_r184110535 --- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/TestNewMathFunctions.java --- @@ -132,4 +148,200 @@ public void testIsNumeric() throws Throwable{ final Object [] expected = new Object[] {1, 1, 1, 0}; runTest(expected, "functions/testIsNumericFunction.json"); } + + @Test + public void testLog10WithDouble() throws Throwable { +String json = "{" + + "\"positive_infinity\" : Infinity," + + "\"negative_infinity\" : -Infinity," + + "\"nan\" : NaN," + + "\"num1\": 0.0," + + "\"num2\": 0.1," + + "\"num3\": 1.0," + + "\"num4\": 1.5," + + "\"num5\": -1.5," + + "\"num6\": 10.0" + + "}"; +String query = "select " + +"log10(positive_infinity) as pos_inf, " + +"log10(negative_infinity) as neg_inf, " + +"log10(nan) as nan, " + +"log10(num1) as num1, " + +"log10(num2) as num2, " + +"log10(num3) as num3, " + +"log10(num4) as num4, " + +"log10(num5) as num5, " + +"log10(num6) as num6 " + +"" + +"from dfs.`data.json`"; +File file = new File(dirTestWatcher.getRootDir(), "data.json"); +try { + FileUtils.writeStringToFile(file, json); + test("alter session set `%s` = true", ExecConstants.JSON_READ_NUMBERS_AS_DOUBLE); + test("alter session set `%s` = true", ExecConstants.JSON_READER_NAN_INF_NUMBERS); + testBuilder() + .sqlQuery(query) + .ordered() + .baselineColumns("pos_inf", "neg_inf", "nan", "num1", "num2", "num3", "num4", "num5", "num6") + .baselineValues(Double.POSITIVE_INFINITY, Double.NaN, Double.NaN, Double.NEGATIVE_INFINITY, + -1.0d, 0d, 0.17609125905568124d, Double.NaN, 1.0d) + .build() + .run(); +} finally { + test("alter session set `%s` = false", ExecConstants.JSON_READ_NUMBERS_AS_DOUBLE); --- End diff -- For the case when default value of the option is changed, disabling it after the tests may cause problems for other tests. Correct behaviour is to reset the value of the option. May be used method `resetSessionOption` ---
[GitHub] drill pull request #1230: DRILL-6345: DRILL Query fails on Function LOG10
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1230#discussion_r184138785 --- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/TestNewMathFunctions.java --- @@ -132,4 +148,200 @@ public void testIsNumeric() throws Throwable{ final Object [] expected = new Object[] {1, 1, 1, 0}; runTest(expected, "functions/testIsNumericFunction.json"); } + + @Test + public void testLog10WithDouble() throws Throwable { +String json = "{" + + "\"positive_infinity\" : Infinity," + + "\"negative_infinity\" : -Infinity," + + "\"nan\" : NaN," + + "\"num1\": 0.0," + + "\"num2\": 0.1," + + "\"num3\": 1.0," + + "\"num4\": 1.5," + + "\"num5\": -1.5," + + "\"num6\": 10.0" + + "}"; +String query = "select " + +"log10(positive_infinity) as pos_inf, " + +"log10(negative_infinity) as neg_inf, " + +"log10(nan) as nan, " + +"log10(num1) as num1, " + +"log10(num2) as num2, " + +"log10(num3) as num3, " + +"log10(num4) as num4, " + +"log10(num5) as num5, " + +"log10(num6) as num6 " + +"" + +"from dfs.`data.json`"; +File file = new File(dirTestWatcher.getRootDir(), "data.json"); +try { + FileUtils.writeStringToFile(file, json); + test("alter session set `%s` = true", ExecConstants.JSON_READ_NUMBERS_AS_DOUBLE); + test("alter session set `%s` = true", ExecConstants.JSON_READER_NAN_INF_NUMBERS); + testBuilder() + .sqlQuery(query) + .ordered() + .baselineColumns("pos_inf", "neg_inf", "nan", "num1", "num2", "num3", "num4", "num5", "num6") + .baselineValues(Double.POSITIVE_INFINITY, Double.NaN, Double.NaN, Double.NEGATIVE_INFINITY, + -1.0d, 0d, 0.17609125905568124d, Double.NaN, 1.0d) + .build() + .run(); +} finally { + test("alter session set `%s` = false", ExecConstants.JSON_READ_NUMBERS_AS_DOUBLE); + test("alter session set `%s` = false", ExecConstants.JSON_READER_NAN_INF_NUMBERS); + FileUtils.deleteQuietly(file); +} + } + + @Test + public void testLog10WithFloat() throws Throwable { +String json = "{" + +"\"positive_infinity\" : Infinity," + +"\"negative_infinity\" : -Infinity," + +"\"nan\" : NaN," + +"\"num1\": 0.0," + +"\"num2\": 0.1," + +"\"num3\": 1.0," + +"\"num4\": 1.5," + +"\"num5\": -1.5," + +"\"num6\": 10.0" + +"}"; +String query = "select " + +"log10(cast(positive_infinity as float)) as pos_inf, " + +"log10(cast(negative_infinity as float)) as neg_inf, " + +"log10(cast(nan as float)) as nan, " + +"log10(cast(num1 as float)) as num1, " + +"log10(cast(num2 as float)) as num2, " + +"log10(cast(num3 as float)) as num3, " + +"log10(cast(num4 as float)) as num4, " + +"log10(cast(num5 as float)) as num5, " + +"log10(cast(num6 as float)) as num6 " + +"" + +"from dfs.`data.json`"; +File file = new File(dirTestWatcher.getRootDir(), "data.json"); +try { + FileUtils.writeStringToFile(file, json); + test("alter session set `%s` = true", ExecConstants.JSON_READER_NAN_INF_NUMBERS); + testBuilder() + .sqlQuery(query) + .ordered() + .baselineColumns("pos_inf", "neg_inf", "nan", "num1", "num2", "num3", "num4", "num5", "num6") +
[GitHub] drill pull request #1230: DRILL-6345: DRILL Query fails on Function LOG10
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1230#discussion_r184145427 --- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/TestNewMathFunctions.java --- @@ -20,12 +20,20 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; +import java.io.File; import java.math.BigDecimal; +import java.nio.file.Files; +import java.util.DoubleSummaryStatistics; +import java.util.List; +import com.google.common.collect.Lists; +import org.apache.commons.io.FileUtils; import org.apache.drill.categories.OperatorTest; import org.apache.drill.categories.UnlikelyTest; import org.apache.drill.common.config.DrillConfig; +import org.apache.drill.exec.ExecConstants; import org.apache.drill.exec.ExecTest; +import org.apache.drill.exec.compile.ExampleTemplateWithInner; --- End diff -- Is this import required? ---
[GitHub] drill pull request #1230: DRILL-6345: DRILL Query fails on Function LOG10
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1230#discussion_r184107121 --- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/TestNewMathFunctions.java --- @@ -132,4 +148,200 @@ public void testIsNumeric() throws Throwable{ final Object [] expected = new Object[] {1, 1, 1, 0}; runTest(expected, "functions/testIsNumericFunction.json"); } + + @Test + public void testLog10WithDouble() throws Throwable { +String json = "{" + + "\"positive_infinity\" : Infinity," + + "\"negative_infinity\" : -Infinity," + + "\"nan\" : NaN," + + "\"num1\": 0.0," + + "\"num2\": 0.1," + + "\"num3\": 1.0," + + "\"num4\": 1.5," + + "\"num5\": -1.5," + + "\"num6\": 10.0" + + "}"; +String query = "select " + +"log10(positive_infinity) as pos_inf, " + +"log10(negative_infinity) as neg_inf, " + +"log10(nan) as nan, " + +"log10(num1) as num1, " + +"log10(num2) as num2, " + +"log10(num3) as num3, " + +"log10(num4) as num4, " + +"log10(num5) as num5, " + +"log10(num6) as num6 " + +"" + +"from dfs.`data.json`"; +File file = new File(dirTestWatcher.getRootDir(), "data.json"); +try { + FileUtils.writeStringToFile(file, json); + test("alter session set `%s` = true", ExecConstants.JSON_READ_NUMBERS_AS_DOUBLE); --- End diff -- Please consider `setSessionOption`. ---
[GitHub] drill pull request #1230: DRILL-6345: DRILL Query fails on Function LOG10
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1230#discussion_r184138601 --- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/TestNewMathFunctions.java --- @@ -132,4 +148,200 @@ public void testIsNumeric() throws Throwable{ final Object [] expected = new Object[] {1, 1, 1, 0}; runTest(expected, "functions/testIsNumericFunction.json"); } + + @Test + public void testLog10WithDouble() throws Throwable { +String json = "{" + + "\"positive_infinity\" : Infinity," + + "\"negative_infinity\" : -Infinity," + + "\"nan\" : NaN," + + "\"num1\": 0.0," + + "\"num2\": 0.1," + + "\"num3\": 1.0," + + "\"num4\": 1.5," + + "\"num5\": -1.5," + + "\"num6\": 10.0" + + "}"; +String query = "select " + +"log10(positive_infinity) as pos_inf, " + +"log10(negative_infinity) as neg_inf, " + +"log10(nan) as nan, " + +"log10(num1) as num1, " + +"log10(num2) as num2, " + +"log10(num3) as num3, " + +"log10(num4) as num4, " + +"log10(num5) as num5, " + +"log10(num6) as num6 " + +"" + +"from dfs.`data.json`"; +File file = new File(dirTestWatcher.getRootDir(), "data.json"); +try { + FileUtils.writeStringToFile(file, json); + test("alter session set `%s` = true", ExecConstants.JSON_READ_NUMBERS_AS_DOUBLE); + test("alter session set `%s` = true", ExecConstants.JSON_READER_NAN_INF_NUMBERS); + testBuilder() + .sqlQuery(query) + .ordered() + .baselineColumns("pos_inf", "neg_inf", "nan", "num1", "num2", "num3", "num4", "num5", "num6") + .baselineValues(Double.POSITIVE_INFINITY, Double.NaN, Double.NaN, Double.NEGATIVE_INFINITY, + -1.0d, 0d, 0.17609125905568124d, Double.NaN, 1.0d) + .build() + .run(); +} finally { + test("alter session set `%s` = false", ExecConstants.JSON_READ_NUMBERS_AS_DOUBLE); + test("alter session set `%s` = false", ExecConstants.JSON_READER_NAN_INF_NUMBERS); + FileUtils.deleteQuietly(file); +} + } + + @Test + public void testLog10WithFloat() throws Throwable { +String json = "{" + +"\"positive_infinity\" : Infinity," + +"\"negative_infinity\" : -Infinity," + +"\"nan\" : NaN," + +"\"num1\": 0.0," + +"\"num2\": 0.1," + +"\"num3\": 1.0," + +"\"num4\": 1.5," + +"\"num5\": -1.5," + +"\"num6\": 10.0" + +"}"; +String query = "select " + +"log10(cast(positive_infinity as float)) as pos_inf, " + +"log10(cast(negative_infinity as float)) as neg_inf, " + +"log10(cast(nan as float)) as nan, " + +"log10(cast(num1 as float)) as num1, " + +"log10(cast(num2 as float)) as num2, " + +"log10(cast(num3 as float)) as num3, " + +"log10(cast(num4 as float)) as num4, " + +"log10(cast(num5 as float)) as num5, " + +"log10(cast(num6 as float)) as num6 " + +"" + +"from dfs.`data.json`"; +File file = new File(dirTestWatcher.getRootDir(), "data.json"); +try { + FileUtils.writeStringToFile(file, json); + test("alter session set `%s` = true", ExecConstants.JSON_READER_NAN_INF_NUMBERS); + testBuilder() + .sqlQuery(query) + .ordered() + .baselineColumns("pos_inf", "neg_inf", "nan", "num1", "num2", "num3", "num4", "num5", "num6") + .baselineValues(Double.POSITIVE_INFINITY, Double.NaN, Double.NaN, Double.NEGATIVE_INFINITY, + -0.3528508d, 0d, 0.17609125905568124d, Double.NaN, 1.0d) + .build() --- End diff -- `build().run()` -> `go()`  ---
[GitHub] drill pull request #1232: DRILL-6094: Decimal data type enhancements
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1232#discussion_r184062425 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/columnreaders/ParquetFixedWidthDictionaryReaders.java --- @@ -248,27 +227,61 @@ protected void readField(long recordsToReadInThisPass) { } } - static class DictionaryDecimal18Reader extends FixedByteAlignedReader { -DictionaryDecimal18Reader(ParquetRecordReader parentReader, int allocateSize, ColumnDescriptor descriptor, - ColumnChunkMetaData columnChunkMetaData, boolean fixedLength, Decimal18Vector v, - SchemaElement schemaElement) throws ExecutionSetupException { + static class DictionaryVarDecimalReader extends FixedByteAlignedReader { + +DictionaryVarDecimalReader(ParquetRecordReader parentReader, int allocateSize, ColumnDescriptor descriptor, +ColumnChunkMetaData columnChunkMetaData, boolean fixedLength, VarDecimalVector v, +SchemaElement schemaElement) throws ExecutionSetupException { super(parentReader, allocateSize, descriptor, columnChunkMetaData, fixedLength, v, schemaElement); } // this method is called by its superclass during a read loop @Override protected void readField(long recordsToReadInThisPass) { + recordsReadInThisIteration = + Math.min(pageReader.currentPageCount - pageReader.valuesRead, + recordsToReadInThisPass - valuesReadInCurrentPass); + + switch (columnDescriptor.getType()) { +case INT32: + if (usingDictionary) { +for (int i = 0; i < recordsReadInThisIteration; i++) { + byte[] bytes = Ints.toByteArray(pageReader.dictionaryValueReader.readInteger()); + setValueBytes(i, bytes); +} +setWriteIndex(); + } else { +super.readField(recordsToReadInThisPass); + } + break; +case INT64: + if (usingDictionary) { +for (int i = 0; i < recordsReadInThisIteration; i++) { + byte[] bytes = Longs.toByteArray(pageReader.dictionaryValueReader.readLong()); + setValueBytes(i, bytes); +} +setWriteIndex(); + } else { +super.readField(recordsToReadInThisPass); + } + break; + } +} - recordsReadInThisIteration = Math.min(pageReader.currentPageCount -- pageReader.valuesRead, recordsToReadInThisPass - valuesReadInCurrentPass); +/** + * Set the write Index. The next page that gets read might be a page that does not use dictionary encoding + * and we will go into the else condition below. The readField method of the parent class requires the + * writer index to be set correctly. + */ +private void setWriteIndex() { + readLengthInBits = recordsReadInThisIteration * dataTypeLengthInBits; + readLength = (int) Math.ceil(readLengthInBits / 8.0); --- End diff -- This is the number of bits in a byte, but a double value is used to avoid integer division. Thanks for pointing this, replaced by constant. ---
[GitHub] drill pull request #1232: DRILL-6094: Decimal data type enhancements
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1232#discussion_r184009513 --- Diff: exec/vector/src/main/codegen/templates/ValueHolders.java --- @@ -32,99 +32,81 @@ /* * This class is generated using freemarker and the ${.template_name} template. */ -public final class ${className} implements ValueHolder{ +public final class ${className} implements ValueHolder { public static final MajorType TYPE = Types.${mode.name?lower_case}(MinorType.${minor.class?upper_case}); -<#if mode.name == "Repeated"> + <#if mode.name == "Repeated"> -/** The first index (inclusive) into the Vector. **/ -public int start; + /** The first index (inclusive) into the Vector. **/ + public int start; -/** The last index (exclusive) into the Vector. **/ -public int end; + /** The last index (exclusive) into the Vector. **/ + public int end; -/** The Vector holding the actual values. **/ -public ${minor.class}Vector vector; + /** The Vector holding the actual values. **/ + public ${minor.class}Vector vector; -<#else> -public static final int WIDTH = ${type.width}; + <#else> + public static final int WIDTH = ${type.width}; -<#if mode.name == "Optional">public int isSet; -<#assign fields = minor.fields!type.fields /> -<#list fields as field> -public ${field.type} ${field.name}; - + <#if mode.name == "Optional">public int isSet; + <#assign fields = minor.fields!type.fields /> + <#list fields as field> + public ${field.type} ${field.name}; + -<#if minor.class.startsWith("Decimal")> -public static final int maxPrecision = ${minor.maxPrecisionDigits}; -<#if minor.class.startsWith("Decimal28") || minor.class.startsWith("Decimal38")> -public static final int nDecimalDigits = ${minor.nDecimalDigits}; + <#if minor.class.startsWith("Decimal")> + public static final int maxPrecision = ${minor.maxPrecisionDigits}; + <#if minor.class.startsWith("Decimal28") || minor.class.startsWith("Decimal38")> --- End diff -- Thanks, good idea, marked as deprecated and added a comment to use VarDecimalHolder instead. ---
[GitHub] drill pull request #1232: DRILL-6094: Decimal data type enhancements
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1232#discussion_r184004929 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScanBatch.java --- @@ -409,7 +409,7 @@ public void clear() { // Check if the field exists. ValueVector v = fieldVectorMap.get(field.getName()); - if (v == null || v.getClass() != clazz) { + if (v == null || !v.getField().getType().equals(field.getType())) { --- End diff -- Thanks, added a comment with the explanation. ---
[GitHub] drill pull request #1232: DRILL-6094: Decimal data type enhancements
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1232#discussion_r184035111 --- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/TestVarDecimalFunctions.java --- @@ -0,0 +1,911 @@ +/* + * 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.fn.impl; + +import org.apache.drill.categories.SqlFunctionTest; +import org.apache.drill.exec.planner.physical.PlannerSettings; +import org.apache.drill.test.BaseTestQuery; +import org.junit.Test; +import org.junit.experimental.categories.Category; + +import java.math.BigDecimal; +import java.math.MathContext; +import java.math.RoundingMode; + +@Category(SqlFunctionTest.class) +public class TestVarDecimalFunctions extends BaseTestQuery { + + // Tests for math functions + + @Test + public void testDecimalAdd() throws Exception { +String query = +"select\n" + +// checks trimming of scale +"cast('999.92345678912' as DECIMAL(38, 11))\n" + +"+ cast('0.32345678912345678912345678912345678912' as DECIMAL(38, 38)) as s1,\n" + +// sanitary checks +"cast('1234567891234567891234567891234567.89' as DECIMAL(36, 2))\n" + +"+ cast('123456789123456789123456789123456.789' as DECIMAL(36, 3)) as s2,\n" + +"cast('1234567891234567891234567891234567.89' as DECIMAL(36, 2))\n" + +"+ cast('0' as DECIMAL(36, 3)) as s3,\n" + +"cast('15.02' as DECIMAL(4, 2)) - cast('12.93' as DECIMAL(4, 2)) as s4,\n" + +"cast('11.02' as DECIMAL(4, 2)) - cast('12.93' as DECIMAL(4, 2)) as s5,\n" + +"cast('0' as DECIMAL(36, 2)) - cast('12.93' as DECIMAL(36, 2)) as s6,\n" + +// check trimming (negative scale) +"cast('2345678912' as DECIMAL(38, 0))\n" + +"+ cast('32345678912345678912345678912345678912' as DECIMAL(38, 0)) as s7"; +try { + alterSession(PlannerSettings.ENABLE_DECIMAL_DATA_TYPE_KEY, true); + testBuilder() +.sqlQuery(query) +.ordered() +.baselineColumns("s1", "s2", "s3", "s4", "s5", "s6", "s7") +.baselineValues( +new BigDecimal("999.92345678912") +.add(new BigDecimal("0.32345678912345678912345678912345678912")) +.round(new MathContext(38, RoundingMode.HALF_UP)), +new BigDecimal("1358024680358024680358024680358024.679"), +new BigDecimal("1234567891234567891234567891234567.890"), +new BigDecimal("2.09"), new BigDecimal("-1.91"), new BigDecimal("-12.93"), +new BigDecimal("1.3234567891234567891234567890469135782E+38")) +.build() --- End diff -- Thanks, replaced. ---
[GitHub] drill pull request #1232: DRILL-6094: Decimal data type enhancements
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1232#discussion_r184099659 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/SqlConverter.java --- @@ -559,6 +560,19 @@ public RexNode makeCast(RelDataType type, RexNode exp, boolean matchNullability) if (matchNullability) { return makeAbstractCast(type, exp); } + // for the case when BigDecimal literal has a scale or precision + // that differs from the value from specified RelDataType, cast cannot be removed + // TODO: remove this code when CALCITE-1468 is fixed + if (type.getSqlTypeName() == SqlTypeName.DECIMAL && exp instanceof RexLiteral) { --- End diff -- Created DRILL-6355. ---
[GitHub] drill pull request #1232: DRILL-6094: Decimal data type enhancements
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1232#discussion_r184061116 --- Diff: exec/java-exec/src/test/java/org/apache/drill/PlanningBase.java --- @@ -20,12 +20,17 @@ import java.io.IOException; import java.net.URL; +import com.google.common.base.Function; --- End diff -- I tested it manually. I considered 2 options: 1. UDF has an input old decimal type. In this case, function resolver adds cast from vardecimal to old decimal type which is used in UDF. 2. UDF returns the old decimal type. In this case, Drill casts the result of UDF to vardecimal. ---
[GitHub] drill pull request #1232: DRILL-6094: Decimal data type enhancements
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1232#discussion_r184012035 --- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/join/TestNestedLoopJoin.java --- @@ -409,4 +409,30 @@ public void testNLJoinCorrectnessRightMultipleBatches() throws Exception { setSessionOption(ExecConstants.SLICE_TARGET, 10); } } + + @Test + public void testNlJoinWithStringsInCondition() throws Exception { +try { + test(DISABLE_NLJ_SCALAR); + test(DISABLE_JOIN_OPTIMIZATION); + + final String query = --- End diff -- Before my changes similar query but with decimal literal in condition passed because it was handled as double. But since with my changes decimal literal is handled as the decimal, it is discovered a bug in nested loop join which is observed for the types that use drillbuf to shore their value. ---
[GitHub] drill pull request #1232: DRILL-6094: Decimal data type enhancements
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1232#discussion_r184008988 --- Diff: exec/vector/src/main/codegen/templates/NullReader.java --- @@ -31,19 +31,19 @@ * This class is generated using freemarker and the ${.template_name} template. */ @SuppressWarnings("unused") -public class NullReader extends AbstractBaseReader implements FieldReader{ +public class NullReader extends AbstractBaseReader implements FieldReader { public static final NullReader INSTANCE = new NullReader(); public static final NullReader EMPTY_LIST_INSTANCE = new NullReader(Types.repeated(TypeProtos.MinorType.NULL)); public static final NullReader EMPTY_MAP_INSTANCE = new NullReader(Types.required(TypeProtos.MinorType.MAP)); private MajorType type; - private NullReader(){ + private NullReader() { --- End diff -- Thanks, removed. ---
[GitHub] drill pull request #1232: DRILL-6094: Decimal data type enhancements
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1232#discussion_r184065631 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/FrameSupportTemplate.java --- @@ -300,7 +300,7 @@ public void cleanup() { * @param index of row to aggregate */ public abstract void evaluatePeer(@Named("index") int index); - public abstract void setupEvaluatePeer(@Named("incoming") VectorAccessible incoming, @Named("outgoing") VectorAccessible outgoing) throws SchemaChangeException; + public abstract void setupEvaluatePeer(@Named("incoming") WindowDataBatch incoming, @Named("outgoing") VectorAccessible outgoing) throws SchemaChangeException; --- End diff -- On the earlier stage of making changes, compilation error has appeared for runtime-generated code without this change, but for now, I don't see it without this change, so I reverted it. ---
[GitHub] drill pull request #1232: DRILL-6094: Decimal data type enhancements
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1232#discussion_r184008307 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetRecordWriter.java --- @@ -228,14 +232,33 @@ private void newSchema() throws IOException { setUp(schema, consumer); } - private PrimitiveType getPrimitiveType(MaterializedField field) { + protected PrimitiveType getPrimitiveType(MaterializedField field) { MinorType minorType = field.getType().getMinorType(); String name = field.getName(); +int length = ParquetTypeHelper.getLengthForMinorType(minorType); PrimitiveTypeName primitiveTypeName = ParquetTypeHelper.getPrimitiveTypeNameForMinorType(minorType); +if (DecimalUtility.isDecimalType(minorType)) { + OptionManager optionManager = oContext.getFragmentContext().getOptions(); + if (optionManager.getString(PARQUET_WRITER_LOGICAL_TYPE_FOR_DECIMALS) --- End diff -- Agree, thanks for pointing this, used `writerOptions`. ---
[GitHub] drill pull request #1232: DRILL-6094: Decimal data type enhancements
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1232#discussion_r184031484 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/stat/ParquetMetaStatCollector.java --- @@ -152,6 +152,7 @@ private ColumnStatistics getStat(Object min, Object max, Long numNull, } if (min != null && max != null ) { + // TODO: add vardecimal type --- End diff -- Thanks, removed. ---
[GitHub] drill pull request #1232: DRILL-6094: Decimal data type enhancements
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1232#discussion_r184035055 --- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/TestVarDecimalFunctions.java --- @@ -0,0 +1,911 @@ +/* + * 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.fn.impl; + +import org.apache.drill.categories.SqlFunctionTest; +import org.apache.drill.exec.planner.physical.PlannerSettings; +import org.apache.drill.test.BaseTestQuery; +import org.junit.Test; +import org.junit.experimental.categories.Category; + +import java.math.BigDecimal; +import java.math.MathContext; +import java.math.RoundingMode; + +@Category(SqlFunctionTest.class) +public class TestVarDecimalFunctions extends BaseTestQuery { + + // Tests for math functions + + @Test + public void testDecimalAdd() throws Exception { +String query = +"select\n" + +// checks trimming of scale +"cast('999.92345678912' as DECIMAL(38, 11))\n" + +"+ cast('0.32345678912345678912345678912345678912' as DECIMAL(38, 38)) as s1,\n" + +// sanitary checks +"cast('1234567891234567891234567891234567.89' as DECIMAL(36, 2))\n" + +"+ cast('123456789123456789123456789123456.789' as DECIMAL(36, 3)) as s2,\n" + +"cast('1234567891234567891234567891234567.89' as DECIMAL(36, 2))\n" + +"+ cast('0' as DECIMAL(36, 3)) as s3,\n" + +"cast('15.02' as DECIMAL(4, 2)) - cast('12.93' as DECIMAL(4, 2)) as s4,\n" + +"cast('11.02' as DECIMAL(4, 2)) - cast('12.93' as DECIMAL(4, 2)) as s5,\n" + +"cast('0' as DECIMAL(36, 2)) - cast('12.93' as DECIMAL(36, 2)) as s6,\n" + +// check trimming (negative scale) +"cast('2345678912' as DECIMAL(38, 0))\n" + +"+ cast('32345678912345678912345678912345678912' as DECIMAL(38, 0)) as s7"; +try { + alterSession(PlannerSettings.ENABLE_DECIMAL_DATA_TYPE_KEY, true); --- End diff -- Thanks, used `@BeforeClass` and `@AfterClass`. ---
[GitHub] drill pull request #1232: DRILL-6094: Decimal data type enhancements
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1232#discussion_r184027803 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillValuesRelBase.java --- @@ -169,12 +168,12 @@ private static void writeLiteral(RexLiteral literal, JsonOutput out) throws IOEx return; case DECIMAL: +// Still used double instead of decimal since the scale and precision of values are unknown --- End diff -- Thanks, reworded. ---
[GitHub] drill pull request #1232: DRILL-6094: Decimal data type enhancements
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1232#discussion_r184051298 --- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/TestFixedlenDecimal.java --- @@ -20,61 +20,74 @@ import org.apache.drill.categories.UnlikelyTest; import org.apache.drill.test.BaseTestQuery; import org.apache.drill.exec.planner.physical.PlannerSettings; -import org.junit.BeforeClass; import org.junit.Test; import org.junit.experimental.categories.Category; @Category({UnlikelyTest.class}) 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 { -testBuilder() -.sqlQuery("select count(*) as c from %s where department_id is null", DATAFILE) -.unOrdered() -.baselineColumns("c") -.baselineValues(1L) -.build() -.run(); +try { + alterSession(PlannerSettings.ENABLE_DECIMAL_DATA_TYPE_KEY, true); + testBuilder() + .sqlQuery("select count(*) as c from %s where department_id is null", DATAFILE) + .unOrdered() + .baselineColumns("c") + .baselineValues(1L) + .build() + .run(); +} finally { + resetSessionOption(PlannerSettings.ENABLE_DECIMAL_DATA_TYPE_KEY); +} } @Test public void testNotNullCount() throws Exception { -testBuilder() -.sqlQuery("select count(*) as c from %s where department_id is not null", DATAFILE) -.unOrdered() -.baselineColumns("c") -.baselineValues(106L) -.build() -.run(); +try { + alterSession(PlannerSettings.ENABLE_DECIMAL_DATA_TYPE_KEY, true); + testBuilder() + .sqlQuery("select count(*) as c from %s where department_id is not null", DATAFILE) + .unOrdered() + .baselineColumns("c") + .baselineValues(106L) + .build() --- End diff -- Thanks, replaced here and in other places. ---
[GitHub] drill pull request #1232: DRILL-6094: Decimal data type enhancements
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1232#discussion_r184002128 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/output/DecimalReturnTypeInference.java --- @@ -281,20 +295,45 @@ @Override public TypeProtos.MajorType getType(List logicalExpressions, FunctionAttributes attributes) { int scale = 0; - int precision = 0; // Get the max scale and precision from the inputs for (LogicalExpression e : logicalExpressions) { scale = Math.max(scale, e.getMajorType().getScale()); -precision = Math.max(precision, e.getMajorType().getPrecision()); } - return (TypeProtos.MajorType.newBuilder() - .setMinorType(attributes.getReturnValue().getType().getMinorType()) + return TypeProtos.MajorType.newBuilder() + .setMinorType(TypeProtos.MinorType.VARDECIMAL) .setScale(scale) - .setPrecision(38) - .setMode(TypeProtos.DataMode.REQUIRED) - .build()); + .setPrecision(DRILL_REL_DATATYPE_SYSTEM.getMaxNumericPrecision()) + .setMode(TypeProtos.DataMode.OPTIONAL) + .build(); +} + } + + /** + * Return type calculation implementation for functions with return type set as + * {@link org.apache.drill.exec.expr.annotations.FunctionTemplate.ReturnType#DECIMAL_AVG_AGGREGATE}. + */ + public static class DecimalAvgAggReturnTypeInference implements ReturnTypeInference { --- End diff -- Thanks, added. ---
[GitHub] drill pull request #1232: DRILL-6094: Decimal data type enhancements
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1232#discussion_r183998921 --- Diff: exec/java-exec/src/main/codegen/templates/Decimal/CastDecimalFloat.java --- @@ -86,24 +46,21 @@ public void eval() { */ @SuppressWarnings("unused") -@FunctionTemplate(name = "cast${type.to?upper_case}", scope = FunctionTemplate.FunctionScope.SIMPLE, nulls=NullHandling.NULL_IF_NULL) +@FunctionTemplate(name = "cast${type.to?upper_case}", + scope = FunctionTemplate.FunctionScope.SIMPLE, + nulls=NullHandling.NULL_IF_NULL) --- End diff -- Thanks, added spaces. ---
[GitHub] drill pull request #1232: DRILL-6094: Decimal data type enhancements
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1232#discussion_r184007677 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/Hash64WithSeedAsDouble.java --- @@ -265,6 +268,42 @@ public void eval() { } } + @FunctionTemplate(name = "hash64AsDouble", scope = FunctionScope.SIMPLE, nulls = FunctionTemplate.NullHandling.INTERNAL) + public static class VarDecimalHash implements DrillSimpleFunc { +@Param VarDecimalHolder in; +@Param BigIntHolder seed; +@Output BigIntHolder out; + +public void setup() { +} + +public void eval() { + java.math.BigDecimal input = org.apache.drill.exec.util.DecimalUtility.getBigDecimalFromDrillBuf(in.buffer, + in.start, in.end - in.start, in.scale); + out.value = org.apache.drill.exec.expr.fn.impl.HashHelper.hash64(input.doubleValue(), seed.value); +} + } + + @FunctionTemplate(name = "hash64AsDouble", scope = FunctionScope.SIMPLE, nulls = FunctionTemplate.NullHandling.INTERNAL) + public static class NullableVarDecimalHash implements DrillSimpleFunc { +@Param NullableVarDecimalHolder in; +@Param BigIntHolder seed; +@Output BigIntHolder out; + +public void setup() { +} + +public void eval() { + if (in.isSet == 0) { +out.value = seed.value; + } else { +java.math.BigDecimal input = org.apache.drill.exec.util.DecimalUtility.getBigDecimalFromDrillBuf(in.buffer, +in.start, in.end - in.start, in.scale); +out.value = org.apache.drill.exec.expr.fn.impl.HashHelper.hash64(input.doubleValue(), seed.value); + } +} + } --- End diff -- Thanks for pointing this, removed functions that use old decimals. ---
[GitHub] drill pull request #1232: DRILL-6094: Decimal data type enhancements
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1232#discussion_r184002812 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/expr/stat/RangeExprEvaluator.java --- @@ -219,6 +219,7 @@ private Statistics evalCastFunc(FunctionHolderExpression holderExpr, Statistics return null; // cast func between srcType and destType is NOT allowed. } + // TODO: add decimal support --- End diff -- Thanks, removed. ---
[GitHub] drill pull request #1232: DRILL-6094: Decimal data type enhancements
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1232#discussion_r184008881 --- Diff: exec/vector/src/main/codegen/templates/AbstractPromotableFieldWriter.java --- @@ -75,12 +75,19 @@ public void endList() { <#list vv.types as type><#list type.minor as minor><#assign name = minor.class?cap_first /> <#assign fields = minor.fields!type.fields /> - <#if !minor.class?starts_with("Decimal") > + <#if minor.class?contains("VarDecimal")> + @Override + public void write${minor.class}(BigDecimal value) { +getWriter(MinorType.${name?upper_case}).write${minor.class}(value); + } + + @Override public void write(${name}Holder holder) { getWriter(MinorType.${name?upper_case}).write(holder); } + <#if !minor.class?contains("Decimal") || minor.class?contains("VarDecimal")> --- End diff -- Thanks, added. ---
[GitHub] drill pull request #1232: DRILL-6094: Decimal data type enhancements
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1232#discussion_r184028590 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/TypeInferenceUtils.java --- @@ -668,46 +706,95 @@ public RelDataType inferReturnType(SqlOperatorBinding opBinding) { } private static class DrillSameSqlReturnTypeInference implements SqlReturnTypeInference { -private static final DrillSameSqlReturnTypeInference INSTANCE = new DrillSameSqlReturnTypeInference(); +private static final DrillSameSqlReturnTypeInference INSTANCE = new DrillSameSqlReturnTypeInference(true); --- End diff -- Thanks, renamed. ---
[GitHub] drill pull request #1232: DRILL-6094: Decimal data type enhancements
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1232#discussion_r183998600 --- Diff: contrib/storage-jdbc/src/main/java/org/apache/drill/exec/store/jdbc/JdbcRecordReader.java --- @@ -225,10 +247,10 @@ public int next() { int counter = 0; Boolean b = true; try { - while (counter < 4095 && b == true) { // loop at 4095 since nullables use one more than record count and we + while (counter < 4095 && b) { // loop at 4095 since nullables use one more than record count and we // allocate on powers of two. b = resultSet.next(); -if(b == false) { +if(!b) { --- End diff -- Thanks, done. ---
[GitHub] drill pull request #1232: DRILL-6094: Decimal data type enhancements
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1232#discussion_r184008595 --- Diff: exec/vector/src/main/codegen/templates/AbstractFieldReader.java --- @@ -29,9 +29,9 @@ * This class is generated using freemarker and the ${.template_name} template. */ @SuppressWarnings("unused") -abstract class AbstractFieldReader extends AbstractBaseReader implements FieldReader{ +abstract class AbstractFieldReader extends AbstractBaseReader implements FieldReader { - AbstractFieldReader(){ + AbstractFieldReader() { --- End diff -- Done. ---
[GitHub] drill pull request #1232: DRILL-6094: Decimal data type enhancements
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1232#discussion_r184010194 --- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/TestVarlenDecimal.java --- @@ -0,0 +1,153 @@ +/* + * 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.commons.io.FileUtils; +import org.apache.drill.exec.ExecConstants; +import org.apache.drill.test.BaseTestQuery; +import org.apache.drill.exec.planner.physical.PlannerSettings; +import org.hamcrest.CoreMatchers; +import org.junit.Assert; +import org.junit.Test; + +import java.math.BigDecimal; +import java.nio.file.Paths; + +public class TestVarlenDecimal extends BaseTestQuery { + + private static final String DATAFILE = "cp.`parquet/varlenDecimal.parquet`"; + + @Test + public void testNullCount() throws Exception { +String query = String.format("select count(*) as c from %s where department_id is null", DATAFILE); --- End diff -- Agree, removed `String.format` here and in other places. ---
[GitHub] drill pull request #1232: DRILL-6094: Decimal data type enhancements
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1232#discussion_r184031285 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/TypeInferenceUtils.java --- @@ -382,13 +407,26 @@ public RelDataType inferReturnType(SqlOperatorBinding opBinding) { final RelDataType operandType = opBinding.getOperandType(0); final TypeProtos.MinorType inputMinorType = getDrillTypeFromCalciteType(operandType); - if(TypeCastRules.getLeastRestrictiveType(Lists.newArrayList(inputMinorType, TypeProtos.MinorType.BIGINT)) + if (TypeCastRules.getLeastRestrictiveType(Lists.newArrayList(inputMinorType, TypeProtos.MinorType.BIGINT)) == TypeProtos.MinorType.BIGINT) { return createCalciteTypeWithNullability( factory, SqlTypeName.BIGINT, isNullable); - } else if(TypeCastRules.getLeastRestrictiveType(Lists.newArrayList(inputMinorType, TypeProtos.MinorType.FLOAT8)) + } else if (TypeCastRules.getLeastRestrictiveType(Lists.newArrayList(inputMinorType, TypeProtos.MinorType.FLOAT4)) --- End diff -- Thanks, added an explanation for sum and avg return type inferences. ---
[GitHub] drill pull request #1230: DRILL-6345: DRILL Query fails on Function LOG10
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1230#discussion_r183097515 --- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/TestNewMathFunctions.java --- @@ -116,8 +115,7 @@ public void testTrigoMathFunc() throws Throwable { @Test public void testExtendedMathFunc() throws Throwable { final BigDecimal d = new BigDecimal("1001.1"); -final Object [] expected = new Object[] {Math.cbrt(1000), Math.log(10), (Math.log(64.0)/Math.log(2.0)), Math.exp(10), Math.toDegrees(0.5), Math.toRadians(45.0), Math.PI, Math.cbrt(d.doubleValue()), Math.log(d.doubleValue()), (Math.log(d.doubleValue())/Math.log(2)), Math.exp(d.doubleValue()), Math.toDegrees(d.doubleValue()), Math.toRadians(d.doubleValue())}; - +final Object [] expected = new Object[] {Math.cbrt(1000), Math.log(10), Math.log10(5), (Math.log(64.0)/Math.log(2.0)), Math.exp(10), Math.toDegrees(0.5), Math.toRadians(45.0), Math.PI, Math.cbrt(d.doubleValue()), Math.log(d.doubleValue()), (Math.log(d.doubleValue())/Math.log(2)), Math.exp(d.doubleValue()), Math.toDegrees(d.doubleValue()), Math.toRadians(d.doubleValue())}; --- End diff -- Could you please add unit tests which check log10 with every type specified above. I suppose it will be easier to use TestBuilder for this. I realize that it is used java method, but anyway it is fine to add checks for some specific values: 1, 10 etc. ---
[GitHub] drill issue #1232: DRILL-6094: Decimal data type enhancements
Github user vvysotskyi commented on the issue: https://github.com/apache/drill/pull/1232 @arina-ielchiieva, @parthchandra could you please take a look at this? ---
[GitHub] drill pull request #1232: DRILL-6094: Decimal data type enhancements
GitHub user vvysotskyi opened a pull request: https://github.com/apache/drill/pull/1232 DRILL-6094: Decimal data type enhancements For details please see [design document](https://docs.google.com/document/d/1kfWUZ_OsEmLOJu_tKZO0-ZUJwc52drrm15fyif7BmzQ/edit?usp=sharing) You can merge this pull request into a Git repository by running: $ git pull https://github.com/vvysotskyi/drill DRILL-6094 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/drill/pull/1232.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1232 commit a276870d6bad293da0311aed746fb68a2268700b Author: Dave Oshinsky Date: 2016-02-09T22:37:47Z DRILL-4184: Support variable length decimal fields in parquet commit 84a31b92dc86873d14a7be601d189bb1cbd59b7d Author: Volodymyr Vysotskyi Date: 2018-04-05T12:35:42Z DRILL-6094: Decimal data type enhancements Add ExprVisitors for VARDECIMAL commit e93cd93211ffa84586ee0d21dab26b128d81b290 Author: Volodymyr Vysotskyi Date: 2018-04-05T14:28:39Z DRILL-6094: Modify writers/readers to support VARDECIMAL - Added usage of VarDecimal for parquet, hive, maprdb, jdbc; - Added options to store decimals as int32 and int64 or fixed_len_byte_array or binary; commit 8690897a4a2820f4e965cb562107782f1e57a99a Author: Volodymyr Vysotskyi Date: 2018-04-03T13:27:25Z DRILL-6094: Refresh protobuf C++ source files commit fbe93c46ed8a7e85a95da3ae145bfc00b7f17dd0 Author: Volodymyr Vysotskyi Date: 2018-04-05T14:44:05Z DRILL-6094: Add UDFs for VARDECIMAL data type - modify type inference rules - remove UDFs for obsolete DECIMAL types commit 746e54f71464a9dea91c3860ba0346e40c1d6ddf Author: Volodymyr Vysotskyi Date: 2018-04-05T14:48:31Z DRILL-6094: Enable DECIMAL data type by default commit 46ce3d1504618ae7e547479280f5d60b35ec1caa Author: Volodymyr Vysotskyi Date: 2018-04-05T14:51:39Z DRILL-6094: Add unit tests for DECIMAL data type commit f62545390a37425b9fbb21881e4cd12ade337e73 Author: Volodymyr Vysotskyi Date: 2018-04-05T14:58:36Z DRILL-6094: Changes in C++ files commit 7a04309605c0cd869a9eb55450aaa43c0088b425 Author: Volodymyr Vysotskyi Date: 2018-04-10T12:55:17Z DRILL-6094: Fix mapping for NLJ when literal with non-primitive type is used in join conditions ---
[GitHub] drill issue #1198: DRILL-6294: Changes to support Calcite 1.16.0
Github user vvysotskyi commented on the issue: https://github.com/apache/drill/pull/1198 @chunhui-shi, I have addressed all CR comments, could you please take a look again? ---
[GitHub] drill pull request #1198: DRILL-6294: Changes to support Calcite 1.16.0
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1198#discussion_r178404654 --- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillResultSetImpl.java --- @@ -1142,16 +1142,8 @@ public void moveToCurrentRow() throws SQLException { } @Override - public AvaticaStatement getStatement() { -try { - throwIfClosed(); -} catch (AlreadyClosedSqlException e) { - // Can't throw any SQLException because AvaticaConnection's - // getStatement() is missing "throws SQLException". - throw new RuntimeException(e.getMessage(), e); -} catch (SQLException e) { - throw new RuntimeException(e.getMessage(), e); -} + public AvaticaStatement getStatement() throws SQLException { +throwIfClosed(); --- End diff -- Thanks, it looks clearer now. ---
[GitHub] drill pull request #1198: DRILL-6294: Changes to support Calcite 1.16.0
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1198#discussion_r178371341 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillReduceAggregatesRule.java --- @@ -218,7 +218,8 @@ private void reduceAggs( RelOptUtil.createProject( newAggRel, projList, -oldAggRel.getRowType().getFieldNames()); +oldAggRel.getRowType().getFieldNames(), +DrillRelFactories.LOGICAL_BUILDER); --- End diff -- In the second commit it was fixed and used relBuilderFactory to create builder and project. ---
[GitHub] drill issue #1198: DRILL-6294: Changes to support Calcite 1.16.0
Github user vvysotskyi commented on the issue: https://github.com/apache/drill/pull/1198 @amansinha100, @chunhui-shi could you please take a look? ---
[GitHub] drill pull request #1198: DRILL-6294: Changes to support Calcite 1.16.0
GitHub user vvysotskyi opened a pull request: https://github.com/apache/drill/pull/1198 DRILL-6294: Changes to support Calcite 1.16.0 You can merge this pull request into a Git repository by running: $ git pull https://github.com/vvysotskyi/drill DRILL-6294 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/drill/pull/1198.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1198 commit a79d6586033b618c95462368520237aab84f47bf Author: Volodymyr Vysotskyi Date: 2018-02-07T14:24:50Z DRILL-6294: Changes to support Calcite 1.16.0 commit 48880eb80d60a15e4ffdfcd6c729bfc75cf5d2da Author: Volodymyr Vysotskyi Date: 2018-03-11T10:11:45Z DRILL-6294: Remove deprecated API usage ---
[GitHub] drill pull request #1194: DRILL-6300: Refresh protobuf C++ source files
GitHub user vvysotskyi opened a pull request: https://github.com/apache/drill/pull/1194 DRILL-6300: Refresh protobuf C++ source files You can merge this pull request into a Git repository by running: $ git pull https://github.com/vvysotskyi/drill DRILL-6300 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/drill/pull/1194.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1194 commit 79bd146782b2f7234789226444c714cf9981c664 Author: Volodymyr Vysotskyi Date: 2018-03-29T15:51:08Z DRILL-6300: Refresh protobuf C++ source files ---
[GitHub] drill pull request #1178: DRILL-6278: Removed temp codegen directory in test...
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1178#discussion_r176936575 --- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TopN/TopNBatchTest.java --- @@ -159,7 +157,6 @@ public void priorityQueueOrderingTest() throws Exception { @Test public void sortOneKeyAscending() throws Throwable { ClusterFixtureBuilder builder = ClusterFixture.builder(dirTestWatcher) - .configProperty(ClassBuilder.CODE_DIR_OPTION, dirTestWatcher.getDir().getAbsolutePath()) .configProperty(CodeCompiler.ENABLE_SAVE_CODE_FOR_DEBUG_TOPN, true); --- End diff -- I think this line should be also removed since `ENABLE_SAVE_CODE_FOR_DEBUG_TOPN` should be disabled for tests. ---
[GitHub] drill issue #1178: DRILL-6278: Removed temp codegen directory in testing fra...
Github user vvysotskyi commented on the issue: https://github.com/apache/drill/pull/1178 @ilooner, I have noticed that `ClassBuilder.CODE_DIR_OPTION` is used in `TopNBatchTest` class. Shoul we also remove it? ---
[GitHub] drill issue #1168: DRILL-6246: Reduced the size of the jdbc-all jar file
Github user vvysotskyi commented on the issue: https://github.com/apache/drill/pull/1168 Classes from `avatica.metrics` are used in `JsonHandler`, `ProtobufHandler` and `LocalService`. If Drill does not use these classes than I agree that we can exclude it from `jdbc-all` jar. Regarding excluding `avatica/org/**`, looks like the problem is in the Avatica pom files since there are no dependencies to `org.apache.commons` and `org.apache.http`, but they are shaded to the jar. Created Jira CALCITE-2215 to fix this issue, but for now, I think it's ok to exclude them. ---
[GitHub] drill pull request #:
Github user vvysotskyi commented on the pull request: https://github.com/apache/drill/commit/9073aed67d89e8b2188870d6c812706085c9c41b#commitcomment-27970713 In logical/src/main/java/org/apache/drill/common/expression/SchemaPath.java: In logical/src/main/java/org/apache/drill/common/expression/SchemaPath.java on line 46: @gparai, for the case, when `DYNAMIC_STAR` is passed into parseFromString() method, the result will be the same, as for the passing "*" string - error. `DYNAMIC_STAR` is used in the same places, where usual column names are used. But column names may contain some symbols which also breaks Lexer/Parser. Therefore column names are wrapped into `SchemaPath` using `getSimplePath()` method. `DYNAMIC_STAR` also wrapped into the `SchemaPath`, and its string representation may be received using `SchemaPath.STAR_COLUMN.toExpr()` -> "\`**\`". So `SchemaPath.parseFromString(SchemaPath.STAR_COLUMN.toExpr())` is able to parse it. ---
[GitHub] drill issue #1104: DRILL-6118: Handle item star columns during project / fil...
Github user vvysotskyi commented on the issue: https://github.com/apache/drill/pull/1104 @paul-rogers, as you know, there were some differences between the commits in the older Calcite version (1.4.0-r23) which was used by Drill and commits merged into Apache master. One of such commits was `[CALCITE-1150] Add dynamic record type and dynamic star for schema-on-read table`. In our older Calcite fork `DYNAMIC_STAR` was `*`, but after rebase onto Calcite 1.15, it became `**`. As I understand, `DYNAMIC_STAR` itself means "default column" which will be added to table row type for the case when we have `select *` query and the schema of the underlying table is not known. As for the name of constant, I think current `DYNAMIC_STAR` more suitable, since Calcite also uses a similar name for the same string: `DynamicRecordType.DYNAMIC_STAR_PREFIX`. So current name at least helps to avoid divergence in naming for Drill and Calcite. ---
[GitHub] drill pull request #1138: DRILL-4120: Allow implicit columns for Avro storag...
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1138#discussion_r172618093 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/types/ExtendableRelDataTypeHolder.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.planner.types; + +import org.apache.calcite.rel.type.RelDataTypeFactory; +import org.apache.calcite.rel.type.RelDataTypeField; +import org.apache.calcite.rel.type.RelDataTypeFieldImpl; +import org.apache.calcite.sql.type.SqlTypeName; + +import java.util.List; + +/** + * Holder for list of RelDataTypeField which may be expanded by partition or implicit columns. --- End diff -- You are right, partition columns are added before table columns in `AvroDrillTable.getRowType()`. Thanks for pointing this, modified comment. ---
[GitHub] drill pull request #1138: DRILL-4120: Allow implicit columns for Avro storag...
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1138#discussion_r172630422 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/SqlConverter.java --- @@ -277,6 +279,25 @@ public String deriveAlias( return SqlValidatorUtil.getAlias(node, ordinal); } +/** + * Checks that specified expression is not implicit column and + * adds is to a select list, ensuring that its alias does not + * clash with any existing expressions on the list. + */ +@Override +protected void addToSelectList( --- End diff -- Added comment into Javadoc, which describes when and why this method is used. As for Avro and Dynamic tables, the key point is the result of `RelDataType.isDynamicStruct()` method. For `AvroDrillTable` we use `ExtendableRelDataType` whose `isDynamicStruct()` method returns `false`, but for `DynamicDrillTable` we use `RelDataTypeDrillImpl` whose `isDynamicStruct()` method returns `true`. In such way, Calcites `SqlValidatorImpl` determines whether columns should be added. ---
[GitHub] drill pull request #1138: DRILL-4120: Allow implicit columns for Avro storag...
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1138#discussion_r172620291 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/avro/AvroDrillTable.java --- @@ -58,16 +65,31 @@ public AvroDrillTable(String storageEngineName, @Override public RelDataType getRowType(RelDataTypeFactory typeFactory) { -List typeList = Lists.newArrayList(); -List fieldNameList = Lists.newArrayList(); +// ExtendableRelDataTypeHolder is reused to preserve previously added implicit columns +if (holder == null) { + List typeList = Lists.newArrayList(); + List fieldNameList = Lists.newArrayList(); -Schema schema = reader.getSchema(); -for (Field field : schema.getFields()) { - fieldNameList.add(field.name()); - typeList.add(getNullableRelDataTypeFromAvroType(typeFactory, field.schema())); + // adds partition columns to RowType + List partitions = ColumnExplorer.getPartitions(((FormatSelection) getSelection()).getSelection(), schemaConfig); --- End diff -- 1. Yes, it is safe, since we are using the same `FormatSelection` instance as we passed to the parent constructor. 2. Thanks, done. ---
[GitHub] drill pull request #1138: DRILL-4120: Allow implicit columns for Avro storag...
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1138#discussion_r172618262 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/ColumnExplorer.java --- @@ -78,6 +79,23 @@ public ColumnExplorer(OptionManager optionManager, List columns) { return map; } + /** + * Returns list with implicit column names taken from specified {@link SchemaConfig}. + * + * @param schemaConfig the source of session options values. + * @return list with implicit column names. + */ + public static List getImplicitColumns(SchemaConfig schemaConfig) { --- End diff -- Thanks, renamed. ---
[GitHub] drill issue #1138: DRILL-4120: Allow implicit columns for Avro storage forma...
Github user vvysotskyi commented on the issue: https://github.com/apache/drill/pull/1138 @arina-ielchiieva, @paul-rogers, I have reworked this pull request to use `AvroDrillTable`, as it was before my changes. Also, with `AvroDrillTable` there is no need to make changes in `AvroRecordReader`, so I reverted them. Could you please take a look again? ---
[GitHub] drill issue #1138: DRILL-4120: Allow implicit columns for Avro storage forma...
Github user vvysotskyi commented on the issue: https://github.com/apache/drill/pull/1138 @paul-rogers, schema is taken from the first file in the `FormatSelection`. Therefore for the case, when we have a table with several files with a different scheme, Drill query will fail. As for the plan-time type information, besides the validation at the stage when a query is converted into rel nodes, field list may be used in project rel nodes instead of the dynamic star for `DynamicDrillTable`. ---
[GitHub] drill pull request #1142: DRILL-6198: OpenTSDB unit tests fail when Lilith c...
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1142#discussion_r171620225 --- Diff: contrib/storage-opentsdb/src/test/java/org/apache/drill/store/openTSDB/TestOpenTSDBPlugin.java --- @@ -185,4 +188,26 @@ public void testDescribe() throws Exception { test("describe `warp.speed.test`"); Assert.assertEquals(1, testSql("show tables")); } + + /** + * Checks that port with specified number is free and returns it. + * Otherwise, increases port number and checks until free port is found + * or the number of attempts is reached specified numAttempts + * + * @param portNum initial port number + * @param numAttempts max number of attempts to find port with greater number + * @return free port number + * @throws BindException if free port was not found and all attempts were used. + */ + private static int getFreePortNum(int portNum, int numAttempts) throws IOException { +while (numAttempts > 0) { --- End diff -- 1. Thanks, it looks better with for loop. 2. Added more details to the error message. ---
[GitHub] drill pull request #1142: DRILL-6198: OpenTSDB unit tests fail when Lilith c...
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1142#discussion_r171617811 --- Diff: contrib/storage-opentsdb/src/test/java/org/apache/drill/store/openTSDB/TestOpenTSDBPlugin.java --- @@ -51,17 +54,17 @@ public class TestOpenTSDBPlugin extends PlanTestBase { - protected static OpenTSDBStoragePlugin storagePlugin; - protected static OpenTSDBStoragePluginConfig storagePluginConfig; + private static int portNum = 10_000; --- End diff -- Thanks, removed. ---
[GitHub] drill pull request #1142: DRILL-6198: OpenTSDB unit tests fail when Lilith c...
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1142#discussion_r171618633 --- Diff: contrib/storage-opentsdb/src/test/java/org/apache/drill/store/openTSDB/TestOpenTSDBPlugin.java --- @@ -51,17 +54,17 @@ public class TestOpenTSDBPlugin extends PlanTestBase { - protected static OpenTSDBStoragePlugin storagePlugin; - protected static OpenTSDBStoragePluginConfig storagePluginConfig; + private static int portNum = 10_000; @Rule - public WireMockRule wireMockRule = new WireMockRule(1); + public WireMockRule wireMockRule = new WireMockRule(portNum); @BeforeClass public static void setup() throws Exception { +portNum = getFreePortNum(portNum, 1000); --- End diff -- Done. ---
[GitHub] drill pull request #1142: DRILL-6198: OpenTSDB unit tests fail when Lilith c...
GitHub user vvysotskyi opened a pull request: https://github.com/apache/drill/pull/1142 DRILL-6198: OpenTSDB unit tests fail when Lilith client is run Added method which checks that the default port 10_000 is free, otherwise this method increases port number and checks until free port is found. You can merge this pull request into a Git repository by running: $ git pull https://github.com/vvysotskyi/drill DRILL-6198 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/drill/pull/1142.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1142 commit 671d3e0d900c19f561cb3d0f744898c0f9bf20e9 Author: Volodymyr Vysotskyi Date: 2018-03-01T12:52:28Z DRILL-6198: OpenTSDB unit tests fail when Lilith client is run ---
[GitHub] drill pull request #1138: DRILL-4120: Allow implicit columns for Avro storag...
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1138#discussion_r171544478 --- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/store/avro/AvroFormatTest.java --- @@ -170,25 +169,35 @@ public void testSimplePrimitiveSchema_SelectColumnSubset() throws Exception { @Test public void testSimplePrimitiveSchema_NoColumnsExistInTheSchema() throws Exception { -final String file = generateSimplePrimitiveSchema_NoNullValues().getFileName(); -try { - test("select h_dummy1, e_dummy2 from dfs.`%s`", file); - Assert.fail("Test should fail as h_dummy1 and e_dummy2 does not exist."); -} catch(UserException ue) { - Assert.assertTrue("Test should fail as h_dummy1 and e_dummy2 does not exist.", - ue.getMessage().contains("Column 'h_dummy1' not found in any table")); -} +final String file = generateSimplePrimitiveSchema_NoNullValues(1).getFileName(); +testBuilder() + .sqlQuery("select h_dummy1, e_dummy2 from dfs.`%s`", file) + .unOrdered() + .baselineColumns("h_dummy1", "e_dummy2") + .baselineValues(null, null) + .go(); } @Test public void testSimplePrimitiveSchema_OneExistAndOneDoesNotExistInTheSchema() throws Exception { -final String file = generateSimplePrimitiveSchema_NoNullValues().getFileName(); -try { - test("select h_boolean, e_dummy2 from dfs.`%s`", file); - Assert.fail("Test should fail as e_dummy2 does not exist."); -} catch(UserException ue) { - Assert.assertTrue("Test should fail as e_dummy2 does not exist.", true); -} +final String file = generateSimplePrimitiveSchema_NoNullValues(1).getFileName(); +testBuilder() + .sqlQuery("select h_boolean, e_dummy2 from dfs.`%s`", file) + .unOrdered() + .baselineColumns("h_boolean", "e_dummy2") + .baselineValues(true, null) + .go(); + } + + @Test + public void testImplicitColumnFilename() throws Exception { +final String file = generateSimplePrimitiveSchema_NoNullValues(1).getFileName(); +testBuilder() + .sqlQuery("select filename from dfs.`%s`", file) --- End diff -- Thanks for pointing this, modified existing test to check except the `filename` also `suffix`, `fqn` and `filepath` implicit columns. Added separate test for partition column. ---
[GitHub] drill pull request #1138: DRILL-4120: Allow implicit columns for Avro storag...
GitHub user vvysotskyi opened a pull request: https://github.com/apache/drill/pull/1138 DRILL-4120: Allow implicit columns for Avro storage format Existing implementation of `AvroDrillTabl` does not allow dynamic columns discovering. `AvroDrillTable.getRowType()` method returns `RelDataTypeImlp` instance with the list of all table columns. It forces validator to check columns from select list in `RowType` list. It makes impossible to use implicit columns. This fix replaces the usage of `AvroDrillTable` by `DynamicDrillTable` for Avro format and also allows usage of non-existent columns in Avro tables to be consistent with other storage formats. You can merge this pull request into a Git repository by running: $ git pull https://github.com/vvysotskyi/drill DRILL-4120 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/drill/pull/1138.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1138 commit 402accca668481bb6816aad438c867781157fac6 Author: Volodymyr Vysotskyi Date: 2018-02-27T16:39:22Z DRILL-4120: Allow implicit columns for Avro storage format ---
[GitHub] drill pull request #1104: DRILL-6118: Handle item star columns during projec...
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1104#discussion_r166310161 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/project/ProjectRecordBatch.java --- @@ -596,10 +596,10 @@ private void classifyExpr(final NamedExpression ex, final RecordBatch incoming, final NameSegment ref = ex.getRef().getRootSegment(); final boolean exprHasPrefix = expr.getPath().contains(StarColumnHelper.PREFIX_DELIMITER); final boolean refHasPrefix = ref.getPath().contains(StarColumnHelper.PREFIX_DELIMITER); -final boolean exprIsStar = expr.getPath().equals(SchemaPath.WILDCARD); -final boolean refContainsStar = ref.getPath().contains(SchemaPath.WILDCARD); -final boolean exprContainsStar = expr.getPath().contains(SchemaPath.WILDCARD); -final boolean refEndsWithStar = ref.getPath().endsWith(SchemaPath.WILDCARD); +final boolean exprIsStar = expr.getPath().equals(SchemaPath.DYNAMIC_STAR); --- End diff -- This change became required after Calcite update. With the changes in CALCITE-1150, `*` is replaced by `**` after a query is parsed and `**` is added to the RowType. Therefore WILDCARD can't come from the plan and its usage should be replaced by `**`. ---
[GitHub] drill issue #1066: DRILL-3993: Changes to support Calcite 1.15
Github user vvysotskyi commented on the issue: https://github.com/apache/drill/pull/1066 FYI: changes in pom files: We observed issue with Avatica similar to the issue described in https://issues.apache.org/jira/browse/CALCITE-1694. Therefore we had to make changes in our pom files similar to the changes, made in CALCITE-1694 to fix NoClassDefFoundError. ---
[GitHub] drill issue #1066: DRILL-3993: Changes to support Calcite 1.15
Github user vvysotskyi commented on the issue: https://github.com/apache/drill/pull/1066 @amansinha100, regarding HashAggregate OOM related change, it was done in the scope of this pull request since with new Calcite a physical plan for the query was changed to the correct one but it caused an infinite loop in HashAgg operator. Therefore I made these changes in order to prevent this infinite loop for the queries that previously worked. I still think that it is better to keep this change in this PR. ---
[GitHub] drill pull request #1066: DRILL-3993: Changes to support Calcite 1.15
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1066#discussion_r162157833 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/DrillRelBuilder.java --- @@ -0,0 +1,69 @@ +/* + * 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.planner; + +import org.apache.calcite.plan.Context; +import org.apache.calcite.plan.Contexts; +import org.apache.calcite.plan.RelOptCluster; +import org.apache.calcite.plan.RelOptSchema; +import org.apache.calcite.rel.RelNode; +import org.apache.calcite.rel.core.RelFactories; +import org.apache.calcite.tools.RelBuilder; +import org.apache.calcite.tools.RelBuilderFactory; +import org.apache.calcite.util.Util; + +public class DrillRelBuilder extends RelBuilder { + private final RelFactories.FilterFactory filterFactory; + + protected DrillRelBuilder(Context context, RelOptCluster cluster, RelOptSchema relOptSchema) { +super(context, cluster, relOptSchema); +this.filterFactory = +Util.first(context.unwrap(RelFactories.FilterFactory.class), +RelFactories.DEFAULT_FILTER_FACTORY); + } + + /** + * Original method {@link RelBuilder#empty} returns empty values rel. + * In the order to preserve dara row types, filter with false predicate is created. --- End diff -- Thanks, fixed ---
[GitHub] drill pull request #1066: DRILL-3993: Changes to support Calcite 1.15
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1066#discussion_r162161040 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/DrillRelBuilder.java --- @@ -0,0 +1,69 @@ +/* + * 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.planner; + +import org.apache.calcite.plan.Context; +import org.apache.calcite.plan.Contexts; +import org.apache.calcite.plan.RelOptCluster; +import org.apache.calcite.plan.RelOptSchema; +import org.apache.calcite.rel.RelNode; +import org.apache.calcite.rel.core.RelFactories; +import org.apache.calcite.tools.RelBuilder; +import org.apache.calcite.tools.RelBuilderFactory; +import org.apache.calcite.util.Util; + +public class DrillRelBuilder extends RelBuilder { + private final RelFactories.FilterFactory filterFactory; + + protected DrillRelBuilder(Context context, RelOptCluster cluster, RelOptSchema relOptSchema) { +super(context, cluster, relOptSchema); +this.filterFactory = +Util.first(context.unwrap(RelFactories.FilterFactory.class), +RelFactories.DEFAULT_FILTER_FACTORY); + } + + /** + * Original method {@link RelBuilder#empty} returns empty values rel. + * In the order to preserve dara row types, filter with false predicate is created. + */ + @Override + public RelBuilder empty() { --- End diff -- In some cases, RowType of the input may not be known at the moment when this method is called (some of the columns or all columns were not dynamically discovered yet), therefore I had to make changes directly in Calcite to allow using of custom RelBuilder and make changes in Drill to pass custom RelBuilder into them. For more details please see [1] and [2]. [1] https://lists.apache.org/list.html?d...@calcite.apache.org:lte=1y:Make%20RelBuilder.filter%28%29%20configurable [2] https://issues.apache.org/jira/browse/CALCITE-2043 ---
[GitHub] drill pull request #1066: DRILL-3993: Changes to support Calcite 1.15
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1066#discussion_r161796052 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/aggregate/HashAggTemplate.java --- @@ -1303,6 +1305,8 @@ private void checkGroupAndAggrValues(int incomingRowIdx) { long memDiff = allocator.getAllocatedMemory() - allocatedBeforeHTput; if ( memDiff > 0 ) { logger.warn("Leak: HashTable put() OOM left behind {} bytes allocated",memDiff); } + checkForSpillPossibility(currentPartition); --- End diff -- These checks were needed to avoid infinite loop when there is not enough memory for the spill. I moved these checks into `spillIfNeeded()` method, so when called `doSpill()`, `forceSpill` in `spillIfNeeded()` is true and check should be done. ---
[GitHub] drill pull request #1066: DRILL-3993: Changes to support Calcite 1.15
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1066#discussion_r161185413 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/AggPruleBase.java --- @@ -82,4 +83,19 @@ protected boolean create2PhasePlan(RelOptRuleCall call, DrillAggregateRel aggreg } return true; } + + /** + * Returns group-by keys with the remapped arguments for specified aggregate. + * + * @param groupSet ImmutableBitSet of aggregate rel node, whose group-by keys should be remapped. + * @return {@link ImmutableBitSet} instance with remapped keys. + */ + public static ImmutableBitSet remapGroupSet(ImmutableBitSet groupSet) { --- End diff -- After the changes in CALCITE-1930 in the class `AggregateExpandDistinctAggregatesRule`, this rule started applying more correctly, since in the older version there were checks like this: ``` aggCall.getAggregation() instanceof SqlCountAggFunction ``` but they were replaced by checks like this: ``` final SqlKind aggCallKind = aggCall.getAggregation().getKind(); switch (aggCallKind) ``` So for the cases when instead of Calcite s `SqlCountAggFunction` were used `DrillCalciteSqlAggFunctionWrapper`s this rule changed its behaviour and instead of returning joins of distinct and non-distinct aggregate rel nodes, it returns distinct aggregate which has an input with non-distinct aggregates grouped by column, which is distinct in outer aggregate. These Drill rules were able to work correctly only for the cases when aggregate rel node does not contain ` aggCalls` and contains only the single field in `rowType` (in this case `groupSet` is always `{0}`, and it still correct for outer aggregates which are created in `*AggPrule`s) With the new version of `AggregateExpandDistinctAggregatesRule` these Drill rules received aggregate rel nodes with the non-empty lists of ` aggCalls`, therefore its `rowType` contains more than one field. But before my changes, the same `groupSet` was passed to the constructor of outer aggregate and row type of aggregate differed from the row type of its input. So it was incorrect `groupSet`. Aggregate rel nodes always specify the group by columns in the first positions of the list, so correct `groupSet` for outer aggregate should be `ImmutableBitSet` with the same size as the `groupSet` of nested aggregate, but the ordinals of columns should start from 0. As for the point of iterating through the group set, `ImmutableBitSet.size()`, `ImmutableBitSet.length()` and `ImmutableBitSet.cardinality()` does not return desired "size" of the `groupSet`. `AggregateExpandDistinctAggregatesRule` also contains similar code which iterating through the `groupSet` for similar purposes. ---
[GitHub] drill pull request #570: DRILL-4834 decimal implementation is vulnerable to ...
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/570#discussion_r161015751 --- Diff: exec/vector/src/main/codegen/templates/VariableLengthVectors.java --- @@ -418,6 +422,16 @@ public void get(int index, Nullable${minor.class}Holder holder){ <#switch minor.class> +<#case "VarDecimal"> +@Override +public ${friendlyType} getObject(int index) { + byte[] b = get(index); + BigInteger bi = new BigInteger(b); + BigDecimal bd = new BigDecimal(bi, getField().getScale()); + //System.out.println("VarDecimal getObject " + index + " scale " + getField().getScale() + " len " + b.length + " bd " + bd); --- End diff -- Please remove this comment ---
[GitHub] drill pull request #570: DRILL-4834 decimal implementation is vulnerable to ...
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/570#discussion_r161006318 --- Diff: exec/java-exec/src/main/codegen/templates/Decimal/CastVarCharDecimal.java --- @@ -85,6 +85,30 @@ public void eval() { // Assign the scale and precision out.scale = (int) scale.value; + +<#if type.to.endsWith("VarDecimal")> + +// VarDecimal gets its own cast logic +int readIndex = in.start; +int endIndex = in.end; +StringBuffer sb = new StringBuffer(); --- End diff -- I think it would be easier to receive string using this code: ``` byte[] buf = new byte[in.end - in.start]; buffer.getBytes(in.start, buf, 0, in.end - in.start); String s = new String(buf, Charsets.UTF_8); ``` ---
[GitHub] drill pull request #570: DRILL-4834 decimal implementation is vulnerable to ...
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/570#discussion_r161004122 --- Diff: exec/java-exec/src/main/codegen/templates/Decimal/CastIntDecimal.java --- @@ -68,15 +68,31 @@ public void setup() { public void eval() { out.scale = (int) scale.value; + +<#if !type.to.endsWith("VarDecimal")> out.precision = (int) precision.value; + -<#if type.to == "Decimal9" || type.to == "Decimal18"> +<#if type.to.endsWith("VarDecimal")> +out.start = 0; +out.buffer = buffer; +String s = Long.toString((long)in.value); +for (int i = 0; i < out.scale; ++i) { // add 0's to get unscaled integer +s += "0"; +} +java.math.BigInteger bi = new java.math.BigInteger(s); +java.math.BigDecimal bd = new java.math.BigDecimal(bi, out.scale); +//System.out.println("CastIntDecimal in " + in.value + " scale " + out.scale + " string " + s + " bd " + bd); --- End diff -- Please remove this comment. ---
[GitHub] drill pull request #570: DRILL-4834 decimal implementation is vulnerable to ...
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/570#discussion_r160963518 --- Diff: exec/vector/src/main/codegen/templates/NullableValueVectors.java --- @@ -327,13 +327,17 @@ public Mutator getMutator(){ return v; } - public void copyFrom(int fromIndex, int thisIndex, Nullable${minor.class}Vector from){ + protected void copyFromUnsafe(int fromIndex, int thisIndex, Nullable${minor.class}Vector from){ --- End diff -- Since we already have `copyFromSafe()` method in this class, `copyFrom()` is supposed to be unsafe. ---
[GitHub] drill pull request #570: DRILL-4834 decimal implementation is vulnerable to ...
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/570#discussion_r160941530 --- Diff: exec/java-exec/src/main/codegen/templates/SqlAccessors.java --- @@ -127,6 +127,25 @@ public String getString(int index) { } <#break> +<#case "VarDecimal"> + +@Override +public String getString(int index){ +<#if mode=="Nullable"> +if(ac.isNull(index)){ + return null; +} + +try { --- End diff -- I think it would be better just throw the exception. Then it is easier to detect a case when data was stored in the vector incorrectly, or if it has come from the stored data, inform a user that data was corrupted. ---
[GitHub] drill pull request #570: DRILL-4834 decimal implementation is vulnerable to ...
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/570#discussion_r160980556 --- Diff: exec/vector/src/main/codegen/templates/VariableLengthVectors.java --- @@ -539,7 +553,12 @@ public void setValueLengthSafe(int index, int length) { } -public void setSafe(int index, int start, int end, DrillBuf buffer){ +<#if type.minor == "VarDecimal"> --- End diff -- Sorry, perhaps I made mistake, yes, it looks OK. But do we need to pass `scale` for `VarDecimal` even if we don't use it in this method? I suppose should be made a change in `ComplexWriters.java` to avoid the usage of this method with the additional argument. ---
[GitHub] drill pull request #570: DRILL-4834 decimal implementation is vulnerable to ...
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/570#discussion_r160987506 --- Diff: exec/java-exec/src/main/codegen/templates/Decimal/CastDecimalVarchar.java --- @@ -150,6 +150,14 @@ public void setup() { public void eval() { +<#if type.from.contains("VarDecimal")> +java.math.BigDecimal bigDecimal = org.apache.drill.exec.util.DecimalUtility.getBigDecimalFromDrillBuf(in.buffer, in.start, in.end - in.start, in.scale); +String str = bigDecimal.toString(); +out.buffer = buffer; +out.start = 0; +out.end = Math.min((int)len.value, str.length()); +out.buffer.setBytes(0, str.getBytes()); --- End diff -- I guess we should do the same thing as for other decimals: ``` out.buffer.setBytes(0, String.valueOf(str.substring(0, out.end)).getBytes()); ``` ---
[GitHub] drill pull request #570: DRILL-4834 decimal implementation is vulnerable to ...
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/570#discussion_r160939410 --- Diff: exec/java-exec/src/main/codegen/templates/Decimal/DecimalFunctions.java --- @@ -102,7 +111,578 @@ <#-- For each DECIMAL... type (in DecimalTypes.tdd) ... --> <#list comparisonTypesDecimal.decimalTypes as type> -<#if type.name.endsWith("Sparse")> +<#if type.name.endsWith("VarDecimal")> + +<@pp.changeOutputFile name="/org/apache/drill/exec/expr/fn/impl/${type.name}Functions.java" /> + +<#include "/@includes/license.ftl" /> + +package org.apache.drill.exec.expr.fn.impl; + +<#include "/@includes/vv_imports.ftl" /> + +import org.apache.drill.exec.expr.DrillSimpleFunc; +import org.apache.drill.exec.expr.annotations.FunctionTemplate; +import org.apache.drill.exec.expr.annotations.FunctionTemplate.NullHandling; +import org.apache.drill.exec.expr.annotations.Output; +import org.apache.drill.exec.expr.annotations.Param; +import org.apache.drill.exec.expr.annotations.Workspace; +import org.apache.drill.exec.expr.fn.FunctionGenerationHelper; +import org.apache.drill.exec.expr.holders.*; +import org.apache.drill.exec.record.RecordBatch; + +import io.netty.buffer.ByteBuf; +import io.netty.buffer.DrillBuf; + +import java.nio.ByteBuffer; + +@SuppressWarnings("unused") +public class ${type.name}Functions { +private static void initBuffer(DrillBuf buffer) { +// for VarDecimal, this method of setting initial size is actually only a very rough heuristic. +int size = (${type.storage} * (org.apache.drill.exec.util.DecimalUtility.INTEGER_SIZE)); +buffer = buffer.reallocIfNeeded(size); + } + +@FunctionTemplate(name = "subtract", scope = FunctionTemplate.FunctionScope.DECIMAL_ADD_SCALE, nulls = NullHandling.NULL_IF_NULL) --- End diff -- By default `checkPrecision` is false, so it is not required to specify it explicitly in this case. ---
[GitHub] drill pull request #570: DRILL-4834 decimal implementation is vulnerable to ...
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/570#discussion_r160958360 --- Diff: exec/vector/src/main/codegen/templates/ComplexWriters.java --- @@ -99,7 +99,7 @@ public void write(Nullable${minor.class?cap_first}Holder h) { <#if !(minor.class == "Decimal9" || minor.class == "Decimal18" || minor.class == "Decimal28Sparse" || minor.class == "Decimal38Sparse" || minor.class == "Decimal28Dense" || minor.class == "Decimal38Dense")> public void write${minor.class}(<#list fields as field>${field.type} ${field.name}<#if field_has_next>, ) { -mutator.addSafe(idx(), <#list fields as field>${field.name}<#if field_has_next>, ); +mutator.addSafe(idx(), <#list fields as field><#if field.name == "scale"><#break>${field.name}<#if field_has_next && fields[field_index+1].name != "scale" >, ); --- End diff -- I meant to replace this line by something like this: ``` <#if minor.class == "VarDecimal"> mutator.addSafe(idx()<#list fields as field><#if field.include!true>, ${field.name}); // or something better <#else> mutator.addSafe(idx(), <#list fields as field>${field.name}<#if field_has_next>, ); ``` Since `field.include` is used only for Decimal types, we can replace all this code by this line: ``` mutator.addSafe(idx()<#list fields as field><#if field.include!true>, ${field.name}); ``` But do we use this method for VarDecimal somewhere? If not, it would be better to add VarDecimal `minor.class` to the if condition with other decimal types above. BTW, please modify similar changes below in this class and in `RepeatedValueVectors.java` ---
[GitHub] drill pull request #570: DRILL-4834 decimal implementation is vulnerable to ...
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/570#discussion_r161002913 --- Diff: common/src/main/java/org/apache/drill/common/util/CoreDecimalUtility.java --- @@ -32,6 +32,14 @@ public static long getDecimal18FromBigDecimal(BigDecimal input, int scale, int p } public static int getMaxPrecision(TypeProtos.MinorType decimalType) { +/* --- End diff -- This method is used in `TypeCastRules.getCost()` and in `CoreDecimalUtility.getPrecisionRange()`. Regarding its usage in `TypeCastRules.getCost()`, we may return `RelDataTypeSystem.getMaxNumericPrecision()` for VarDecimal. But regarding its usage in `CoreDecimalUtility.getPrecisionRange()` method, `getPrecisionRange()` is used in DecimalScalePrecisionFunction classes for calculating resulting precision. I suppose these methods should be rewritten in the same way as it is implemented in Calcite `RelDataTypeFactoryImpl`. But I think it would be better to make these changes in separate Jira. But now please remove these comments. ---
[GitHub] drill pull request #570: DRILL-4834 decimal implementation is vulnerable to ...
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/570#discussion_r160489866 --- Diff: exec/vector/src/main/codegen/templates/ComplexWriters.java --- @@ -99,7 +99,7 @@ public void write(Nullable${minor.class?cap_first}Holder h) { <#if !(minor.class == "Decimal9" || minor.class == "Decimal18" || minor.class == "Decimal28Sparse" || minor.class == "Decimal38Sparse" || minor.class == "Decimal28Dense" || minor.class == "Decimal38Dense")> public void write${minor.class}(<#list fields as field>${field.type} ${field.name}<#if field_has_next>, ) { -mutator.addSafe(idx(), <#list fields as field>${field.name}<#if field_has_next>, ); +mutator.addSafe(idx(), <#list fields as field><#if field.name == "scale"><#break>${field.name}<#if field_has_next && fields[field_index+1].name != "scale" >, ); --- End diff -- I think it would be better to replace these checks by a check for `minor.class` ---
[GitHub] drill pull request #570: DRILL-4834 decimal implementation is vulnerable to ...
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/570#discussion_r160480509 --- Diff: exec/java-exec/src/main/codegen/templates/SqlAccessors.java --- @@ -127,6 +127,25 @@ public String getString(int index) { } <#break> +<#case "VarDecimal"> + +@Override +public String getString(int index){ +<#if mode=="Nullable"> +if(ac.isNull(index)){ + return null; +} + +try { --- End diff -- I don't see the necessity of try catch there. Could you please explain? ---
[GitHub] drill pull request #570: DRILL-4834 decimal implementation is vulnerable to ...
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/570#discussion_r160481004 --- Diff: exec/java-exec/src/main/codegen/templates/StringOutputRecordWriter.java --- @@ -146,7 +146,7 @@ public void writeField() throws IOException { // TODO: error check addField(fieldId, reader.readObject().toString()); - <#elseif minor.class == "VarChar" || minor.class == "Var16Char" || minor.class == "VarBinary"> + <#elseif minor.class == "VarChar" || minor.class == "Var16Char" || minor.class == "VarBinary" || minor.class == "VarDecimal"> --- End diff -- May these types be moved into previous `elseif`? ---
[GitHub] drill pull request #570: DRILL-4834 decimal implementation is vulnerable to ...
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/570#discussion_r160492087 --- Diff: exec/vector/src/main/codegen/templates/VariableLengthVectors.java --- @@ -539,7 +553,12 @@ public void setValueLengthSafe(int index, int length) { } -public void setSafe(int index, int start, int end, DrillBuf buffer){ +<#if type.minor == "VarDecimal"> --- End diff -- I suppose condition should be negated or if and else should be swapped ---
[GitHub] drill pull request #570: DRILL-4834 decimal implementation is vulnerable to ...
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/570#discussion_r160458242 --- Diff: exec/java-exec/src/main/codegen/templates/Decimal/CastDecimalVarchar.java --- @@ -150,6 +150,14 @@ public void setup() { public void eval() { +<#if type.from.contains("VarDecimal")> +java.math.BigDecimal bigDecimal = org.apache.drill.exec.util.DecimalUtility.getBigDecimalFromDrillBuf(in.buffer, in.start, in.end - in.start, in.scale); +String str = bigDecimal.toString(); +out.buffer = buffer; +out.start = 0; +out.end = Math.min((int)len.value, str.length()); +out.buffer.setBytes(0, str.getBytes()); --- End diff -- What about the case when `str` has a length greater than `len`? I think it would be better to use `setBytes(int index, byte[] src, int srcIndex, int length)` method here. ---
[GitHub] drill pull request #570: DRILL-4834 decimal implementation is vulnerable to ...
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/570#discussion_r160492409 --- Diff: exec/vector/src/main/java/org/apache/drill/exec/util/DecimalUtility.java --- @@ -159,9 +159,20 @@ public static BigDecimal getBigDecimalFromSparse(DrillBuf data, int startIndex, } public static BigDecimal getBigDecimalFromDrillBuf(DrillBuf bytebuf, int start, int length, int scale) { + if (length <= 0) { +// if the length is somehow non-positive, interpret this as zero +//System.out.println("getBigDecimal forces 0 with start " + start + " len " + length); + try { + throw new Exception("hi there"); --- End diff -- Please remove ---
[GitHub] drill pull request #570: DRILL-4834 decimal implementation is vulnerable to ...
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/570#discussion_r160473858 --- Diff: exec/java-exec/src/main/codegen/templates/Decimal/DecimalAggrTypeFunctions2.java --- @@ -108,9 +108,12 @@ public void output() { out.buffer = buffer; out.start = 0; out.scale = outputScale.value; -out.precision = 38; java.math.BigDecimal average = ((java.math.BigDecimal)(value.obj)).divide(java.math.BigDecimal.valueOf(count.value, 0), out.scale, java.math.BigDecimal.ROUND_HALF_UP); +<#if type.inputType.contains("VarDecimal")> --- End diff -- It would be better to swap if and else and negate if condition for better readability. ---
[GitHub] drill pull request #570: DRILL-4834 decimal implementation is vulnerable to ...
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/570#discussion_r160476071 --- Diff: exec/java-exec/src/main/codegen/templates/Decimal/DecimalFunctions.java --- @@ -102,7 +111,578 @@ <#-- For each DECIMAL... type (in DecimalTypes.tdd) ... --> <#list comparisonTypesDecimal.decimalTypes as type> -<#if type.name.endsWith("Sparse")> +<#if type.name.endsWith("VarDecimal")> + +<@pp.changeOutputFile name="/org/apache/drill/exec/expr/fn/impl/${type.name}Functions.java" /> + +<#include "/@includes/license.ftl" /> + +package org.apache.drill.exec.expr.fn.impl; + +<#include "/@includes/vv_imports.ftl" /> + +import org.apache.drill.exec.expr.DrillSimpleFunc; +import org.apache.drill.exec.expr.annotations.FunctionTemplate; +import org.apache.drill.exec.expr.annotations.FunctionTemplate.NullHandling; +import org.apache.drill.exec.expr.annotations.Output; +import org.apache.drill.exec.expr.annotations.Param; +import org.apache.drill.exec.expr.annotations.Workspace; +import org.apache.drill.exec.expr.fn.FunctionGenerationHelper; +import org.apache.drill.exec.expr.holders.*; +import org.apache.drill.exec.record.RecordBatch; + +import io.netty.buffer.ByteBuf; +import io.netty.buffer.DrillBuf; + +import java.nio.ByteBuffer; + +@SuppressWarnings("unused") +public class ${type.name}Functions { +private static void initBuffer(DrillBuf buffer) { +// for VarDecimal, this method of setting initial size is actually only a very rough heuristic. +int size = (${type.storage} * (org.apache.drill.exec.util.DecimalUtility.INTEGER_SIZE)); +buffer = buffer.reallocIfNeeded(size); + } + +@FunctionTemplate(name = "subtract", scope = FunctionTemplate.FunctionScope.DECIMAL_ADD_SCALE, nulls = NullHandling.NULL_IF_NULL) --- End diff -- Please specify `FunctionTemplate` correct scope, returnType and checkPrecisionRange for all functions in this class. ---
[GitHub] drill pull request #570: DRILL-4834 decimal implementation is vulnerable to ...
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/570#discussion_r160490947 --- Diff: exec/vector/src/main/codegen/templates/NullableValueVectors.java --- @@ -327,13 +327,17 @@ public Mutator getMutator(){ return v; } - public void copyFrom(int fromIndex, int thisIndex, Nullable${minor.class}Vector from){ + protected void copyFromUnsafe(int fromIndex, int thisIndex, Nullable${minor.class}Vector from){ --- End diff -- Are changes in this class necessary? ---
[GitHub] drill pull request #570: DRILL-4834 decimal implementation is vulnerable to ...
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/570#discussion_r160465652 --- Diff: exec/java-exec/src/main/codegen/templates/Decimal/CastIntDecimal.java --- @@ -68,15 +68,31 @@ public void setup() { public void eval() { out.scale = (int) scale.value; + +<#if !type.to.endsWith("VarDecimal")> out.precision = (int) precision.value; + -<#if type.to == "Decimal9" || type.to == "Decimal18"> +<#if type.to.endsWith("VarDecimal")> +out.start = 0; +out.buffer = buffer; +String s = Long.toString((long)in.value); +for (int i = 0; i < out.scale; ++i) { // add 0's to get unscaled integer +s += "0"; --- End diff -- I think the usage of `StringBuilder` will be more suitable here. ---
[GitHub] drill issue #1066: DRILL-3993: Changes to support Calcite 1.15
Github user vvysotskyi commented on the issue: https://github.com/apache/drill/pull/1066 @chunhui-shi, I have made additional fixes in new commits (commits after DRILL-3993: Changes after code review. 3120762). Could you please take a look? Also, I have created pull request on incubator-calcite: https://github.com/mapr/incubator-calcite/pull/16 ---
[GitHub] drill issue #1049: DRILL-5971: Fix INT64, INT32 logical types in complex par...
Github user vvysotskyi commented on the issue: https://github.com/apache/drill/pull/1049 @parthchandra, thanks for the pull request! LGTM, +1. ---
[GitHub] drill issue #1066: DRILL-3993: Changes to support Calcite 1.15
Github user vvysotskyi commented on the issue: https://github.com/apache/drill/pull/1066 @chunhui-shi, this branch is not ready completely. There is opened a pull request on Apache Calcite [CALCITE-2018](https://issues.apache.org/jira/browse/CALCITE-2018) which should be cherry-picked into this branch. When it is merged into Apache master, we will update both branches and create a pull request on mapr/incubator-calcite. ---
[GitHub] drill pull request #1049: DRILL-5971: Fix INT64, INT32 logical types in comp...
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1049#discussion_r158702875 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/columnreaders/ColumnReaderFactory.java --- @@ -138,6 +138,8 @@ return new ParquetFixedWidthDictionaryReaders.DictionaryBigIntReader(recordReader, allocateSize, descriptor, columnChunkMetaData, fixedLength, (BigIntVector) v, schemaElement); --- End diff -- Sorry, I didn't notice a check for `DATE`, but what about `INT_32`? ---
[GitHub] drill pull request #1049: DRILL-5971: Fix INT64, INT32 logical types in comp...
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1049#discussion_r158700125 --- Diff: exec/vector/src/main/codegen/templates/FixedValueVectors.java --- @@ -394,9 +394,11 @@ public void get(int index, Nullable${minor.class}Holder holder){ final int offsetIndex = index * VALUE_WIDTH; final int months = data.getInt(offsetIndex); final int days= data.getInt(offsetIndex + ${minor.daysOffset}); - final int millis = data.getInt(offsetIndex + ${minor.millisecondsOffset}); + int millis = data.getInt(offsetIndex + 8); --- End diff -- I think we should revert this change. It is easier to change `daysOffset` and `millisecondsOffset` values in `ValueVectorTypes.tdd` rather find all usages if it will be needed. ---
[GitHub] drill pull request #1049: DRILL-5971: Fix INT64, INT32 logical types in comp...
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1049#discussion_r158702957 --- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/writer/TestParquetWriter.java --- @@ -636,8 +636,10 @@ public void testCTASWithIntervalTypes() throws Exception { "interval '-10 20:12:23.999' day(10) to second col2 " + "from cp.`employee.json` limit 2", tableName); -Period row1Col1 = new Period(0, 0, 0, 10, 0, 0, 0, 73840123); -Period row1Col2 = new Period(0, 0, 0, -10, 0, 0, 0, -72743999); +//Period row1Col1 = new Period(0, 0, 0, 10, 0, 0, 0, 73840123); +//Period row1Col2 = new Period(0, 0, 0, -10, 0, 0, 0, -72743999); --- End diff -- Please remove these comment lines. ---
[GitHub] drill pull request #1049: DRILL-5971: Fix INT64, INT32 logical types in comp...
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1049#discussion_r158703477 --- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/TestParquetComplex.java --- @@ -193,4 +200,168 @@ public void notxistsField() throws Exception { .run(); } + @Test //DRILL-5971 + public void testComplexLogicalIntTypes() throws Exception { +String query = String.format("select t.complextype as complextype, " + +"t.uint_64 as uint_64, t.uint_32 as uint_32, t.uint_16 as uint_16, t.uint_8 as uint_8, " + +"t.int_64 as int_64, t.int_32 as int_32, t.int_16 as int_16, t.int_8 as int_8 " + +"from cp.`store/parquet/complex/logical_int_complex.parquet` t" ); +String[] columns = {"complextype", "uint_64", "uint_32", "uint_16", "uint_8", "int_64", "int_32", "int_16", "int_8" }; +testBuilder() +.sqlQuery(query) +.unOrdered() +.baselineColumns(columns) +.baselineValues( mapOf("a","a","b","b") , 0L , 0 , 0, 0 , 0L, 0, 0 ,0 ) +.baselineValues( mapOf("a","a","b","b") , -1L , -1 , -1 , -1 , -1L , -1 , -1 , -1 ) +.baselineValues( mapOf("a","a","b","b") , 1L , 1 , 1, 1 , -9223372036854775808L , 1, 1 , 1 ) +.baselineValues( mapOf("a","a","b","b") , 9223372036854775807L , 2147483647 , 65535, 255 , 9223372036854775807L , -2147483648 , -32768 , -128 ) +.build() +.run(); + } + + @Test //DRILL-5971 + public void testComplexLogicalIntTypes2() throws Exception { +byte[] bytes12 = {'1', '2', '3', '4', '5', '6', '7', '8', '9', '0', 'a', 'b' }; +byte[] bytesOnes = new byte[12]; Arrays.fill(bytesOnes, (byte)1); +byte[] bytesZeros = new byte[12]; +String query = String.format( +" select " + +" t.rowKey as rowKey, " + +" t.StringTypes._UTF8 as _UTF8, " + +" t.StringTypes._Enum as _Enum, " + +" t.NumericTypes.Int32._INT32_RAW as _INT32_RAW, " + +" t.NumericTypes.Int32._INT_8 as _INT_8, " + +" t.NumericTypes.Int32._INT_16 as _INT_16, " + +" t.NumericTypes.Int32._INT_32 as _INT_32, " + +" t.NumericTypes.Int32._UINT_8 as _UINT_8, " + +" t.NumericTypes.Int32._UINT_16 as _UINT_16, " + +" t.NumericTypes.Int32._UINT_32 as _UINT_32, " + +" t.NumericTypes.Int64._INT64_RAW as _INT64_RAW, " + +" t.NumericTypes.Int64._INT_64 as _INT_64, " + +" t.NumericTypes.Int64._UINT_64 as _UINT_64, " + +" t.NumericTypes.DateTimeTypes._DATE_int32 as _DATE_int32, " + +" t.NumericTypes.DateTimeTypes._TIME_MILLIS_int32 as _TIME_MILLIS_int32, " + +" t.NumericTypes.DateTimeTypes._TIMESTAMP_MILLIS_int64 as _TIMESTAMP_MILLIS_int64, " + +" t.NumericTypes.DateTimeTypes._INTERVAL_fixed_len_byte_array_12 as _INTERVAL_fixed_len_byte_array_12, " + +" t.NumericTypes.Int96._INT96_RAW as _INT96_RAW " + +" from " + +" cp.`store/parquet/complex/parquet_logical_types_complex.parquet` t " + +" order by t.rowKey " +); +String[] columns = { +"rowKey " , +"_UTF8" , +"_Enum" , +"_INT32_RAW" , +"_INT_8" , +"_INT_16" , +"_INT_32" , +"_UINT_8" , +"_UINT_16" , +"_UINT_32" , +"_INT64_RAW" , +"_INT_64" , +"_UINT_64" , +"_DATE_int32" , +"_TIME_MILLIS_int32" , +"_TIMESTAMP_MILLIS_int64" , +"_INTERVAL_fixed_len_byte_array_12" , +"_INT96_RAW" + +}; +testBuilder() +.sqlQuery(query)
[GitHub] drill pull request #1049: DRILL-5971: Fix INT64, INT32 logical types in comp...
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1049#discussion_r158703375 --- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/TestParquetComplex.java --- @@ -193,4 +200,168 @@ public void notxistsField() throws Exception { .run(); } + @Test //DRILL-5971 + public void testComplexLogicalIntTypes() throws Exception { +String query = String.format("select t.complextype as complextype, " + +"t.uint_64 as uint_64, t.uint_32 as uint_32, t.uint_16 as uint_16, t.uint_8 as uint_8, " + +"t.int_64 as int_64, t.int_32 as int_32, t.int_16 as int_16, t.int_8 as int_8 " + +"from cp.`store/parquet/complex/logical_int_complex.parquet` t" ); +String[] columns = {"complextype", "uint_64", "uint_32", "uint_16", "uint_8", "int_64", "int_32", "int_16", "int_8" }; +testBuilder() +.sqlQuery(query) +.unOrdered() +.baselineColumns(columns) +.baselineValues( mapOf("a","a","b","b") , 0L , 0 , 0, 0 , 0L, 0, 0 ,0 ) --- End diff -- Please remove space before `mapOf` and remove all spaces before commas. ---
[GitHub] drill pull request #1049: DRILL-5971: Fix INT64, INT32 logical types in comp...
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1049#discussion_r158703216 --- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/TestParquetComplex.java --- @@ -193,4 +200,168 @@ public void notxistsField() throws Exception { .run(); } + @Test //DRILL-5971 + public void testComplexLogicalIntTypes() throws Exception { +String query = String.format("select t.complextype as complextype, " + +"t.uint_64 as uint_64, t.uint_32 as uint_32, t.uint_16 as uint_16, t.uint_8 as uint_8, " + +"t.int_64 as int_64, t.int_32 as int_32, t.int_16 as int_16, t.int_8 as int_8 " + +"from cp.`store/parquet/complex/logical_int_complex.parquet` t" ); +String[] columns = {"complextype", "uint_64", "uint_32", "uint_16", "uint_8", "int_64", "int_32", "int_16", "int_8" }; +testBuilder() +.sqlQuery(query) +.unOrdered() +.baselineColumns(columns) +.baselineValues( mapOf("a","a","b","b") , 0L , 0 , 0, 0 , 0L, 0, 0 ,0 ) +.baselineValues( mapOf("a","a","b","b") , -1L , -1 , -1 , -1 , -1L , -1 , -1 , -1 ) +.baselineValues( mapOf("a","a","b","b") , 1L , 1 , 1, 1 , -9223372036854775808L , 1, 1 , 1 ) +.baselineValues( mapOf("a","a","b","b") , 9223372036854775807L , 2147483647 , 65535, 255 , 9223372036854775807L , -2147483648 , -32768 , -128 ) +.build() +.run(); + } + + @Test //DRILL-5971 + public void testComplexLogicalIntTypes2() throws Exception { +byte[] bytes12 = {'1', '2', '3', '4', '5', '6', '7', '8', '9', '0', 'a', 'b' }; +byte[] bytesOnes = new byte[12]; Arrays.fill(bytesOnes, (byte)1); --- End diff -- Please move `Arrays.fill(bytesOnes, (byte)1);` to the new line and add space before 1. ---
[GitHub] drill pull request #1049: DRILL-5971: Fix INT64, INT32 logical types in comp...
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1049#discussion_r158703447 --- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/TestParquetComplex.java --- @@ -193,4 +200,168 @@ public void notxistsField() throws Exception { .run(); } + @Test //DRILL-5971 + public void testComplexLogicalIntTypes() throws Exception { +String query = String.format("select t.complextype as complextype, " + +"t.uint_64 as uint_64, t.uint_32 as uint_32, t.uint_16 as uint_16, t.uint_8 as uint_8, " + +"t.int_64 as int_64, t.int_32 as int_32, t.int_16 as int_16, t.int_8 as int_8 " + +"from cp.`store/parquet/complex/logical_int_complex.parquet` t" ); +String[] columns = {"complextype", "uint_64", "uint_32", "uint_16", "uint_8", "int_64", "int_32", "int_16", "int_8" }; +testBuilder() +.sqlQuery(query) +.unOrdered() +.baselineColumns(columns) +.baselineValues( mapOf("a","a","b","b") , 0L , 0 , 0, 0 , 0L, 0, 0 ,0 ) +.baselineValues( mapOf("a","a","b","b") , -1L , -1 , -1 , -1 , -1L , -1 , -1 , -1 ) +.baselineValues( mapOf("a","a","b","b") , 1L , 1 , 1, 1 , -9223372036854775808L , 1, 1 , 1 ) +.baselineValues( mapOf("a","a","b","b") , 9223372036854775807L , 2147483647 , 65535, 255 , 9223372036854775807L , -2147483648 , -32768 , -128 ) +.build() +.run(); + } + + @Test //DRILL-5971 + public void testComplexLogicalIntTypes2() throws Exception { +byte[] bytes12 = {'1', '2', '3', '4', '5', '6', '7', '8', '9', '0', 'a', 'b' }; +byte[] bytesOnes = new byte[12]; Arrays.fill(bytesOnes, (byte)1); +byte[] bytesZeros = new byte[12]; +String query = String.format( +" select " + +" t.rowKey as rowKey, " + +" t.StringTypes._UTF8 as _UTF8, " + +" t.StringTypes._Enum as _Enum, " + +" t.NumericTypes.Int32._INT32_RAW as _INT32_RAW, " + +" t.NumericTypes.Int32._INT_8 as _INT_8, " + +" t.NumericTypes.Int32._INT_16 as _INT_16, " + +" t.NumericTypes.Int32._INT_32 as _INT_32, " + +" t.NumericTypes.Int32._UINT_8 as _UINT_8, " + +" t.NumericTypes.Int32._UINT_16 as _UINT_16, " + +" t.NumericTypes.Int32._UINT_32 as _UINT_32, " + +" t.NumericTypes.Int64._INT64_RAW as _INT64_RAW, " + +" t.NumericTypes.Int64._INT_64 as _INT_64, " + +" t.NumericTypes.Int64._UINT_64 as _UINT_64, " + +" t.NumericTypes.DateTimeTypes._DATE_int32 as _DATE_int32, " + +" t.NumericTypes.DateTimeTypes._TIME_MILLIS_int32 as _TIME_MILLIS_int32, " + +" t.NumericTypes.DateTimeTypes._TIMESTAMP_MILLIS_int64 as _TIMESTAMP_MILLIS_int64, " + +" t.NumericTypes.DateTimeTypes._INTERVAL_fixed_len_byte_array_12 as _INTERVAL_fixed_len_byte_array_12, " + +" t.NumericTypes.Int96._INT96_RAW as _INT96_RAW " + +" from " + +" cp.`store/parquet/complex/parquet_logical_types_complex.parquet` t " + +" order by t.rowKey " +); +String[] columns = { +"rowKey " , +"_UTF8" , +"_Enum" , +"_INT32_RAW" , +"_INT_8" , +"_INT_16" , +"_INT_32" , +"_UINT_8" , +"_UINT_16" , +"_UINT_32" , +"_INT64_RAW" , +"_INT_64" , +"_UINT_64" , +"_DATE_int32" , +"_TIME_MILLIS_int32" , +"_TIMESTAMP_MILLIS_int64" , +"_INTERVAL_fixed_len_byte_array_12" , +"_INT96_RAW" + +}; +testBuilder() +.sqlQuery(query) +.ordered() --- End diff -- Maybe `unOrdered`? ---
[GitHub] drill pull request #1066: DRILL-3993: Changes to support Calcite 1.15
Github user vvysotskyi commented on a diff in the pull request: https://github.com/apache/drill/pull/1066#discussion_r158374197 --- Diff: exec/java-exec/src/main/codegen/includes/parserImpls.ftl --- @@ -351,4 +351,23 @@ SqlNode SqlDropFunction() : { return new SqlDropFunction(pos, jar); } -} \ No newline at end of file +} + +<#if !parser.includeCompoundIdentifier > --- End diff -- Actually, it does not cause a new behaviour or functionality, it just helps to preserve old one after changes in Calcite `Parser.jj`. Therefore existing unit tests cover this change. ---