TEIID-5550 fix for inappropriate dup node removal over join

teiid · Nov 21, 2018 · 2528028 · 2528028
1 parent 540622d
commit 2528028
Show file tree

Hide file tree

Showing 2 changed files with 28 additions and 10 deletions.
diff --git a/engine/src/main/java/org/teiid/query/optimizer/relational/rules/RuleCollapseSource.java b/engine/src/main/java/org/teiid/query/optimizer/relational/rules/RuleCollapseSource.java
@@ -275,7 +275,7 @@ private PlanNode addDistinct(QueryMetadataInterface metadata,
 		if (queryCommand.getLimit() != null) {
 			return root; //TODO: could create an inline view
 		}
-		boolean requireDupPush = false;
+		boolean canRemoveParentDup = false;
 		if (queryCommand.getOrderBy() == null) {
 			/* 
 			 * we're assuming that a pushed order by implies that the cost of the distinct operation 
@@ -285,8 +285,11 @@ private PlanNode addDistinct(QueryMetadataInterface metadata,
 			 * cardinality without = c
 			 * assume cost ~ c lg c for c' cardinality and a modification for associated bandwidth savings
 			 * recompute cost of processing plan with c' and see if new cost + c lg c < original cost
+			 * 
+			 * We stop at join nodes they can alter the cardinality
+			 * - further checking could determine if the cardinality is preserved without the parent distinct
 			 */
-			PlanNode dupRemove = NodeEditor.findParent(accessNode, NodeConstants.Types.DUP_REMOVE, NodeConstants.Types.SOURCE);
+			PlanNode dupRemove = NodeEditor.findParent(accessNode, NodeConstants.Types.DUP_REMOVE, NodeConstants.Types.SOURCE | NodeConstants.Types.JOIN);
 			if (dupRemove != null) { //TODO: what about when sort/dup remove have been combined
 				PlanNode project = NodeEditor.findParent(accessNode, NodeConstants.Types.PROJECT, NodeConstants.Types.DUP_REMOVE);
 				if (project != null) {
@@ -300,19 +303,17 @@ private PlanNode addDistinct(QueryMetadataInterface metadata,
 					/*
 					 * If we can simply move the dupremove below the projection, then we'll do that as well
 					 */
-					requireDupPush = true;
+					canRemoveParentDup = true;
 				}
 			}
 			if (accessNode.hasBooleanProperty(Info.IS_MULTI_SOURCE)) {
 			    if (dupRemove == null) {
                     return root;
                 }
-			    if (requireDupPush) {
-			        //if multi-source we still need to process above
-	                requireDupPush = false;
-			    }
-			} else if (!requireDupPush) {
-				return root;
+			    //if multi-source we still need to process above
+			    canRemoveParentDup = false;
+			} else if (!canRemoveParentDup) {
+			    return root;
 			}
 		}
 		// ensure that all columns are comparable - they might not be if there is an intermediate project
@@ -332,7 +333,7 @@ private PlanNode addDistinct(QueryMetadataInterface metadata,
 			HashSet<GroupSymbol> keyPreservingGroups = new HashSet<GroupSymbol>();
 			ResolverUtil.findKeyPreserved(query, keyPreservingGroups, metadata);
 			if (!QueryRewriter.isDistinctWithGroupBy(query) && !NewCalculateCostUtil.usesKey(query.getSelect().getProjectedSymbols(), keyPreservingGroups, metadata, true)) {
-				if (requireDupPush) { //remove the upper dup remove
+				if (canRemoveParentDup) { //remove the upper dup remove
 					PlanNode dupRemove = NodeEditor.findParent(accessNode, NodeConstants.Types.DUP_REMOVE, NodeConstants.Types.SOURCE);
 					if (dupRemove.getParent() == null) {
 						root = dupRemove.getFirstChild();

diff --git a/engine/src/test/java/org/teiid/query/optimizer/TestSortOptimization.java b/engine/src/test/java/org/teiid/query/optimizer/TestSortOptimization.java
@@ -40,6 +40,7 @@
 import org.teiid.query.processor.HardcodedDataManager;
 import org.teiid.query.processor.ProcessorPlan;
 import org.teiid.query.processor.TestProcessor;
+import org.teiid.query.processor.relational.DupRemoveNode;
 import org.teiid.query.processor.relational.LimitNode;
 import org.teiid.query.processor.relational.ProjectNode;
 import org.teiid.query.processor.relational.RelationalPlan;
@@ -523,6 +524,22 @@ public class TestSortOptimization {
                 new String[] {"SELECT DISTINCT g_0.e1 FROM pm1.g1 AS g_0"}, ComparisonMode.EXACT_COMMAND_STRING); //$NON-NLS-1$
     }
 
+    /**
+     * Ensure that the distinct operation is not inappropriately removed
+     * @throws Exception
+     */
+    @Test public void testDistinctPushdownOverJoin() throws Exception{
+        String sql = "select distinct pm2.g2.e1 from pm1.g1 left outer join pm2.g2 on pm1.g1.e2 = pm2.g2.e2";
+
+        BasicSourceCapabilities caps = TestOptimizer.getTypicalCapabilities();
+        caps.setCapabilitySupport(Capability.QUERY_SELECT_DISTINCT, true);
+        caps.setCapabilitySupport(Capability.QUERY_ORDERBY, false);
+
+        ProcessorPlan plan = helpPlan(sql, RealMetadataFactory.example1Cached(), null, new DefaultCapabilitiesFinder(caps), 
+                new String[] {"SELECT g_0.e2 FROM pm1.g1 AS g_0", "SELECT g_0.e2, g_0.e1 FROM pm2.g2 AS g_0"}, ComparisonMode.EXACT_COMMAND_STRING); //$NON-NLS-1$
+        checkNodeTypes(plan, new int[] {1}, new Class[] {DupRemoveNode.class});        
+    }
+
     @Test public void testDistinctPushdownWithGrouping() throws Exception{
         String sql = "select distinct e1, e2, 1, 2 from (select e1, e2 from pm1.g1 group by e1, e2) v";