HPCC-20595 When simplified expression is too complex, add to "do not simplify" list

Signed-off-by: Shamser Ahmed <shamser.ahmed@lexisnexis.co.uk>

ecl/eclcc/eclcc.cpp

@@ -1325,6 +1325,7 @@ void EclCC::processSingleQuery(EclCompileInstance & instance,
                 updateWorkunitStat(instance.wu, SSTcompilestage, "compile:cache", StNumAttribsSimplified, NULL, parseCtx.numAttribsSimplified);
                 updateWorkunitStat(instance.wu, SSTcompilestage, "compile:cache", StNumAttribsProcessed, NULL, parseCtx.numAttribsProcessed);
                 updateWorkunitStat(instance.wu, SSTcompilestage, "compile:cache", StNumAttribsFromCache, NULL, parseCtx.numAttribsFromCache);
+                updateWorkunitStat(instance.wu, SSTcompilestage, "compile:cache", StNumAttribsSimplifiedTooComplex, NULL, parseCtx.numSimplifiedTooComplex);
             }
 
             if (exportDependencies)

ecl/hql/hqlcache.cpp

@@ -133,7 +133,6 @@ protected:
         return EclCachedDefinition::calcUpToDate(optionHash);
     }
 
-
     const char * queryName() const { return cacheTree ? cacheTree->queryProp("@name") : nullptr; }
 
 private:

ecl/hql/hqlexpr.cpp

@@ -1071,7 +1071,7 @@ bool HqlParseContext::checkEndMeta()
     return wasGathering;
 }
 
-bool HqlParseContext::createCache(const char *simplifiedEcl, bool isMacro)
+bool HqlParseContext::createCache(const char * simplifiedEcl, bool isMacro)
 {
     StringBuffer fullName;
     StringBuffer baseFilename;

ecl/hql/hqlexpr.hpp

@@ -921,7 +921,7 @@ public:
     void beginMetaScope() { metaStack.append(*new FileParseMeta); }
     void beginMetaScope(FileParseMeta & active) { metaStack.append(OLINK(active)); }
     void endMetaScope() { metaStack.pop(); }
-    bool createCache(const char *simplifiedEcl, bool isMacro);
+    bool createCache(const char * simplifiedEcl, bool isMacro);
     inline FileParseMeta & curMeta() { return metaStack.tos(); }
     inline bool hasCacheLocation( ) const { return !metaOptions.cacheLocation.isEmpty();}
 public:
@@ -954,6 +954,7 @@ public:
     unsigned numAttribsSimplified = 0;
     unsigned numAttribsProcessed = 0;
     unsigned numAttribsFromCache = 0;
+    unsigned numSimplifiedTooComplex = 0;
 
 private:
     void createDependencyEntry(IHqlScope * scope, IIdAtom * name);
@@ -1032,6 +1033,7 @@ public:
     inline void incrementAttribsSimplified() { ++parseCtx.numAttribsSimplified; }
     inline void incrementAttribsProcessed() { ++parseCtx.numAttribsProcessed; }
     inline void incrementAttribsFromCache() { ++parseCtx.numAttribsFromCache; }
+    inline void incrementSimplifiedTooComplex() { ++parseCtx.numSimplifiedTooComplex; }
     inline bool neverSimplify(const char *fullname) { return parseCtx.neverSimplify(fullname); }
 protected:
 

ecl/hql/hqlgram2.cpp

@@ -12294,25 +12294,31 @@ bool parseForwardModuleMember(HqlGramCtx & _parent, IHqlScope *scope, IHqlExpres
 // if ctx.checkSimpleDef() then simplified definitions are created and checked - even if there is no cache configured.
 void parseAttribute(IHqlScope * scope, IFileContents * contents, HqlLookupContext & ctx, IIdAtom * name, const char * fullName)
 {
-    bool alreadySimplified = false;
+    bool usingSimplifiedFromCache = false;
     bool cacheUptoDate = false;
     HqlLookupContext attrCtx(ctx);
     attrCtx.noteBeginAttribute(scope, contents, name);
 
-    // Check if cache is up to date and obtain simplified definition from cache
+    // Set cacheUptoDate to true if cache is upto date
+    // Set usingSimplified to true if cache is upto date and option to ignoreSimplified & ignoreCache are both false
+    // Use simplified expression from cache if usingSimplified is true & syntaxChecking
+    // * None of these are set of ctx.regenerateCache==true, as the simplified expression & cache will need to regenerated.
     if (ctx.hasCacheLocation() && !ctx.regenerateCache())
     {
         HqlParseContext & parseContext = ctx.queryParseContext();
         Owned<IEclCachedDefinition> cached = parseContext.cache->getDefinition(fullName);
         cacheUptoDate = cached->isUpToDate(parseContext.optionHash);
-        if (cacheUptoDate && ctx.syntaxChecking() && !ctx.ignoreSimplified() && !ctx.ignoreCache())
+        if (cacheUptoDate && !ctx.ignoreSimplified() && !ctx.ignoreCache())
         {
-            IFileContents * cachecontents = cached->querySimplifiedEcl();
-            if (cachecontents)
+            usingSimplifiedFromCache = true;
+            if (ctx.syntaxChecking())
             {
-                contents = cachecontents;
-                alreadySimplified = true;
-                ctx.incrementAttribsFromCache();
+                IFileContents * cachecontents = cached->querySimplifiedEcl();
+                if (cachecontents)
+                {
+                    contents = cachecontents;
+                    ctx.incrementAttribsFromCache();
+                }
             }
         }
     }
@@ -12338,32 +12344,50 @@ void parseAttribute(IHqlScope * scope, IFileContents * contents, HqlLookupContex
     OwnedHqlExpr parsed = scope->lookupSymbol(name, LSFsharedOK|LSFnoreport, ctx);
     ctx.incrementAttribsProcessed();
 
-    if (parsed && !alreadySimplified)
+    if (parsed && !usingSimplifiedFromCache)
     {
         //Forward scopes cannot be cached because the dependencies are not known as it is parsed
         const bool canCache = parsed->getOperator() != no_forwardscope;
         if (canCache)
         {
             const bool isMacro = parsed->isMacro();
-            bool updateCache = ctx.hasCacheLocation() && (!cacheUptoDate || ctx.regenerateCache());
-            bool useSimplified = ctx.syntaxChecking() && !ctx.ignoreSimplified();
+            const bool updateCache = ctx.hasCacheLocation() && (!cacheUptoDate || ctx.regenerateCache());
+            const bool useSimplified = ctx.syntaxChecking() && !ctx.ignoreSimplified();
 
             OwnedHqlExpr simplified;
             StringBuffer simplifiedEcl;
+            // Simplified expression will be generated when
+            // - Not a macro and ignoreSimplified == false
+            // - And simplifiedExpression is required because cache is no up to date, verify option is used or syntaxChecking
+            // - And attribute has not been specifically excluded with the neverSimplify option
             if (!isMacro && !ctx.ignoreSimplified() &&
                 (updateCache || ctx.checkSimpleDef() || useSimplified) && !ctx.neverSimplify(fullName))
                 simplified.setown(createSimplifiedDefinition(parsed));
 
-            // create plain text ecl representation of the simplified expression (if it will be needed later)
+            // If plain text ecl representation of the simplified expression is needed for some reason
+            // (i.e. to update cache or verify simplified expression)
+            // And the simplified expression is less complex than the original expression
+            // Then a plain text ecl representation of the simplified expression is produced here
             if (simplified && (updateCache||ctx.checkSimpleDef()))
             {
                 regenerateDefinition(simplified, simplifiedEcl);
-                if (ctx.checkSimpleDef())
+                // Ensure the simplified expression is less complex
+                if (simplifiedEcl.length()<contents->length())
+                {
+                    if (ctx.checkSimpleDef())
+                    {
+                        simplifiedEcl.append("\n/* Simplified expression expression tree:\n");
+                        EclIR::getIRText(simplifiedEcl, 0, queryLocationIndependent(simplified));
+                        simplifiedEcl.append("*/\n");
+                    }
+                }
+                else
                 {
-                    // Dump of simplified expression in an ecl comment for diagnostics
-                    simplifiedEcl.append("\n/* Simplified expression IR:\n");
-                    EclIR::getIRText(simplifiedEcl, 0, queryLocationIndependent(simplified));
-                    simplifiedEcl.append("*/\n");
+                    // Simplified expr is more complex, so blank out plain text ecl string
+                    // to ensure it's not written to cache or used for verifying
+                    simplifiedEcl.clear();
+                    simplified.clear();
+                    ctx.incrementSimplifiedTooComplex();
                 }
             }
             if (updateCache)

system/jlib/jstatcodes.h

@@ -212,6 +212,7 @@ enum StatisticKind
     StNumAttribsFromCache,
     StNumSmartJoinDegradedToLocal,      // number of times global smart join degraded to local smart join (<=1 unless in loop)
     StNumSmartJoinSlavesDegradedToStd,  // number of times a slave in smart join degraded from local smart join to standard hash join
+    StNumAttribsSimplifiedTooComplex,
     StMax,
 
     //For any quantity there is potentially the following variants.

system/jlib/jstats.cpp

@@ -853,6 +853,7 @@ static const StatisticMeta statsMetaData[StMax] = {
     { NUMSTAT(AttribsFromCache) },
     { NUMSTAT(SmartJoinDegradedToLocal) },
     { NUMSTAT(SmartJoinSlavesDegradedToStd) },
+    { NUMSTAT(AttribsSimplifiedTooComplex) },
 };