Merge pull request #1120 from grandinj/1097_slow_querygroup

#1097 reduce memory overhead of the group by data structures

h2/src/main/org/h2/expression/Aggregate.java

@@ -283,8 +283,7 @@ public class Aggregate extends Expression {
         // if (on != null) {
         // on.updateAggregate();
         // }
-        HashMap<Expression, Object> group = select.getCurrentGroup();
-        if (group == null) {
+        if (!select.isCurrentGroup()) {
             // this is a different level (the enclosing query)
             return;
         }
@@ -296,10 +295,10 @@ public class Aggregate extends Expression {
         }
         lastGroupRowId = groupRowId;
 
-        AggregateData data = (AggregateData) group.get(this);
+        AggregateData data = (AggregateData) select.getCurrentGroupExprData(this);
         if (data == null) {
             data = AggregateData.create(type);
-            group.put(this, data);
+            select.setCurrentGroupExprData(this, data);
         }
         Value v = on == null ? null : on.getValue(session);
         if (type == AggregateType.GROUP_CONCAT) {
@@ -372,13 +371,13 @@ public class Aggregate extends Expression {
                 DbException.throwInternalError("type=" + type);
             }
         }
-        HashMap<Expression, Object> group = select.getCurrentGroup();
-        if (group == null) {
-            throw DbException.get(ErrorCode.INVALID_USE_OF_AGGREGATE_FUNCTION_1, getSQL());
+        if (!select.isCurrentGroup()) {    
+            throw DbException.get(ErrorCode.INVALID_USE_OF_AGGREGATE_FUNCTION_1, getSQL());    
         }
-        AggregateData data = (AggregateData) group.get(this);
+        AggregateData data = (AggregateData)select.getCurrentGroupExprData(this);
         if (data == null) {
             data = AggregateData.create(type);
+            select.setCurrentGroupExprData(this, data);
         }
         if (type == AggregateType.GROUP_CONCAT) {
             Value[] array = ((AggregateDataCollecting) data).getArray();

h2/src/main/org/h2/expression/AggregateDataHistogram.java

@@ -7,6 +7,7 @@ package org.h2.expression;
 
 import java.util.Arrays;
 import java.util.Comparator;
+import java.util.Map;
 import org.h2.engine.Constants;
 import org.h2.engine.Database;
 import org.h2.util.ValueHashMap;
@@ -47,9 +48,9 @@ class AggregateDataHistogram extends AggregateData {
         }
         ValueArray[] values = new ValueArray[distinctValues.size()];
         int i = 0;
-        for (Value dv : distinctValues.keys()) {
-            AggregateDataHistogram d = distinctValues.get(dv);
-            values[i] = ValueArray.get(new Value[] { dv, ValueLong.get(d.count) });
+        for (Map.Entry<Value,AggregateDataHistogram> entry : distinctValues.entries()) {
+            AggregateDataHistogram d = entry.getValue();
+            values[i] = ValueArray.get(new Value[] { entry.getKey(), ValueLong.get(d.count) });
             i++;
         }
         final CompareMode compareMode = database.getCompareMode();

h2/src/main/org/h2/expression/ExpressionColumn.java

@@ -159,14 +159,13 @@ public class ExpressionColumn extends Expression {
         if (select == null) {
             throw DbException.get(ErrorCode.MUST_GROUP_BY_COLUMN_1, getSQL());
         }
-        HashMap<Expression, Object> values = select.getCurrentGroup();
-        if (values == null) {
+        if (!select.isCurrentGroup()) {
             // this is a different level (the enclosing query)
             return;
         }
-        Value v = (Value) values.get(this);
+        Value v = (Value) select.getCurrentGroupExprData(this);
         if (v == null) {
-            values.put(this, now);
+            select.setCurrentGroupExprData(this, now);
         } else {
             if (!database.areEqual(now, v)) {
                 throw DbException.get(ErrorCode.MUST_GROUP_BY_COLUMN_1, getSQL());
@@ -178,9 +177,8 @@ public class ExpressionColumn extends Expression {
     public Value getValue(Session session) {
         Select select = columnResolver.getSelect();
         if (select != null) {
-            HashMap<Expression, Object> values = select.getCurrentGroup();
-            if (values != null) {
-                Value v = (Value) values.get(this);
+            if (select.isCurrentGroup()) {
+                Value v = (Value) select.getCurrentGroupExprData(this);
                 if (v != null) {
                     return v;
                 }

h2/src/main/org/h2/expression/JavaAggregate.java

@@ -167,15 +167,14 @@ public class JavaAggregate extends Expression {
 
     @Override
     public Value getValue(Session session) {
-        HashMap<Expression, Object> group = select.getCurrentGroup();
-        if (group == null) {
+        if (!select.isCurrentGroup()) {
             throw DbException.get(ErrorCode.INVALID_USE_OF_AGGREGATE_FUNCTION_1, getSQL());
         }
         try {
             Aggregate agg;
             if (distinct) {
                 agg = getInstance();
-                AggregateDataCollecting data = (AggregateDataCollecting) group.get(this);
+                AggregateDataCollecting data = (AggregateDataCollecting) select.getCurrentGroupExprData(this);
                 if (data != null) {
                     for (Value value : data.values) {
                         if (args.length == 1) {
@@ -191,7 +190,7 @@ public class JavaAggregate extends Expression {
                     }
                 }
             } else {
-                agg = (Aggregate) group.get(this);
+                agg = (Aggregate) select.getCurrentGroupExprData(this);
                 if (agg == null) {
                     agg = getInstance();
                 }
@@ -208,8 +207,7 @@ public class JavaAggregate extends Expression {
 
     @Override
     public void updateAggregate(Session session) {
-        HashMap<Expression, Object> group = select.getCurrentGroup();
-        if (group == null) {
+        if (!select.isCurrentGroup()) {
             // this is a different level (the enclosing query)
             return;
         }
@@ -229,10 +227,10 @@ public class JavaAggregate extends Expression {
 
         try {
             if (distinct) {
-                AggregateDataCollecting data = (AggregateDataCollecting) group.get(this);
+                AggregateDataCollecting data = (AggregateDataCollecting) select.getCurrentGroupExprData(this);
                 if (data == null) {
                     data = new AggregateDataCollecting();
-                    group.put(this, data);
+                    select.setCurrentGroupExprData(this, data);
                 }
                 Value[] argValues = new Value[args.length];
                 Value arg = null;
@@ -243,10 +241,10 @@ public class JavaAggregate extends Expression {
                 }
                 data.add(session.getDatabase(), dataType, true, args.length == 1 ? arg : ValueArray.get(argValues));
             } else {
-                Aggregate agg = (Aggregate) group.get(this);
+                Aggregate agg = (Aggregate) select.getCurrentGroupExprData(this);
                 if (agg == null) {
                     agg = getInstance();
-                    group.put(this, agg);
+                    select.setCurrentGroupExprData(this, agg);
                 }
                 Object[] argValues = new Object[args.length];
                 Object arg = null;

h2/src/main/org/h2/util/ValueHashMap.java

@@ -5,20 +5,38 @@
  */
 package org.h2.util;
 
+import java.util.AbstractMap;
 import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.Map;
+import java.util.NoSuchElementException;
 import org.h2.message.DbException;
 import org.h2.value.Value;
 import org.h2.value.ValueNull;
 
 /**
  * This hash map supports keys of type Value.
+ * <p>
+ * ValueHashMap is a very simple implementation without allocation of additional
+ * objects for entries. It's very fast with good distribution of hashes, but if
+ * hashes have a lot of collisions this implementation tends to be very slow.
+ * <p>
+ * HashMap in archaic versions of Java have some overhead for allocation of
+ * entries, but slightly better behaviour with limited number of collisions,
+ * because collisions have no impact on non-colliding entries. HashMap in modern
+ * versions of Java also have the same overhead, but it builds a trees of keys
+ * with colliding hashes, that's why even if the all keys have exactly the same
+ * hash code it still offers a good performance similar to TreeMap. So
+ * ValueHashMap is faster in typical cases, but may behave really bad in some
+ * cases. HashMap is slower in typical cases, but its performance does not
+ * degrade too much even in the worst possible case (if keys are comparable).
  *
  * @param <V> the value type
  */
 public class ValueHashMap<V> extends HashBase {
 
-    private Value[] keys;
-    private V[] values;
+    Value[] keys;
+    V[] values;
 
     /**
      * Create a new value hash map.
@@ -174,6 +192,51 @@ public class ValueHashMap<V> extends HashBase {
         }
         return list;
     }
+    
+    public Iterable<Map.Entry<Value, V>> entries() {
+        return new EntryIterable();
+    }
+    
+    private final class EntryIterable implements Iterable<Map.Entry<Value, V>> {
+        EntryIterable() {
+        }
+
+        @Override
+        public Iterator<Map.Entry<Value, V>> iterator() {
+            return new EntryIterator();
+        }
+    }
+
+    private final class EntryIterator implements Iterator<Map.Entry<Value, V>> {
+        private int keysIndex = -1;
+        private int left = size;
+
+        EntryIterator() {
+        }
+
+        @Override
+        public boolean hasNext() {
+            return left > 0;
+        }
+
+        @Override
+        public Map.Entry<Value, V> next() {
+            if (left <= 0)
+                throw new NoSuchElementException();
+            left--;
+            for (;;) {
+                keysIndex++;
+                Value key = keys[keysIndex];
+                if (key != null && key != ValueNull.DELETED)
+                    return new AbstractMap.SimpleImmutableEntry<Value, V>(key, values[keysIndex]);
+            }
+        }
+        
+        @Override
+        public void remove() {
+            throw new UnsupportedOperationException();
+        }
+    }
 
     /**
      * Get the list of values.