You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by xi...@apache.org on 2021/11/12 01:40:40 UTC
[calcite] branch master updated: [CALCITE-4844] IN-list that
references columns is wrongly converted to Values,
and gives incorrect results
This is an automated email from the ASF dual-hosted git repository.
xiong pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/calcite.git
The following commit(s) were added to refs/heads/master by this push:
new bcaa978 [CALCITE-4844] IN-list that references columns is wrongly converted to Values, and gives incorrect results
bcaa978 is described below
commit bcaa97871877f9dbbb1959f0531287de0f97417b
Author: NobiGo <no...@gmail.com>
AuthorDate: Sun Oct 10 09:11:41 2021 +0800
[CALCITE-4844] IN-list that references columns is wrongly converted to Values, and gives incorrect results
---
.../java/org/apache/calcite/prepare/Prepare.java | 9 +++-
.../apache/calcite/sql2rel/SqlToRelConverter.java | 42 ++++++++++++++++++-
core/src/test/resources/sql/sub-query.iq | 49 ++++++++++++++++++++++
.../java/org/apache/calcite/test/QuidemTest.java | 5 +++
4 files changed, 102 insertions(+), 3 deletions(-)
diff --git a/core/src/main/java/org/apache/calcite/prepare/Prepare.java b/core/src/main/java/org/apache/calcite/prepare/Prepare.java
index 0408823..e56281e 100644
--- a/core/src/main/java/org/apache/calcite/prepare/Prepare.java
+++ b/core/src/main/java/org/apache/calcite/prepare/Prepare.java
@@ -78,6 +78,7 @@ import java.util.Collections;
import java.util.List;
import static org.apache.calcite.linq4j.Nullness.castNonNull;
+import static org.apache.calcite.sql2rel.SqlToRelConverter.DEFAULT_IN_SUB_QUERY_THRESHOLD;
import static java.util.Objects.requireNonNull;
@@ -112,6 +113,10 @@ public abstract class Prepare {
public static final TryThreadLocal<@Nullable Boolean> THREAD_EXPAND =
TryThreadLocal.of(false);
+ // temporary. for testing.
+ public static final TryThreadLocal<@Nullable Integer> THREAD_INSUBQUERY_THRESHOLD =
+ TryThreadLocal.of(DEFAULT_IN_SUB_QUERY_THRESHOLD);
+
protected Prepare(CalcitePrepare.Context context, CatalogReader catalogReader,
Convention resultConvention) {
assert context != null;
@@ -232,6 +237,7 @@ public abstract class Prepare {
SqlToRelConverter.config()
.withTrimUnusedFields(true)
.withExpand(castNonNull(THREAD_EXPAND.get()))
+ .withInSubQueryThreshold(castNonNull(THREAD_INSUBQUERY_THRESHOLD.get()))
.withExplain(sqlQuery.getKind() == SqlKind.EXPLAIN);
final Holder<SqlToRelConverter.Config> configHolder = Holder.of(config);
Hook.SQL2REL_CONVERTER_CONFIG_BUILDER.run(configHolder);
@@ -369,7 +375,8 @@ public abstract class Prepare {
protected RelRoot trimUnusedFields(RelRoot root) {
final SqlToRelConverter.Config config = SqlToRelConverter.config()
.withTrimUnusedFields(shouldTrim(root.rel))
- .withExpand(castNonNull(THREAD_EXPAND.get()));
+ .withExpand(castNonNull(THREAD_EXPAND.get()))
+ .withInSubQueryThreshold(castNonNull(THREAD_INSUBQUERY_THRESHOLD.get()));
final SqlToRelConverter converter =
getSqlToRelConverter(getSqlValidator(), catalogReader, config);
final boolean ordered = !root.collation.getFieldCollations().isEmpty();
diff --git a/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java b/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
index edf9636..d28f218 100644
--- a/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
+++ b/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
@@ -1141,8 +1141,9 @@ public class SqlToRelConverter {
if (query instanceof SqlNodeList) {
SqlNodeList valueList = (SqlNodeList) query;
- if (valueList.size() < config.getInSubQueryThreshold()) {
- // We're under the threshold, so convert to OR.
+ // When the list size under the threshold or the list references columns, we convert to OR.
+ if (valueList.size() < config.getInSubQueryThreshold()
+ || valueList.accept(new SqlIdentifierFinder())) {
subQuery.expr =
convertInToOr(
bb,
@@ -6115,6 +6116,43 @@ public class SqlToRelConverter {
}
/**
+ * Visitor that looks for an SqlIdentifier inside a tree of
+ * {@link SqlNode} objects and return {@link Boolean#TRUE} when it finds
+ * one.
+ */
+ public static class SqlIdentifierFinder implements SqlVisitor<Boolean> {
+
+ @Override public Boolean visit(SqlCall sqlCall) {
+ return sqlCall.getOperandList().stream().anyMatch(sqlNode -> sqlNode.accept(this));
+ }
+
+ @Override public Boolean visit(SqlNodeList nodeList) {
+ return nodeList.stream().anyMatch(sqlNode -> sqlNode.accept(this));
+ }
+
+ @Override public Boolean visit(SqlIdentifier identifier) {
+ return true;
+ }
+
+ @Override public Boolean visit(SqlLiteral literal) {
+ return false;
+ }
+
+ @Override public Boolean visit(SqlDataTypeSpec type) {
+ return false;
+ }
+
+ @Override public Boolean visit(SqlDynamicParam param) {
+ return false;
+ }
+
+ @Override public Boolean visit(SqlIntervalQualifier intervalQualifier) {
+ return false;
+ }
+
+ }
+
+ /**
* Visitor that collects all aggregate functions in a {@link SqlNode} tree.
*/
private static class AggregateFinder extends SqlBasicVisitor<Void> {
diff --git a/core/src/test/resources/sql/sub-query.iq b/core/src/test/resources/sql/sub-query.iq
index 84084b6..be9e9dd 100644
--- a/core/src/test/resources/sql/sub-query.iq
+++ b/core/src/test/resources/sql/sub-query.iq
@@ -3257,4 +3257,53 @@ EnumerableCalc(expr#0..7=[{inputs}], expr#8=[null:BOOLEAN], proj#0..8=[{exprs}])
EnumerableTableScan(table=[[scott, EMP]])
!plan
+# [CALCITE-4844] IN-list that references columns is wrongly converted to Values, and gives incorrect results
+
+!set insubquerythreshold 0
+
+SELECT empno, ename, mgr FROM "scott".emp WHERE 7782 IN (empno, mgr);
++-------+--------+------+
+| EMPNO | ENAME | MGR |
++-------+--------+------+
+| 7782 | CLARK | 7839 |
+| 7934 | MILLER | 7782 |
++-------+--------+------+
+(2 rows)
+
+!ok
+
+EnumerableCalc(expr#0..7=[{inputs}], expr#8=[7782], expr#9=[CAST($t0):INTEGER NOT NULL], expr#10=[=($t8, $t9)], expr#11=[CAST($t3):INTEGER], expr#12=[=($t8, $t11)], expr#13=[OR($t10, $t12)], proj#0..1=[{exprs}], MGR=[$t3], $condition=[$t13])
+ EnumerableTableScan(table=[[scott, EMP]])
+!plan
+
+SELECT empno, ename, mgr FROM "scott".emp WHERE (7782, 7839) IN ((empno, mgr), (mgr, empno));
++-------+-------+------+
+| EMPNO | ENAME | MGR |
++-------+-------+------+
+| 7782 | CLARK | 7839 |
++-------+-------+------+
+(1 row)
+
+!ok
+
+EnumerableCalc(expr#0..7=[{inputs}], expr#8=[7782], expr#9=[CAST($t0):INTEGER NOT NULL], expr#10=[=($t8, $t9)], expr#11=[7839], expr#12=[CAST($t3):INTEGER], expr#13=[=($t11, $t12)], expr#14=[AND($t10, $t13)], expr#15=[=($t8, $t12)], expr#16=[=($t11, $t9)], expr#17=[AND($t15, $t16)], expr#18=[OR($t14, $t17)], proj#0..1=[{exprs}], MGR=[$t3], $condition=[$t18])
+ EnumerableTableScan(table=[[scott, EMP]])
+!plan
+
+SELECT empno, ename, mgr FROM "scott".emp WHERE (7782, 7839) IN ((empno, 7839), (7782, mgr));
++-------+-------+------+
+| EMPNO | ENAME | MGR |
++-------+-------+------+
+| 7566 | JONES | 7839 |
+| 7698 | BLAKE | 7839 |
+| 7782 | CLARK | 7839 |
++-------+-------+------+
+(3 rows)
+
+!ok
+
+EnumerableCalc(expr#0..7=[{inputs}], expr#8=[7782], expr#9=[CAST($t0):INTEGER NOT NULL], expr#10=[=($t8, $t9)], expr#11=[7839], expr#12=[CAST($t3):INTEGER], expr#13=[=($t11, $t12)], expr#14=[OR($t10, $t13)], proj#0..1=[{exprs}], MGR=[$t3], $condition=[$t14])
+ EnumerableTableScan(table=[[scott, EMP]])
+!plan
+
# End sub-query.iq
diff --git a/testkit/src/main/java/org/apache/calcite/test/QuidemTest.java b/testkit/src/main/java/org/apache/calcite/test/QuidemTest.java
index 9c8f42b..21699dc 100644
--- a/testkit/src/main/java/org/apache/calcite/test/QuidemTest.java
+++ b/testkit/src/main/java/org/apache/calcite/test/QuidemTest.java
@@ -48,6 +48,7 @@ import java.io.Reader;
import java.io.Writer;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
+import java.math.BigDecimal;
import java.net.URL;
import java.sql.Connection;
import java.sql.DriverManager;
@@ -150,6 +151,10 @@ public abstract class QuidemTest {
&& (Boolean) value;
closer.add(Prepare.THREAD_EXPAND.push(b));
}
+ if (propertyName.equals("insubquerythreshold")) {
+ int thresholdValue = ((BigDecimal) value).intValue();
+ closer.add(Prepare.THREAD_INSUBQUERY_THRESHOLD.push(thresholdValue));
+ }
})
.withEnv(QuidemTest::getEnv)
.build();