Author: shawkins Date: 2009-05-12 21:37:34 -0400 (Tue, 12 May 2009) New Revision: 923 Modified: trunk/engine/src/main/java/com/metamatrix/query/optimizer/relational/rules/FrameUtil.java trunk/engine/src/main/java/com/metamatrix/query/rewriter/QueryRewriter.java trunk/engine/src/main/java/com/metamatrix/query/sql/visitor/ExpressionMappingVisitor.java trunk/engine/src/test/java/com/metamatrix/query/sql/visitor/TestExpressionMappingVisitor.java Log: TEIID-582 fixing the expression mapping visitor to prevent recursive mappings. Modified: trunk/engine/src/main/java/com/metamatrix/query/optimizer/relational/rules/FrameUtil.java =================================================================== --- trunk/engine/src/main/java/com/metamatrix/query/optimizer/relational/rules/FrameUtil.java 2009-05-13 01:01:53 UTC (rev 922) +++ trunk/engine/src/main/java/com/metamatrix/query/optimizer/relational/rules/FrameUtil.java 2009-05-13 01:37:34 UTC (rev 923) @@ -55,7 +55,6 @@ import com.metamatrix.query.sql.lang.QueryCommand; import com.metamatrix.query.sql.lang.Select; import com.metamatrix.query.sql.lang.StoredProcedure; -import com.metamatrix.query.sql.symbol.AggregateSymbol; import com.metamatrix.query.sql.symbol.Constant; import com.metamatrix.query.sql.symbol.ElementSymbol; import com.metamatrix.query.sql.symbol.Expression; @@ -264,24 +263,14 @@ return expression; } - if(expression instanceof ElementSymbol) { + if(expression instanceof SingleElementSymbol) { Expression mappedSymbol = (Expression) symbolMap.get(expression); if (mappedSymbol != null) { return mappedSymbol; } return expression; } - - if(expression instanceof AggregateSymbol) { - AggregateSymbol aggSymbol = (AggregateSymbol) expression; - - // First try to replace the entire aggregate - SingleElementSymbol replacement = (SingleElementSymbol) symbolMap.get(aggSymbol); - if(replacement != null) { - return replacement; - } - } - + ExpressionMappingVisitor.mapExpressions(expression, symbolMap); return expression; @@ -507,7 +496,7 @@ if (project != null) { return (List<SingleElementSymbol>)project.getProperty(NodeConstants.Info.PROJECT_COLS); } - Assertion.failed("no top cols in frame"); + Assertion.failed("no top cols in frame"); //$NON-NLS-1$ return null; } Modified: trunk/engine/src/main/java/com/metamatrix/query/rewriter/QueryRewriter.java =================================================================== --- trunk/engine/src/main/java/com/metamatrix/query/rewriter/QueryRewriter.java 2009-05-13 01:01:53 UTC (rev 922) +++ trunk/engine/src/main/java/com/metamatrix/query/rewriter/QueryRewriter.java 2009-05-13 01:37:34 UTC (rev 923) @@ -751,14 +751,14 @@ for (SingleElementSymbol symbol : (List<SingleElementSymbol>)query.getSelect().getProjectedSymbols()) { expressionMap.put((Expression)SymbolMap.getExpression(symbol).clone(), (SingleElementSymbol)iter.next()); } - ExpressionMappingVisitor.mapExpressions(groupBy, expressionMap, true); + ExpressionMappingVisitor.mapExpressions(groupBy, expressionMap); outerQuery.setGroupBy(groupBy); - ExpressionMappingVisitor.mapExpressions(having, expressionMap, true); + ExpressionMappingVisitor.mapExpressions(having, expressionMap); outerQuery.setHaving(having); - ExpressionMappingVisitor.mapExpressions(orderBy, expressionMap, true); + ExpressionMappingVisitor.mapExpressions(orderBy, expressionMap); outerQuery.setOrderBy(orderBy); outerQuery.setLimit(limit); - ExpressionMappingVisitor.mapExpressions(select, expressionMap, true); + ExpressionMappingVisitor.mapExpressions(select, expressionMap); outerQuery.setSelect(select); outerQuery.setInto(into); outerQuery.setOption(query.getOption()); Modified: trunk/engine/src/main/java/com/metamatrix/query/sql/visitor/ExpressionMappingVisitor.java =================================================================== --- trunk/engine/src/main/java/com/metamatrix/query/sql/visitor/ExpressionMappingVisitor.java 2009-05-13 01:01:53 UTC (rev 922) +++ trunk/engine/src/main/java/com/metamatrix/query/sql/visitor/ExpressionMappingVisitor.java 2009-05-13 01:37:34 UTC (rev 923) @@ -24,9 +24,11 @@ import java.util.ArrayList; import java.util.Collection; +import java.util.HashSet; import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.Set; import com.metamatrix.query.sql.LanguageObject; import com.metamatrix.query.sql.LanguageVisitor; @@ -47,7 +49,6 @@ import com.metamatrix.query.sql.lang.StoredProcedure; import com.metamatrix.query.sql.lang.SubqueryCompareCriteria; import com.metamatrix.query.sql.lang.SubquerySetCriteria; -import com.metamatrix.query.sql.navigator.PostOrderNavigator; import com.metamatrix.query.sql.navigator.PreOrderNavigator; import com.metamatrix.query.sql.proc.AssignmentStatement; import com.metamatrix.query.sql.symbol.AggregateSymbol; @@ -74,7 +75,7 @@ public ExpressionMappingVisitor(Map symbolMap) { this.symbolMap = symbolMap; } - + protected boolean createAliases() { return true; } @@ -293,27 +294,25 @@ } /** - * Take a language object (currently only criteria and expressions will be - * properly mapped) and use the map to swap out expressions with new expressions. - * If an expression is not found in the map, it is not swapped. The object - * is modified in place, so is not returned. + * The object is modified in place, so is not returned. * @param obj Language object * @param exprMap Expression map, Expression to Expression */ public static void mapExpressions(LanguageObject obj, Map exprMap) { - mapExpressions(obj, exprMap, false); - } - - public static void mapExpressions(LanguageObject obj, Map exprMap, boolean preOrder) { if(obj == null || exprMap == null || exprMap.isEmpty()) { return; } - ExpressionMappingVisitor visitor = new ExpressionMappingVisitor(exprMap); - if (preOrder) { - PreOrderNavigator.doVisit(obj, visitor); - } else { - PostOrderNavigator.doVisit(obj, visitor); - } + final Set reverseSet = new HashSet(exprMap.values()); + final ExpressionMappingVisitor visitor = new ExpressionMappingVisitor(exprMap); + PreOrderNavigator pon = new PreOrderNavigator(visitor) { + @Override + protected void visitNode(LanguageObject obj) { + if (!(obj instanceof Expression) || !reverseSet.contains(obj)) { + super.visitNode(obj); + } + } + }; + obj.acceptVisitor(pon); } protected void setVariableValues(Map variableValues) { Modified: trunk/engine/src/test/java/com/metamatrix/query/sql/visitor/TestExpressionMappingVisitor.java =================================================================== --- trunk/engine/src/test/java/com/metamatrix/query/sql/visitor/TestExpressionMappingVisitor.java 2009-05-13 01:01:53 UTC (rev 922) +++ trunk/engine/src/test/java/com/metamatrix/query/sql/visitor/TestExpressionMappingVisitor.java 2009-05-13 01:37:34 UTC (rev 923) @@ -22,18 +22,23 @@ package com.metamatrix.query.sql.visitor; +import static org.junit.Assert.*; + import java.util.ArrayList; import java.util.Arrays; import java.util.HashMap; import java.util.List; import java.util.Map; -import junit.framework.TestCase; +import org.junit.Test; +import org.teiid.connector.api.SourceSystemFunctions; import com.metamatrix.query.sql.LanguageObject; +import com.metamatrix.query.sql.ReservedWords; import com.metamatrix.query.sql.lang.CompareCriteria; import com.metamatrix.query.sql.lang.Select; import com.metamatrix.query.sql.lang.SetCriteria; +import com.metamatrix.query.sql.symbol.AggregateSymbol; import com.metamatrix.query.sql.symbol.CaseExpression; import com.metamatrix.query.sql.symbol.Constant; import com.metamatrix.query.sql.symbol.ElementSymbol; @@ -44,19 +49,15 @@ import com.metamatrix.query.sql.symbol.TestSearchedCaseExpression; -public class TestExpressionMappingVisitor extends TestCase { +public class TestExpressionMappingVisitor { - public TestExpressionMappingVisitor(String arg0) { - super(arg0); - } - public void helpTest(LanguageObject original, Map map, LanguageObject expected) { ExpressionMappingVisitor.mapExpressions(original, map); assertEquals("Did not get expected mapped expression", expected, original); //$NON-NLS-1$ } - public void testCompareCriteria1() { + @Test public void testCompareCriteria1() { ElementSymbol e1 = new ElementSymbol("e1"); //$NON-NLS-1$ Function f = new Function("+", new Expression[] { new Constant(new Integer(2)), new Constant(new Integer(5)) }); //$NON-NLS-1$ Map map = new HashMap(); @@ -66,7 +67,7 @@ helpTest(before, map, after); } - public void testCompareCriteria2() { + @Test public void testCompareCriteria2() { ElementSymbol e1 = new ElementSymbol("e1"); //$NON-NLS-1$ Function f = new Function("+", new Expression[] { new Constant(new Integer(2)), new Constant(new Integer(5)) }); //$NON-NLS-1$ Map map = new HashMap(); @@ -76,7 +77,7 @@ helpTest(before, map, after); } - public void testFunction1() { + @Test public void testFunction1() { ElementSymbol e1 = new ElementSymbol("e1"); //$NON-NLS-1$ ElementSymbol e2 = new ElementSymbol("e2"); //$NON-NLS-1$ ElementSymbol e3 = new ElementSymbol("e3"); //$NON-NLS-1$ @@ -100,7 +101,7 @@ helpTest(f3, map, f7); } - public void testSetCriteria() { + @Test public void testSetCriteria() { ElementSymbol e1 = new ElementSymbol("e1"); //$NON-NLS-1$ ElementSymbol e2 = new ElementSymbol("e2"); //$NON-NLS-1$ Constant c1 = new Constant("xyz"); //$NON-NLS-1$ @@ -122,7 +123,7 @@ helpTest(before, map, after); } - public void testCaseExpression1() { + @Test public void testCaseExpression1() { ElementSymbol x = new ElementSymbol("x"); //$NON-NLS-1$ ElementSymbol y = new ElementSymbol("y"); //$NON-NLS-1$ Constant a = new Constant(String.valueOf('a')); @@ -145,7 +146,7 @@ helpTest(TestCaseExpression.example(3), map, mapped); } - public void testCaseExpression2() { + @Test public void testCaseExpression2() { ElementSymbol x = new ElementSymbol("x"); //$NON-NLS-1$ ElementSymbol y = new ElementSymbol("y"); //$NON-NLS-1$ @@ -168,7 +169,7 @@ /** * We do not need to create an alias if the canonical short names match */ - public void testSelectAlias() { + @Test public void testSelectAlias() { ElementSymbol x = new ElementSymbol("y.x"); //$NON-NLS-1$ ElementSymbol y = new ElementSymbol("z.X"); //$NON-NLS-1$ @@ -181,6 +182,26 @@ assertEquals("Did not get expected mapped expression", "SELECT z.X", toMap.toString()); //$NON-NLS-1$ //$NON-NLS-2$ } - + /** + * we are not careful about ensuring that that every symbol is + * unique in a plan, so there's a chance mapping the expression in an + * aggregate in a project node will cause the same symbol to be + * updated in a sort node. to ensure that we don't get into + * recursion trouble we detect if we're replacing an expression + * that already exists as a mapping. + * + * we simulate that situation here using the same aggregate twice in + * a function. + */ + @Test public void testRecursionDetection() { + ElementSymbol e1 = new ElementSymbol("e1"); //$NON-NLS-1$ + AggregateSymbol a1 = new AggregateSymbol("x", ReservedWords.SUM, false, e1); //$NON-NLS-1$ + Function f = new Function(SourceSystemFunctions.ADD_OP, new Expression[] {a1, a1}); + HashMap<AggregateSymbol, AggregateSymbol> map = new HashMap<AggregateSymbol, AggregateSymbol>(); + map.put(a1, new AggregateSymbol("x", ReservedWords.SUM, false, a1)); //$NON-NLS-1$ + ExpressionMappingVisitor.mapExpressions(f, map); + assertEquals("(SUM(SUM(e1)) + SUM(SUM(e1)))", f.toString()); //$NON-NLS-1$ + } + }