HPCC-13017 Updates following review

Signed-off-by: Gavin Halliday <gavin.halliday@lexisnexis.com>

ecl/hql/hqlmeta.cpp

@@ -3394,7 +3394,8 @@ extern HQL_API bool hasKnownSortGroupDistribution(IHqlExpression * expr, bool is
 
 //---------------------------------------------------------------------------------------------------------------------
 
-//Mark all selectors that are fully included in the sort criteria. Err on the side of caution.
+//Mark all selectors that are fully included in the sort criteria. This may not catch all cases
+//but it is preferable to have false negatives than false positives.
 void markValidSelectors(IHqlExpression * expr, IHqlExpression * dsSelector)
 {
     switch (expr->getOperator())
@@ -3431,7 +3432,7 @@ void markValidSelectors(IHqlExpression * expr, IHqlExpression * dsSelector)
         markValidSelectors(expr->queryChild(i), dsSelector);
 }
 
-extern HQL_API bool allFieldsAreSorted(IHqlExpression * record, IHqlExpression * sortOrder, IHqlExpression * dsSelector)
+extern HQL_API bool allFieldsAreSorted(IHqlExpression * record, IHqlExpression * sortOrder, IHqlExpression * dsSelector, bool strict)
 {
     TransformMutexBlock block;
 
@@ -3442,8 +3443,21 @@ extern HQL_API bool allFieldsAreSorted(IHqlExpression * record, IHqlExpression *
     RecordSelectIterator iter(record, dsSelector);
     ForEach(iter)
     {
-        if (!iter.query()->queryTransformExtra())
+        IHqlExpression * select = iter.query();
+        if (!select->queryTransformExtra())
             return false;
+
+        if (strict && isUnknownSize(select->queryType()))
+        {
+            //Comparisons on strings (and unicode) ignore trailing spaces, so strictly speaking sorting by a variable
+            //length string field doesn't compare all the information from a field.
+            ITypeInfo * type = select->queryType();
+            if (type->getTypeCode() != type_data)
+            {
+                if (isStringType(type) || isUnicodeType(type))
+                    return false;
+            }
+        }
     }
     return true;
 }

ecl/hql/hqlmeta.hpp

@@ -129,7 +129,7 @@ extern HQL_API CHqlMetaProperty * querySimpleDatasetMeta(IHqlExpression * expr);
 extern HQL_API bool hasSameSortGroupDistribution(IHqlExpression * expr, IHqlExpression * other);
 extern HQL_API bool hasKnownSortGroupDistribution(IHqlExpression * expr, bool isLocal);
 
-extern HQL_API bool allFieldsAreSorted(IHqlExpression * record, IHqlExpression * sortOrder, IHqlExpression * selector);
+extern HQL_API bool allFieldsAreSorted(IHqlExpression * record, IHqlExpression * sortOrder, IHqlExpression * selector, bool strict);
 
 inline IHqlExpression * queryRemoveOmitted(IHqlExpression * expr)
 {

ecl/hqlcpp/hqlcpp.cpp

@@ -1755,6 +1755,7 @@ void HqlCppTranslator::cacheOptions()
         DebugOption(options.keyedJoinPreservesOrder,"keyedJoinPreservesOrder",true),
         DebugOption(options.expandSelectCreateRow,"expandSelectCreateRow",false),
         DebugOption(options.optimizeSortAllFields,"optimizeSortAllFields",true),
+        DebugOption(options.optimizeSortAllFieldsStrict,"optimizeSortAllFieldsStrict",false),
     };
 
     //get options values from workunit

ecl/hqlcpp/hqlcpp.ipp

@@ -745,6 +745,7 @@ struct HqlCppOptions
     bool                keyedJoinPreservesOrder;
     bool                expandSelectCreateRow;
     bool                optimizeSortAllFields;
+    bool                optimizeSortAllFieldsStrict;
 };
 
 //Any information gathered while processing the query should be moved into here, rather than cluttering up the translator class

ecl/hqlcpp/hqlhtcpp.cpp

@@ -16396,9 +16396,10 @@ ABoundActivity * HqlCppTranslator::doBuildActivitySort(BuildCtx & ctx, IHqlExpre
         //If a dataset is sorted by all fields then it is impossible to determine if the original order
         //was preserved - so mark the sort as potentially unstable (to reduce memory usage at runtime)
         if (options.optimizeSortAllFields &&
-            allFieldsAreSorted(expr->queryRecord(), sortlist, dataset->queryNormalizedSelector()))
-
-        flags.append("|TAFunstable");
+            allFieldsAreSorted(expr->queryRecord(), sortlist, dataset->queryNormalizedSelector(), options.optimizeSortAllFieldsStrict))
+        {
+            flags.append("|TAFunstable");
+        }
     }
 
     if (spill)

ecl/hqlcpp/hqlttcpp.cpp

@@ -1780,12 +1780,13 @@ static IHqlExpression * simplifySortlistComplexity(IHqlExpression * sortlist)
         }
         else if (cur->getOperator() == no_trim)
         {
-            //TRIM(fixed-length-string) can just sort by the string instead.  (Don't match LEFT/RIGHT versions.)
+            //Strings are always compared as if they are trimmed (or padded with arbitrary spaces)
+            //=> sort by TRIM(string) can just sort by the string instead.  (Don't match LEFT/RIGHT versions.)
             if (!cur->queryChild(1))
             {
                 IHqlExpression * arg = cur->queryChild(0);
                 ITypeInfo * argType = arg->queryType();
-                if (isFixedSize(argType) && (cur->queryType()->getTypeCode() == argType->getTypeCode()))
+                if (cur->queryType()->getTypeCode() == argType->getTypeCode())
                 {
                     expand = true;
                     appendComponent(cpts, invert, arg);

testing/regress/ecl/sort3.ecl

@@ -1,6 +1,6 @@
 /*##############################################################################
 
-    HPCC SYSTEMS software Copyright (C) 2013 HPCC Systems.
+    HPCC SYSTEMS software Copyright (C) 2015 HPCC Systems.
 
     Licensed under the Apache License, Version 2.0 (the "License");
     you may not use this file except in compliance with the License.
@@ -15,8 +15,6 @@
     limitations under the License.
 ############################################################################## */
 
-#onwarning(1036, ignore);
-
 r1 := { unsigned id };
 MyRec := RECORD
     UNSIGNED2 uv;