From 2e32cd0f479138c83defb9a367198e47be5a7609 Mon Sep 17 00:00:00 2001 From: Vladimir Sitnikov Date: Thu, 26 Jun 2014 10:31:04 +0400 Subject: [PATCH 1/2] [OPTIQ-311] Wrong results when filtering the results of windowed aggregation Temporary fix that just disables pushing filters through window aggregates --- .../rel/rules/PushFilterPastProjectRule.java | 14 +++- .../net/hydromatic/optiq/test/JdbcTest.java | 64 +++++++++++++++++++ .../org/eigenbase/test/RelOptRulesTest.java | 8 +++ .../org/eigenbase/test/RelOptRulesTest.xml | 21 ++++++ 4 files changed, 104 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/org/eigenbase/rel/rules/PushFilterPastProjectRule.java b/core/src/main/java/org/eigenbase/rel/rules/PushFilterPastProjectRule.java index a98277ab2..5d61e3f26 100644 --- a/core/src/main/java/org/eigenbase/rel/rules/PushFilterPastProjectRule.java +++ b/core/src/main/java/org/eigenbase/rel/rules/PushFilterPastProjectRule.java @@ -36,9 +36,17 @@ public class PushFilterPastProjectRule extends RelOptRule { */ private PushFilterPastProjectRule() { super( - operand( - FilterRel.class, - operand(ProjectRel.class, any()))); + operand(FilterRel.class, + some(new RelOptRuleOperand(ProjectRel.class, null, any()) { + @Override + public boolean matches(RelNode rel) { + // Avoid pushing filters through windowed aggregates + // TODO: Implement real filter push that considers partition by + return super.matches(rel) + && !RexOver.containsOver(((ProjectRel) rel).getProjects(), + null); + } + }))); } //~ Methods ---------------------------------------------------------------- diff --git a/core/src/test/java/net/hydromatic/optiq/test/JdbcTest.java b/core/src/test/java/net/hydromatic/optiq/test/JdbcTest.java index 26f72bdec..9d5ba4d41 100644 --- a/core/src/test/java/net/hydromatic/optiq/test/JdbcTest.java +++ b/core/src/test/java/net/hydromatic/optiq/test/JdbcTest.java @@ -3028,6 +3028,70 @@ private static ImmutableMultimap x() { "M=10002.0"); } + /** + * Tests if optiq can push filters through WindowRel when filter matches + * partition by. + */ + @Test + public void testWinAggWithFilter() { + OptiqAssert.that() + .with(OptiqAssert.Config.REGULAR) + .query( + "select * from (select \"empid\", \"deptno\",\n" + + "count(*) over () c\n" + + "from \"hr\".\"emps\"\n" + + ") where \"deptno\"=10 and \"empid\"=100") + .explainContains( + "EnumerableCalcRel(expr#0..5=[{inputs}], expr#6=[CAST($t1):INTEGER NOT NULL], expr#7=[10], expr#8=[=($t6, $t7)], expr#9=[CAST($t0):INTEGER NOT NULL], expr#10=[100], expr#11=[=($t9, $t10)], expr#12=[AND($t8, $t11)], proj#0..1=[{exprs}], $2=[$t5], $condition=[$t12])\n" + + " EnumerableWindowRel(window#0=[window(partition {} order by [] range between UNBOUNDED PRECEDING and UNBOUNDED FOLLOWING aggs [COUNT()])])\n" + + " EnumerableTableAccessRel(table=[[hr, emps]])") + .returns("empid=100; deptno=10; C=4\n"); + // There are 4 employees in total + } + + /** + * Tests that optiq does not push filter through WindowRel when it is + * not allowed. + */ + @Test + public void testWinAggWithFilterPush() { + OptiqAssert.that() + .with(OptiqAssert.Config.REGULAR) + .query( + "select * from (select \"empid\", \"deptno\",\n" + + " count(*) over (partition by \"deptno\") c\n" + + "from \"hr\".\"emps\"\n" + + ") where \"deptno\"=10 and \"empid\"=100") + .explainContains( + "EnumerableCalcRel(expr#0..5=[{inputs}], expr#6=[CAST($t1):INTEGER NOT NULL], expr#7=[10], expr#8=[=($t6, $t7)], expr#9=[CAST($t0):INTEGER NOT NULL], expr#10=[100], expr#11=[=($t9, $t10)], expr#12=[AND($t8, $t11)], proj#0..1=[{exprs}], $2=[$t5], $condition=[$t12])\n" + + " EnumerableWindowRel(window#0=[window(partition {1} order by [] range between UNBOUNDED PRECEDING and UNBOUNDED FOLLOWING aggs [COUNT()])])\n" + + " EnumerableTableAccessRel(table=[[hr, emps]])") + .returns("empid=100; deptno=10; C=3\n"); + // There are 3 employees in department 10 + } + + /** + * Tests if optiq does not evaluate filters early as a part of input + * calculation for the window aggregate. + */ + @Test + public void testWinAggWithFilterCalcFirst() { + OptiqAssert.that() + .with(OptiqAssert.Config.REGULAR) + .query( + "select * from (select \"empid\", \"deptno\",\n" + + "count(*) over (partition by \"deptno\"*0) c\n" + + "from \"hr\".\"emps\"\n" + + ") where \"deptno\"=10 and \"empid\"=100") + .explainContains( + "EnumerableCalcRel(expr#0..3=[{inputs}], expr#4=[CAST($t1):INTEGER NOT NULL], expr#5=[10], expr#6=[=($t4, $t5)], expr#7=[CAST($t0):INTEGER NOT NULL], expr#8=[100], expr#9=[=($t7, $t8)], expr#10=[AND($t6, $t9)], proj#0..1=[{exprs}], $2=[$t3], $condition=[$t10])\n" + + " EnumerableWindowRel(window#0=[window(partition {2} order by [] range between UNBOUNDED PRECEDING and UNBOUNDED FOLLOWING aggs [COUNT()])])\n" + + " EnumerableCalcRel(expr#0..4=[{inputs}], expr#5=[0], expr#6=[*($t1, $t5)], proj#0..1=[{exprs}], $2=[$t6])\n" + + " EnumerableTableAccessRel(table=[[hr, emps]])") + .returns("empid=100; deptno=10; C=4\n"); + // There are 4 employees in total + } + /** Tests for RANK and ORDER BY ... DESCENDING, NULLS FIRST, NULLS LAST. */ @Test public void testWinAggRank() { OptiqAssert.that() diff --git a/core/src/test/java/org/eigenbase/test/RelOptRulesTest.java b/core/src/test/java/org/eigenbase/test/RelOptRulesTest.java index 8378563f9..e9bf4a0b7 100644 --- a/core/src/test/java/org/eigenbase/test/RelOptRulesTest.java +++ b/core/src/test/java/org/eigenbase/test/RelOptRulesTest.java @@ -95,6 +95,14 @@ protected DiffRepository getDiffRepos() { + " where d.name = 'Charlie'"); } + @Test public void testPushFilterThroughWindow() { + // TODO: implement true push filter through window + checkPlanning( + PushFilterPastProjectRule.INSTANCE, + "select * from (select e.ename, e.deptno+e.empno qq, row_number() over (partition by e.deptno+e.empno order by e.empno) r from sales.emp e) where qq=10" + ); + } + @Test public void testReduceAverage() { checkPlanning( ReduceAggregatesRule.INSTANCE, diff --git a/core/src/test/resources/org/eigenbase/test/RelOptRulesTest.xml b/core/src/test/resources/org/eigenbase/test/RelOptRulesTest.xml index 68ba466fc..4607d092d 100644 --- a/core/src/test/resources/org/eigenbase/test/RelOptRulesTest.xml +++ b/core/src/test/resources/org/eigenbase/test/RelOptRulesTest.xml @@ -89,6 +89,27 @@ ProjectRel(EXPR$0=[1]) FilterRel(condition=[=($1, 'Charlie')]) TableAccessRel(table=[[CATALOG, SALES, DEPT]]) TableAccessRel(table=[[CATALOG, SALES, EMP]]) +]]> + + + + + + + + + + + From fe7ecd2423bd6cb819d0191675d4a248f0b2712e Mon Sep 17 00:00:00 2001 From: Vladimir Sitnikov Date: Thu, 26 Jun 2014 20:25:52 +0400 Subject: [PATCH 2/2] [OPTIQ-312] Trim non-required fields before WindowRel --- .../org/eigenbase/rel/rules/CalcRelSplitter.java | 14 ++++++++------ .../java/net/hydromatic/optiq/test/JdbcTest.java | 10 ++++++---- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/core/src/main/java/org/eigenbase/rel/rules/CalcRelSplitter.java b/core/src/main/java/org/eigenbase/rel/rules/CalcRelSplitter.java index 9e26fe67c..d80eb8d39 100644 --- a/core/src/main/java/org/eigenbase/rel/rules/CalcRelSplitter.java +++ b/core/src/main/java/org/eigenbase/rel/rules/CalcRelSplitter.java @@ -86,6 +86,9 @@ public abstract class CalcRelSplitter { this.typeFactory = calc.getCluster().getTypeFactory(); this.child = calc.getChild(); this.relTypes = relTypes; + assert "CalcRelType".equals(relTypes[0].name) + : "The first RelType should be CalcRelType for proper RexLiteral" + + " implementation at the last level, got " + relTypes[0].name; } //~ Methods ---------------------------------------------------------------- @@ -101,11 +104,11 @@ RelNode execute() { assert !RexUtil.containComplexExprs(exprList); // Figure out what level each expression belongs to. - int[] exprLevels = new int[exprs.length]; + int[] exprLevels = new int[exprs.length + 1]; // The reltype of a level is given by // relTypes[levelTypeOrdinals[level]]. - int[] levelTypeOrdinals = new int[exprs.length]; + int[] levelTypeOrdinals = new int[exprs.length + 1]; int levelCount = chooseLevels(exprs, -1, exprLevels, levelTypeOrdinals); @@ -249,7 +252,9 @@ private int chooseLevels( int[] levelTypeOrdinals) { final int inputFieldCount = program.getInputRowType().getFieldCount(); - int levelCount = 0; + // Ensure the first level is CalcRelType so the input projection + // trims unnecessary fields. + int levelCount = 1; final MaxInputFinder maxInputFinder = new MaxInputFinder(exprLevels); boolean[] relTypesPossibleForTopLevel = new boolean[relTypes.length]; Arrays.fill(relTypesPossibleForTopLevel, true); @@ -371,9 +376,6 @@ private int chooseLevels( if (levelCount > 0) { // The latest level should be CalcRelType otherwise literals cannot be // implemented. - assert "CalcRelType".equals(relTypes[0].name) - : "The first RelType should be CalcRelType for proper RexLiteral" - + " implementation at the last level, got " + relTypes[0].name; if (levelTypeOrdinals[levelCount - 1] != 0) { levelCount++; } diff --git a/core/src/test/java/net/hydromatic/optiq/test/JdbcTest.java b/core/src/test/java/net/hydromatic/optiq/test/JdbcTest.java index 9d5ba4d41..49ee9928e 100644 --- a/core/src/test/java/net/hydromatic/optiq/test/JdbcTest.java +++ b/core/src/test/java/net/hydromatic/optiq/test/JdbcTest.java @@ -3042,9 +3042,10 @@ public void testWinAggWithFilter() { + "from \"hr\".\"emps\"\n" + ") where \"deptno\"=10 and \"empid\"=100") .explainContains( - "EnumerableCalcRel(expr#0..5=[{inputs}], expr#6=[CAST($t1):INTEGER NOT NULL], expr#7=[10], expr#8=[=($t6, $t7)], expr#9=[CAST($t0):INTEGER NOT NULL], expr#10=[100], expr#11=[=($t9, $t10)], expr#12=[AND($t8, $t11)], proj#0..1=[{exprs}], $2=[$t5], $condition=[$t12])\n" + "EnumerableCalcRel(expr#0..2=[{inputs}], expr#3=[CAST($t1):INTEGER NOT NULL], expr#4=[10], expr#5=[=($t3, $t4)], expr#6=[CAST($t0):INTEGER NOT NULL], expr#7=[100], expr#8=[=($t6, $t7)], expr#9=[AND($t5, $t8)], proj#0..2=[{exprs}], $condition=[$t9])\n" + " EnumerableWindowRel(window#0=[window(partition {} order by [] range between UNBOUNDED PRECEDING and UNBOUNDED FOLLOWING aggs [COUNT()])])\n" - + " EnumerableTableAccessRel(table=[[hr, emps]])") + + " EnumerableCalcRel(expr#0..4=[{inputs}], proj#0..1=[{exprs}])\n" + + " EnumerableTableAccessRel(table=[[hr, emps]])") .returns("empid=100; deptno=10; C=4\n"); // There are 4 employees in total } @@ -3063,9 +3064,10 @@ public void testWinAggWithFilterPush() { + "from \"hr\".\"emps\"\n" + ") where \"deptno\"=10 and \"empid\"=100") .explainContains( - "EnumerableCalcRel(expr#0..5=[{inputs}], expr#6=[CAST($t1):INTEGER NOT NULL], expr#7=[10], expr#8=[=($t6, $t7)], expr#9=[CAST($t0):INTEGER NOT NULL], expr#10=[100], expr#11=[=($t9, $t10)], expr#12=[AND($t8, $t11)], proj#0..1=[{exprs}], $2=[$t5], $condition=[$t12])\n" + "EnumerableCalcRel(expr#0..2=[{inputs}], expr#3=[CAST($t1):INTEGER NOT NULL], expr#4=[10], expr#5=[=($t3, $t4)], expr#6=[CAST($t0):INTEGER NOT NULL], expr#7=[100], expr#8=[=($t6, $t7)], expr#9=[AND($t5, $t8)], proj#0..2=[{exprs}], $condition=[$t9])\n" + " EnumerableWindowRel(window#0=[window(partition {1} order by [] range between UNBOUNDED PRECEDING and UNBOUNDED FOLLOWING aggs [COUNT()])])\n" - + " EnumerableTableAccessRel(table=[[hr, emps]])") + + " EnumerableCalcRel(expr#0..4=[{inputs}], proj#0..1=[{exprs}])\n" + + " EnumerableTableAccessRel(table=[[hr, emps]])") .returns("empid=100; deptno=10; C=3\n"); // There are 3 employees in department 10 }