MONDRIAN: Prevent parent-child closure table from being used to compu…

…te cells at

    'all' level of the parent-child hierarchy. It is not valid to roll up,
    because each member occurs many times.

[git-p4: depot-paths = "//open/mondrian-release/3.2/": change = 13552]

src/main/mondrian/rolap/FastBatchingCellReader.java

@@ -694,7 +694,8 @@ && constraintsMatch(other)
                 && hasSameMeasureList(other)
                 && !hasDistinctCountMeasure()
                 && !other.hasDistinctCountMeasure()
-                && haveSameStarAndAggregation(other);
+                && haveSameStarAndAggregation(other)
+                && haveSameClosureColumns(other);
         }
 
         /**
@@ -820,6 +821,28 @@ boolean haveSameStarAndAggregation(Batch other) {
             return hasSameStar && hasSameAggregation && hasSameRollupOption;
         }
 
+        /**
+         * Returns whether this batch has the same closure columns as another.
+         *
+         * <p>Ensures that we do not group together a batch that includes a
+         * level of a parent-child closure dimension with a batch that does not.
+         * It is not safe to roll up from a parent-child closure level; due to
+         * multiple accounting, the 'all' level is less than the sum of the
+         * members of the closure level.
+         *
+         * @param other Other batch
+         * @return Whether batches have the same closure columns
+         */
+        boolean haveSameClosureColumns(Batch other) {
+            final BitKey closureColumns =
+                this.batchKey.getConstrainedColumnsBitKey()
+                    .and(cube.closureColumnBitKey);
+            final BitKey otherClosureColumns =
+                other.batchKey.getConstrainedColumnsBitKey()
+                    .and(cube.closureColumnBitKey);
+            return closureColumns.equals(otherClosureColumns);
+        }
+
         /**
          * @param rollup Out parameter
          * @return AggStar

src/main/mondrian/rolap/RolapCube.java

@@ -91,6 +91,8 @@ public class RolapCube extends CubeBase {
     private Map<RolapLevel, RolapCubeLevel> virtualToBaseMap =
         new HashMap<RolapLevel, RolapCubeLevel>();
 
+    final BitKey closureColumnBitKey;
+
     /**
      * Private constructor used by both normal cubes and virtual cubes.
      *
@@ -135,6 +137,10 @@ private RolapCube(
             if (! isCache) {
                 star.setCacheAggregations(isCache);
             }
+            closureColumnBitKey =
+                BitKey.Factory.makeBitKey(star.getColumnCount());
+        } else {
+            closureColumnBitKey = null;
         }
 
         if (getLogger().isDebugEnabled()) {

src/main/mondrian/rolap/RolapCubeLevel.java

@@ -114,6 +114,11 @@ void init(MondrianDef.CubeDimension xmlDimension) {
             closedPeerCubeLevel = (RolapCubeLevel)
                 cubeDimension.getHierarchies()[0].getLevels()[1];
 
+            if (!getCube().isVirtual()) {
+                getCube().closureColumnBitKey.set(
+                    closedPeerCubeLevel.starKeyColumn.getBitPosition());
+            }
+
             this.levelReader = new ParentChildLevelReaderImpl(this);
         } else {
             this.levelReader = new RegularLevelReader(this);

testsrc/main/mondrian/test/ParentChildHierarchyTest.java

@@ -12,9 +12,7 @@
 package mondrian.test;
 
 import junit.framework.Assert;
-import mondrian.olap.Result;
-import mondrian.olap.Member;
-import mondrian.olap.Cell;
+import mondrian.olap.*;
 
 /**
  * <code>ParentChildHierarchyTest</code> tests parent-child hierarchies.
@@ -1153,18 +1151,61 @@ public void testClosureVsNoClosure() {
             + "\n"
             + "  <Measure name=\"Count\" column=\"employee_id\" aggregator=\"count\" />\n"
             + "</Cube>\n";
-        String mdx =
-            "select {[Measures].[Count]} ON COLUMNS, NON EMPTY {[Employees].AllMembers} ON ROWS from [HR4C]";
+
         TestContext testClosureContext = TestContext.create(
             null, cubestart + closure + cubeend, null, null, null, null);
-        String expect =
-            TestContext.toString(testClosureContext.executeQuery(mdx));
         TestContext testNoClosureContext = TestContext.create(
             null, cubestart + cubeend, null, null, null, null);
+
+        String mdx;
+        String expected;
+
+        // 1. Run a big query on both contexts and check that both give same.
+        mdx =
+            "select {[Measures].[Count]} ON COLUMNS,\n"
+            + " NON EMPTY {[Employees].AllMembers} ON ROWS\n"
+            + "from [HR4C]";
+        expected =
+            TestContext.toString(testClosureContext.executeQuery(mdx));
+        assertTrue(expected, expected.contains("Row #0: 21,252\n"));
         // Need to unfold because 'expect' has platform-specific line-endings,
         // yet assertQueryReturns assumes that it contains linefeeds.
         testNoClosureContext.assertQueryReturns(
-            mdx, TestContext.unfold(expect));
+            mdx, TestContext.unfold(expected));
+
+        // 2. Run a small query with known results on both contexts.
+        // Note in particular the total for [All] is 21,252, same as for
+        // [Sheri Nowmer]. There was a bug where [All] had a much higher total.
+        mdx =
+            "select {[Measures].[Count]} ON COLUMNS,\n"
+            + " Descendants([Employees], 2, SELF_AND_BEFORE) ON ROWS\n"
+            + "from [HR4C]";
+        expected =
+            "Axis #0:\n"
+            + "{}\n"
+            + "Axis #1:\n"
+            + "{[Measures].[Count]}\n"
+            + "Axis #2:\n"
+            + "{[Employees].[All]}\n"
+            + "{[Employees].[Sheri Nowmer]}\n"
+            + "{[Employees].[Sheri Nowmer].[Derrick Whelply]}\n"
+            + "{[Employees].[Sheri Nowmer].[Michael Spence]}\n"
+            + "{[Employees].[Sheri Nowmer].[Maya Gutierrez]}\n"
+            + "{[Employees].[Sheri Nowmer].[Roberta Damstra]}\n"
+            + "{[Employees].[Sheri Nowmer].[Rebecca Kanagaki]}\n"
+            + "{[Employees].[Sheri Nowmer].[Darren Stanz]}\n"
+            + "{[Employees].[Sheri Nowmer].[Donna Arnold]}\n"
+            + "Row #0: 21,252\n"
+            + "Row #1: 21,252\n"
+            + "Row #2: 14,472\n"
+            + "Row #3: 1,128\n"
+            + "Row #4: 5,244\n"
+            + "Row #5: 96\n"
+            + "Row #6: 60\n"
+            + "Row #7: 168\n"
+            + "Row #8: 60\n";
+        testClosureContext.assertQueryReturns(mdx, expected);
+        testNoClosureContext.assertQueryReturns(mdx, expected);
     }
 }