From 2eb75a2dc2227784fe62b87fde7bf8056b29c388 Mon Sep 17 00:00:00 2001 From: Luc Boudreau Date: Tue, 4 Sep 2012 16:07:44 -0400 Subject: [PATCH] Fix for MONDRIAN-1197. RolapMemberBase was comparing members with null keys by unique name, which only made sense for VisualTotalsMember but would create problems with other normal members. This fix makes it so that null keys are always equal, but VisualTotalsMember overrides the compareTo(Object) method so the delegate member is compared. This is in line with other parts of the code, like RolapCubeMember. --- .../mondrian/olap/fun/VisualTotalsFunDef.java | 15 ++ src/main/mondrian/rolap/RolapMemberBase.java | 16 +-- src/main/mondrian/spi/SegmentCache.java | 4 +- .../test/ParentChildHierarchyTest.java | 34 +++-- .../mondrian/test/SteelWheelsSchemaTest.java | 130 ++++++++++++++++++ 5 files changed, 176 insertions(+), 23 deletions(-) diff --git a/src/main/mondrian/olap/fun/VisualTotalsFunDef.java b/src/main/mondrian/olap/fun/VisualTotalsFunDef.java index b609a953c4..8fc8896ea6 100644 --- a/src/main/mondrian/olap/fun/VisualTotalsFunDef.java +++ b/src/main/mondrian/olap/fun/VisualTotalsFunDef.java @@ -225,6 +225,18 @@ public boolean equals(Object o) { && this.member.equals(o); } + @Override + public int compareTo(Object o) { + if (o instanceof VisualTotalMember) { + // VisualTotals members are a special case. We have + // to compare the delegate member. + return this.getMember().compareTo( + ((VisualTotalMember) o).getMember()); + } else { + return super.compareTo(o); + } + } + @Override public int hashCode() { return member.hashCode(); @@ -241,6 +253,9 @@ protected boolean computeCalculated(final MemberType memberType) { public int getSolveOrder() { // high solve order, so it is expanded after other calculations + // REVIEW: 99...really?? I've seen many queries with higher SO. + // I don't think we should be abusing arbitrary constants + // like this. return 99; } diff --git a/src/main/mondrian/rolap/RolapMemberBase.java b/src/main/mondrian/rolap/RolapMemberBase.java index c83a3aef5d..6a3b59a50e 100644 --- a/src/main/mondrian/rolap/RolapMemberBase.java +++ b/src/main/mondrian/rolap/RolapMemberBase.java @@ -512,20 +512,16 @@ public Object getKey() { public int compareTo(Object o) { RolapMemberBase other = (RolapMemberBase)o; if (this.key != null && other.key == null) { - return 1; // not null is greater than null + // not null is greater than null + return 1; } if (this.key == null && other.key != null) { - return -1; // null is less than not null + // null is less than not null + return -1; } - // compare by unique name, if both keys are null if (this.key == null && other.key == null) { - return this.getUniqueName().compareTo(other.getUniqueName()); - } - // compare by unique name, if one ore both members are null - if (this.key == RolapUtil.sqlNullValue - || other.key == RolapUtil.sqlNullValue) - { - return this.getUniqueName().compareTo(other.getUniqueName()); + // if both keys are null, they are equal. + return 0; } // as both keys are not null, compare by key // String, Double, Integer should be possible diff --git a/src/main/mondrian/spi/SegmentCache.java b/src/main/mondrian/spi/SegmentCache.java index dbea6922cf..cf993001d2 100644 --- a/src/main/mondrian/spi/SegmentCache.java +++ b/src/main/mondrian/spi/SegmentCache.java @@ -69,9 +69,7 @@ public interface SegmentCache { * segment corresponding to the header could be found. * *

Cache implementations are at liberty to 'forget' segments. Therefore - * it is allowable for this method to return null at any time, even if - * {@link #contains(SegmentHeader)} for this segment previously returned - * true.

+ * it is allowable for this method to return null at any time

* * @param header The header of the segment to find. * Consider this as a key. diff --git a/testsrc/main/mondrian/test/ParentChildHierarchyTest.java b/testsrc/main/mondrian/test/ParentChildHierarchyTest.java index 2095ba3979..306ceef1c3 100644 --- a/testsrc/main/mondrian/test/ParentChildHierarchyTest.java +++ b/testsrc/main/mondrian/test/ParentChildHierarchyTest.java @@ -1327,6 +1327,20 @@ public void testBridgeTable() { + "Row #0: 28,435.38\n"); } + /** + * Fix for + * MONDRIAN-1225 + * + *

When nativizing a set which contained a PC + * hierarchy, the SqlTupleReader would ask the cache for the parent + * member of the member it was populating, but the members are only + * put in cache at the second phase of the tuple computation, once all + * the members have been populated from SQL. Now, the SqlTupleReader + * keeps an intermediate key->member map so it can construct PC + * hierarchies correctly. This map gets picked up by the GC as soon as + * the SQL result set reaches its end and the tuple reader is closed, + * so there are no added cost to this. + */ public void testPCCacheKeyBug() throws Exception { final String mdx = "With\n" @@ -1357,26 +1371,26 @@ public void testPCCacheKeyBug() throws Exception { + "Axis #1:\n" + "{[Measures].[*FORMATTED_MEASURE_0]}\n" + "Axis #2:\n" - + "{[Employees].[Sheri Nowmer], [Store].[USA], [Pay Type].[Monthly]}\n" + "{[Employees].[Sheri Nowmer], [Store].[USA], [Pay Type].[Hourly]}\n" - + "{[Employees].[Sheri Nowmer].[Derrick Whelply], [Store].[USA], [Pay Type].[Monthly]}\n" + + "{[Employees].[Sheri Nowmer], [Store].[USA], [Pay Type].[Monthly]}\n" + "{[Employees].[Sheri Nowmer].[Derrick Whelply], [Store].[USA], [Pay Type].[Hourly]}\n" - + "{[Employees].[Sheri Nowmer].[Derrick Whelply].[Beverly Baker], [Store].[USA], [Pay Type].[Monthly]}\n" + + "{[Employees].[Sheri Nowmer].[Derrick Whelply], [Store].[USA], [Pay Type].[Monthly]}\n" + "{[Employees].[Sheri Nowmer].[Derrick Whelply].[Beverly Baker], [Store].[USA], [Pay Type].[Hourly]}\n" - + "{[Employees].[Sheri Nowmer].[Derrick Whelply].[Beverly Baker].[Jacqueline Wyllie], [Store].[USA], [Pay Type].[Monthly]}\n" + + "{[Employees].[Sheri Nowmer].[Derrick Whelply].[Beverly Baker], [Store].[USA], [Pay Type].[Monthly]}\n" + "{[Employees].[Sheri Nowmer].[Derrick Whelply].[Beverly Baker].[Jacqueline Wyllie], [Store].[USA], [Pay Type].[Hourly]}\n" - + "{[Employees].[Sheri Nowmer].[Derrick Whelply].[Beverly Baker].[Jacqueline Wyllie].[Ralph Mccoy], [Store].[USA], [Pay Type].[Monthly]}\n" + + "{[Employees].[Sheri Nowmer].[Derrick Whelply].[Beverly Baker].[Jacqueline Wyllie], [Store].[USA], [Pay Type].[Monthly]}\n" + "{[Employees].[Sheri Nowmer].[Derrick Whelply].[Beverly Baker].[Jacqueline Wyllie].[Ralph Mccoy], [Store].[USA], [Pay Type].[Hourly]}\n" - + "{[Employees].[Sheri Nowmer].[Derrick Whelply].[Pedro Castillo], [Store].[USA], [Pay Type].[Monthly]}\n" + + "{[Employees].[Sheri Nowmer].[Derrick Whelply].[Beverly Baker].[Jacqueline Wyllie].[Ralph Mccoy], [Store].[USA], [Pay Type].[Monthly]}\n" + "{[Employees].[Sheri Nowmer].[Derrick Whelply].[Pedro Castillo], [Store].[USA], [Pay Type].[Hourly]}\n" - + "{[Employees].[Sheri Nowmer].[Derrick Whelply].[Pedro Castillo].[Jose Bernard], [Store].[USA], [Pay Type].[Monthly]}\n" + + "{[Employees].[Sheri Nowmer].[Derrick Whelply].[Pedro Castillo], [Store].[USA], [Pay Type].[Monthly]}\n" + "{[Employees].[Sheri Nowmer].[Derrick Whelply].[Pedro Castillo].[Jose Bernard], [Store].[USA], [Pay Type].[Hourly]}\n" - + "{[Employees].[Sheri Nowmer].[Derrick Whelply].[Laurie Borges], [Store].[USA], [Pay Type].[Monthly]}\n" + + "{[Employees].[Sheri Nowmer].[Derrick Whelply].[Pedro Castillo].[Jose Bernard], [Store].[USA], [Pay Type].[Monthly]}\n" + "{[Employees].[Sheri Nowmer].[Derrick Whelply].[Laurie Borges], [Store].[USA], [Pay Type].[Hourly]}\n" - + "{[Employees].[Sheri Nowmer].[Derrick Whelply].[Laurie Borges].[Mary Solimena], [Store].[USA], [Pay Type].[Monthly]}\n" + + "{[Employees].[Sheri Nowmer].[Derrick Whelply].[Laurie Borges], [Store].[USA], [Pay Type].[Monthly]}\n" + "{[Employees].[Sheri Nowmer].[Derrick Whelply].[Laurie Borges].[Mary Solimena], [Store].[USA], [Pay Type].[Hourly]}\n" - + "{[Employees].[Sheri Nowmer].[Derrick Whelply].[Laurie Borges].[Mary Solimena].[Matthew Hunter], [Store].[USA], [Pay Type].[Monthly]}\n" + + "{[Employees].[Sheri Nowmer].[Derrick Whelply].[Laurie Borges].[Mary Solimena], [Store].[USA], [Pay Type].[Monthly]}\n" + "{[Employees].[Sheri Nowmer].[Derrick Whelply].[Laurie Borges].[Mary Solimena].[Matthew Hunter], [Store].[USA], [Pay Type].[Hourly]}\n" + + "{[Employees].[Sheri Nowmer].[Derrick Whelply].[Laurie Borges].[Mary Solimena].[Matthew Hunter], [Store].[USA], [Pay Type].[Monthly]}\n" + "Row #0: 3,996\n" + "Row #1: 3,396\n" + "Row #2: 3,840\n" diff --git a/testsrc/main/mondrian/test/SteelWheelsSchemaTest.java b/testsrc/main/mondrian/test/SteelWheelsSchemaTest.java index 4c440eaee7..4c0097820c 100644 --- a/testsrc/main/mondrian/test/SteelWheelsSchemaTest.java +++ b/testsrc/main/mondrian/test/SteelWheelsSchemaTest.java @@ -583,6 +583,136 @@ public void testEsr1587() { + "{[Time].[2005], [Measures].[*ZERO]}\n" + "Row #0: 0\n" + "Row #0: 0\n" + "Row #0: 0\n"); } + + /** + * Fix for + * MONDRIAN-1197 + * + * RolapMemberBase was comparing members with null keys by unique name, + * which only made sense for VisualTotalsMember but would create problems + * with other normal members. This fix makes it so that null keys are + * always equal, but VisualTotalsMember overrides the compareTo(Object) + * method so the delegate member is compared. This is in line with other + * parts of the code, like RolapCubeMember. + */ + public void testMondrian1197() { + final TestContext testContext = getTestContext(); + if (!testContext.databaseIsValid()) { + return; + } + final String mdx = + "With\n" + + "Set [*NATIVE_CJ_SET] as '[*BASE_MEMBERS_Markets]'\n" + + "Set [*SORTED_ROW_AXIS] as 'Order([*CJ_ROW_AXIS],[Markets].CurrentMember.OrderKey,DESC,Ancestor([Markets].CurrentMember,[Markets].[Country]).OrderKey,DESC)'\n" + + "Set [*BASE_MEMBERS_Markets] as '[Markets].[State Province].Members'\n" + + "Set [*BASE_MEMBERS_Measures] as '{[Measures].[*ZERO]}'\n" + + "Set [*CJ_ROW_AXIS] as 'Generate([*NATIVE_CJ_SET], {([Markets].currentMember)})'\n" + + "Set [*CJ_COL_AXIS] as '[*NATIVE_CJ_SET]'\n" + + "Member [Measures].[*ZERO] as '0', SOLVE_ORDER=0\n" + + "Select\n" + + "[*BASE_MEMBERS_Measures] on columns,\n" + + "[*SORTED_ROW_AXIS] on rows\n" + + "From [SteelWheelsSales]\n"; + testContext.assertQueryReturns( + mdx, + "Axis #0:\n" + + "{}\n" + + "Axis #1:\n" + + "{[Measures].[*ZERO]}\n" + + "Axis #2:\n" + + "{[Markets].[NA].[USA].[PA]}\n" + + "{[Markets].[NA].[USA].[NY]}\n" + + "{[Markets].[NA].[USA].[NV]}\n" + + "{[Markets].[NA].[USA].[NJ]}\n" + + "{[Markets].[NA].[USA].[NH]}\n" + + "{[Markets].[NA].[USA].[MA]}\n" + + "{[Markets].[NA].[USA].[CT]}\n" + + "{[Markets].[NA].[USA].[CA]}\n" + + "{[Markets].[NA].[Canada].[Quu00e9bec]}\n" + + "{[Markets].[NA].[Canada].[BC]}\n" + + "{[Markets].[Japan].[Singapore].[#null]}\n" + + "{[Markets].[Japan].[Philippines].[#null]}\n" + + "{[Markets].[Japan].[Japan].[Tokyo]}\n" + + "{[Markets].[Japan].[Japan].[Osaka]}\n" + + "{[Markets].[Japan].[Hong Kong].[#null]}\n" + + "{[Markets].[EMEA].[UK].[Isle of Wight]}\n" + + "{[Markets].[EMEA].[UK].[#null]}\n" + + "{[Markets].[EMEA].[Switzerland].[#null]}\n" + + "{[Markets].[EMEA].[Sweden].[#null]}\n" + + "{[Markets].[EMEA].[Spain].[#null]}\n" + + "{[Markets].[EMEA].[Norway].[#null]}\n" + + "{[Markets].[EMEA].[Italy].[#null]}\n" + + "{[Markets].[EMEA].[Ireland].[#null]}\n" + + "{[Markets].[EMEA].[Germany].[#null]}\n" + + "{[Markets].[EMEA].[France].[#null]}\n" + + "{[Markets].[EMEA].[Finland].[#null]}\n" + + "{[Markets].[EMEA].[Denmark].[#null]}\n" + + "{[Markets].[EMEA].[Belgium].[#null]}\n" + + "{[Markets].[EMEA].[Austria].[#null]}\n" + + "{[Markets].[APAC].[Singapore].[#null]}\n" + + "{[Markets].[APAC].[New Zealand].[]}\n" + + "{[Markets].[APAC].[New Zealand].[#null]}\n" + + "{[Markets].[APAC].[Australia].[Victoria]}\n" + + "{[Markets].[APAC].[Australia].[Queensland]}\n" + + "{[Markets].[APAC].[Australia].[NSW]}\n" + + "{[Markets].[#null].[Switzerland].[#null]}\n" + + "{[Markets].[#null].[Spain].[#null]}\n" + + "{[Markets].[#null].[South Africa].[#null]}\n" + + "{[Markets].[#null].[Singapore].[#null]}\n" + + "{[Markets].[#null].[Russia].[#null]}\n" + + "{[Markets].[#null].[Portugal].[#null]}\n" + + "{[Markets].[#null].[Poland].[#null]}\n" + + "{[Markets].[#null].[Netherlands].[#null]}\n" + + "{[Markets].[#null].[Israel].[#null]}\n" + + "{[Markets].[#null].[Ireland].[Co. Cork]}\n" + + "{[Markets].[#null].[Germany].[#null]}\n" + + "Row #0: 0\n" + + "Row #1: 0\n" + + "Row #2: 0\n" + + "Row #3: 0\n" + + "Row #4: 0\n" + + "Row #5: 0\n" + + "Row #6: 0\n" + + "Row #7: 0\n" + + "Row #8: 0\n" + + "Row #9: 0\n" + + "Row #10: 0\n" + + "Row #11: 0\n" + + "Row #12: 0\n" + + "Row #13: 0\n" + + "Row #14: 0\n" + + "Row #15: 0\n" + + "Row #16: 0\n" + + "Row #17: 0\n" + + "Row #18: 0\n" + + "Row #19: 0\n" + + "Row #20: 0\n" + + "Row #21: 0\n" + + "Row #22: 0\n" + + "Row #23: 0\n" + + "Row #24: 0\n" + + "Row #25: 0\n" + + "Row #26: 0\n" + + "Row #27: 0\n" + + "Row #28: 0\n" + + "Row #29: 0\n" + + "Row #30: 0\n" + + "Row #31: 0\n" + + "Row #32: 0\n" + + "Row #33: 0\n" + + "Row #34: 0\n" + + "Row #35: 0\n" + + "Row #36: 0\n" + + "Row #37: 0\n" + + "Row #38: 0\n" + + "Row #39: 0\n" + + "Row #40: 0\n" + + "Row #41: 0\n" + + "Row #42: 0\n" + + "Row #43: 0\n" + + "Row #44: 0\n" + + "Row #45: 0\n"); + } } // End SteelWheelsSchemaTest.java