TEIID-4129: Wrong results with cross model join on 2 varchar fields (…

…removing the pushdown of a string sort for sort/merge in more)

circumstances

build/kits/jboss-as7/overlay/docs/teiid/teiid-releasenotes.html

@@ -226,6 +226,9 @@ <h4 class="western">from ${project.version}</h4>
 <ul>
     <li/>
 	<p style="margin-bottom: 0in">
+<a href='https://issues.jboss.org/browse/TEIID-4129'>TEIID-4129</a> - Wrong results with cross model join on 2 varchar fields (removing the pushdown of a string sort for sort/merge in more)
+    <li/>
+	<p style="margin-bottom: 0in">
 <a href='https://issues.jboss.org/browse/TEIID-4646'>TEIID-4646</a> - Add engine support for lead/lag aggregate functions
     <li/>
 	<p style="margin-bottom: 0in">

common-core/src/main/java/org/teiid/core/types/DataTypeManager.java

@@ -63,6 +63,7 @@ public class DataTypeManager {
 	private static final boolean COMPARABLE_LOBS = PropertiesUtils.getBooleanProperty(System.getProperties(), "org.teiid.comparableLobs", false); //$NON-NLS-1$
 	private static final boolean COMPARABLE_OBJECT = PropertiesUtils.getBooleanProperty(System.getProperties(), "org.teiid.comparableObject", false); //$NON-NLS-1$
 	public static final boolean PAD_SPACE = PropertiesUtils.getBooleanProperty(System.getProperties(), "org.teiid.padSpace", false); //$NON-NLS-1$
+	public static final String DEFAULT_COLLATION = "UCS-2"; //$NON-NLS-1$
 	public static final String COLLATION_LOCALE = System.getProperties().getProperty("org.teiid.collationLocale"); //$NON-NLS-1$
 
 	private static boolean valueCacheEnabled = USE_VALUE_CACHE;
@@ -982,8 +983,12 @@ public static final String getCanonicalString(String value) {
     }
 
 	public static boolean isHashable(Class<?> type) {
+		if (type == DataTypeManager.DefaultDataClasses.STRING
+				|| type == DataTypeManager.DefaultDataClasses.CHAR) {
+			return COLLATION_LOCALE == null;
+		}
 		if (type == DataTypeManager.DefaultDataClasses.STRING) {
-			return !PAD_SPACE && COLLATION_LOCALE == null;
+			return 	!PAD_SPACE; 
 		}
 		return !(type == DataTypeManager.DefaultDataClasses.BIG_DECIMAL
 				|| type == DataTypeManager.DefaultDataClasses.BLOB

engine/src/main/java/org/teiid/dqp/internal/process/DQPCore.java

@@ -663,6 +663,11 @@ public void onCompletion(
 			}
 		});
         dataTierMgr.setEventDistributor(eventDistributor);
+        //for now options are scoped to the engine - vdb scoping is a todo
+        options = new Options();
+        options.setAssumeMatchingCollation(false);
+        options.setProperties(config.getProperties());
+        PropertiesUtils.setBeanProperties(options, options.getProperties(), "org.teiid", true); //$NON-NLS-1$
         LogManager.logDetail(LogConstants.CTX_DQP, "DQPCore started maxThreads", this.config.getMaxThreads(), "maxActivePlans", this.maxActivePlans, "source concurrency", this.userRequestSourceConcurrency); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$
 	}
 

engine/src/main/java/org/teiid/query/optimizer/relational/rules/CriteriaCapabilityValidatorVisitor.java

@@ -286,7 +286,9 @@ public void visit(OrderBy obj) {
 		CommandContext commandContext = CommandContext.getThreadLocalContext();
 		if (collation != null && commandContext != null && commandContext.getOptions().isRequireTeiidCollation() && !collation.equals(DataTypeManager.COLLATION_LOCALE)) {
 	        for (OrderByItem symbol : obj.getOrderByItems()) {
-	            if (symbol.getSymbol().getType() == DataTypeManager.DefaultDataClasses.STRING) {
+	            if (symbol.getSymbol().getType() == DataTypeManager.DefaultDataClasses.STRING
+	            		|| symbol.getSymbol().getType() == DataTypeManager.DefaultDataClasses.CLOB
+	            		|| symbol.getSymbol().getType() == DataTypeManager.DefaultDataClasses.CHAR) {
 	    	    	//we require the collation to match
 	        		markInvalid(obj, "source is not using the same collation as Teiid"); //$NON-NLS-1$
 	            	break;

engine/src/main/java/org/teiid/query/optimizer/relational/rules/RuleImplementJoinStrategy.java

@@ -194,7 +194,7 @@ public PlanNode execute(PlanNode plan,
         		}
             }
 
-			boolean pushedLeft = insertSort(joinNode.getFirstChild(), leftExpressions, joinNode, metadata, capabilitiesFinder, pushLeft);	
+			boolean pushedLeft = insertSort(joinNode.getFirstChild(), leftExpressions, joinNode, metadata, capabilitiesFinder, pushLeft, context);	
 
 			//TODO: this check could be performed, as it implies we're using enhanced and can back out of the sort
 			//      but this not valid in all circumstances
@@ -214,7 +214,7 @@ public PlanNode execute(PlanNode plan,
 	        	}
 			}
 
-			boolean pushedRight = insertSort(joinNode.getLastChild(), rightExpressions, joinNode, metadata, capabilitiesFinder, pushRight);
+			boolean pushedRight = insertSort(joinNode.getLastChild(), rightExpressions, joinNode, metadata, capabilitiesFinder, pushRight, context);
         	if ((!pushedRight || !pushedLeft) && (joinType == JoinType.JOIN_INNER || (joinType == JoinType.JOIN_LEFT_OUTER && !pushedLeft))) {
         		joinNode.setProperty(NodeConstants.Info.JOIN_STRATEGY, JoinStrategyType.ENHANCED_SORT);
         	}
@@ -233,7 +233,7 @@ public PlanNode execute(PlanNode plan,
      * @throws QueryMetadataException 
      */
     static boolean insertSort(PlanNode childNode, List<Expression> expressions, PlanNode jnode, QueryMetadataInterface metadata, CapabilitiesFinder capFinder,
-    		boolean attemptPush) throws QueryMetadataException, TeiidComponentException {
+    		boolean attemptPush, CommandContext context) throws QueryMetadataException, TeiidComponentException {
         Set<Expression> orderSymbols = new LinkedHashSet<Expression>(expressions); 
 
         PlanNode sourceNode = FrameUtil.findJoinSourceNode(childNode);
@@ -273,7 +273,7 @@ static boolean insertSort(PlanNode childNode, List<Expression> expressions, Plan
         	if (!usesKey && RuleRaiseAccess.getModelIDFromAccess(sourceNode, metadata) == TempMetadataAdapter.TEMP_MODEL) {
         	    attemptPush = false;
         	}
-            if (attemptPush && RuleRaiseAccess.canRaiseOverSort(sourceNode, metadata, capFinder, sortNode, null, false, true)) {
+	        if (attemptPush && RuleRaiseAccess.canRaiseOverSort(sourceNode, metadata, capFinder, sortNode, null, false, context, true)) {
 	            sourceNode.getFirstChild().addAsParent(sortNode);
 
 	            if (needsCorrection) {

engine/src/main/java/org/teiid/query/optimizer/relational/rules/RuleMergeCriteria.java

@@ -394,7 +394,7 @@ private PlanNode planMergeJoin(PlanNode current, PlanNode root) throws QueryMeta
             semiJoin.addLastChild(node);
             PlanNode result = current.getParent();
             NodeEditor.removeChildNode(result, current);
-            RuleImplementJoinStrategy.insertSort(semiJoin.getFirstChild(), (List<Expression>) plannedResult.leftExpressions, semiJoin, metadata, capFinder, true);
+            RuleImplementJoinStrategy.insertSort(semiJoin.getFirstChild(), (List<Expression>) plannedResult.leftExpressions, semiJoin, metadata, capFinder, true, context);
             if (plannedResult.makeInd && !plannedResult.not) {
             	//TODO: would like for an enhanced sort merge with the semi dep option to avoid the sorting
             	//this is a little different than a typical dependent join in that the right is the independent side

engine/src/main/java/org/teiid/query/optimizer/relational/rules/RuleRaiseAccess.java

@@ -393,13 +393,13 @@ static boolean canRaiseOverSort(PlanNode accessNode,
             CapabilitiesFinder capFinder,
             PlanNode parentNode, AnalysisRecord record, boolean compensateForUnrelated, CommandContext context) throws QueryMetadataException,
                                 TeiidComponentException {
-    	return canRaiseOverSort(accessNode, metadata, capFinder, parentNode, record, compensateForUnrelated, context.getOptions().isRequireTeiidCollation());
+    	return canRaiseOverSort(accessNode, metadata, capFinder, parentNode, record, compensateForUnrelated, context, context.getOptions().isRequireTeiidCollation());
     }
 
 	static boolean canRaiseOverSort(PlanNode accessNode,
                                    QueryMetadataInterface metadata,
                                    CapabilitiesFinder capFinder,
-                                   PlanNode parentNode, AnalysisRecord record, boolean compensateForUnrelated, boolean checkCollation) throws QueryMetadataException,
+                                   PlanNode parentNode, AnalysisRecord record, boolean compensateForUnrelated, CommandContext context, boolean checkCollation) throws QueryMetadataException,
                                                        TeiidComponentException {
         // Find the model for this node by getting ACCESS node's model
         Object modelID = getModelIDFromAccess(accessNode, metadata);
@@ -417,7 +417,10 @@ static boolean canRaiseOverSort(PlanNode accessNode,
             if (!CapabilitiesUtil.supportsNullOrdering(metadata, capFinder, modelID, symbol)) {
             	return false;
             }
-            if (symbol.getSymbol().getType() == DataTypeManager.DefaultDataClasses.STRING) {
+            Class<?> type = symbol.getSymbol().getType();
+			if (type == DataTypeManager.DefaultDataClasses.STRING 
+					|| type == DataTypeManager.DefaultDataClasses.CHAR 
+					|| type == DataTypeManager.DefaultDataClasses.CLOB) {
             	stringType = true;
             }
         }
@@ -456,12 +459,27 @@ static boolean canRaiseOverSort(PlanNode accessNode,
         	return false;
         }
 
-    	String collation = (String) CapabilitiesUtil.getProperty(Capability.COLLATION_LOCALE, modelID, metadata, capFinder);
-
-    	//we require the collation to match
-    	if (stringType && checkCollation && collation != null && !collation.equals(DataTypeManager.COLLATION_LOCALE)) {
-    		return false;
-    	}
+        if (stringType) {
+	        if (!checkCollation && parentNode.getParent() != null 
+	        		&& parentNode.getParent().getType() == NodeConstants.Types.TUPLE_LIMIT 
+	        		&& NodeEditor.findParent(parentNode.getParent(), NodeConstants.Types.JOIN) != null) {
+	        	checkCollation = true;
+	        }
+
+	        if (checkCollation) {
+	        	String collation = (String) CapabilitiesUtil.getProperty(Capability.COLLATION_LOCALE, modelID, metadata, capFinder);
+
+	        	//we require the collation to match
+	        	if (collation != null) {
+	        		if ((DataTypeManager.COLLATION_LOCALE != null && !collation.equals(DataTypeManager.COLLATION_LOCALE))
+	        				|| (DataTypeManager.COLLATION_LOCALE == null && !collation.equals(DataTypeManager.DEFAULT_COLLATION))) {
+	        			return false;
+	        		}
+	        	} else if (!context.getOptions().isAssumeMatchingCollation()) {
+    				return false;
+	        	}
+	        }
+        }
 
         //we don't need to check for extended grouping here since we'll create an inline view later
 

engine/src/main/java/org/teiid/query/processor/relational/MergeJoinStrategy.java

@@ -27,6 +27,7 @@
 import org.teiid.core.TeiidComponentException;
 import org.teiid.core.TeiidProcessingException;
 import org.teiid.core.util.Assertion;
+import org.teiid.query.QueryPlugin;
 import org.teiid.query.processor.relational.SourceState.ImplicitBuffer;
 import org.teiid.query.sql.lang.JoinType;
 import org.teiid.query.sql.symbol.Constant;
@@ -290,6 +291,11 @@ protected boolean compareToPrevious(SourceState target) throws TeiidComponentExc
             int compare = 1;
             if (!target.isExpresssionDistinct()) {
                 compare = compare(previousTuple, target.getCurrentTuple(), target.getExpressionIndexes(), target.getExpressionIndexes());
+                if (compare < 0) {
+                	//sanity check - this means the sort order is not as expected
+                	//note this is not a complete check - it will not detect all invalid circumstances as we exit early
+                	throw new TeiidComponentException(QueryPlugin.Util.gs(QueryPlugin.Event.TEIID31202));
+                }
             }
             if (compare != 0) {
                 target.setMaxProbeMatch(target.getIterator().getCurrentIndex() - 1);
@@ -308,6 +314,18 @@ protected int compare(List leftProbe,
 				rightExpressionIndecies, grouping, false);
     }
 
+    /**
+     * 0 if the values match
+     * positive if right is greater than left
+     * negative if left is greater than right
+     * @param leftProbe
+     * @param rightProbe
+     * @param leftExpressionIndecies
+     * @param rightExpressionIndecies
+     * @param nullEquals
+     * @param columnDiff
+     * @return
+     */
 	public static int compareTuples(List<?> leftProbe, List<?> rightProbe,
 			int[] leftExpressionIndecies, int[] rightExpressionIndecies, boolean nullEquals, boolean columnDiff) {
 		for (int i = 0; i < leftExpressionIndecies.length; i++) {

engine/src/main/java/org/teiid/query/util/Options.java

@@ -40,6 +40,7 @@ public class Options {
 	public static final String DEFAULT_NULL_ORDER = "org.teiid.defaultNullOrder"; //$NON-NLS-1$
 	public static final String AGGRESSIVE_JOIN_GROUPING = "org.teiid.aggressiveJoinGrouping"; //$NON-NLS-1$
 	public static final String MAX_SESSION_BUFFER_SIZE_ESTIMATE = "org.teiid.maxSessionBufferSizeEstimate"; //$NON-NLS-1$
+	public static final String ASSUME_MATCHING_COLLATION = "org.teiid.assumeMatchingCollation"; //$NON-NLS-1$
 
 	private Properties properties;
 	private boolean subqueryUnnestDefault = false;
@@ -51,6 +52,7 @@ public class Options {
 	private NullOrder defaultNullOrder = NullOrder.LOW;
 	private boolean aggressiveJoinGrouping = true;
 	private long maxSessionBufferSizeEstimate = Long.MAX_VALUE;
+	private boolean assumeMatchingCollation = true;
 
 	public Properties getProperties() {
 		return properties;
@@ -179,4 +181,18 @@ public long getMaxSessionBufferSizeEstimate() {
         return maxSessionBufferSizeEstimate;
     }
 
+
+	public boolean isAssumeMatchingCollation() {
+		return this.assumeMatchingCollation;
+	}
+
+	public void setAssumeMatchingCollation(boolean assumeMatchingCollation) {
+		this.assumeMatchingCollation = assumeMatchingCollation;
+	}
+
+	public Options assumeMatchingCollation(boolean b) {
+		this.assumeMatchingCollation = b;
+		return this;
+	}
+
 }

engine/src/test/java/org/teiid/query/optimizer/TestJoinOptimization.java

@@ -23,6 +23,7 @@
 package org.teiid.query.optimizer;
 
 import static org.junit.Assert.*;
+import static org.teiid.query.processor.TestProcessor.*;
 
 import java.math.BigDecimal;
 import java.util.ArrayList;
@@ -49,6 +50,7 @@
 import org.teiid.query.optimizer.capabilities.SourceCapabilities.Capability;
 import org.teiid.query.optimizer.relational.rules.JoinUtil;
 import org.teiid.query.parser.QueryParser;
+import org.teiid.query.processor.FakeDataManager;
 import org.teiid.query.processor.HardcodedDataManager;
 import org.teiid.query.processor.ProcessorPlan;
 import org.teiid.query.processor.TestProcessor;
@@ -62,6 +64,7 @@
 import org.teiid.query.sql.symbol.Expression;
 import org.teiid.query.sql.symbol.GroupSymbol;
 import org.teiid.query.unittest.RealMetadataFactory;
+import org.teiid.query.util.CommandContext;
 import org.teiid.translator.ExecutionFactory.SupportedJoinCriteria;
 import org.teiid.translator.SourceSystemFunctions;
 
@@ -1237,6 +1240,48 @@ private void helpTestNullDependent(String expressionSQL,
 	   				"SELECT g_1.e2 AS c_0, g_0.e3 AS c_1 FROM pm1.g1 AS g_0 LEFT OUTER JOIN pm1.g3 AS g_1 ON g_0.e1 = g_1.e1 ORDER BY c_0"}, new DefaultCapabilitiesFinder(caps), ComparisonMode.EXACT_COMMAND_STRING); //$NON-NLS-1$
 	 }
 
+    @Test public void testMergeJoinOrderNotPushed() throws TeiidComponentException, TeiidProcessingException {
+        String sql = "select bqt1.smalla.intkey, bqt2.smalla.intkey "
+        		+ "from bqt1.smalla inner join bqt2.smalla on (bqt2.smalla.stringkey = bqt1.smalla.stringkey)"; //$NON-NLS-1$
+        BasicSourceCapabilities bsc = TestOptimizer.getTypicalCapabilities();
+        bsc.setCapabilitySupport(Capability.QUERY_SEARCHED_CASE, true);
+        bsc.setSourceProperty(Capability.COLLATION_LOCALE, "nowhere");
+        // Plan query
+        ProcessorPlan plan = TestOptimizer.helpPlan(sql, RealMetadataFactory.exampleBQTCached(), 
+        		new String[] {"SELECT g_0.StringKey, g_0.IntKey FROM BQT1.SmallA AS g_0", 
+        	"SELECT g_0.StringKey, g_0.IntKey FROM BQT2.SmallA AS g_0"}, new DefaultCapabilitiesFinder(bsc), ComparisonMode.EXACT_COMMAND_STRING); //$NON-NLS-1$ //$NON-NLS-2$
+
+        HardcodedDataManager hdm = new HardcodedDataManager();
+        hdm.addData("SELECT g_0.StringKey, g_0.IntKey FROM BQT1.SmallA AS g_0", Arrays.asList("b", 1), Arrays.asList("a", 3));
+        hdm.addData("SELECT g_0.StringKey, g_0.IntKey FROM BQT2.SmallA AS g_0", Arrays.asList("c", 1), Arrays.asList("a", 2));
+
+        TestProcessor.helpProcess(plan, hdm, new List<?>[] {Arrays.asList(3, 2)});
+    }
+
+    /**
+     * Same as above but using the system/option property
+     * @throws TeiidComponentException
+     * @throws TeiidProcessingException
+     */
+    @Test public void testMergeJoinOrderNotPushed1() throws Exception {
+    	String sql = "select bqt1.smalla.intkey, bqt2.smalla.intkey "
+        		+ "from bqt1.smalla inner join bqt2.smalla on (bqt2.smalla.stringkey = bqt1.smalla.stringkey)"; //$NON-NLS-1$
+        BasicSourceCapabilities bsc = TestOptimizer.getTypicalCapabilities();
+        bsc.setCapabilitySupport(Capability.QUERY_SEARCHED_CASE, true);
+
+        CommandContext cc = TestProcessor.createCommandContext();
+        cc.getOptions().setAssumeMatchingCollation(false);
+
+        // Plan query
+        ProcessorPlan plan = TestProcessor.helpGetPlan(TestOptimizer.helpGetCommand(sql, RealMetadataFactory.exampleBQTCached(), null), RealMetadataFactory.exampleBQTCached(), new DefaultCapabilitiesFinder(bsc), cc);
+
+        HardcodedDataManager hdm = new HardcodedDataManager();
+        hdm.addData("SELECT g_0.StringKey, g_0.IntKey FROM BQT1.SmallA AS g_0", Arrays.asList("b", 1), Arrays.asList("a", 3));
+        hdm.addData("SELECT g_0.StringKey, g_0.IntKey FROM BQT2.SmallA AS g_0", Arrays.asList("c", 1), Arrays.asList("a", 2));
+
+        TestProcessor.helpProcess(plan, hdm, new List<?>[] {Arrays.asList(3, 2)});
+    }
+
     @Test public void testOutputColumnsWithMergeJoinAndNonPushedSelect() throws TeiidComponentException, TeiidProcessingException {
         String sql = "select bqt1.smalla.intkey, bqt2.smalla.intkey "
         		+ "from bqt1.smalla inner join bqt2.smalla on (bqt2.smalla.intkey = case when bqt1.smalla.intkey = 1 then 2 else 3 end) where right(bqt1.smalla.stringkey, 1) = 'a'"; //$NON-NLS-1$
@@ -1432,4 +1477,16 @@ private void helpTestNullDependent(String expressionSQL,
         assertNotNull(joinNode.getJoinCriteria());
     }
 
+	@Test(expected=AssertionError.class) public void testDetectInvalidSort() throws Exception {
+	    String sql = "select * from (with a (x, y, z) as /*+ no_inline */ (select e1, e2, e3 from pm1.g1) SELECT pm1.g2.e2, a.x, z from pm1.g2, a where e1 = x order by x) as x where z = 1"; //$NON-NLS-1$
+
+	    FakeDataManager dataManager = new FakeDataManager();
+	    sampleData1(dataManager);
+
+	    //we're allowing the sort to be pushed, but it's not honored by FakeDataManager
+	    ProcessorPlan plan = TestOptimizer.helpPlan(sql, RealMetadataFactory.example1Cached(), null, new DefaultCapabilitiesFinder(TestOptimizer.getTypicalCapabilities()), 
+	    		new String[] {"SELECT a.x, a.z FROM a WHERE a.z = TRUE ORDER BY a.x", "SELECT g_0.e1 AS c_0, g_0.e2 AS c_1 FROM pm1.g2 AS g_0 WHERE g_0.e1 IN (<dependent values>) ORDER BY c_0"}, ComparisonMode.EXACT_COMMAND_STRING);
+
+	    helpProcess(plan, createCommandContext(), dataManager, null);
+	}
 }