diff --git a/core/src/main/java/org/apache/calcite/sql2rel/AggConverter.java b/core/src/main/java/org/apache/calcite/sql2rel/AggConverter.java index 6193c33df4ee..65915b69e197 100644 --- a/core/src/main/java/org/apache/calcite/sql2rel/AggConverter.java +++ b/core/src/main/java/org/apache/calcite/sql2rel/AggConverter.java @@ -597,7 +597,7 @@ private int lookupOrCreateGroupExpr(RexNode expr) { /** * If an expression is structurally identical to one of the group-by * expressions, returns a reference to the expression, otherwise returns - * null. + * -1. */ int lookupGroupExpr(SqlNode expr) { return SqlUtil.indexOfDeep(groupExprs, expr, Litmus.IGNORE); 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 00bebe369f17..571d6abc5021 100644 --- a/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java +++ b/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java @@ -69,7 +69,6 @@ import org.apache.calcite.rel.logical.LogicalValues; import org.apache.calcite.rel.metadata.RelColumnMapping; import org.apache.calcite.rel.metadata.RelMetadataQuery; -import org.apache.calcite.rel.rel2sql.SqlImplementor; import org.apache.calcite.rel.stream.Delta; import org.apache.calcite.rel.stream.LogicalDelta; import org.apache.calcite.rel.type.RelDataType; @@ -1168,15 +1167,7 @@ private void replaceSubQueries( final Blackboard bb, final SqlNode expr, RelOptUtil.Logic logic) { - replaceSubQueries(bb, expr, logic, null); - } - - private void replaceSubQueries( - final Blackboard bb, - final SqlNode expr, - RelOptUtil.Logic logic, - final SqlImplementor.@Nullable Clause clause) { - findSubQueries(bb, expr, logic, false, clause); + findSubQueries(bb, expr, logic, false); for (SubQuery node : bb.subQueryList) { substituteSubQuery(bb, node); } @@ -2044,14 +2035,17 @@ private static boolean isRowConstructor(SqlNode node) { * corresponds to a variation of a select * node, only register it if it's a scalar * sub-query - * @param clause A clause inside which sub-query is searched */ private void findSubQueries( Blackboard bb, SqlNode node, RelOptUtil.Logic logic, - boolean registerOnlyScalarSubQueries, - SqlImplementor.@Nullable Clause clause) { + boolean registerOnlyScalarSubQueries) { + // If node is structurally identical to one of the group-by, + // then it is not a sub-query; it is a reference. + if (bb.agg != null && bb.agg.lookupGroupExpr(node) != -1) { + return; + } final SqlKind kind = node.getKind(); switch (kind) { case EXISTS: @@ -2066,7 +2060,7 @@ private void findSubQueries( case SCALAR_QUERY: if (!registerOnlyScalarSubQueries || (kind == SqlKind.SCALAR_QUERY)) { - bb.registerSubQuery(node, RelOptUtil.Logic.TRUE_FALSE, clause); + bb.registerSubQuery(node, RelOptUtil.Logic.TRUE_FALSE); } return; case IN: @@ -2098,7 +2092,7 @@ private void findSubQueries( findSubQueries(bb, operand, logic, kind == SqlKind.IN || kind == SqlKind.NOT_IN || kind == SqlKind.SOME || kind == SqlKind.ALL - || registerOnlyScalarSubQueries, clause); + || registerOnlyScalarSubQueries); } } } else if (node instanceof SqlNodeList) { @@ -2106,7 +2100,7 @@ private void findSubQueries( findSubQueries(bb, child, logic, kind == SqlKind.IN || kind == SqlKind.NOT_IN || kind == SqlKind.SOME || kind == SqlKind.ALL - || registerOnlyScalarSubQueries, clause); + || registerOnlyScalarSubQueries); } } @@ -2140,13 +2134,11 @@ private void findSubQueries( && ((SqlQuantifyOperator) ((SqlCall) node).getOperator()) .tryDeriveTypeForCollection(bb.getValidator(), bb.scope, (SqlCall) node) != null) { - findSubQueries(bb, ((SqlCall) node).operand(0), logic, registerOnlyScalarSubQueries, - clause); - findSubQueries(bb, ((SqlCall) node).operand(1), logic, registerOnlyScalarSubQueries, - clause); + findSubQueries(bb, ((SqlCall) node).operand(0), logic, registerOnlyScalarSubQueries); + findSubQueries(bb, ((SqlCall) node).operand(1), logic, registerOnlyScalarSubQueries); break; } - bb.registerSubQuery(node, logic, clause); + bb.registerSubQuery(node, logic); break; default: break; @@ -3523,13 +3515,13 @@ private void createAggImpl(Blackboard bb, // also replace sub-queries inside ordering spec in the aggregates replaceSubQueries(bb, aggregateFinder.orderList, RelOptUtil.Logic.TRUE_FALSE_UNKNOWN); + // If group-by clause is missing, pretend that it has zero elements. if (groupList == null) { groupList = SqlNodeList.EMPTY; } - replaceSubQueries(bb, groupList, RelOptUtil.Logic.TRUE_FALSE_UNKNOWN, - SqlImplementor.Clause.GROUP_BY); + replaceSubQueries(bb, groupList, RelOptUtil.Logic.TRUE_FALSE_UNKNOWN); // register the group exprs @@ -3633,8 +3625,7 @@ private void createAggImpl(Blackboard bb, // This needs to be done separately from the sub-query inside // any aggregate in the select list, and after the aggregate rel // is allocated. - replaceSubQueries(bb, selectList, RelOptUtil.Logic.TRUE_FALSE_UNKNOWN, - SqlImplementor.Clause.SELECT); + replaceSubQueries(bb, selectList, RelOptUtil.Logic.TRUE_FALSE_UNKNOWN); // Now sub-queries in the entire select list have been converted. // Convert the select expressions to get the final list to be @@ -5534,30 +5525,26 @@ public void flatten( } } - void registerSubQuery(SqlNode node, RelOptUtil.Logic logic, - SqlImplementor.@Nullable Clause clause) { - if (getSubQuery(node, clause) == null) { - subQueryList.add(new SubQuery(node, logic, clause)); + void registerSubQuery(SqlNode node, RelOptUtil.Logic logic) { + for (SubQuery subQuery : subQueryList) { + // Compare the reference to make sure the matched node has + // exact scope where it belongs. + if (node == subQuery.node) { + return; + } } + subQueryList.add(new SubQuery(node, logic)); } - @Nullable SubQuery getSubQuery(SqlNode expr, SqlImplementor.@Nullable Clause exprClause) { + @Nullable SubQuery getSubQuery(SqlNode expr) { for (SubQuery subQuery : subQueryList) { // Compare the reference to make sure the matched node has // exact scope where it belongs. if (expr == subQuery.node) { return subQuery; } - - // Reference comparing does not work in case when select list has column which refers - // to the column inside `GROUP BY` clause. - // For example: SELECT deptno IN (1,2) FROM emp.deptno GROUP BY deptno IN (1,2); - if (exprClause == SqlImplementor.Clause.SELECT - && subQuery.clause == SqlImplementor.Clause.GROUP_BY - && expr.equalsDeep(subQuery.node, Litmus.IGNORE)) { - return subQuery; - } } + return null; } @@ -5732,7 +5719,7 @@ ImmutableList retrieveCursors() { case CURSOR: case IN: case NOT_IN: - subQuery = getSubQuery(expr, null); + subQuery = getSubQuery(expr); if (subQuery == null && (kind == SqlKind.SOME || kind == SqlKind.ALL)) { break; } @@ -5748,7 +5735,7 @@ ImmutableList retrieveCursors() { case ARRAY_QUERY_CONSTRUCTOR: case MAP_QUERY_CONSTRUCTOR: case MULTISET_QUERY_CONSTRUCTOR: - subQuery = requireNonNull(getSubQuery(expr, null)); + subQuery = requireNonNull(getSubQuery(expr)); rex = requireNonNull(subQuery.expr); if (((kind == SqlKind.SCALAR_QUERY) @@ -5934,7 +5921,7 @@ private boolean isConvertedSubq(RexNode rex) { } @Override public RexRangeRef getSubQueryExpr(SqlCall call) { - final SubQuery subQuery = requireNonNull(getSubQuery(call, null)); + final SubQuery subQuery = requireNonNull(getSubQuery(call)); return (RexRangeRef) requireNonNull(subQuery.expr, () -> "subQuery.expr for " + call); } @@ -6277,13 +6264,10 @@ private static class SubQuery { final SqlNode node; final RelOptUtil.Logic logic; @Nullable RexNode expr; - final SqlImplementor.@Nullable Clause clause; - private SubQuery(SqlNode node, RelOptUtil.Logic logic, - SqlImplementor.@Nullable Clause clause) { + private SubQuery(SqlNode node, RelOptUtil.Logic logic) { this.node = node; this.logic = logic; - this.clause = clause; } } diff --git a/core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java b/core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java index 0e2e835846f0..19aba36c1742 100644 --- a/core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java +++ b/core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java @@ -523,6 +523,18 @@ public static void checkActualAndReferenceFiles() { sql(sql).ok(); } + /** Test case for + * [CALCITE-4549] + * IndexOutOfBoundsException when group view by a sub query. */ + @Test void testGroupView() { + final String sql = "SELECT " + + "case when ENAME in( 'a', 'b') then 'c' else 'd' end " + + "from EMP_20 " + + "group by " + + "case when ENAME in( 'a', 'b') then 'c' else 'd' end "; + sql(sql).ok(); + } + @Test void testGroupExpressionsInsideAndOut() { // Expressions inside and outside aggs. Common sub-expressions should be // eliminated: 'sal' always translates to expression #2. diff --git a/core/src/test/resources/org/apache/calcite/test/SqlToRelConverterTest.xml b/core/src/test/resources/org/apache/calcite/test/SqlToRelConverterTest.xml index 8fa923380954..549444311486 100644 --- a/core/src/test/resources/org/apache/calcite/test/SqlToRelConverterTest.xml +++ b/core/src/test/resources/org/apache/calcite/test/SqlToRelConverterTest.xml @@ -2560,6 +2560,19 @@ LogicalAggregate(group=[{0}], SUM_SAL=[SUM($1)]) + + + + + + ($5, 1000))]) + LogicalTableScan(table=[[CATALOG, SALES, EMP]]) +]]> + +