[Mondrian] RE: Eigenbase perforce change 13092 for review

Fri Nov 6 10:51:29 EST 2009

Julian,

  We investigated that possibility, and were considering making that
proposal to you.  We thought making that change would affect a lot of
code, and thus has a high potential to introduce bugs.  And it would
result in some duplicated code between the visitors.  Also, we
considered that there may be other groups that are using MdxVisitor
outside of the Mondrian code base, and this change would force them to
make a change as well.  

I've just about finished coding the proposal I outlined.  As usual, the
code turned out a little different than the original proposal, but I am
pretty happy with it.  It allows individual visits to override the
default behavior.  How about I check in what I've got and you can review
it and let us know what you think?

Kurt

-----Original Message-----
From: Julian Hyde [mailto:jhyde at pentaho.com] 
Sent: Thursday, November 05, 2009 4:28 PM
To: Walker, Kurtis (Healthcare USA); mondrian at pentaho.org
Subject: RE: [Mondrian] RE: Eigenbase perforce change 13092 for review

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
> 
> 
> 