Skip to content

[CALCITE-4844] IN-list that references columns is wrongly converted to Values, and gives incorrect results #2594

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
NobiGo merged 1 commit into from
Nov 12, 2021
Merged

[CALCITE-4844] IN-list that references columns is wrongly converted to Values, and gives incorrect results #2594

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion core/src/main/java/org/apache/calcite/prepare/Prepare.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@
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;

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -232,6 +237,7 @@ public PreparedResult prepareSql(
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);
Expand Down Expand Up @@ -369,7 +375,8 @@ protected abstract RelNode decorrelate(SqlToRelConverter sqlToRelConverter,
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();
Expand Down
42 changes: 40 additions & 2 deletions core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1141,8 +1141,9 @@ private void substituteSubQuery(Blackboard bb, SubQuery subQuery) {

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,
Expand Down Expand Up @@ -6114,6 +6115,43 @@ private SubQuery(SqlNode node, RelOptUtil.Logic logic) {
}
}

/**
* 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.
*/
Expand Down
49 changes: 49 additions & 0 deletions core/src/test/resources/sql/sub-query.iq
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
5 changes: 5 additions & 0 deletions testkit/src/main/java/org/apache/calcite/test/QuidemTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
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;
Expand Down Expand Up @@ -150,6 +151,10 @@ protected void checkRun(String path) throws Exception {
&& (Boolean) value;
closer.add(Prepare.THREAD_EXPAND.push(b));
}
if (propertyName.equals("insubquerythreshold")) {
Copy link
Copy Markdown
Contributor

@zinking zinking Nov 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this temporary ? surprised code gen part follows naturely and requires no change.

Copy link
Copy Markdown
Contributor Author

@NobiGo NobiGo Nov 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. This can be used in the .iq file to set the insubquerythreshold property.

Copy link
Copy Markdown
Contributor Author

@NobiGo NobiGo Nov 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!set insubquerythreshold 0

Copy link
Copy Markdown
Contributor Author

@NobiGo NobiGo Nov 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zinking Please review again, Then we can merge.

int thresholdValue = ((BigDecimal) value).intValue();
closer.add(Prepare.THREAD_INSUBQUERY_THRESHOLD.push(thresholdValue));
}
})
.withEnv(QuidemTest::getEnv)
.build();
Expand Down