[
https://issues.apache.org/jira/browse/DRILL-4525?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16481031#comment-16481031
]
ASF GitHub Bot commented on DRILL-4525:
---------------------------------------
asfgit closed pull request #1268: DRILL-4525: Allow SqlBetweenOperator to
accept LOWER_OPERAND and UPPER_OPERAND with different types
URL: https://github.com/apache/drill/pull/1268
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git
a/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillCalciteSqlBetweenOperatorWrapper.java
b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillCalciteSqlBetweenOperatorWrapper.java
new file mode 100644
index 0000000000..f112e57908
--- /dev/null
+++
b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillCalciteSqlBetweenOperatorWrapper.java
@@ -0,0 +1,76 @@
+/*
+ * 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.sql;
+
+import org.apache.calcite.sql.fun.SqlBetweenOperator;
+import org.apache.calcite.sql.SqlCallBinding;
+import org.apache.calcite.sql.SqlOperator;
+
+import org.apache.drill.common.types.TypeProtos;
+import org.apache.drill.exec.resolver.TypeCastRules;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * This class serves as a wrapper class for SqlBetweenOperator. The motivation
is to plug-in the return type inference and operand
+ * type check algorithms of Drill into Calcite's sql validation procedure.
+ *
+ * Except for the methods which are relevant to the return type inference and
operand type check algorithms, the wrapper
+ * simply forwards the method calls to the wrapped SqlOperator.
+ *
+ * Note that SqlBetweenOperator cannot be wrapped in {@link
DrillCalciteSqlOperatorWrapper}. The reason is when RexNode
+ * conversion is happening, StandardConvertletTable.convertBetween expects the
SqlOperator to be a subclass of SqlBetweenOperator.
+ */
+public class DrillCalciteSqlBetweenOperatorWrapper extends SqlBetweenOperator
implements DrillCalciteSqlWrapper {
+ private final SqlBetweenOperator operator;
+
+ public DrillCalciteSqlBetweenOperatorWrapper(SqlBetweenOperator
sqlBetweenOperator) {
+ super(sqlBetweenOperator.flag, sqlBetweenOperator.isNegated());
+ operator = sqlBetweenOperator;
+ }
+
+ @Override
+ public SqlOperator getOperator() {
+ return operator;
+ }
+
+ /**
+ * Since Calcite has its rule for type compatibility
+ * (see {@link
org.apache.calcite.sql.type.SqlTypeUtil#isComparable(org.apache.calcite.rel.type.RelDataType,
+ * org.apache.calcite.rel.type.RelDataType)}), which is usually different
from Drill's, this method is overridden here to avoid
+ * Calcite early terminating the queries.
+ */
+ @Override
+ public boolean checkOperandTypes(SqlCallBinding callBinding, boolean
throwOnFailure) {
+ final List<TypeProtos.MinorType> types = new ArrayList<>();
+ for (int i = 0; i < callBinding.getOperandCount(); i++) {
+ final TypeProtos.MinorType inMinorType =
TypeInferenceUtils.getDrillTypeFromCalciteType(callBinding.getOperandType(i));
+ if (inMinorType == TypeProtos.MinorType.LATE) {
+ return true;
+ }
+ types.add(inMinorType);
+ }
+
+ final boolean isCompatible = TypeCastRules.getLeastRestrictiveType(types)
!= null;
+ if (!isCompatible && throwOnFailure) {
+ throw callBinding.newValidationSignatureError();
+ }
+ return isCompatible;
+ }
+}
diff --git
a/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillOperatorTable.java
b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillOperatorTable.java
index cf858d3e27..aea73d80db 100644
---
a/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillOperatorTable.java
+++
b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillOperatorTable.java
@@ -22,6 +22,7 @@
import com.google.common.collect.Maps;
import org.apache.calcite.sql.SqlAggFunction;
import org.apache.calcite.sql.SqlFunction;
+import org.apache.calcite.sql.fun.SqlBetweenOperator;
import org.apache.drill.common.expression.FunctionCallFactory;
import org.apache.drill.exec.expr.fn.DrillFuncHolder;
import org.apache.drill.exec.expr.fn.FunctionImplementationRegistry;
@@ -43,7 +44,6 @@
* {@link #inner SqlStdOperatorTable}, and Drill User Defined Functions.
*/
public class DrillOperatorTable extends SqlStdOperatorTable {
-// private static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(DrillOperatorTable.class);
private static final SqlOperatorTable inner = SqlStdOperatorTable.instance();
private final List<SqlOperator> calciteOperators = Lists.newArrayList();
private final List<SqlOperator> drillOperatorsWithoutInference =
Lists.newArrayList();
@@ -101,8 +101,8 @@ public void addOperatorWithInference(String name,
SqlOperator op) {
@Override
public void lookupOperatorOverloads(SqlIdentifier opName,
SqlFunctionCategory category,
- SqlSyntax syntax, List<SqlOperator> operatorList) {
- if(isInferenceEnabled()) {
+ SqlSyntax syntax, List<SqlOperator>
operatorList) {
+ if (isInferenceEnabled()) {
populateFromTypeInference(opName, category, syntax, operatorList);
} else {
populateFromWithoutTypeInference(opName, category, syntax, operatorList);
@@ -110,12 +110,12 @@ public void lookupOperatorOverloads(SqlIdentifier opName,
SqlFunctionCategory ca
}
private void populateFromTypeInference(SqlIdentifier opName,
SqlFunctionCategory category,
- SqlSyntax syntax, List<SqlOperator> operatorList) {
+ SqlSyntax syntax, List<SqlOperator>
operatorList) {
final List<SqlOperator> calciteOperatorList = Lists.newArrayList();
inner.lookupOperatorOverloads(opName, category, syntax,
calciteOperatorList);
- if(!calciteOperatorList.isEmpty()) {
- for(SqlOperator calciteOperator : calciteOperatorList) {
- if(calciteToWrapper.containsKey(calciteOperator)) {
+ if (!calciteOperatorList.isEmpty()) {
+ for (SqlOperator calciteOperator : calciteOperatorList) {
+ if (calciteToWrapper.containsKey(calciteOperator)) {
operatorList.add(calciteToWrapper.get(calciteOperator));
} else {
operatorList.add(calciteOperator);
@@ -133,7 +133,7 @@ private void populateFromTypeInference(SqlIdentifier
opName, SqlFunctionCategory
}
private void populateFromWithoutTypeInference(SqlIdentifier opName,
SqlFunctionCategory category,
- SqlSyntax syntax, List<SqlOperator> operatorList) {
+ SqlSyntax syntax,
List<SqlOperator> operatorList) {
inner.lookupOperatorOverloads(opName, category, syntax, operatorList);
if (operatorList.isEmpty() && (syntax == SqlSyntax.FUNCTION || syntax ==
SqlSyntax.FUNCTION_ID) && opName.isSimple()) {
List<SqlOperator> drillOps =
drillOperatorsWithoutInferenceMap.get(opName.getSimple().toLowerCase());
@@ -147,7 +147,7 @@ private void populateFromWithoutTypeInference(SqlIdentifier
opName, SqlFunctionC
public List<SqlOperator> getOperatorList() {
final List<SqlOperator> sqlOperators = Lists.newArrayList();
sqlOperators.addAll(calciteOperators);
- if(isInferenceEnabled()) {
+ if (isInferenceEnabled()) {
sqlOperators.addAll(drillOperatorsWithInference);
} else {
sqlOperators.addAll(drillOperatorsWithoutInference);
@@ -158,7 +158,7 @@ private void populateFromWithoutTypeInference(SqlIdentifier
opName, SqlFunctionC
// Get the list of SqlOperator's with the given name.
public List<SqlOperator> getSqlOperator(String name) {
- if(isInferenceEnabled()) {
+ if (isInferenceEnabled()) {
return drillOperatorsWithInferenceMap.get(name.toLowerCase());
} else {
return drillOperatorsWithoutInferenceMap.get(name.toLowerCase());
@@ -166,18 +166,31 @@ private void
populateFromWithoutTypeInference(SqlIdentifier opName, SqlFunctionC
}
private void populateWrappedCalciteOperators() {
- for(SqlOperator calciteOperator : inner.getOperatorList()) {
+ for (SqlOperator calciteOperator : inner.getOperatorList()) {
final SqlOperator wrapper;
- if(calciteOperator instanceof SqlAggFunction) {
+ if (calciteOperator instanceof SqlAggFunction) {
wrapper = new DrillCalciteSqlAggFunctionWrapper((SqlAggFunction)
calciteOperator,
getFunctionListWithInference(calciteOperator.getName()));
- } else if(calciteOperator instanceof SqlFunction) {
+ } else if (calciteOperator instanceof SqlFunction) {
wrapper = new DrillCalciteSqlFunctionWrapper((SqlFunction)
calciteOperator,
getFunctionListWithInference(calciteOperator.getName()));
+ } else if (calciteOperator instanceof SqlBetweenOperator) {
+ // During the procedure of converting to RexNode,
+ // StandardConvertletTable.convertBetween expects the SqlOperator to
be a subclass of SqlBetweenOperator
+ final SqlBetweenOperator sqlBetweenOperator = (SqlBetweenOperator)
calciteOperator;
+ wrapper = new
DrillCalciteSqlBetweenOperatorWrapper(sqlBetweenOperator);
} else {
- final String drillOpName =
FunctionCallFactory.replaceOpWithFuncName(calciteOperator.getName());
+ final String drillOpName;
+ // For UNARY_MINUS (-) or UNARY_PLUS (+), we do not rename them as
function_add or function_subtract.
+ // Otherwise, Calcite will mix them up with binary operator subtract
(-) or add (+)
+ if (calciteOperator == SqlStdOperatorTable.UNARY_MINUS ||
calciteOperator == SqlStdOperatorTable.UNARY_PLUS) {
+ drillOpName = calciteOperator.getName();
+ } else {
+ drillOpName =
FunctionCallFactory.replaceOpWithFuncName(calciteOperator.getName());
+ }
+
final List<DrillFuncHolder> drillFuncHolders =
getFunctionListWithInference(drillOpName);
- if(drillFuncHolders.isEmpty() || calciteOperator ==
SqlStdOperatorTable.UNARY_MINUS || calciteOperator ==
SqlStdOperatorTable.UNARY_PLUS) {
+ if (drillFuncHolders.isEmpty()) {
continue;
}
@@ -189,17 +202,17 @@ private void populateWrappedCalciteOperators() {
private List<DrillFuncHolder> getFunctionListWithInference(String name) {
final List<DrillFuncHolder> functions = Lists.newArrayList();
- for(SqlOperator sqlOperator :
drillOperatorsWithInferenceMap.get(name.toLowerCase())) {
- if(sqlOperator instanceof DrillSqlOperator) {
+ for (SqlOperator sqlOperator :
drillOperatorsWithInferenceMap.get(name.toLowerCase())) {
+ if (sqlOperator instanceof DrillSqlOperator) {
final List<DrillFuncHolder> list = ((DrillSqlOperator)
sqlOperator).getFunctions();
- if(list != null) {
+ if (list != null) {
functions.addAll(list);
}
}
- if(sqlOperator instanceof DrillSqlAggOperator) {
+ if (sqlOperator instanceof DrillSqlAggOperator) {
final List<DrillFuncHolder> list = ((DrillSqlAggOperator)
sqlOperator).getFunctions();
- if(list != null) {
+ if (list != null) {
functions.addAll(list);
}
}
diff --git
a/exec/java-exec/src/test/java/org/apache/drill/TestFunctionsWithTypeExpoQueries.java
b/exec/java-exec/src/test/java/org/apache/drill/TestFunctionsWithTypeExpoQueries.java
index 88558ae000..cdb7d42d5c 100644
---
a/exec/java-exec/src/test/java/org/apache/drill/TestFunctionsWithTypeExpoQueries.java
+++
b/exec/java-exec/src/test/java/org/apache/drill/TestFunctionsWithTypeExpoQueries.java
@@ -28,6 +28,7 @@
import org.junit.experimental.categories.Category;
import java.nio.file.Paths;
+import java.util.ArrayList;
import java.util.List;
@Category(SqlFunctionTest.class)
@@ -44,9 +45,9 @@ public void testConcatWithMoreThanTwoArgs() throws Exception {
List<Pair<SchemaPath, TypeProtos.MajorType>> expectedSchema =
Lists.newArrayList();
TypeProtos.MajorType majorType = TypeProtos.MajorType.newBuilder()
- .setMinorType(TypeProtos.MinorType.VARCHAR)
- .setMode(TypeProtos.DataMode.REQUIRED)
- .build();
+ .setMinorType(TypeProtos.MinorType.VARCHAR)
+ .setMode(TypeProtos.DataMode.REQUIRED)
+ .build();
expectedSchema.add(Pair.of(SchemaPath.getSimplePath("col"), majorType));
testBuilder()
@@ -125,9 +126,9 @@ public void testTrim() throws Exception {
List<Pair<SchemaPath, TypeProtos.MajorType>> expectedSchema =
Lists.newArrayList();
TypeProtos.MajorType majorType = TypeProtos.MajorType.newBuilder()
- .setMinorType(TypeProtos.MinorType.VARCHAR)
- .setMode(TypeProtos.DataMode.REQUIRED)
- .build();
+ .setMinorType(TypeProtos.MinorType.VARCHAR)
+ .setMode(TypeProtos.DataMode.REQUIRED)
+ .build();
expectedSchema.add(Pair.of(SchemaPath.getSimplePath("col"), majorType));
testBuilder()
@@ -397,9 +398,9 @@ public void testEqualBetweenIntervalAndTimestampDiff()
throws Exception {
final List<Pair<SchemaPath, TypeProtos.MajorType>> expectedSchema =
Lists.newArrayList();
final TypeProtos.MajorType majorType = TypeProtos.MajorType.newBuilder()
- .setMinorType(TypeProtos.MinorType.TIMESTAMP)
- .setMode(TypeProtos.DataMode.REQUIRED)
- .build();
+ .setMinorType(TypeProtos.MinorType.TIMESTAMP)
+ .setMode(TypeProtos.DataMode.REQUIRED)
+ .build();
expectedSchema.add(Pair.of(SchemaPath.getSimplePath("col"), majorType));
testBuilder()
@@ -530,19 +531,19 @@ public void testWindowSumAvg() throws Exception {
final List<Pair<SchemaPath, TypeProtos.MajorType>> expectedSchema =
Lists.newArrayList();
final TypeProtos.MajorType majorType1 = TypeProtos.MajorType.newBuilder()
- .setMinorType(TypeProtos.MinorType.BIGINT)
- .setMode(TypeProtos.DataMode.OPTIONAL)
- .build();
+ .setMinorType(TypeProtos.MinorType.BIGINT)
+ .setMode(TypeProtos.DataMode.OPTIONAL)
+ .build();
final TypeProtos.MajorType majorType2 = TypeProtos.MajorType.newBuilder()
- .setMinorType(TypeProtos.MinorType.FLOAT8)
- .setMode(TypeProtos.DataMode.OPTIONAL)
- .build();
+ .setMinorType(TypeProtos.MinorType.FLOAT8)
+ .setMode(TypeProtos.DataMode.OPTIONAL)
+ .build();
final TypeProtos.MajorType majorType3 = TypeProtos.MajorType.newBuilder()
- .setMinorType(TypeProtos.MinorType.BIGINT)
- .setMode(TypeProtos.DataMode.REQUIRED)
- .build();
+ .setMinorType(TypeProtos.MinorType.BIGINT)
+ .setMode(TypeProtos.DataMode.REQUIRED)
+ .build();
expectedSchema.add(Pair.of(SchemaPath.getSimplePath("col1"), majorType1));
expectedSchema.add(Pair.of(SchemaPath.getSimplePath("col2"), majorType2));
expectedSchema.add(Pair.of(SchemaPath.getSimplePath("col3"), majorType3));
@@ -724,4 +725,48 @@ public void testWindowSumConstant() throws Exception {
final String[] excludedPlan = {};
PlanTestBase.testPlanMatchingPatterns(query, expectedPlan, excludedPlan);
}
+
+ @Test // DRILL-4525
+ public void testBetweenDateAndTimeStamp() throws Exception {
+ final String query = "select count(*) as col \n" +
+ "from cp.`employee.json` \n" +
+ "where cast(birth_date as DATE) BETWEEN cast('1970-01-01' AS DATE) AND
(cast('1999-01-01' AS DATE) + INTERVAL '60' day) \n" +
+ "limit 0";
+
+ final TypeProtos.MajorType majorType = TypeProtos.MajorType.newBuilder()
+ .setMinorType(TypeProtos.MinorType.BIGINT)
+ .setMode(TypeProtos.DataMode.REQUIRED)
+ .build();
+
+ final List<Pair<SchemaPath, TypeProtos.MajorType>> expectedSchema = new
ArrayList<>();
+ expectedSchema.add(Pair.of(SchemaPath.getSimplePath("col"), majorType));
+
+ testBuilder()
+ .sqlQuery(query)
+ .schemaBaseLine(expectedSchema)
+ .build()
+ .run();
+ }
+
+ @Test // DRILL-4525
+ public void testBetweenDecimalAndDouble() throws Exception {
+ final String query = "select cast(r_regionkey as Integer) as col \n" +
+ "from cp.`tpch/region.parquet` \n" +
+ "where cast(r_regionkey as double) BETWEEN 1.1 AND 4.5 \n" +
+ "limit 0";
+
+ final TypeProtos.MajorType majorType = TypeProtos.MajorType.newBuilder()
+ .setMinorType(TypeProtos.MinorType.INT)
+ .setMode(TypeProtos.DataMode.REQUIRED)
+ .build();
+
+ final List<Pair<SchemaPath, TypeProtos.MajorType>> expectedSchema = new
ArrayList<>();
+ expectedSchema.add(Pair.of(SchemaPath.getSimplePath("col"), majorType));
+
+ testBuilder()
+ .sqlQuery(query)
+ .schemaBaseLine(expectedSchema)
+ .build()
+ .run();
+ }
}
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
> Query with BETWEEN clause on Date and Timestamp values fails with Validation
> Error
> ----------------------------------------------------------------------------------
>
> Key: DRILL-4525
> URL: https://issues.apache.org/jira/browse/DRILL-4525
> Project: Apache Drill
> Issue Type: Improvement
> Components: Query Planning & Optimization
> Reporter: Abhishek Girish
> Assignee: Bohdan Kazydub
> Priority: Critical
> Labels: ready-to-commit
> Fix For: 1.14.0
>
>
> Query: (simplified variant of TPC-DS Query37)
> {code}
> SELECT
> *
> FROM
> date_dim
> WHERE
> d_date BETWEEN Cast('1999-03-06' AS DATE) AND (
> Cast('1999-03-06' AS DATE) + INTERVAL '60' day)
> LIMIT 10;
> {code}
> Error:
> {code}
> Error: VALIDATION ERROR: From line 6, column 8 to line 7, column 64: Cannot
> apply 'BETWEEN ASYMMETRIC' to arguments of type '<ANY> BETWEEN ASYMMETRIC
> <DATE> AND <TIMESTAMP(0)>'. Supported form(s): '<COMPARABLE_TYPE> BETWEEN
> <COMPARABLE_TYPE> AND <COMPARABLE_TYPE>'
> SQL Query null
> [Error Id: 223fb37c-f561-4a37-9283-871dc6f4d6d0 on abhi2:31010]
> (state=,code=0)
> {code}
> This is a regression from 1.6.0.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
