[Mondrian] Closure table with a non-primary key relation

[Julian Hyde]

I had to reject a patch (for http://jira.pentaho.com/browse/MONDRIAN-441) a few weeks ago. I see you're involved on that thread, so I suspect I'll be seeing another incarnation of that patch... :)

I'll second everything Luc said. In particular, it's really important that it doesn't break any other tests. That was the problem with MONDRIAN-441 patch.

Also, as Luc says, doing the work on the 4.0 branch would be best. It was depressing spending a few hours working on the patch, because I knew that code has all gone now.

Here's some general advice to anyone considering submitting a patch. Specifications (i.e. test cases and documentation) are more re-usable than implementation. So, here is a way to get the a lot of bang for your buck. Don't write the feature, just write the unit test (or tests) that demonstrates the feature, and documentation, if necessary. If applicable write one or two unit negative tests that show the feature being used in error (e.g. if you are introducing a <Foo/> element, a test with '<Foo level="a non existent level"/>' that checks for a nice error message). Make the tests work fully automatically, on the standard foodmart schema. (I know that isn't easy for you sometimes, but it isn't easy for us either, and if you don't do it, we'd have to.) If the feature gives new functionality to the end-user or administrator, provide an example that can be put in the documentation. It should be on a simple, easily understood schema, not your own application's schema. Then log a bug with the unit test(s) attached as a patch and the documentation in the body of the case. Luc or I should find it easy to write the feature, and it will be easy to port the test from the 3.x to the 4.0 branch. Lastly, nag Luc & me until we fix it.

Julian


On Mar 8, 2012, at 8:05 AM, Luc Boudreau wrote:

> We consider that contributions should have the following:
> 
> - At least one test case for the feature itself.
> - At least a test case for failures and errors.
> - In a unified DIFF format.
> - ...And it doesn't break any other tests.
> 
> We won't necessarily reject a contribution that doesn't meet all of
> the above requirements, but meeting those expectations make the
> contribution process much much faster and you get to have that feature
> integrated as fast as can be. (After a few contributions, we usually
> grant commit access, when asked.)
> 
> I've compared the Lagunitas branch and the 3.X branch and I can tell
> you for sure that most of the code in
> RolapHierarchy.createClosedPeerDimension has changed significantly. I
> suggest you write your contribution against the 4.X branch instead.
> Besides, we are in the process of testing our internal 3.4 RC before
> GA, so I think it would be unwise at this point to introduce this new
> feature.
> 
> Branch wise, we use the following branches for now.
> 
> - //open/mondrian   - This is the 3.X branch.
> - //open/mondrian-release/lagunitas    - This is the 4.X branch.
> 
> Please also note that we will be switching over to a GIT repository in
> the following days. More information will be sent out as soon as all
> the details are figured out.
> 
> Luc
> 
> 
> On Thu, Mar 8, 2012 at 10:50 AM, Patrick Leckey <patl at seewind.com> wrote:
>> Cool, if you could let us know what branch to best work against that'd be great.  Just wanted to make sure this didn't violate some spec or something, and that it would be a valid / acceptable patch.
>> 
>> Thanks,
>> Pat
>> 
>> On 2012-03-08, at 10:28 AM, Luc Boudreau wrote:
>> 
>>> Hi Pat,
>>> 
>>> Contributions are always welcome, obviously :) Yet for now we are in
>>> the process of wrapping up the 3.4.0 release, and after that, we will
>>> switch all development efforts on the 4.X branch. I think the code
>>> might be quite different in that area (Mondrian 4 introduces a new way
>>> of joining tables and columns).
>>> 
>>> Julian, do you think that they could develop this feature on the 3.4.0
>>> branch and that it would be portable enough to be worth the effort, or
>>> will his efforts be wasted?
>>> 
>>> Luc
>>> 
>>> 
>>> 
>>> On Thu, Mar 8, 2012 at 10:23 AM, Patrick Leckey <patl at seewind.com> wrote:
>>>> Hi Mondrian,
>>>> 
>>>> In our structure we have a dimension that contains a parent-child hierarchy ("State" - not geographical state, a treatment state) as well as other hierarchies that are contextually related to State.  In our dimension table, State is not a primary key, so when we try to use it with a closure table we end up with an incorrect join generated by Mondrian.
>>>> 
>>>> Looking at RolapHierarchy.createClosedPeerDimension(), this is the way Mondrian was designed - it relates the parent column of the closure table to the child column of the unclosed base table, assuming primary keys.
>>>> 
>>>> Would Mondrian be open to a patch that allows us to create closure tables where the primary key of the closure's hierarchy doesn't match the child column of the unclosed table?  It would do so by adding a secondary join through the closure table to the base table, and then joining from the closure table to the primary key.  The new patch would only kick in if the closure tables hierarchy primary key doesn't match the closure child column.  If they do match (as is expected now), behaviour would remain unchanged.
>>>> 
>>>> Thanks,
>>>> Pat
>>>> 
>>>> _______________________________________________
>>>> Mondrian mailing list
>>>> Mondrian at pentaho.org
>>>> http://lists.pentaho.org/mailman/listinfo/mondrian
>>> _______________________________________________
>>> Mondrian mailing list
>>> Mondrian at pentaho.org
>>> http://lists.pentaho.org/mailman/listinfo/mondrian
>> 
>> _______________________________________________
>> Mondrian mailing list
>> Mondrian at pentaho.org
>> http://lists.pentaho.org/mailman/listinfo/mondrian
> _______________________________________________
> Mondrian mailing list
> Mondrian at pentaho.org
> http://lists.pentaho.org/mailman/listinfo/mondrian