You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by vl...@apache.org on 2018/08/28 07:08:09 UTC
calcite git commit: [CALCITE-2271] Join of two views with window
aggregates produces incorrect results or NPE
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 <si...@gmail.com>
Authored: Sun Aug 26 13:28:31 2018 +0300
Committer: Vladimir Sitnikov <si...@gmail.com>
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());