Re: PrefetchHelper bug

From: Twan Kogels (twa..wansoft.com)
Date: Tue Aug 31 2004 - 08:21:03 EDT

  • Next message: Twan Kogels: "Re: show sql"

    Hello Arndt,

    Thanks for the reply. When i readed the comments above the
    "resolveToManyRelations" function i already thought that this class wasn't
    tested fully in all cases. But i'm still happy you did commit it, i'm only
    using resolveToOneRelations but it's a real life/time saver for me.

    Cheers,
    Twan

    At 10:56 31-8-2004, you wrote:
    >Hi Twan,
    >
    >sorry, my fault, PrefetchHelper
    >is a class I once contributed,
    >but cause I'm not a committer,
    >I used a local copy for corrections
    >(I think I found the same bug)
    >and forgot to distribute the fixes.
    >
    >I attach my working copy, maybe someone
    >can care about committing to CVS?
    >
    >regards,
    >
    >Arndt
    >
    >PS: there are still some issues with this code,
    >e.g. filtering duplicates from the result-lists
    >(to increase performance)
    >
    >I am using it heavily to speed-up batch processing,
    >so if more people are using it it could be worth
    >investing some more engineering?
    >
    >PS2: we had a very nasty problem with ORACLE in some
    >cases refusing to use indices with this kind of
    >"where xy in (?,?,?...)" prefeteches, which we
    >finally solved by changing some optimizer parameters
    >in the ORACLE configuration.
    >Ask me for details if you encouter this type of problem
    >
    >Twan Kogels wrote:
    >
    >>Hello,
    >>I think i have found a bug in PrefetchHelper.java, i discovered it when
    >>testing my application which uses resolveToOneRelations() defined in
    >>PrefetchHelper.
    >>In "PrefetchHelper.java" there is a function called
    >>resolveToOneRelations(). Here's a the interesting code:
    >>===============
    >>List oids = new ArrayList(nobjects);
    >> for (int i = 0; i < nobjects; i++) {
    >> DataObject sourceObject = (DataObject) objects.get(i);
    >> DataObject targetObject = (DataObject)
    >> sourceObject.readProperty(relName);
    >> if (targetObject.getPersistenceState() ==
    >> PersistenceState.HOLLOW) {
    >> ObjectId oid = targetObject.getObjectId();
    >> oids.add(oid);
    >> }
    >> }
    >> // this maybe suboptimal, cause it uses an OR .. OR .. OR ..
    >> expression
    >> // instead of IN (..) - to be compatble with compound keys -
    >> // however, it seems to be quite fast as well
    >> SelectQuery sel = QueryUtils.selectQueryForIds(oids);
    >> context.performQuery(sel);
    >>===============
    >>It is possible that "List oids" will be a empty list when it reaches the
    >>line:
    >>===============
    >>SelectQuery sel = QueryUtils.selectQueryForIds(oids);
    >>===============
    >>because when a object has already been retrieved once the following line
    >>will return false and no object is added to oids list.
    >>===============
    >>if (targetObject.getPersistenceState() == PersistenceState.HOLLOW) {
    >>===============
    >>"QueryUtils.selectQueryForIds(oids);" will raise a
    >>IllegalArgumentException when the oids list is empty.
    >>===============
    >>IllegalArgumentException("List must contain at least one ObjectId")
    >>===============
    >>I now just catch this "IllegalArgumentException" in my code and ignore
    >>it. But a check in PrefetchHelper would be nice, something like:
    >>===============
    >>if(oids.size()!=0){
    >> SelectQuery sel = QueryUtils.selectQueryForIds(oids);
    >> context.performQuery(sel);
    >>}
    >>===============
    >>Cheers,
    >>Twan Kogels
    >
    >
    >
    >/*
    > * $Log: PrefetchHelper.java,v $
    > * Revision 1.8 2003/09/15 08:45:11 bl
    > * checkstyle fixes
    > *
    > * Revision 1.7 2003/09/12 14:28:32 sb2
    > * (ab)
    > * added a filter logic to prevent
    > * re-feteching of already resolved
    > * toMany relations
    > *
    > * (still 2 TODOs:
    > * - detect multiple occurences of the same object in the given list
    > * - look at the return value of resolveToOne: does
    > * it already return the targets already resolved?)
    > *
    > */
    >
    >package my.local.working_package;
    >
    >import org.objectstyle.cayenne.CayenneRuntimeException;
    >import org.objectstyle.cayenne.DataObject;
    >import org.objectstyle.cayenne.ObjectId;
    >import org.objectstyle.cayenne.access.DataContext;
    >import org.objectstyle.cayenne.access.ToManyList;
    >import org.objectstyle.cayenne.access.util.QueryUtils;
    >import org.objectstyle.cayenne.exp.Expression;
    >import org.objectstyle.cayenne.exp.ExpressionFactory;
    >import org.objectstyle.cayenne.map.DbAttributePair;
    >import org.objectstyle.cayenne.map.DbRelationship;
    >import org.objectstyle.cayenne.map.ObjEntity;
    >import org.objectstyle.cayenne.map.ObjRelationship;
    >import org.objectstyle.cayenne.query.SelectQuery;
    >
    >import java.util.ArrayList;
    >import java.util.HashMap;
    >import java.util.List;
    >import java.util.Map;
    >
    >
    >/**
    > *..uthor Arndt Brenschede
    > */
    >public final class PrefetchHelper
    >{
    > private PrefetchHelper()
    > {
    > }
    >
    > /**
    > * Resolves a toOne relationship for a list of objects.
    > * (performance tuning only)
    > */
    > public static List resolveToOneRelations( DataContext context, List
    > objects,
    > String relName )
    > {
    > int nobjects = objects.size();
    >
    > if ( nobjects == 0 )
    > {
    > return new ArrayList();
    > }
    >
    > List oids = new ArrayList( nobjects );
    >
    > for ( int i = 0; i < nobjects; i++ )
    > {
    > DataObject sourceObject = (DataObject)objects.get( i );
    > DataObject targetObject =
    > (DataObject)sourceObject.readPropertyDirectly( relName );
    >
    > if ( targetObject != null )
    > {
    > ObjectId oid = targetObject.getObjectId();
    > oids.add( oid );
    > }
    > }
    >
    > // this maybe suboptimal, cause it uses an OR .. OR .. OR .. expression
    > // instead of IN (..) - to be compatble with compound keys -
    > // however, it seems to be quite fast as well
    > List results;
    >
    > if ( oids.size() > 0 )
    > {
    > SelectQuery sel = QueryUtils.selectQueryForIds( oids );
    > results = context.performQuery( sel );
    > }
    > else
    > {
    > results = new ArrayList();
    > }
    >
    > return results;
    > }
    >
    > /**
    > * Resolves a toMany relation for a list of objects.
    > * (performance tuning only)
    > *
    > * <p>WARNING: this is a bit of a hack - it works for my
    > * toMany's, but it possibly doesn't work in all cases.</p>
    > *
    > *
    > * <p>*** It definitly does not work for compound keys ***</p>
    > */
    > public static List resolveToManyRelations( DataContext context, List
    > objectList,
    > String relName )
    > {
    > List objectsResolved = new ArrayList();
    >
    > // filter the list of objects to exclude those
    > // with the relation already resolved
    > List objects = new ArrayList();
    > for ( int i = 0; i < objectList.size(); i++ )
    > {
    > DataObject object = (DataObject)objectList.get( i );
    > ToManyList tml = (ToManyList)object.readPropertyDirectly( relName );
    >
    > if ( tml.needsFetch() )
    > {
    > objects.add( object );
    > }
    > else
    > {
    > objectsResolved.addAll( tml );
    > }
    > }
    >
    > int nobjects = objects.size();
    >
    > if ( nobjects == 0 )
    > {
    > return objectsResolved;
    > }
    >
    > // get the relationship meta object
    > ObjEntity ent = context.getEntityResolver().lookupObjEntity(
    > (DataObject)objects.get( 0 ) );
    > ObjRelationship rel = (ObjRelationship)ent.getRelationship( relName );
    >
    > // sanity check
    > if ( rel == null )
    > {
    > throw new CayenneRuntimeException( "ObjEntity '" + ent.getName()
    > + "' has no relationship of name '" + relName + "'" );
    > }
    >
    > ObjEntity destEnt = (ObjEntity)rel.getTargetEntity();
    >
    > List dbRels = rel.getDbRelationships();
    >
    > // sanity check
    > if ( ( dbRels == null ) || ( dbRels.size() != 1 ) )
    > {
    > throw new CayenneRuntimeException( "ObjRelationship '" + rel.getName()
    > + "' has no or >1 DbRels" );
    > }
    >
    > DbRelationship dbRel = (DbRelationship)dbRels.get( 0 );
    >
    > List joins = dbRel.getJoins();
    >
    > if ( joins.size() != 1 )
    > {
    > throw new CayenneRuntimeException( "DbRelationship '" + dbRel.getName()
    > + "' has no or >1 joins" );
    > }
    >
    > DbAttributePair join = (DbAttributePair)joins.get( 0 );
    > String dbKey = join.getSource().getName();
    > String foreignKey = join.getTarget().getName();
    >
    > // put the object-ids in a map for later assignment of the
    > // query results
    > Map listMap = new HashMap( nobjects );
    >
    > for ( int i = 0; i < nobjects; i++ )
    > {
    > DataObject object = (DataObject)objects.get( i );
    > ObjectId oid = object.getObjectId();
    > listMap.put( oid.getValueForAttribute( dbKey ), new ArrayList() );
    > }
    >
    > // get the reverse db-rel
    > DbRelationship reverse = dbRel.getReverseRelationship();
    >
    > if ( reverse == null )
    > {
    > throw new CayenneRuntimeException( "DbRelatitionship '" +
    > dbRel.getName()
    > + "' has no reverse relationship" );
    > }
    >
    > // do the query
    > SelectQuery sel = new SelectQuery( destEnt,
    > ExpressionFactory.binaryDbPathExp( Expression.IN, reverse.getName(),
    > ExpressionFactory.unaryExp( Expression.LIST, objects ) ) );
    > sel.setFetchingDataRows( true );
    >
    > List results = context.performQuery( sel );
    >
    > // sort the resulting objects into individual lists for each source
    > object
    > int nrows = results.size();
    >
    > for ( int k = 0; k < nrows; k++ )
    > {
    > Map row = (Map)results.get( k );
    > Object obj = context.objectFromDataRow( destEnt, row, false );
    > objectsResolved.add( obj );
    > ( (List)listMap.get( row.get( foreignKey ) ) ).add( obj );
    > }
    >
    > // and finally set these lists in the relation targets
    > for ( int i = 0; i < nobjects; i++ )
    > {
    > DataObject object = (DataObject)objects.get( i );
    > ObjectId oid = object.getObjectId();
    > List list = (List)listMap.get( oid.getValueForAttribute( dbKey ) );
    >
    > ( (ToManyList)object.readPropertyDirectly( relName )
    > ).setObjectList( list );
    > }
    >
    > return objectsResolved;
    > }
    >}
    >



    This archive was generated by hypermail 2.0.0 : Tue Aug 31 2004 - 08:18:42 EDT