wave8b: 438/196 — rails-0017, hanami-0001, spark-0002 + PDF

defects/janusgraph/patch/janusgraph-0001-multicondition-arraylist-contains.md

@@ -0,0 +1,81 @@
+# janusgraph-0001: MultiCondition extends ArrayList — O(C²) condition deduplication in query builder
+
+## Severity
+MEDIUM
+
+## Location
+`janusgraph-core/src/main/java/org/janusgraph/graphdb/query/condition/MultiCondition.java`
+Line 29 — class declaration
+
+`janusgraph-core/src/main/java/org/janusgraph/graphdb/query/QueryUtil.java`
+Line 339 — call site
+
+## Description
+`MultiCondition<E>` is the base class for both `And` and `Or` condition nodes in
+JanusGraph's query tree.  It **extends `ArrayList<Condition<E>>`**, inheriting
+`ArrayList.contains()` which is an O(N) linear scan.
+
+`QueryUtil.addConstraint()` (line 339) calls `conditions.contains(pc)` before
+adding a new `PredicateCondition` to guard against duplicates.  This method is
+called once per predicate being added to a query's condition tree.
+
+For a query with C conditions, building the tree costs O(1 + 2 + … + C) = O(C²)
+comparisons.  Since JanusGraph queries often carry dozens of predicate conditions
+(property filters, label constraints, range queries), this is a realistic
+quadratic bottleneck in the query planning path.
+
+## Defective Code
+```java
+// MultiCondition.java:29
+public abstract class MultiCondition<E extends JanusGraphElement>
+        extends ArrayList<Condition<E>> implements Condition<E> {
+    // inherits O(N) contains() from ArrayList
+}
+
+// QueryUtil.java:339 — called once per predicate addition
+if (!conditions.contains(pc)) conditions.add(pc);
+```
+
+## Root Cause
+Extending `ArrayList` for a semantic "set of conditions" conflates ordered list
+storage with membership testing.  The design choice to back conditions with a
+`List` means every duplicate-check is O(C).
+
+## Fix
+Change `MultiCondition` to maintain a parallel `HashSet` for O(1) membership
+testing:
+
+```java
+public abstract class MultiCondition<E extends JanusGraphElement>
+        extends ArrayList<Condition<E>> implements Condition<E> {
+
+    private final Set<Condition<E>> conditionSet = new HashSet<>();
+
+    @Override
+    public boolean add(Condition<E> condition) {
+        assert condition != null;
+        if (conditionSet.add(condition)) {
+            return super.add(condition);
+        }
+        return false;
+    }
+
+    @Override
+    public boolean contains(Object o) {
+        return conditionSet.contains(o);
+    }
+}
+```
+
+This keeps insertion order (for any callers that iterate conditions in order)
+while making `contains()` O(1).  The `QueryUtil.addConstraint()` call site
+requires no change.
+
+Alternatively, replace the backing structure with `LinkedHashSet` entirely
+and change the `getChildren()` return type — but that is a larger API change.
+
+## Complexity
+| Operation | Before | After |
+|-----------|--------|-------|
+| `conditions.contains(pc)` per call | O(C) | O(1) |
+| Build C-condition query tree | O(C²) | O(C) |

defects/janusgraph/unit/JanusGraphTest.java

@@ -0,0 +1,163 @@
+package unit;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Set;
+
+/**
+ * janusgraph-0001: MultiCondition extends ArrayList — O(C²) condition dedup
+ *
+ * Simulates JanusGraph's MultiCondition.contains() / QueryUtil.addConstraint()
+ * pattern where conditions are stored in an ArrayList but membership is tested
+ * with contains() before each add().
+ *
+ * Slow path: ArrayList-backed condition set (as in MultiCondition).
+ * Fast path: HashSet-backed condition set (proposed fix).
+ *
+ * Each "condition" (PredicateCondition) is a unique Integer-keyed object.
+ * We count equals() calls to measure O(C²) vs O(C) growth.
+ */
+public class JanusGraphTest {
+
+    static int slowEqOps = 0;
+    static int fastEqOps = 0;
+
+    static class SlowCondition {
+        final int id;
+        SlowCondition(int id) { this.id = id; }
+
+        @Override
+        public boolean equals(Object o) {
+            if (this == o) return true;
+            if (!(o instanceof SlowCondition)) return false;
+            slowEqOps++;
+            return this.id == ((SlowCondition) o).id;
+        }
+
+        @Override
+        public int hashCode() {
+            // Deliberately return constant to simulate broken hash — worst-case
+            // for HashSet (though that's not what we're testing here; this tests
+            // the ArrayList path which never uses hashCode).
+            return 42;
+        }
+    }
+
+    static class FastCondition {
+        final int id;
+        FastCondition(int id) { this.id = id; }
+
+        @Override
+        public boolean equals(Object o) {
+            if (this == o) return true;
+            if (!(o instanceof FastCondition)) return false;
+            fastEqOps++;
+            return this.id == ((FastCondition) o).id;
+        }
+
+        @Override
+        public int hashCode() {
+            return id;  // proper hash — O(1) HashSet lookup
+        }
+    }
+
+    /**
+     * Defective path: ArrayList-backed MultiCondition.
+     * addConstraint() pattern: if (!conditions.contains(pc)) conditions.add(pc)
+     * O(i) scan for the i-th unique item => O(C^2) total.
+     */
+    static void slowPath(int n) {
+        List<SlowCondition> conditions = new ArrayList<>();
+        for (int i = 0; i < n; i++) {
+            SlowCondition pc = new SlowCondition(i);
+            if (!conditions.contains(pc)) {
+                conditions.add(pc);
+            }
+        }
+        // Second pass: try adding duplicates (all present — full scan each time)
+        for (int i = 0; i < n; i++) {
+            SlowCondition pc = new SlowCondition(i);
+            if (!conditions.contains(pc)) {
+                conditions.add(pc);
+            }
+        }
+    }
+
+    /**
+     * Fixed path: HashSet-backed condition set with preserved insertion order.
+     * contains() is O(1) regardless of set size.
+     */
+    static void fastPath(int n) {
+        Set<FastCondition> conditionSet = new LinkedHashSet<>();
+        for (int i = 0; i < n; i++) {
+            FastCondition pc = new FastCondition(i);
+            conditionSet.add(pc);  // Set.add() deduplicates with O(1) contains check
+        }
+        // Second pass: duplicates — O(1) per add, idempotent
+        for (int i = 0; i < n; i++) {
+            conditionSet.add(new FastCondition(i));
+        }
+    }
+
+    public static void main(String[] args) {
+        int N = 400;
+        int PASSES = 3;
+
+        // Warm up
+        slowPath(10);
+        fastPath(10);
+        slowEqOps = 0;
+        fastEqOps = 0;
+
+        // Measure
+        for (int p = 0; p < PASSES; p++) {
+            slowPath(N);
+            fastPath(N);
+        }
+
+        // slow: first-pass adds N unique items, i-th add scans i existing => N*(N-1)/2
+        //       second-pass each of N items scans full N => N*N
+        //       per call total: ~1.5*N^2; for PASSES calls: ~1.5*N^2*PASSES
+        long expectedSlowMin = (long)(N * N / 4);      // conservative lower bound
+        // fast: proper hash, each add/lookup is O(1), ~0 equals() for distinct ids
+        long expectedFastMax = (long)(N * PASSES * 4); // generous upper bound
+
+        System.out.println("N=" + N + "  PASSES=" + PASSES);
+        System.out.println("slow equals ops   : " + slowEqOps + " (expected >=" + expectedSlowMin + ")");
+        System.out.println("fast equals ops   : " + fastEqOps + " (expected <=" + expectedFastMax + ")");
+
+        int passed = 0;
+        int total = 0;
+
+        total++;
+        if (slowEqOps >= expectedSlowMin) {
+            System.out.println("PASS 1/" + total + ": slow path O(n^2) confirmed (ops=" + slowEqOps + " >= " + expectedSlowMin + ")");
+            passed++;
+        } else {
+            System.out.println("FAIL 1/" + total + ": slow ops=" + slowEqOps + " < expected " + expectedSlowMin);
+        }
+
+        total++;
+        if (fastEqOps <= expectedFastMax) {
+            System.out.println("PASS 2/" + total + ": fast path O(1) confirmed (ops=" + fastEqOps + " <= " + expectedFastMax + ")");
+            passed++;
+        } else {
+            System.out.println("FAIL 2/" + total + ": fast ops=" + fastEqOps + " > expected " + expectedFastMax);
+        }
+
+        total++;
+        boolean ratioOk = fastEqOps == 0 || slowEqOps >= fastEqOps * 10;
+        if (ratioOk) {
+            System.out.println("PASS 3/" + total + ": speedup ratio slow/fast = " +
+                (fastEqOps == 0 ? "inf" : (slowEqOps / fastEqOps)) + "x (expected >=10x)");
+            passed++;
+        } else {
+            System.out.println("FAIL 3/" + total + ": ratio too small: slow=" + slowEqOps + " fast=" + fastEqOps);
+        }
+
+        System.out.println(passed + "/" + total + " PASS");
+        if (passed != total) System.exit(1);
+    }
+}