[Mondrian] RE: Eigenbase perforce change 13092 for review

Julian Hyde jhyde at pentaho.com
Thu Nov 5 16:27:45 EST 2009


Kurtis,

I've been thinking about your original change -- which made it the
responsibility of the visitor to visit the children, not the responsibility
of the visitee (parse tree node). It makes a lot of sense to divide up the
responsibility that way.

How do you feel about going with that original proposal? The main problem I
had with your change was that you had changed some code but not others, and
you had introduced one or two bugs because you hadn't thought out the impact
of the change. 

You would need to change all uses of MdxVisitor in the code base, and update
the javadoc to make it clear where the responsibilities lie. Hopefully you
would be able to take out the code that prevents recursion from happening
twice.

Julian

> -----Original Message-----
> From: Kurtis.Walker at thomsonreuters.com 
> [mailto:Kurtis.Walker at thomsonreuters.com] 
> Sent: Wednesday, November 04, 2009 10:21 AM
> To: mondrian at pentaho.org; jhyde at pentaho.com
> Subject: RE: [Mondrian] RE: Eigenbase perforce change 13092 for review
> 
> Hi Julian,
>    Sorry to resurrect an old thread.  The workaround we came 
> up with for
> this issue has proven to be insufficient for a couple of our
> implementations of MdxVisitor that we have outside of the 
> Mondrian code
> base.  So, we've come up with an alternative.
>   We'd like to create a boolean method on MdxVisitor that indicates
> whether or not the visitee should visit its children.  The current
> default implementation, MdxVisitorImpl, would return true.  Then, we
> would create a second base implementation that returns false 
> and manages
> the child visits within the visitor.  This allows for the least amount
> of code change while supporting both variations on the 
> visitor pattern.
> Let us know if you have any concerns.  Thanks!
> 
> Kurt
> 
> -----Original Message-----
> From: mondrian-bounces at pentaho.org 
> [mailto:mondrian-bounces at pentaho.org]
> On Behalf Of Walker, Kurtis (Healthcare USA)
> Sent: Wednesday, October 14, 2009 8:55 AM
> To: jhyde at pentaho.com; mondrian at pentaho.org; Campbell, Matthew
> (Healthcare USA)
> Subject: RE: [Mondrian] RE: Eigenbase perforce change 13092 for review
> 
> I'll back it out.  I think we can accomplish the same thing 
> by keeping a
> list of fun calls that we have visited, so that we only visit each one
> once.
> 
> Kurt
> 
> -----Original Message-----
> From: Julian Hyde [mailto:jhyde at pentaho.com] 
> Sent: Tuesday, October 13, 2009 9:49 PM
> To: Walker, Kurtis (Healthcare USA); mondrian at pentaho.org; Campbell,
> Matthew (Healthcare USA)
> Subject: RE: [Mondrian] RE: Eigenbase perforce change 13092 for review
> 
> Well, it's a big change moving the iteration from the visitee to the
> visitor, and it is bound to introduce bugs. You did introduce 
> one bug --
> you
> changed ResolvedFunCall but you didn't change UnresolvedFunCall. As I
> said
> before it's non-standard -- the convention is for the visitee 
> to iterate
> over children. So other people using the visitor are likely to be
> confused.
> 
> What do you want to do about this? Is there a way to convert 
> it back to
> the
> old style, and say throw an exception to abort the iteration. Or if we
> leave
> it this way, you should add more doc to the visitor and fix 
> the bug you
> introduced.
> 
> Julian
> 
> 
> 
> 
> 
> 
> 
> > -----Original Message-----
> > From: Kurtis.Walker at thomsonreuters.com 
> > [mailto:Kurtis.Walker at thomsonreuters.com] 
> > Sent: Tuesday, October 13, 2009 12:25 PM
> > To: jhyde at pentaho.com; mondrian at pentaho.org; 
> > matthew.campbell at thomsonreuters.com
> > Subject: RE: [Mondrian] RE: Eigenbase perforce change 13092 
> for review
> > 
> > Hi Julian,
> >    Before we made the change of moving the iteration out of 
> > the visitee
> > and into the MdxVisitorImpl, we experienced the same 
> performance issue
> > that you have mentioned.  We override the visit of 
> ResolvedFunCall in
> > our new visitor and make decisions based on the result of 
> > visiting each
> > of the children.  So, the iteration in the visitee object was pure
> > wasted processing when using our new visitor.  The change 
> allowed the
> > other MdxVisitors(except RolapCell.DrillThroughVisitor) to remain
> > unchanged while keeping the same functionality.  
> > 
> > Kurt
> > 
> > -----Original Message-----
> > From: mondrian-bounces at pentaho.org 
> > [mailto:mondrian-bounces at pentaho.org]
> > On Behalf Of Julian Hyde
> > Sent: Tuesday, October 13, 2009 2:36 PM
> > To: Campbell, Matthew (Healthcare USA)
> > Cc: 'Mondrian developer mailing list'
> > Subject: [Mondrian] RE: Eigenbase perforce change 13092 for review
> > 
> > I don't know the cause of the build breakage, but this change is of
> > concern:
> > 
> > > ==== 
> > > //open/mondrian/src/main/mondrian/mdx/MdxVisitorImpl.java#4 
> > > (ktext) ====
> > > 
> > > 41a42,45
> > > >         // visit the call's arguments
> > > >         for (Exp arg : call.getArgs()) {
> > > >             arg.accept(this);
> > > >         }
> > 
> > In bug http://jira.pentaho.com/browse/MONDRIAN-608 both the 
> > visitor and
> > the
> > visitee were iterating over children. Exponentially bad performance.
> > This
> > looks very similar.
> > 
> > Conventionally in the visitor pattern, it is the visitee 
> that iterates
> > over
> > children (in the accept method) not the visitor (in the 
> visit method).
> > 
> > Julian
> > 
> > _______________________________________________
> > 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
> 
> 
> 




More information about the Mondrian mailing list