Merge pull request #5698 from ghalliday/issue11226

HPCC-10983 Don't constant fold to avoid expression duplication in optimizer

Reviewed-By: Jamie Noss <james.noss@lexisnexis.com>
Reviewed-By: Richard Chapman <rchapman@hpccsystems.com>

ecl/hql/hqlfold.cpp

@@ -6069,7 +6069,8 @@ IHqlExpression * foldScopedHqlExpression(IHqlExpression * dataset, IHqlExpressio
 
     CExprFolderTransformer folder(NULL, foldOptions);
 
-    folder.setScope(dataset);
+    if (dataset)
+        folder.setScope(dataset);
 
     IHqlExpression * ret = folder.transformRoot(expr);
 

ecl/hql/hqlopt.cpp

@@ -282,15 +282,8 @@ IHqlExpression * CTreeOptimizer::swapNodeWithChild(IHqlExpression * parent)
 IHqlExpression * CTreeOptimizer::forceSwapNodeWithChild(IHqlExpression * parent)
 {
     OwnedHqlExpr swapped = swapNodeWithChild(parent);
-    return replaceOwnedAttribute(swapped, getNoHoistAttr());
-}
-
-IHqlExpression * CTreeOptimizer::getNoHoistAttr()
-{
-    //Ensure the attribute is unique for each call to the optimizer - otherwise it stops items being hoisted that could be.
-    if (!noHoistAttr)
-        noHoistAttr.setown(createAttribute(_noHoist_Atom, createUniqueId()));
-    return LINK(noHoistAttr);
+    queryBodyExtra(swapped)->setStopHoist();
+    return swapped.getClear();
 }
 
 IHqlExpression * CTreeOptimizer::swapNodeWithChild(IHqlExpression * parent, unsigned childIndex)
@@ -320,26 +313,40 @@ IHqlExpression * CTreeOptimizer::swapIntoIf(IHqlExpression * expr, bool force)
     OwnedHqlExpr newLeft = replaceChildDataset(body, left, 0);
     OwnedHqlExpr newRight = replaceChildDataset(body, right, 0);
 
-    OwnedHqlExpr transformedLeft = transform(newLeft);
-    OwnedHqlExpr transformedRight = transform(newRight);
+    HqlExprArray args;
+    args.append(*LINK(cond));
+    args.append(*LINK(newLeft));
+    args.append(*LINK(newRight));
+    OwnedHqlExpr newIf = child->clone(args);
+
+    if (!alreadyHasUsage(newIf))
+    {
+        incUsage(newLeft);
+        incUsage(newRight);
+    }
 
-    //Don't bother moving the condition over the if if it doesn't improve the code elsewhere
-    if (force || (newLeft != transformedLeft) || (newRight != transformedRight))
+    OwnedHqlExpr transformedIf = transform(newIf);
+    if (force || (newIf != transformedIf))
     {
         //Need to call dec on all expressions that are no longer used... left and right still used by newLeft/newRight
-        noteUnused(child);
-        DBGLOG("Optimizer: Swap %s and %s", queryNode0Text(expr), queryNode1Text(child));
-        HqlExprArray args;
-        args.append(*LINK(cond));
-        args.append(*LINK(transformedLeft));
-        args.append(*LINK(transformedRight));
-        OwnedHqlExpr ret = child->clone(args);
-        if (!alreadyHasUsage(ret))
+        if (!alreadyHasUsage(newIf))
         {
-            incUsage(transformedLeft);
-            incUsage(transformedRight);
+            //This may possibly leave left/right linked once if transformed(newLeft) doesn't use left any more.
+            //But a recursiveDecUsage could cause too much to be decremented.
+            if (newLeft != transformedIf->queryChild(1))
+                decUsage(newLeft);
+            if (newRight != transformedIf->queryChild(2))
+                decUsage(newRight);
         }
-        return ret.getClear();
+        noteUnused(child);
+        DBGLOG("Optimizer: Swap %s and %s", queryNode0Text(expr), queryNode1Text(child));
+        return transformedIf.getClear();
+    }
+
+    if (!alreadyHasUsage(newIf))
+    {
+        decUsage(newLeft);
+        decUsage(newRight);
     }
     return LINK(expr);
 }
@@ -1464,8 +1471,13 @@ IHqlExpression * CTreeOptimizer::optimizeProjectInlineTable(IHqlExpression * tra
         if (!next || monitor.isComplex())
             return NULL;
 
-        if (onlyFoldConstant && !isConstantTransform(next))
-            return NULL;
+        if (onlyFoldConstant)
+        {
+            next.setown(foldScopedHqlExpression(NULL, next));
+
+            if (!isConstantTransform(next))
+                return NULL;
+        }
         newValues.append(*ensureTransformType(next, no_transform));
     }
 
@@ -1543,6 +1555,10 @@ IHqlExpression * CTreeOptimizer::inheritUsage(IHqlExpression * newExpr, IHqlExpr
 {
     OptTransformInfo * newExtra = queryBodyExtra(newExpr);
     OptTransformInfo * oldExtra = queryBodyExtra(oldExpr);
+
+    if (oldExtra->getStopHoist())
+        newExtra->setStopHoist();
+
 #ifdef TRACE_USAGE
     if (newExpr->isDataset() || newExpr->isDatarow())
         DBGLOG("%lx inherit %d,%d (from %lx) [%s]", (unsigned)newExpr, newExtra->useCount, oldExtra->useCount, (unsigned)oldExpr, queryNode0Text(newExpr));
@@ -1719,7 +1735,7 @@ bool CTreeOptimizer::childrenAreShared(IHqlExpression * expr)
 
 bool CTreeOptimizer::isWorthMovingProjectOverLimit(IHqlExpression * project)
 {
-    if (noHoistAttr && project->queryAttribute(_noHoist_Atom) == noHoistAttr)
+    if (queryBodyExtra(project)->getStopHoist())
         return false;
 
     IHqlExpression * expr = project->queryChild(0);
@@ -1905,8 +1921,8 @@ ANewTransformInfo * CTreeOptimizer::createTransformInfo(IHqlExpression * expr)
 IHqlExpression * CTreeOptimizer::expandFields(TableProjectMapper * mapper, IHqlExpression * expr, IHqlExpression * oldDataset, IHqlExpression * newDataset, IExpandCallback * _expandCallback)
 {
     OwnedHqlExpr expandedFilter = mapper->expandFields(expr, oldDataset, newDataset, _expandCallback);
-    if (options & HOOfold)
-        expandedFilter.setown(foldHqlExpression(expandedFilter));
+    //There used to be code to constant fold filters here - but it can cause dataset expressions to become duplicated
+    //causing code to be duplicated.  Only fold expressions that are reduced to constants.
     return expandedFilter.getClear();
 }
 
@@ -3840,10 +3856,12 @@ IHqlExpression * optimizeHqlExpression(IHqlExpression * expr, unsigned options)
     unwindCommaCompound(args, expr);
     optimizeHqlExpression(newArgs, args, options);
     OwnedHqlExpr optimized = createActionList(newArgs);
+
+    if (expr == optimized)
+        return optimized.getClear();
+
     //If the graph was optimized then it is highly likely there are now constant folding opportunities
-    if (expr != optimized)
-        return foldHqlExpression(optimized);
-    return optimized.getClear();
+    return foldHqlExpression(optimized);
 }
 
 

ecl/hql/hqlopt.ipp

@@ -32,6 +32,9 @@ class OptTransformInfo : public NewTransformInfo
 public:
     OptTransformInfo(IHqlExpression * _expr) : NewTransformInfo(_expr) { useCount = 0; }
 
+    bool getStopHoist() { return spareByte1; }
+    void setStopHoist() { spareByte1 = true; }
+
 public:
     unsigned useCount;
 };
@@ -117,15 +120,12 @@ protected:
     inline const char * queryNode0Text(IHqlExpression * expr) { return queryChildNodeTraceText(nodeText[0], expr); }
     inline const char * queryNode1Text(IHqlExpression * expr) { return queryChildNodeTraceText(nodeText[1], expr); }
 
-    IHqlExpression * getNoHoistAttr();
-
     inline bool isAlwaysLocal() const { return (options & HOOalwayslocal) != 0; }
 
 protected:
     typedef NewHqlTransformer PARENT;
     unsigned options;
     StringBuffer nodeText[2];
-    HqlExprAttr noHoistAttr;
 };