[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_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_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_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_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_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_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_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_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 #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 #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 <daveoshinsky@...> Date: 2016-02-09T22:37:47Z DRILL-4184: Support variable length decimal fields in parquet commit 84a31b92dc86873d14a7be601d189bb1cbd59b7d Author: Volodymyr Vysotskyi <vvovyk@...> Date: 2018-04-05T12:35:42Z DRILL-6094: Decimal data type enhancements Add ExprVisitors for VARDECIMAL commit e93cd93211ffa84586ee0d21dab26b128d81b290 Author: Volodymyr Vysotskyi <vvovyk@...> 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 <vvovyk@...> Date: 2018-04-03T13:27:25Z DRILL-6094: Refresh protobuf C++ source files commit fbe93c46ed8a7e85a95da3ae145bfc00b7f17dd0 Author: Volodymyr Vysotskyi <vvovyk@...> 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 <vvovyk@...> Date: 2018-04-05T14:48:31Z DRILL-6094: Enable DECIMAL data type by default commit 46ce3d1504618ae7e547479280f5d60b35ec1caa Author: Volodymyr Vysotskyi <vvovyk@...> Date: 2018-04-05T14:51:39Z DRILL-6094: Add unit tests for DECIMAL data type commit f62545390a37425b9fbb21881e4cd12ade337e73 Author: Volodymyr Vysotskyi <vvovyk@...> Date: 2018-04-05T14:58:36Z DRILL-6094: Changes in C++ files commit 7a04309605c0cd869a9eb55450aaa43c0088b425 Author: Volodymyr Vysotskyi <vvovyk@...> 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 <vvovyk@...> Date: 2018-02-07T14:24:50Z DRILL-6294: Changes to support Calcite 1.16.0 commit 48880eb80d60a15e4ffdfcd6c729bfc75cf5d2da Author: Volodymyr Vysotskyi <vvovyk@...> 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 <vvovyk@...> 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 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 <vvovyk@...> 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 <vvovyk@...> 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_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_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_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_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_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_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_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_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)
+.ordered()
+.baselineColumns(columns)
+.baselineValues(1, "UTF8 string1", &q
[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.
---
[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_r158259594 --- Diff: contrib/storage-jdbc/src/main/java/org/apache/drill/exec/store/jdbc/JdbcPrel.java --- @@ -62,7 +62,7 @@ public JdbcPrel(RelOptCluster cluster, RelTraitSet traitSet, JdbcIntermediatePre (JavaTypeFactory) getCluster().getTypeFactory()); final JdbcImplementor.Result result = jdbcImplementor.visitChild(0, input.accept(new SubsetRemover())); -sql = result.asQuery().toSqlString(dialect).getSql(); +sql = result.asSelect().toSqlString(dialect).getSql(); --- End diff -- It is also may be another `SqlCall` instance. Thanks for pointing this, replaced it by `asStatement()` method. ---
[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_r158258451
--- Diff:
contrib/storage-hbase/src/main/java/org/apache/drill/exec/store/hbase/HBaseSchemaFactory.java
---
@@ -72,7 +72,16 @@ public AbstractSchema getSubSchema(String name) {
@Override
public Table getTable(String name) {
HBaseScanSpec scanSpec = new HBaseScanSpec(name);
- return new DrillHBaseTable(schemaName, plugin, scanSpec);
+ try {
+return new DrillHBaseTable(schemaName, plugin, scanSpec);
+ } catch (Exception e) {
+// Calcite firstly is looking for a table in the default schema,
if a table was not found,
+// it is looking in root schema.
+// If a table does not exist, a query will fail at validation
stage,
+// so the error should not be thrown there.
--- End diff --
Yes, I meant it. Thanks, replaced.
---
[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_r158258149 --- Diff: exec/java-exec/src/main/codegen/data/Parser.tdd --- @@ -75,6 +72,26 @@ implementationFiles: [ "parserImpls.ftl" ] + + # List of methods for parsing extensions to "CREATE [OR REPLACE]" calls. + # Each must accept arguments "(SqlParserPos pos, boolean replace)". + createStatementParserMethods: [ --- End diff -- Calcites `Parser.jj` uses these lists to extend existing functionality when desired methods specified inside it. If we did not specify these lists, java class `Parser.java` could not be generated and build would fail with the error: ``` [ERROR] Failed to execute goal org.apache.drill.tools:drill-fmpp-maven-plugin:1.12.0-SNAPSHOT:generate (generate-fmpp) on project drill-java-exec: FMPP processing session failed. [ERROR] Caused by: freemarker.core.InvalidReferenceException: The following has evaluated to null or missing: [ERROR] ==> parser.createStatementParserMethods [in template "Parser.jj" at line 881, column 6] ``` ---
[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_r158258329
--- Diff:
contrib/storage-hbase/src/main/java/org/apache/drill/exec/store/hbase/HBaseSchemaFactory.java
---
@@ -72,7 +72,16 @@ public AbstractSchema getSubSchema(String name) {
@Override
public Table getTable(String name) {
HBaseScanSpec scanSpec = new HBaseScanSpec(name);
- return new DrillHBaseTable(schemaName, plugin, scanSpec);
+ try {
+return new DrillHBaseTable(schemaName, plugin, scanSpec);
+ } catch (Exception e) {
+// Calcite firstly is looking for a table in the default schema,
if a table was not found,
--- End diff --
Thanks, 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_r158260235
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/SqlConverter.java
---
@@ -470,34 +576,32 @@ public void disallowTemporaryTables() {
* @throws UserException if temporary tables usage is disallowed
*/
@Override
-public RelOptTableImpl getTable(final List names) {
- RelOptTableImpl temporaryTable = null;
-
- if (mightBeTemporaryTable(names, session.getDefaultSchemaPath(),
drillConfig)) {
-String temporaryTableName =
session.resolveTemporaryTableName(names.get(names.size() - 1));
-if (temporaryTableName != null) {
- List temporaryNames =
Lists.newArrayList(temporarySchema, temporaryTableName);
- temporaryTable = super.getTable(temporaryNames);
+public Prepare.PreparingTable getTable(final List names) {
+ String originalTableName =
session.getOriginalTableNameFromTemporaryTable(names.get(names.size() - 1));
+ if (originalTableName != null) {
+if (!allowTemporaryTables) {
+ throw UserException
+ .validationError()
+ .message("Temporary tables usage is disallowed. Used
temporary table name: [%s].", originalTableName)
+ .build(logger);
}
}
- if (temporaryTable != null) {
-if (allowTemporaryTables) {
- return temporaryTable;
+ // Fix for select from hbase table with schema name in query
(example: "SELECT col FROM hbase.t)
+ // from hbase schema (did "USE hbase" before).
--- End diff --
This change was made before the change, where we catch and log exceptions
from `HBaseSchema.getTable()` method. So yes, it is not needed now.
---
