Improve internal calculation of orm query plan hash key (Require use of inline hints like +query(50) to be comma delimited with properties) (#2159)

* WIP Tidy DefaultOrmQuery planDescription()

* WIP Tidy DefaultOrmQuery planDescription()

* Refactor OrmQueryPropertiesParser with hints requiring comma delimiting (previously didn't)

* Refactor OrmQueryPropertiesParser (reuse inputProperties) and DRawSqlColumnsParser (to return Set)

* Refactor OrmQueryPropertiesParser - remove JunkMain

* Refactor OrmQueryDetail, OrmQueryProperties - properties

ebean-core/src/main/java/io/ebeaninternal/server/query/SqlTreeBuilder.java

@@ -452,7 +452,7 @@ public final class SqlTreeBuilder {
     // Also note that this can include transient properties.
     // This makes sense for transient properties used to
     // hold sum() count() type values (with SqlSelect)
-    final Set<String> selectInclude = queryProps.getSelectInclude();
+    final Set<String> selectInclude = queryProps.getIncluded();
     for (String propName : selectInclude) {
       if (!propName.isEmpty()) {
         addProperty(selectProps, desc, queryProps, propName);

ebean-core/src/main/java/io/ebeaninternal/server/querydefn/DefaultOrmQuery.java

@@ -1136,81 +1136,81 @@ public class DefaultOrmQuery<T> implements SpiQuery<T> {
   private String planDescription() {
     StringBuilder sb = new StringBuilder(300);
     if (type != null) {
-      sb.append("t:").append(type.ordinal());
+      sb.append(type.ordinal());
     }
     if (useDocStore) {
-      sb.append(",ds:");
+      sb.append("/ds");
     }
     if (beanDescriptor.getDiscValue() != null) {
-      sb.append(",disc:").append(beanDescriptor.getDiscValue());
+      sb.append("/dv").append(beanDescriptor.getDiscValue());
     }
     if (temporalMode != SpiQuery.TemporalMode.CURRENT) {
-      sb.append(",temp:").append(temporalMode.ordinal());
+      sb.append("/tm").append(temporalMode.ordinal());
       if (versionsStart != null) {
-        sb.append(",vb:");
+        sb.append("v");
       }
     }
     if (forUpdate != null) {
-      sb.append(",forUpd:").append(forUpdate.ordinal());
+      sb.append("/fu").append(forUpdate.ordinal());
       if (lockType != null) {
-        sb.append(",lt:").append(lockType.ordinal());
+        sb.append("t").append(lockType.ordinal());
       }
     }
     if (id != null) {
-      sb.append(",id:");
+      sb.append("/id");
     }
     if (manualId) {
-      sb.append(",manId:");
+      sb.append("/md");
     }
     if (distinct) {
-      sb.append(",dist:");
+      sb.append("/dt");
     }
     if (allowLoadErrors) {
-      sb.append(",allowLoadErrors:");
+      sb.append("/ae");
     }
     if (disableLazyLoading) {
-      sb.append(",disLazy:");
+      sb.append("/dl");
     }
     if (baseTable != null) {
-      sb.append(",baseTable:").append(baseTable);
+      sb.append("/bt").append(baseTable);
     }
     if (rootTableAlias != null) {
-      sb.append(",root:").append(rootTableAlias);
+      sb.append("/ra").append(rootTableAlias);
     }
     if (orderBy != null) {
-      sb.append(",orderBy:").append(orderBy.toStringFormat());
+      sb.append("/ob").append(orderBy.toStringFormat());
     }
     if (m2mIncludeJoin != null) {
-      sb.append(",m2m:").append(m2mIncludeJoin.getTable());
+      sb.append("/m2").append(m2mIncludeJoin.getTable());
     }
     if (mapKey != null) {
-      sb.append(",mapKey:").append(mapKey);
+      sb.append("/mk").append(mapKey);
     }
     if (countDistinctOrder != null) {
-      sb.append(",countDistOrd:").append(countDistinctOrder.name());
+      sb.append("/cd").append(countDistinctOrder.name());
     }
     if (detail != null) {
-      sb.append(" detail[");
+      sb.append("/d[");
       detail.queryPlanHash(sb);
       sb.append("]");
     }
     if (bindParams != null) {
-      sb.append(" bindParams[");
+      sb.append("/b[");
       bindParams.buildQueryPlanHash(sb);
       sb.append("]");
     }
     if (whereExpressions != null) {
-      sb.append(" where[");
+      sb.append("/w[");
       whereExpressions.queryPlanHash(sb);
       sb.append("]");
     }
     if (havingExpressions != null) {
-      sb.append(" having[");
+      sb.append("/h[");
       havingExpressions.queryPlanHash(sb);
       sb.append("]");
     }
     if (updateProperties != null) {
-      sb.append(" update[");
+      sb.append("/u[");
       updateProperties.buildQueryPlanHash(sb);
       sb.append("]");
     }

ebean-core/src/main/java/io/ebeaninternal/server/querydefn/OrmQueryDetail.java

@@ -106,25 +106,20 @@ public class OrmQueryDetail implements Serializable {
     return p1 == null ? p2 == null : p1.isSameByAutoTune(p2);
   }
 
-  @Override
-  public String toString() {
-    return asString();
-  }
-
   /**
    * Return the detail in string form.
    */
-  public String asString() {
+  public String asStringDebug() {
     StringBuilder sb = new StringBuilder();
     if (!baseProps.isEmpty()) {
-      baseProps.append("select ", sb);
+      baseProps.asStringDebug("select ", sb);
     }
     if (fetchPaths != null) {
       for (OrmQueryProperties join : fetchPaths.values()) {
         if (sb.length() > 0) {
           sb.append(" ");
         }
-        join.append("fetch ", sb);
+        join.asStringDebug("fetch ", sb);
       }
     }
     return sb.toString();
@@ -150,7 +145,7 @@ public class OrmQueryDetail implements Serializable {
    * Set the base query properties to be empty.
    */
   public void setEmptyBase() {
-    this.baseProps = new OrmQueryProperties(null, new LinkedHashSet<>());
+    this.baseProps = new OrmQueryProperties(null, Collections.emptySet());
   }
 
   /**
@@ -316,7 +311,7 @@ public class OrmQueryDetail implements Serializable {
           if (addId) {
             parentProp = new OrmQueryProperties(parentPath, assocOne.getTargetIdProperty());
           } else {
-            parentProp = new OrmQueryProperties(parentPath, new LinkedHashSet<>());
+            parentProp = new OrmQueryProperties(parentPath, Collections.emptySet());
           }
         }
 

ebean-core/src/main/java/io/ebeaninternal/server/querydefn/OrmQueryPlanKey.java

@@ -29,7 +29,7 @@ class OrmQueryPlanKey implements CQueryPlanKey {
 
   @Override
   public CQueryPlanKey withDeleteByIds() {
-    return new OrmQueryPlanKey(description + ":deleteByIds", 0, 0, null);
+    return new OrmQueryPlanKey(description + "/deleteByIds", 0, 0, null);
   }
 
   @Override

ebean-core/src/main/java/io/ebeaninternal/server/querydefn/OrmQueryProperties.java

@@ -30,12 +30,8 @@ public class OrmQueryProperties implements Serializable {
 
   private final String parentPath;
   private final String path;
-
-  private final String rawProperties;
-  private final String trimmedProperties;
-
-  private final LinkedHashSet<String> included;
-
+  private final String properties;
+  private final Set<String> included;
   private final FetchConfig fetchConfig;
 
   /**
@@ -84,8 +80,7 @@ public class OrmQueryProperties implements Serializable {
   public OrmQueryProperties(String path) {
     this.path = path;
     this.parentPath = SplitName.parent(path);
-    this.rawProperties = null;
-    this.trimmedProperties = null;
+    this.properties = null;
     this.included = null;
     this.fetchConfig = DEFAULT_FETCH;
   }
@@ -100,8 +95,7 @@ public class OrmQueryProperties implements Serializable {
 
     this.path = path;
     this.parentPath = SplitName.parent(path);
-    this.rawProperties = rawProperties;
-    this.trimmedProperties = response.properties;
+    this.properties = response.properties;
     this.included = response.included;
     this.cache = response.cache;
     this.readOnly = response.readOnly;
@@ -115,39 +109,17 @@ public class OrmQueryProperties implements Serializable {
     }
   }
 
-  public OrmQueryProperties(String path, LinkedHashSet<String> parsedProperties) {
-    if (parsedProperties == null) {
-      throw new IllegalArgumentException("parsedProperties is null");
-    }
-
+  public OrmQueryProperties(String path, Set<String> included) {
     this.path = path;
     this.parentPath = SplitName.parent(path);
     // for rawSql parsedProperties can be empty (when only fetching Id property)
-    this.included = parsedProperties;
-    this.rawProperties = join(parsedProperties);
-    this.trimmedProperties = rawProperties;
+    this.included = included;
+    this.properties = String.join(",", included);
     this.cache = false;
     this.readOnly = false;
     this.fetchConfig = DEFAULT_FETCH;
   }
 
-  /**
-   * Join the set of properties into a comma delimited string.
-   */
-  private String join(LinkedHashSet<String> parsedProperties) {
-    StringBuilder sb = new StringBuilder(50);
-    boolean first = true;
-    for (String property : parsedProperties) {
-      if (first) {
-        first = false;
-      } else {
-        sb.append(",");
-      }
-      sb.append(property);
-    }
-    return sb.toString();
-  }
-
   /**
    * Copy constructor.
    */
@@ -155,8 +127,7 @@ public class OrmQueryProperties implements Serializable {
     this.fetchConfig = sourceFetchConfig;
     this.parentPath = source.parentPath;
     this.path = source.path;
-    this.rawProperties = source.rawProperties;
-    this.trimmedProperties = source.trimmedProperties;
+    this.properties = source.properties;
     this.cache = source.cache;
     this.readOnly = source.readOnly;
     this.filterMany = source.filterMany;
@@ -240,8 +211,8 @@ public class OrmQueryProperties implements Serializable {
   @SuppressWarnings("unchecked")
   public void configureBeanQuery(SpiQuery<?> query) {
 
-    if (trimmedProperties != null && !trimmedProperties.isEmpty()) {
-      query.select(trimmedProperties);
+    if (properties != null && !properties.isEmpty()) {
+      query.select(properties);
     }
 
     if (filterMany != null) {
@@ -268,7 +239,7 @@ public class OrmQueryProperties implements Serializable {
   }
 
   public boolean hasSelectClause() {
-    if ("*".equals(trimmedProperties)) {
+    if ("*".equals(properties)) {
       // explicitly selected all properties
       return true;
     }
@@ -280,25 +251,17 @@ public class OrmQueryProperties implements Serializable {
    * Return true if the properties and configuration are empty.
    */
   public boolean isEmpty() {
-    return rawProperties == null || rawProperties.isEmpty();
+    return properties == null || properties.isEmpty();
   }
 
-  @Override
-  public String toString() {
-    StringBuilder sb = new StringBuilder(40);
-    append("", sb);
-    return sb.toString();
-  }
-
-  public String append(String prefix, StringBuilder sb) {
+  public void asStringDebug(String prefix, StringBuilder sb) {
     sb.append(prefix);
     if (path != null) {
       sb.append(path).append(" ");
     }
     if (!isEmpty()) {
-      sb.append("(").append(rawProperties).append(")");
+      sb.append("(").append(properties).append(")");
     }
-    return sb.toString();
   }
 
   boolean isChild(OrmQueryProperties possibleChild) {
@@ -319,7 +282,7 @@ public class OrmQueryProperties implements Serializable {
    * Return the raw properties.
    */
   public String getProperties() {
-    return rawProperties;
+    return properties;
   }
 
   /**
@@ -350,10 +313,6 @@ public class OrmQueryProperties implements Serializable {
     includedBeanJoin.add(propertyName);
   }
 
-  public Set<String> getSelectInclude() {
-    return included;
-  }
-
   public Set<String> getSelectQueryJoin() {
     return secondaryQueryJoins;
   }
@@ -373,7 +332,6 @@ public class OrmQueryProperties implements Serializable {
   }
 
   boolean isIncluded(String propName) {
-
     if (includedBeanJoin != null && includedBeanJoin.contains(propName)) {
       return false;
     }
@@ -474,24 +432,21 @@ public class OrmQueryProperties implements Serializable {
    * Calculate the query plan hash.
    */
   public void queryPlanHash(StringBuilder builder) {
-
-    builder.append("qpp[");
+    builder.append("p[");
     builder.append(path);
     if (included != null){
-      builder.append(" included:").append(included);
+      builder.append("/i").append(included);
     }
     if (secondaryQueryJoins != null) {
-      builder.append(" secondary:").append(secondaryQueryJoins);
+      builder.append("/s").append(secondaryQueryJoins);
     }
-
     if (filterMany != null) {
-      builder.append(" filterMany[");
+      builder.append("/f[");
       filterMany.queryPlanHash(builder);
       builder.append("]");
     }
-
     if (fetchConfig != null) {
-      builder.append(" config:").append(fetchConfig.hashCode());
+      builder.append("/c:").append(fetchConfig.hashCode());
     }
     builder.append("]");
   }

ebean-core/src/main/java/io/ebeaninternal/server/querydefn/OrmQueryPropertiesParser.java

@@ -3,8 +3,8 @@ package io.ebeaninternal.server.querydefn;
 import io.ebean.FetchConfig;
 import io.ebeaninternal.server.util.DSelectColumnsParser;
 
-import java.util.LinkedHashSet;
-import java.util.List;
+import java.util.Iterator;
+import java.util.Set;
 
 /**
  * Parses the path properties string.
@@ -22,9 +22,9 @@ class OrmQueryPropertiesParser {
     final boolean cache;
     final FetchConfig fetchConfig;
     final String properties;
-    final LinkedHashSet<String> included;
+    final Set<String> included;
 
-    Response(boolean readOnly, boolean cache, int queryFetchBatch, int lazyFetchBatch, String properties, LinkedHashSet<String> included) {
+    Response(boolean readOnly, boolean cache, int queryFetchBatch, int lazyFetchBatch, String properties, Set<String> included) {
       this.readOnly = readOnly;
       this.cache = cache;
       this.properties = properties;
@@ -53,9 +53,7 @@ class OrmQueryPropertiesParser {
     return new OrmQueryPropertiesParser(rawProperties).parse();
   }
 
-  private String inputProperties;
-
-  private String outputProperties = "";
+  private final String inputProperties;
   private boolean allProperties;
   private boolean readOnly;
   private boolean cache;
@@ -70,124 +68,61 @@ class OrmQueryPropertiesParser {
    * Parse the raw string properties input.
    */
   private Response parse() {
-
     if (inputProperties == null || inputProperties.isEmpty()) {
       return EMPTY;
     }
-    int pos = inputProperties.indexOf("+readonly");
-    if (pos > -1) {
-      inputProperties = inputProperties.replace("+readonly", "");
-      readOnly = true;
-    }
-    pos = inputProperties.indexOf("+cache");
-    if (pos > -1) {
-      inputProperties = inputProperties.replace("+cache", "");
-      cache = true;
-    }
-    pos = inputProperties.indexOf("+query");
-    if (pos > -1) {
-      queryFetchBatch = parseBatchHint(pos, "+query");
-    }
-    pos = inputProperties.indexOf("+lazy");
-    if (pos > -1) {
-      lazyFetchBatch = parseBatchHint(pos, "+lazy");
-    }
-
-    LinkedHashSet<String> included = parseIncluded();
-    String properties = (allProperties) ? "*" : outputProperties;
-    return new Response(readOnly, cache, queryFetchBatch, lazyFetchBatch, properties, included);
-  }
-
-  /**
-   * Parse the include separating by comma or semicolon.
-   */
-  private LinkedHashSet<String> parseIncluded() {
-
-    inputProperties = inputProperties.trim();
-    if (inputProperties.isEmpty()) {
-      // default properties
-      return null;
-    }
     if (inputProperties.equals("*")) {
       // explicit all properties
       allProperties = true;
-      return null;
+      return new Response(readOnly, cache, queryFetchBatch, lazyFetchBatch, "*", null);
     }
-
-    List<String> res = splitRawSelect(inputProperties);
-
-    StringBuilder sb = new StringBuilder(70);
-    LinkedHashSet<String> propertySet = new LinkedHashSet<>(res.size() * 2);
-
-    int count = 0;
-    String temp;
-    for (String re : res) {
-      temp = re.trim();
-      if (!temp.isEmpty()) {
-        if (count > 0) {
-          sb.append(",");
-        }
-        sb.append(temp);
-        propertySet.add(temp);
-        count++;
+    boolean hints = false;
+    Set<String> fields = splitRawSelect(inputProperties);
+    final Iterator<String> iterator = fields.iterator();
+    while (iterator.hasNext()) {
+      String val = iterator.next();
+      if (val.startsWith("+")) {
+        hints = true;
+        iterator.remove();
+        parseHint(val);
+      } else if (val.equals("*")) {
+        allProperties = true;
       }
     }
-
-    if (propertySet.isEmpty()) {
-      // default properties
-      return null;
+    String properties = allProperties ? "*" : hints ? String.join(",", fields) : inputProperties;
+    if (fields.isEmpty()) {
+      fields = null;
     }
+    return new Response(readOnly, cache, queryFetchBatch, lazyFetchBatch, properties, fields);
+  }
 
-    if (propertySet.contains("*")) {
-      // explicit all properties
-      allProperties = true;
-      return null;
+  private void parseHint(String val) {
+    if (val.equals("+readonly")) {
+      readOnly = true;
+    } else if (val.equals("+cache")) {
+      cache = true;
+    } else if (val.startsWith("+query")) {
+      queryFetchBatch = parseBatch(val);
+    } else if (val.startsWith("+lazy")) {
+      lazyFetchBatch = parseBatch(val);
     }
+  }
 
-    // partial properties
-    outputProperties = sb.toString();
-    return propertySet;
+  private int parseBatch(String val) {
+    if (val.endsWith(")")) {
+      int start = val.lastIndexOf('(');
+      if (start > 0) {
+        return Integer.parseInt(val.substring(start + 1, val.length() - 1));
+      }
+    }
+    return 0;
   }
 
   /**
    * Split allowing 'dynamic function based properties'.
    */
-  private List<String> splitRawSelect(String inputProperties) {
+  private Set<String> splitRawSelect(String inputProperties) {
     return DSelectColumnsParser.parse(inputProperties);
   }
 
-  private int parseBatchHint(int pos, String option) {
-
-    int startPos = pos + option.length();
-    int endPos = findEndPos(startPos, inputProperties);
-    if (endPos == -1) {
-      inputProperties = inputProperties.replace(option, "");
-      return 0;
-
-    } else {
-
-      String batchParam = inputProperties.substring(startPos + 1, endPos);
-
-      if (endPos + 1 >= inputProperties.length()) {
-        inputProperties = inputProperties.substring(0, pos);
-      } else {
-        inputProperties = inputProperties.substring(0, pos) + inputProperties.substring(endPos + 1);
-      }
-      return Integer.parseInt(batchParam);
-    }
-  }
-
-  private int findEndPos(int pos, String props) {
-
-    if (pos < props.length()) {
-      if (props.charAt(pos) == '(') {
-        int endPara = props.indexOf(')', pos + 1);
-        if (endPara == -1) {
-          throw new RuntimeException("Error could not find ')' in " + props + " after position " + pos);
-        }
-        return endPara;
-      }
-    }
-    return -1;
-  }
 }

ebean-core/src/main/java/io/ebeaninternal/server/rawsql/DRawSqlColumnsParser.java

@@ -6,6 +6,7 @@ import io.ebeaninternal.server.util.DSelectColumnsParser;
 import javax.persistence.PersistenceException;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Set;
 import java.util.regex.Pattern;
 
 /**
@@ -28,11 +29,8 @@ final class DRawSqlColumnsParser {
   }
 
   private ColumnMapping parse() {
-
-    List<String> columnList = DSelectColumnsParser.parse(sqlSelect);
-
+    Set<String> columnList = DSelectColumnsParser.parse(sqlSelect);
     List<ColumnMapping.Column> columns = new ArrayList<>(columnList.size());
-
     for (String rawColumn : columnList) {
       columns.add(parseColumn(rawColumn));
     }
@@ -40,7 +38,6 @@ final class DRawSqlColumnsParser {
   }
 
   private ColumnMapping.Column parseColumn(String colInfo) {
-
     String[] split = COLINFO_SPLIT.split(colInfo);
     if (split.length > 1) {
       ArrayList<String> tmp = new ArrayList<>(split.length);

ebean-core/src/main/java/io/ebeaninternal/server/util/DSelectColumnsParser.java

@@ -1,7 +1,7 @@
 package io.ebeaninternal.server.util;
 
-import java.util.ArrayList;
-import java.util.List;
+import java.util.LinkedHashSet;
+import java.util.Set;
 
 /**
  * Splits a select clause into 'logical columns' taking into account functions and quotes.
@@ -14,7 +14,7 @@ public final class DSelectColumnsParser {
 
   private int pos;
 
-  public static List<String> parse(String sqlSelect) {
+  public static Set<String> parse(String sqlSelect) {
     return new DSelectColumnsParser(sqlSelect).parse();
   }
 
@@ -23,9 +23,8 @@ public final class DSelectColumnsParser {
     this.end = selectClause.length();
   }
 
-  private List<String> parse() {
-
-    ArrayList<String> columns = new ArrayList<>();
+  private Set<String> parse() {
+    LinkedHashSet<String> columns = new LinkedHashSet<>();
     while (pos <= end) {
       columns.add(nextColumnInfo());
     }