[Mondrian] RE: Eigenbase perforce change 13092 for review

[Julian Hyde]

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