Address some of the issues from the code review

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

ecl/hql/hqlutil.cpp

@@ -3235,6 +3235,41 @@ IHqlExpression * createGetResultFromSetResult(IHqlExpression * setResult, ITypeI
     return createValue(no_getresult, valueType.getLink(), LINK(seqAttr), LINK(aliasAttr));
 }
 
+
+//---------------------------------------------------------------------------------------------------------------------
+
+IHqlExpression * convertScalarToGraphResult(IHqlExpression * value, ITypeInfo * fieldType, IHqlExpression * represents, unsigned seq)
+{
+    OwnedHqlExpr row = convertScalarToRow(value, fieldType);
+    OwnedHqlExpr ds = createDatasetFromRow(LINK(row));
+    HqlExprArray args;
+    args.append(*LINK(ds));
+    args.append(*LINK(represents));
+    args.append(*getSizetConstant(seq));
+    args.append(*createAttribute(rowAtom));
+    return createValue(no_setgraphresult, makeVoidType(), args);
+}
+
+static IHqlExpression * createScalarFromGraphResult(ITypeInfo * scalarType, ITypeInfo * fieldType, IHqlExpression * represents, unsigned seq)
+{
+    OwnedHqlExpr counterField = createField(unnamedAtom, LINK(fieldType), NULL, NULL);
+    OwnedHqlExpr counterRecord = createRecord(counterField);
+    HqlExprArray args;
+    args.append(*LINK(counterRecord));
+    args.append(*LINK(represents));
+    args.append(*getSizetConstant(seq));
+    args.append(*createAttribute(rowAtom));
+    OwnedHqlExpr counterResult = createDataset(no_getgraphresult, args);
+    OwnedHqlExpr select = createNewSelectExpr(createRow(no_selectnth, LINK(counterResult), getSizetConstant(1)), LINK(counterField));
+    OwnedHqlExpr cast = ensureExprType(select, scalarType);
+    return createAlias(cast, internalAttrExpr);
+}
+
+IHqlExpression * createCounterAsGraphResult(IHqlExpression * counter, IHqlExpression * represents, unsigned seq)
+{
+    return createScalarFromGraphResult(counter->queryType(), unsignedType, represents, seq);
+}
+
 _ATOM queryCsvEncoding(IHqlExpression * mode)
 {
     if (mode)

ecl/hql/hqlutil.hpp

@@ -129,6 +129,10 @@ extern HQL_API void unwindFields(HqlExprCopyArray & fields, IHqlExpression * rec
 extern HQL_API unsigned numAttributes(const HqlExprArray & args);
 extern HQL_API unsigned numAttributes(const IHqlExpression * expr);
 extern HQL_API IHqlExpression * createGetResultFromSetResult(IHqlExpression * setExpr, ITypeInfo * type=NULL);
+
+extern HQL_API IHqlExpression * convertScalarToGraphResult(IHqlExpression * value, ITypeInfo * fieldType, IHqlExpression * represents, unsigned seq);
+extern HQL_API IHqlExpression * createCounterAsGraphResult(IHqlExpression * counter, IHqlExpression * represents, unsigned seq);
+
 extern HQL_API IHqlExpression * createTrimExpr(IHqlExpression * value, IHqlExpression * flags);
 extern HQL_API bool isLengthPreservingCast(IHqlExpression * expr);
 

ecl/hqlcpp/hqlcpp.ipp

@@ -1963,8 +1963,6 @@ protected:
     unsigned numResults;
 };
 
-//move to a better header file
-IHqlExpression * createCounterAsResult(IHqlExpression * counter, IHqlExpression * represents, unsigned seq);
 
 //===========================================================================
 

ecl/hqlcpp/hqlhoist.cpp

@@ -1,6 +1,6 @@
 /*##############################################################################
 
-    Copyright (C) 2011 HPCC Systems.
+    Copyright (C) 2012 HPCC Systems.
 
     All rights reserved. This program is free software: you can redistribute it and/or modify
     it under the terms of the GNU Affero General Public License as
@@ -15,6 +15,7 @@
     You should have received a copy of the GNU Affero General Public License
     along with this program.  If not, see <http://www.gnu.org/licenses/>.
 ############################################################################## */
+
 #include "platform.h"
 #include "jlib.hpp"
 
@@ -553,8 +554,8 @@ CHqlExprMultiGuard * ConditionalContextTransformer::createIfGuard(ConditionalCon
     if (!trueGuard && !falseGuard && !cur->isCandidateThatMoves())
         return LINK(condGuard);
 
-    //If you want to common up the conditions between the child query and the parent code you might acheive
-    //if with forcing the condition into an alias.  E.g.,
+    //If you want to common up the conditions between the child query and the parent code you might achieve
+    //it by forcing the condition into an alias.  E.g.,
     //queryBodyExtra(ifCond)->createAlias = true;
 
     Owned<CHqlExprMultiGuard> newGuards = new CHqlExprMultiGuard;

ecl/hqlcpp/hqlhoist.hpp

@@ -1,6 +1,6 @@
 /*##############################################################################
 
-    Copyright (C) 2011 HPCC Systems.
+    Copyright (C) 2012 HPCC Systems.
 
     All rights reserved. This program is free software: you can redistribute it and/or modify
     it under the terms of the GNU Affero General Public License as

ecl/hqlcpp/hqlhtcpp.cpp

@@ -365,12 +365,13 @@ public:
     NewChildDatasetSpotter(HqlCppTranslator & _translator, BuildCtx & _ctx, bool _forceRoot)
         : ConditionalContextTransformer(childDatasetSpotterInfo, true), translator(_translator), ctx(_ctx), forceRoot(_forceRoot)
     {
-        //The child dataset code could create a single no_childquery at any place in the graph, or multiple subgraphs
-        //however adding a no_compound(no_childquery, ...) into the tree can cause problems for subsequent optimizations
-        //if any of them caused the no_getgraphresult to be hoisted before the no_childquery it would create an
-        //invalid expression.
-        //Instead, add the no_childquery before the first item that changes, and repeat for children of if actions.
-        createRootGraph = true;//_forceRoot;
+        //The following line forces the conditionalContextTransformer code to generate a single root subgraph.
+        //An alternative would be to generate one (or more) graphs at the first unconditional place they are
+        //used.  e.g., adding a no_compound(no_childquery, f(no_getgraphresult)) into the tree.
+        //This was the initial approach, but it causes problems for subsequent optimizations -
+        //if an optimzation causes an expression containing the no_getgraphresult to be hoisted so it is
+        //evaluated before the no_childquery it creates an out-of-order dependency.
+        createRootGraph = true;
     }
 
     virtual void analyseExpr(IHqlExpression * expr)
@@ -7951,41 +7952,6 @@ ABoundActivity * HqlCppTranslator::doBuildActivitySpill(BuildCtx & ctx, IHqlExpr
 }
 
 
-//---------------------------------------------------------------------------------------------------------------------
-
-static IHqlExpression * convertScalarToResult(IHqlExpression * value, ITypeInfo * fieldType, IHqlExpression * represents, unsigned seq)
-{
-    OwnedHqlExpr row = convertScalarToRow(value, fieldType);
-    OwnedHqlExpr ds = createDatasetFromRow(LINK(row));
-    HqlExprArray args;
-    args.append(*LINK(ds));
-    args.append(*LINK(represents));
-    args.append(*getSizetConstant(seq));
-    args.append(*createAttribute(rowAtom));
-    return createValue(no_setgraphresult, makeVoidType(), args);
-}
-
-static IHqlExpression * createScalarFromResult(ITypeInfo * scalarType, ITypeInfo * fieldType, IHqlExpression * represents, unsigned seq)
-{
-    OwnedHqlExpr counterField = createField(unnamedAtom, LINK(fieldType), NULL, NULL);
-    OwnedHqlExpr counterRecord = createRecord(counterField);
-    HqlExprArray args;
-    args.append(*LINK(counterRecord));
-    args.append(*LINK(represents));
-    args.append(*getSizetConstant(seq));
-    args.append(*createAttribute(rowAtom));
-    OwnedHqlExpr counterResult = createDataset(no_getgraphresult, args);
-    OwnedHqlExpr select = createNewSelectExpr(createRow(no_selectnth, LINK(counterResult), getSizetConstant(1)), LINK(counterField));
-    OwnedHqlExpr cast = ensureExprType(select, scalarType);
-    return createAlias(cast, internalAttrExpr);
-}
-
-IHqlExpression * createCounterAsResult(IHqlExpression * counter, IHqlExpression * represents, unsigned seq)
-{
-    return createScalarFromResult(counter->queryType(), unsignedType, represents, seq);
-}
-
-
 bool HqlCppTranslator::isCurrentActiveGraph(BuildCtx & ctx, IHqlExpression * graphTag)
 {
     SubGraphInfo * activeSubgraph = queryActiveSubGraph(ctx);
@@ -8024,7 +7990,7 @@ IHqlExpression * HqlCppTranslator::createLoopSubquery(IHqlExpression * dataset,
     OwnedHqlExpr counterResult;
     if (counter)
     {
-        OwnedHqlExpr select = createCounterAsResult(counter, graphid, 2);
+        OwnedHqlExpr select = createCounterAsGraphResult(counter, graphid, 2);
         transformedBody.setown(replaceExpression(transformedBody, counter, select));
         if (transformedAgain)
         {
@@ -8061,9 +8027,9 @@ IHqlExpression * HqlCppTranslator::createLoopSubquery(IHqlExpression * dataset,
         //MORE: Don't let this get past review - purely to ensure the same code is generated.
         if (loopAgainResult == 2)
             loopAgainResult = graphBuilder.addInput();
-        //more: Add loopAgainResult as an attribute on the no_childquery rather than using a reference parameter
+        //MORE: Add loopAgainResult as an attribute on the no_childquery rather than using a reference parameter
 
-        OwnedHqlExpr againResult = convertScalarToResult(transformedAgain, queryBoolType(), graphid, loopAgainResult);
+        OwnedHqlExpr againResult = convertScalarToGraphResult(transformedAgain, queryBoolType(), graphid, loopAgainResult);
         graphBuilder.addAction(againResult);
 
     }