HPCC-20078 Avoid some calls to Link/Release on IHqlExpressions

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

ecl/hql/hqlexpr.cpp

@@ -4028,12 +4028,13 @@ IHqlExpression * CHqlExpression::calcNormalizedSelector() const
 {
     IHqlExpression * left = &operands.item(0);
     IHqlExpression * normalizedLeft = left->queryNormalizedSelector();
-    if ((normalizedLeft != left) || ((operands.ordinality() > 2) && hasAttribute(newAtom)))
+
+    //Normalized selector only has two arguments - remove any extra arguments including attr(newAtom)
+    if ((normalizedLeft != left) || (operands.ordinality() > 2))
     {
         HqlExprArray args;
-        appendArray(args, operands);
-        args.replace(*LINK(normalizedLeft), 0);
-        removeAttribute(args, newAtom);
+        args.append(*LINK(normalizedLeft));
+        args.append(OLINK(operands.item(1)));
         return doCreateSelectExpr(args);
     }
     return NULL;
@@ -6169,8 +6170,9 @@ IHqlExpression * CHqlSelectBaseExpression::makeSelectExpression(HqlExprArray & o
 #endif
     IHqlExpression * left = &ownedOperands.item(0);
     IHqlExpression * normalizedLeft = left->queryNormalizedSelector();
-    bool needNormalize = (normalizedLeft != left) || ((ownedOperands.ordinality() > 2) && ::hasAttribute(newAtom, ownedOperands));
 
+    //Normalized selector only has two arguments - remove any extra arguments including attr(newAtom)
+    bool needNormalize = (normalizedLeft != left) || (ownedOperands.ordinality() > 2);
     CHqlSelectBaseExpression * select;
     if (needNormalize)
         select = new CHqlSelectExpression;
@@ -17251,6 +17253,15 @@ IHqlExpression * annotateIndexBlobs(IHqlExpression * expr)
     return transformer.transform(expr);
 }
 
+unsigned __int64 querySeqId(IHqlExpression * seq)
+{
+#ifdef DEBUG_TRACK_INSTANCEID
+    return static_cast<CHqlExpression *>(seq)->seqid;
+#else
+    return 0;
+#endif
+}
+
 /*
 List of changes:
 

ecl/hql/hqlexpr.hpp

@@ -1983,6 +1983,6 @@ inline bool isUnknownSize(IHqlExpression * expr) { return isUnknownSize(expr->qu
 extern HQL_API void setActiveSource(const char * filename);
 
 extern HQL_API IHqlExpression * annotateIndexBlobs(IHqlExpression * expr);
-
+extern HQL_API unsigned __int64 querySeqId(IHqlExpression * seq);   // Only for debugging when DEBUG_TRACK_INSTANCEID is defined
 
 #endif

ecl/hql/hqlexpr.ipp

@@ -164,7 +164,9 @@ protected:
     unsigned hashcode;          // CInterface is 4 byte aligned in 64bits, so use this to pad
                                 // Worth storing because it significantly speeds up equality checking
 #ifdef DEBUG_TRACK_INSTANCEID
+public:
     unsigned __int64 seqid;
+protected:
 #endif
     IInterface * transformExtra[NUM_PARALLEL_TRANSFORMS];
     HqlExprArray operands;

ecl/hql/hqlopt.cpp

@@ -30,6 +30,22 @@
 
 #define MIGRATE_JOIN_CONDITIONS             // This works, but I doubt it is generally worth the effort. - maybe on a flag.
 //#define TRACE_USAGE
+#ifdef _DEBUG
+//#define TRACK_EXPR_SEQ  nnnnn
+#endif
+//#define CHECK_USAGE_COUNT                 // For tracking down issues where usage count is inconsistent (may fail for datasets child queries)
+
+#ifdef TRACK_EXPR_SEQ
+void CHECK_EXPR_ID(IHqlExpression * expr)
+{
+    if (querySeqId(expr->queryBody()) == TRACK_EXPR_SEQ)
+        expr->numChildren(); // Add a breakpoint on this line to check whenever usage count for the expression changes
+}
+#else
+#define CHECK_EXPR_ID(expr)
+#endif
+
+
 
 /*
 Notes:
@@ -1668,6 +1684,8 @@ bool CTreeOptimizer::decUsage(IHqlExpression * expr)
     if (expr->isDataset() || expr->isDatarow())
         DBGLOG("%lx dec %d [%s]", (unsigned)expr, extra->useCount, queryNode0Text(expr));
 #endif
+    CHECK_EXPR_ID(expr);
+
     if (extra->useCount)
         return extra->useCount-- == 1;
     return false;
@@ -1686,6 +1704,7 @@ bool CTreeOptimizer::incUsage(IHqlExpression * expr)
     if (expr->isDataset() || expr->isDatarow())
         DBGLOG("%lx inc %d [%s]", (unsigned)expr, extra->useCount, queryNode0Text(expr));
 #endif
+    CHECK_EXPR_ID(expr);
     return (extra->useCount++ != 0);
 }
 
@@ -1704,6 +1723,7 @@ IHqlExpression * CTreeOptimizer::inheritUsage(IHqlExpression * newExpr, IHqlExpr
     if ((oldExtra->useCount == 0) && (newExpr->isDataset() || newExpr->isDatarow()))
         DBGLOG("Inherit0: %lx inherit %d,%d (from %lx)", (unsigned)newExpr, newExtra->useCount, oldExtra->useCount, (unsigned)oldExpr);
 #endif
+    CHECK_EXPR_ID(newExpr);
     newExtra->useCount += oldExtra->useCount;
     return newExpr;
 }
@@ -1813,7 +1833,11 @@ IHqlExpression * CTreeOptimizer::optimizeAggregateCompound(IHqlExpression * tran
         }
     }
     if (newOp)
-        return createDataset(newOp, removeChildNode(transformed));
+    {
+        OwnedHqlExpr modified = removeChildNode(transformed);
+        incUsage(modified);
+        return createDataset(newOp, modified.getClear());
+    }
     return NULL;
 }
 
@@ -2425,7 +2449,7 @@ IHqlExpression * CTreeOptimizer::doCreateTransformed(IHqlExpression * transforme
             }
 
             //MORE: The OHOinsidecompound isn't really good enough - because might remove projects from
-            //nested child aggregates which could benifit from them.  Probably not as long as all compound 
+            //nested child aggregates which could benefit from them.  Probably not as long as all compound
             //activities support aggregation.  In fact test should be removable everywhere once all 
             //engines support the new activities.
             if (isGrouped(transformed->queryChild(0)) || (queryRealChild(transformed, 3) && !(options & HOOinsidecompound)))
@@ -4068,6 +4092,22 @@ bool CTreeOptimizer::isShared(IHqlExpression * expr)
     case no_commonspill:
         return true;
     }
+
+#ifdef CHECK_USEAGE_COUNT
+    //Note: This currently fails when transforms are merged, because the new datasets in the merged transforms do not
+    //inherit the usage counts from datasets that have new expressions.
+    if (queryBodyExtra(expr)->useCount == 0)
+        throwUnexpected();
+#endif
+
+#if 0
+    //If enabled this prevents transformed datasets in child queries from being optimized.  In some cases that is correct
+    //in others it is invalid.  Some false positives can be avoided by repeatedly optimizing expressions.
+    //Revisit again with HPCC-20054.
+    if (queryBodyExtra(expr)->useCount == 0)
+        return true;
+#endif
+
     return (queryBodyExtra(expr)->useCount > 1);
 }
 

ecl/hql/hqlutil.cpp

@@ -1890,26 +1890,37 @@ IHqlExpression * getExpandSelectExpr(IHqlExpression * expr)
 
 IHqlExpression * replaceChildDataset(IHqlExpression * expr, IHqlExpression * newChild, unsigned whichChild)
 {
-    if (!(getChildDatasetType(expr) & childdataset_hasdataset))
+    if (getChildDatasetType(expr) & childdataset_hasdataset)
     {
-        HqlExprArray args;
-        unwindChildren(args, expr);
-        args.replace(*LINK(newChild), whichChild);
-        return expr->clone(args);
+        IHqlExpression * oldChild = expr->queryChild(whichChild);
+        if (oldChild->queryNormalizedSelector() != newChild->queryNormalizedSelector())
+        {
+            HqlMapSelectorTransformer mapper(oldChild, newChild);
+            HqlExprArray args;
+            ForEachChild(i, expr)
+            {
+                IHqlExpression * cur = expr->queryChild(i);
+                if (i == whichChild)
+                    args.append(*LINK(newChild));
+                else
+                    args.append(*mapper.transformRoot(cur));
+            }
+
+            return expr->clone(args);
+        }
     }
 
-    IHqlExpression * oldChild = expr->queryChild(whichChild);
-    HqlMapSelectorTransformer mapper(oldChild, newChild);
     HqlExprArray args;
-    ForEachChild(i, expr)
+    if (whichChild == 0)
     {
-        IHqlExpression * cur = expr->queryChild(i);
-        if (i == whichChild)
-            args.append(*LINK(newChild));
-        else
-            args.append(*mapper.transformRoot(cur));
+        args.append(*LINK(newChild));
+        unwindChildren(args, expr, 1);
+    }
+    else
+    {
+        unwindChildren(args, expr);
+        args.replace(*LINK(newChild), whichChild);
     }
-
     return expr->clone(args);
 }