Repository: calcite Updated Branches: refs/heads/master 6cad2ee13 -> af3e35d64
[CALCITE-2271] Join of two views with window aggregates produces incorrect results or NPE Avoid NPE in BlockBuilder.append when empty variable initializer is used closes #673 Project: http://git-wip-us.apache.org/repos/asf/calcite/repo Commit: http://git-wip-us.apache.org/repos/asf/calcite/commit/af3e35d6 Tree: http://git-wip-us.apache.org/repos/asf/calcite/tree/af3e35d6 Diff: http://git-wip-us.apache.org/repos/asf/calcite/diff/af3e35d6 Branch: refs/heads/master Commit: af3e35d64a7c29dfaa451c4ab4880424a8fe8cee Parents: 6cad2ee Author: Vladimir Sitnikov <[email protected]> Authored: Sun Aug 26 13:28:31 2018 +0300 Committer: Vladimir Sitnikov <[email protected]> Committed: Tue Aug 28 10:05:36 2018 +0300 ---------------------------------------------------------------------- .../enumerable/EnumerableRelImplementor.java | 1 - .../adapter/enumerable/EnumerableWindow.java | 5 +- core/src/test/resources/sql/winagg.iq | 57 +++++++++++++++++++- .../calcite/linq4j/tree/BlockBuilder.java | 37 +++++++++---- .../apache/calcite/linq4j/tree/Expressions.java | 3 ++ .../calcite/linq4j/test/BlockBuilderTest.java | 46 ++++++++++++++++ .../calcite/linq4j/test/ExpressionTest.java | 3 +- 7 files changed, 136 insertions(+), 16 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/calcite/blob/af3e35d6/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableRelImplementor.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableRelImplementor.java b/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableRelImplementor.java index b331511..58f26e7 100644 --- a/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableRelImplementor.java +++ b/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableRelImplementor.java @@ -72,7 +72,6 @@ public class EnumerableRelImplementor extends JavaRelImplementor { new HashMap<>(); private final Map<Object, ParameterExpression> stashedParameters = new IdentityHashMap<>(); - int windowCount = 0; protected final Function1<String, RexToLixTranslator.InputGetter> allCorrelateVariables = this::getCorrelVariableGetter; http://git-wip-us.apache.org/repos/asf/calcite/blob/af3e35d6/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableWindow.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableWindow.java b/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableWindow.java index 110c724..6b55983 100644 --- a/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableWindow.java +++ b/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableWindow.java @@ -175,11 +175,10 @@ public class EnumerableWindow extends Window implements EnumerableRel { PhysType inputPhysType = result.physType; - final int w = implementor.windowCount++; ParameterExpression prevStart = - Expressions.parameter(int.class, builder.newName("prevStart" + w)); + Expressions.parameter(int.class, builder.newName("prevStart")); ParameterExpression prevEnd = - Expressions.parameter(int.class, builder.newName("prevEnd" + w)); + Expressions.parameter(int.class, builder.newName("prevEnd")); builder.add(Expressions.declare(0, prevStart, null)); builder.add(Expressions.declare(0, prevEnd, null)); http://git-wip-us.apache.org/repos/asf/calcite/blob/af3e35d6/core/src/test/resources/sql/winagg.iq ---------------------------------------------------------------------- diff --git a/core/src/test/resources/sql/winagg.iq b/core/src/test/resources/sql/winagg.iq index fbd0dde..ff0aadc 100644 --- a/core/src/test/resources/sql/winagg.iq +++ b/core/src/test/resources/sql/winagg.iq @@ -420,16 +420,69 @@ limit 5; +--------+-----+-----+ | deptno | AR | BR | +--------+-----+-----+ +| 10 | 110 | 100 | | 10 | 110 | 110 | | 10 | 110 | 110 | -| 10 | 110 | 110 | +| 10 | 110 | 150 | | 20 | 200 | 200 | -| 20 | 200 | | +--------+-----+-----+ (5 rows) !ok +select a."empid", a."deptno", a."commission", a.r as ar, b.r as br +from ( + select "empid", "deptno", "commission", first_value("empid") over w as r + from "hr"."emps" + window w as (partition by "deptno" order by "commission")) a +join ( + select "empid", "deptno", "commission", last_value("empid") over w as r + from "hr"."emps" + window w as (partition by "deptno" order by "commission")) b +on a."empid" = b."empid" +limit 5; + ++-------+--------+------------+-----+-----+ +| empid | deptno | commission | AR | BR | ++-------+--------+------------+-----+-----+ +| 100 | 10 | 1000 | 110 | 100 | +| 110 | 10 | 250 | 110 | 110 | +| 150 | 10 | | 110 | 150 | +| 200 | 20 | 500 | 200 | 200 | ++-------+--------+------------+-----+-----+ +(4 rows) + +!ok + +# [CALCITE-2271] Two windows under a JOIN 2 +select + t1.l, t1.key as key1, t2.key as key2 +from + ( + select + dense_rank() over (order by key) l, + key + from + unnest(map[1,1,2,2]) k + ) t1 + join + ( + select + dense_rank() over(order by key) l, + key + from + unnest(map[2,2]) k + ) t2 on (t1.l = t2.l and t1.key + 1 = t2.key); + ++---+------+------+ +| L | KEY1 | KEY2 | ++---+------+------+ +| 1 | 1 | 2 | ++---+------+------+ +(1 row) + +!ok + # NTH_VALUE select emp."ENAME", emp."DEPTNO", nth_value(emp."DEPTNO", 1) over() as "first_value", http://git-wip-us.apache.org/repos/asf/calcite/blob/af3e35d6/linq4j/src/main/java/org/apache/calcite/linq4j/tree/BlockBuilder.java ---------------------------------------------------------------------- diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/BlockBuilder.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/BlockBuilder.java index 653ae23..e59bc65 100644 --- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/BlockBuilder.java +++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/BlockBuilder.java @@ -121,10 +121,23 @@ public class BlockBuilder { } if (statement instanceof DeclarationStatement) { DeclarationStatement declaration = (DeclarationStatement) statement; - if (variables.contains(declaration.parameter.name)) { - Expression x = append( - newName(declaration.parameter.name, optimize), - declaration.initializer); + if (!variables.contains(declaration.parameter.name)) { + add(statement); + } else { + String newName = newName(declaration.parameter.name, optimize); + Expression x; + // When initializer is null, append(name, initializer) can't deduce expression type + if (declaration.initializer != null && isSafeForReuse(declaration)) { + x = append(newName, declaration.initializer); + } else { + ParameterExpression pe = Expressions.parameter( + declaration.parameter.type, newName); + DeclarationStatement newDeclaration = Expressions.declare( + declaration.modifiers, pe, declaration.initializer + ); + x = pe; + add(newDeclaration); + } statement = null; result = x; if (declaration.parameter != x) { @@ -132,8 +145,6 @@ public class BlockBuilder { // declaration was present in BlockBuilder replacements.put(declaration.parameter, x); } - } else { - add(statement); } } else { add(statement); @@ -237,7 +248,7 @@ public class BlockBuilder { } protected boolean isSafeForReuse(DeclarationStatement decl) { - return (decl.modifiers & Modifier.FINAL) != 0; + return (decl.modifiers & Modifier.FINAL) != 0 && !decl.parameter.name.startsWith("_"); } protected void addExpressionForReuse(DeclarationStatement decl) { @@ -340,7 +351,7 @@ public class BlockBuilder { } final Map<ParameterExpression, Expression> subMap = new IdentityHashMap<>(useCounter.map.size()); - final SubstituteVariableVisitor visitor = new SubstituteVariableVisitor( + final Shuttle visitor = new InlineVariableVisitor( subMap); final ArrayList<Statement> oldStatements = new ArrayList<>(statements); statements.clear(); @@ -493,7 +504,7 @@ public class BlockBuilder { /** Substitute Variable Visitor. */ private static class SubstituteVariableVisitor extends Shuttle { - private final Map<ParameterExpression, Expression> map; + protected final Map<ParameterExpression, Expression> map; private final Map<ParameterExpression, Boolean> actives = new IdentityHashMap<>(); @@ -519,6 +530,14 @@ public class BlockBuilder { } return super.visit(parameterExpression); } + } + + /** Inline Variable Visitor. */ + private static class InlineVariableVisitor extends SubstituteVariableVisitor { + InlineVariableVisitor( + Map<ParameterExpression, Expression> map) { + super(map); + } @Override public Expression visit(UnaryExpression unaryExpression, Expression expression) { http://git-wip-us.apache.org/repos/asf/calcite/blob/af3e35d6/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java ---------------------------------------------------------------------- diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java index bf6ef79..0e4c3c2 100644 --- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java +++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java @@ -2940,6 +2940,9 @@ public abstract class Expressions { */ public static DeclarationStatement declare(int modifiers, String name, Expression initializer) { + assert initializer != null + : "empty initializer for variable declaration with name '" + name + "', modifiers " + + modifiers + ". Please use declare(int, ParameterExpression, initializer) instead"; return declare(modifiers, parameter(initializer.getType(), name), initializer); } http://git-wip-us.apache.org/repos/asf/calcite/blob/af3e35d6/linq4j/src/test/java/org/apache/calcite/linq4j/test/BlockBuilderTest.java ---------------------------------------------------------------------- diff --git a/linq4j/src/test/java/org/apache/calcite/linq4j/test/BlockBuilderTest.java b/linq4j/src/test/java/org/apache/calcite/linq4j/test/BlockBuilderTest.java index 86c68c1..2886269 100644 --- a/linq4j/src/test/java/org/apache/calcite/linq4j/test/BlockBuilderTest.java +++ b/linq4j/src/test/java/org/apache/calcite/linq4j/test/BlockBuilderTest.java @@ -22,6 +22,7 @@ import org.apache.calcite.linq4j.tree.Expression; import org.apache.calcite.linq4j.tree.ExpressionType; import org.apache.calcite.linq4j.tree.Expressions; import org.apache.calcite.linq4j.tree.OptimizeShuttle; +import org.apache.calcite.linq4j.tree.ParameterExpression; import org.apache.calcite.linq4j.tree.Shuttle; import org.junit.Before; @@ -78,6 +79,51 @@ public class BlockBuilderTest { b.add(Expressions.return_(null, Expressions.add(ONE, TWO))); assertEquals("{\n return 4;\n}\n", b.toBlock().toString()); } + + private BlockBuilder appendBlockWithSameVariable( + Expression initializer1, Expression initializer2) { + BlockBuilder outer = new BlockBuilder(); + ParameterExpression outerX = Expressions.parameter(int.class, "x"); + outer.add(Expressions.declare(0, outerX, initializer1)); + outer.add(Expressions.statement(Expressions.assign(outerX, Expressions.constant(1)))); + + BlockBuilder inner = new BlockBuilder(); + ParameterExpression innerX = Expressions.parameter(int.class, "x"); + inner.add(Expressions.declare(0, innerX, initializer2)); + inner.add(Expressions.statement(Expressions.assign(innerX, Expressions.constant(42)))); + inner.add(Expressions.return_(null, innerX)); + outer.append("x", inner.toBlock()); + return outer; + } + + @Test public void testRenameVariablesWithEmptyInitializer() { + BlockBuilder outer = appendBlockWithSameVariable(null, null); + + assertEquals("x in the second block should be renamed to avoid name clash", + "{\n" + + " int x;\n" + + " x = 1;\n" + + " int x0;\n" + + " x0 = 42;\n" + + "}\n", + Expressions.toString(outer.toBlock())); + } + + @Test public void testRenameVariablesWithInitializer() { + BlockBuilder outer = appendBlockWithSameVariable( + Expressions.constant(7), Expressions.constant(8)); + + assertEquals("x in the second block should be renamed to avoid name clash", + "{\n" + + " int x = 7;\n" + + " x = 1;\n" + + " int x0 = 8;\n" + + " x0 = 42;\n" + + "}\n", + Expressions.toString(outer.toBlock())); + } + + } // End BlockBuilderTest.java http://git-wip-us.apache.org/repos/asf/calcite/blob/af3e35d6/linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java ---------------------------------------------------------------------- diff --git a/linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java b/linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java index f457906..6e77bd9 100644 --- a/linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java +++ b/linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java @@ -995,7 +995,8 @@ public class ExpressionTest { + " final int _b = 1 + 2;\n" + " final int _c = 1 + 3;\n" + " final int _d = 1 + 4;\n" - + " org.apache.calcite.linq4j.test.ExpressionTest.bar(1, _b, _c, _d, org.apache.calcite.linq4j.test.ExpressionTest.foo(_c));\n" + + " final int _b0 = 1 + 3;\n" + + " org.apache.calcite.linq4j.test.ExpressionTest.bar(1, _b, _c, _d, org.apache.calcite.linq4j.test.ExpressionTest.foo(_b0));\n" + "}\n", Expressions.toString(expression)); expression.accept(new Shuttle());
