Break a much gentler FieldDefinition out of FieldOverride. The former *must* be supplied for any computed field; the latter must *only* be supplied for fields that would otherwise reference concrete columns. This lets us be more intelligent about what we expand where. Also bring back the old semi-conservative selection criterion for two-pass cache-happy collection queries; it gets simply too hairy when parts of an object's primary key comes from a separate table.

src/java/com/samskivert/jdbc/depot/BindVisitor.java

@@ -26,7 +26,7 @@ import java.util.Set;
 
 import com.samskivert.jdbc.depot.Key.WhereCondition;
 import com.samskivert.jdbc.depot.clause.DeleteClause;
-import com.samskivert.jdbc.depot.clause.FieldOverride;
+import com.samskivert.jdbc.depot.clause.FieldDefinition;
 import com.samskivert.jdbc.depot.clause.ForUpdate;
 import com.samskivert.jdbc.depot.clause.FromOverride;
 import com.samskivert.jdbc.depot.clause.GroupBy;
@@ -63,7 +63,7 @@ public class BindVisitor implements ExpressionVisitor
     {
     }
 
-    public void visit (FieldOverride fieldOverride)
+    public void visit (FieldDefinition fieldOverride)
         throws Exception
     {
     }

src/java/com/samskivert/jdbc/depot/BuildVisitor.java

@@ -29,6 +29,7 @@ import java.util.Set;
 import com.samskivert.jdbc.depot.Key.WhereCondition;
 import com.samskivert.jdbc.depot.annotation.Computed;
 import com.samskivert.jdbc.depot.clause.DeleteClause;
+import com.samskivert.jdbc.depot.clause.FieldDefinition;
 import com.samskivert.jdbc.depot.clause.FieldOverride;
 import com.samskivert.jdbc.depot.clause.ForUpdate;
 import com.samskivert.jdbc.depot.clause.FromOverride;
@@ -81,13 +82,13 @@ public abstract class BuildVisitor implements ExpressionVisitor
         }
     }
 
-    public void visit (FieldOverride fieldOverride)
+    public void visit (FieldDefinition definition)
         throws Exception
     {
-        fieldOverride.getOverride().accept(this);
-        if (_aliasFields) {
+        definition.getDefinition().accept(this);
+        if (_enableAliasing) {
             _builder.append(" as ");
-            appendIdentifier(fieldOverride.getField());
+            appendIdentifier(definition.getField());
         }
     }
 
@@ -280,9 +281,7 @@ public abstract class BuildVisitor implements ExpressionVisitor
         _builder.append(" as ");
         appendTableAbbreviation(join.getJoinClass());
         _builder.append(" on ");
-        _ignoreOverrides = true;
         join.getJoinCondition().accept(this);
-        _ignoreOverrides = false;
     }
 
     public void visit (Limit limit)
@@ -315,23 +314,23 @@ public abstract class BuildVisitor implements ExpressionVisitor
         }
         _builder.append("select ");
 
-        if (_overrides.containsKey(pClass)) {
+        if (_definitions.containsKey(pClass)) {
             throw new IllegalArgumentException(
                 "Can not yet nest SELECTs on the same persistent record.");
         }
 
-        Map<String, FieldOverride> overrideMap = new HashMap<String, FieldOverride>();
-        for (FieldOverride override : selectClause.getFieldOverrides()) {
-            overrideMap.put(override.getField(), override);
+        Map<String, FieldDefinition> definitionMap = new HashMap<String, FieldDefinition>();
+        for (FieldDefinition definition : selectClause.getFieldDefinitions()) {
+            definitionMap.put(definition.getField(), definition);
         }
-        _overrides.put(pClass, overrideMap);
+        _definitions.put(pClass, definitionMap);
 
         try {
             // iterate over the fields we're filling in and figure out whence each one comes
             boolean skip = true;
 
-            // while expanding column names in the SELECT query, do aliasing
-            _aliasFields = true;
+            // while expanding column names in the SELECT query, do aliasing and expansion
+            _enableAliasing = _enableOverrides = true;
 
             for (String field : selectClause.getFields()) {
                 if (!skip) {
@@ -349,7 +348,7 @@ public abstract class BuildVisitor implements ExpressionVisitor
             }
 
             // then stop
-            _aliasFields = false;
+            _enableAliasing = _enableOverrides = false;
 
             if (selectClause.getFromOverride() != null) {
                 selectClause.getFromOverride().accept(this);
@@ -384,7 +383,7 @@ public abstract class BuildVisitor implements ExpressionVisitor
             }
 
         } finally {
-            _overrides.remove(pClass);
+            _definitions.remove(pClass);
         }
         if (isInner) {
             _builder.append(")");
@@ -508,19 +507,36 @@ public abstract class BuildVisitor implements ExpressionVisitor
                 "Unknown field on persistent record [record=" + type + ", field=" + field + "]");
         }
 
-        if (!_ignoreOverrides) {
-            Map<String, FieldOverride> fieldOverrides = _overrides.get(type);
-            if (fieldOverrides != null) {
-                // first, see if there's a field override
-                FieldOverride override = fieldOverrides.get(field);
-                if (override != null) {
+        Map<String, FieldDefinition> fieldOverrides = _definitions.get(type);
+        if (fieldOverrides != null) {
+            // first, see if there's a field override
+            FieldDefinition override = fieldOverrides.get(field);
+
+            if (override != null) {
+                boolean useOverride;
+                if (override instanceof FieldOverride) {
+                    if (fm.getComputed() != null || dm.getComputed() != null) {
+                        throw new IllegalArgumentException(
+                            "FieldOverride cannot be used on @Computed field: " + field);
+                    }
+                    useOverride = _enableOverrides;
+                } else if (fm.getComputed() == null && dm.getComputed() == null) {
+                    throw new IllegalArgumentException(
+                        "FieldDefinition must not be used on concrete field: " + field);
+                } else {
+                    useOverride = true;
+                }
+
+                if (useOverride) {
                     // If a FieldOverride's target is in turn another FieldOverride, the second
                     // one is ignored. As an example, when creating ItemRecords from CloneRecords,
                     // we make Item.itemId = Clone.itemId. We also make Item.parentId = Item.itemId
                     // and would be dismayed to find Item.parentID = Item.itemId = Clone.itemId.
-                    _ignoreOverrides = true;
+
+                    boolean saved = _enableOverrides;
+                    _enableOverrides = false;
                     override.accept(this);
-                    _ignoreOverrides = false;
+                    _enableOverrides = saved;
                     return;
                 }
             }
@@ -548,7 +564,7 @@ public abstract class BuildVisitor implements ExpressionVisitor
             // check if the computed field has a literal SQL definition
             if (fieldComputed.fieldDefinition().length() > 0) {
                 _builder.append(fieldComputed.fieldDefinition());
-                if (_aliasFields) {
+                if (_enableAliasing) {
                     _builder.append(" as ");
                     appendIdentifier(field);
                 }
@@ -590,13 +606,13 @@ public abstract class BuildVisitor implements ExpressionVisitor
     protected StringBuilder _builder = new StringBuilder();
 
     /** A mapping of field overrides per persistent record. */
-    protected Map<Class<? extends PersistentRecord>, Map<String, FieldOverride>> _overrides =
-        new HashMap<Class<? extends PersistentRecord>, Map<String,FieldOverride>>();
+    protected Map<Class<? extends PersistentRecord>, Map<String, FieldDefinition>> _definitions=
+        new HashMap<Class<? extends PersistentRecord>, Map<String,FieldDefinition>>();
 
     /** A flag that's set to true for inner SELECT's */
     protected boolean _innerClause = false;
 
-    protected boolean _ignoreOverrides = false;
+    protected boolean _enableOverrides = false;
 
-    protected boolean _aliasFields = false;
+    protected boolean _enableAliasing = false;
 }

src/java/com/samskivert/jdbc/depot/DepotRepository.java

@@ -37,6 +37,8 @@ import com.samskivert.jdbc.JDBCUtil;
 
 import com.samskivert.jdbc.depot.Modifier.*;
 import com.samskivert.jdbc.depot.clause.DeleteClause;
+import com.samskivert.jdbc.depot.clause.FieldOverride;
+import com.samskivert.jdbc.depot.clause.FromOverride;
 import com.samskivert.jdbc.depot.clause.InsertClause;
 import com.samskivert.jdbc.depot.clause.QueryClause;
 import com.samskivert.jdbc.depot.clause.UpdateClause;
@@ -147,6 +149,14 @@ public abstract class DepotRepository
         boolean useExplicit =
             (marsh.getTableName() == null) || !marsh.hasPrimaryKey() || !_ctx.isUsingCache();
 
+        // TODO: The two-pass query still doesn't do well in circumstances where we do tricky
+        // TODO: things such as override the table from whence the primary key fields come, so
+        // TODO: somewhat conservatively we fall back on the traditional explicit approach on
+        // TODO: any query involving FromOverrides or FieldOverrides. FieldDefinitions should
+        // TODO: not present a problem, however.
+        for (QueryClause clause : clauses) {
+            useExplicit |= (clause instanceof FieldOverride || clause instanceof FromOverride);
+        }
         return _ctx.invoke(useExplicit ?
             new FindAllQuery.Explicitly<T>(_ctx, type, clauses) :
             new FindAllQuery.WithCache<T>(_ctx, type, clauses));

src/java/com/samskivert/jdbc/depot/FindAllQuery.java

@@ -35,6 +35,7 @@ import com.samskivert.util.ArrayUtil;
 import com.samskivert.jdbc.DatabaseLiaison;
 import com.samskivert.jdbc.JDBCUtil;
 
+import com.samskivert.jdbc.depot.clause.FieldDefinition;
 import com.samskivert.jdbc.depot.clause.FieldOverride;
 import com.samskivert.jdbc.depot.clause.Join;
 import com.samskivert.jdbc.depot.clause.QueryClause;
@@ -102,7 +103,7 @@ public abstract class FindAllQuery<T extends PersistentRecord>
                 // a select subset of query clauses are preserved for the entity query
                 QueryClause[] newClauses = new QueryClause[_clauses.length + 1];
                 for (int ii = 0; ii < _clauses.length; ii ++) {
-                    if (_clauses[ii] instanceof Join || _clauses[ii] instanceof FieldOverride) {
+                    if (_clauses[ii] instanceof Join || _clauses[ii] instanceof FieldDefinition) {
                         newClauses[jj ++] = _clauses[ii];
                     }
                 }

src/java/com/samskivert/jdbc/depot/clause/FieldDefinition.java

@@ -0,0 +1,94 @@
+//
+// $Id$
+//
+// samskivert library - useful routines for java programs
+// Copyright (C) 2006-2007 Michael Bayne, Pär Winzell
+//
+// This library is free software; you can redistribute it and/or modify it
+// under the terms of the GNU Lesser General Public License as published
+// by the Free Software Foundation; either version 2.1 of the License, or
+// (at your option) any later version.
+//
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+// Lesser General Public License for more details.
+//
+// You should have received a copy of the GNU Lesser General Public
+// License along with this library; if not, write to the Free Software
+// Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+
+package com.samskivert.jdbc.depot.clause;
+
+import java.util.Collection;
+
+import com.samskivert.io.PersistenceException;
+import com.samskivert.jdbc.depot.PersistentRecord;
+import com.samskivert.jdbc.depot.expression.ColumnExp;
+import com.samskivert.jdbc.depot.expression.ExpressionVisitor;
+import com.samskivert.jdbc.depot.expression.LiteralExp;
+import com.samskivert.jdbc.depot.expression.SQLExpression;
+
+/**
+ * Supplies a definition for a computed field of the persistent object we're creating.
+ *
+ * Thus the select portion of a query can include a reference to a different column in a different
+ * table through a {@link ColumnExp}, or a literal expression such as COUNT(*) through a
+ * {@link LiteralExp}.
+ *
+ * @see FieldOverride
+ */
+public class FieldDefinition extends QueryClause
+{
+    public FieldDefinition (String field, String str)
+        throws PersistenceException
+    {
+        this(field, new LiteralExp(str));
+    }
+
+    public FieldDefinition (String field, Class<? extends PersistentRecord> pClass, String pCol)
+        throws PersistenceException
+    {
+        this(field, new ColumnExp(pClass, pCol));
+    }
+
+    public FieldDefinition (String field, SQLExpression override)
+        throws PersistenceException
+    {
+        _field = field;
+        _definition = override;
+    }
+
+    /**
+     * The field we're defining. The Query object uses this for indexing.
+     */
+    public String getField ()
+    {
+        return _field;
+    }
+
+    public SQLExpression getDefinition ()
+    {
+        return _definition;
+    }
+
+    // from SQLExpression
+    public void addClasses (Collection<Class<? extends PersistentRecord>> classSet)
+    {
+        _definition.addClasses(classSet);
+    }
+
+    // from SQLExpression
+    public void accept (ExpressionVisitor visitor)
+        throws Exception
+    {
+        visitor.visit(this);
+    }
+
+    /** The name of the field on the persistent object to override. */
+    protected String _field;
+
+    /** The defining expression. */
+    protected SQLExpression _definition;
+
+}

src/java/com/samskivert/jdbc/depot/clause/FieldOverride.java

@@ -3,7 +3,7 @@
 //
 // samskivert library - useful routines for java programs
 // Copyright (C) 2006-2007 Michael Bayne, Pär Winzell
-// 
+//
 // This library is free software; you can redistribute it and/or modify it
 // under the terms of the GNU Lesser General Public License as published
 // by the Free Software Foundation; either version 2.1 of the License, or
@@ -20,12 +20,9 @@
 
 package com.samskivert.jdbc.depot.clause;
 
-import java.util.Collection;
-
 import com.samskivert.io.PersistenceException;
 import com.samskivert.jdbc.depot.PersistentRecord;
 import com.samskivert.jdbc.depot.expression.ColumnExp;
-import com.samskivert.jdbc.depot.expression.ExpressionVisitor;
 import com.samskivert.jdbc.depot.expression.LiteralExp;
 import com.samskivert.jdbc.depot.expression.SQLExpression;
 
@@ -37,57 +34,23 @@ import com.samskivert.jdbc.depot.expression.SQLExpression;
  * table through a {@link ColumnExp}, or a literal expression such as COUNT(*) through a
  * {@link LiteralExp}.
  */
-public class FieldOverride extends QueryClause
+public class FieldOverride extends FieldDefinition
 {
     public FieldOverride (String field, String str)
         throws PersistenceException
     {
-        this(field, new LiteralExp(str));
+        super(field, str);
     }
 
     public FieldOverride (String field, Class<? extends PersistentRecord> pClass, String pCol)
         throws PersistenceException
     {
-        this(field, new ColumnExp(pClass, pCol));
+        super(field, pClass, pCol);
     }
 
     public FieldOverride (String field, SQLExpression override)
         throws PersistenceException
     {
-        _field = field;
-        _override = override;
+        super(field, override);
     }
-
-    /**
-     * The field we're overriding. The Query object uses this for indexing.
-     */
-    public String getField ()
-    {
-        return _field;
-    }
-
-    public SQLExpression getOverride ()
-    {
-        return _override;
-    }
-
-    // from SQLExpression
-    public void addClasses (Collection<Class<? extends PersistentRecord>> classSet)
-    {
-        _override.addClasses(classSet);
-    }
-
-    // from SQLExpression
-    public void accept (ExpressionVisitor visitor)
-        throws Exception
-    {
-        visitor.visit(this); 
-    }
-
-    /** The name of the field on the persistent object to override. */
-    protected String _field;
-
-    /** The overriding expression. */
-    protected SQLExpression _override;
-
 }

src/java/com/samskivert/jdbc/depot/clause/SelectClause.java

@@ -67,8 +67,8 @@ public class SelectClause<T extends PersistentRecord> extends QueryClause
             } else if (clause instanceof Join) {
                 _joinClauses.add((Join) clause);
 
-            } else if (clause instanceof FieldOverride) {
-                _disMap.put(((FieldOverride) clause).getField(), ((FieldOverride) clause));
+            } else if (clause instanceof FieldDefinition) {
+                _disMap.put(((FieldDefinition) clause).getField(), ((FieldDefinition) clause));
 
             } else if (clause instanceof OrderBy) {
                 if (_orderBy != null) {
@@ -101,12 +101,12 @@ public class SelectClause<T extends PersistentRecord> extends QueryClause
         }
     }
 
-    public FieldOverride lookupOverride (String field)
+    public FieldDefinition lookupDefinition (String field)
     {
         return _disMap.get(field);
     }
 
-    public Collection<FieldOverride> getFieldOverrides ()
+    public Collection<FieldDefinition> getFieldDefinitions ()
     {
         return _disMap.values();
     }
@@ -169,7 +169,7 @@ public class SelectClause<T extends PersistentRecord> extends QueryClause
         for (Join join : _joinClauses) {
             join.addClasses(classSet);
         }
-        for (FieldOverride override : _disMap.values()) {
+        for (FieldDefinition override : _disMap.values()) {
             override.addClasses(classSet);
         }
     }
@@ -181,7 +181,7 @@ public class SelectClause<T extends PersistentRecord> extends QueryClause
     }
 
     /** Persistent class fields mapped to field override clauses. */
-    protected Map<String, FieldOverride> _disMap = new HashMap<String, FieldOverride>();
+    protected Map<String, FieldDefinition> _disMap = new HashMap<String, FieldDefinition>();
 
     /** The persistent class this select defines. */
     protected Class<T> _pClass;

src/java/com/samskivert/jdbc/depot/expression/ExpressionVisitor.java

@@ -26,7 +26,7 @@ import com.samskivert.jdbc.depot.MultiKey;
 import com.samskivert.jdbc.depot.PersistentRecord;
 
 import com.samskivert.jdbc.depot.clause.DeleteClause;
-import com.samskivert.jdbc.depot.clause.FieldOverride;
+import com.samskivert.jdbc.depot.clause.FieldDefinition;
 import com.samskivert.jdbc.depot.clause.ForUpdate;
 import com.samskivert.jdbc.depot.clause.FromOverride;
 import com.samskivert.jdbc.depot.clause.GroupBy;
@@ -50,7 +50,7 @@ import com.samskivert.jdbc.depot.operator.SQLOperator.MultiOperator;
  */
 public interface ExpressionVisitor
 {
-    public void visit (FieldOverride fieldOverride)
+    public void visit (FieldDefinition fieldOverride)
         throws Exception;
     public void visit (WhereCondition<? extends PersistentRecord> whereCondition)
         throws Exception;