OpenJPA
  1. OpenJPA
  2. OPENJPA-554

The GetMapValue class should have/supply an alias for ORDER-BY clauses.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.0.2
    • Fix Version/s: 1.0.3
    • Component/s: jdbc
    • Labels:
      None
    • Environment:
      MS SQLServer

      Description

      A generated select query asks for one of it's columns returned as
      a subselect, and then asks that the results be ordered by that subselect.
      The DBMS is throwing a spurious error message, saying that in order to
      do a SELECT DISTINCT/ORDER BY, the select list has to contain the
      column to be ordered by. It's spurious because the query clearly does
      list the identical subselect in the select list and the order-by, but the DBMS
      is apparently not smart enough to equate those.

      Here is a slightly simplified example:

      s.executeQuery("SELECT DISTINCT "
      + " t0.id, "
      + " (SELECT PMH_testPCKeyStringValue.value "
      + " FROM PMH_testPCKeyStringValue "
      + " WHERE PMH_testPCKeyStringValue.PERSISTENTMAPHOLDER_ID = t0.id "
      + " AND PMH_testPCKeyStringValue.testPCKeyStringValue = 1) "
      + "FROM PMH t0 "
      + "INNER JOIN PMH_testPCKeyStringValue t1 ON t0.id = t1.PERSISTENTMAPHOLDER_ID "
      + "WHERE ("
      + " (SELECT PMH_testPCKeyStringValue.value "
      + " FROM PMH_testPCKeyStringValue "
      + " WHERE PMH_testPCKeyStringValue.PERSISTENTMAPHOLDER_ID = t0.id "
      + " AND PMH_testPCKeyStringValue.testPCKeyStringValue = 1) "
      + " IS NOT NULL) "
      + "ORDER BY "
      + " (SELECT PMH_testPCKeyStringValue.value "
      + " FROM PMH_testPCKeyStringValue "
      + " WHERE PMH_testPCKeyStringValue.PERSISTENTMAPHOLDER_ID = t0.id "
      + " AND PMH_testPCKeyStringValue.testPCKeyStringValue = 1) "
      + "DESC");

      The actual SQL generated has parameter markers for the testPCKeyStringValue
      value, and is executed with a prepared statement.

      A modified query that works, which initially simply enough, involves declaring
      a column name for the subselect, and then using that column name in the order-by:

      s.executeQuery("SELECT DISTINCT "
      + " t0.id, "
      + " (SELECT PMH_testPCKeyStringValue.value "
      + " FROM PMH_testPCKeyStringValue "
      + " WHERE PMH_testPCKeyStringValue.PERSISTENTMAPHOLDER_ID = t0.id "
      + " AND PMH_testPCKeyStringValue.testPCKeyStringValue = 1) AS MY_COL_ALIAS "
      + "FROM PMH t0 "
      + "INNER JOIN PMH_testPCKeyStringValue t1 ON t0.id = t1.PERSISTENTMAPHOLDER_ID "
      + "WHERE ("
      + " (SELECT PMH_testPCKeyStringValue.value "
      + " FROM PMH_testPCKeyStringValue "
      + " WHERE PMH_testPCKeyStringValue.PERSISTENTMAPHOLDER_ID = t0.id "
      + " AND PMH_testPCKeyStringValue.testPCKeyStringValue = 1) "
      + " IS NOT NULL) "
      + "ORDER BY MY_COL_ALIAS "
      + "DESC");

      The fix, suggested by Abe White, and tested successfully by me (in this case/DBMS only) is:

      " - When we find JDOQL of the form "<map>.get(<value>)", we add the result
      of ExpressionFactory.getMapValue(...) to the expression tree.

      • In the case we're concerned with the ExpressionFactory in question is
        the org.apache.openjpa.jdbc.kernel.exps.JDBCExpressionFactory, and the
        return value is an org.apache.openjpa.jdbc.kernel.exps.GetMapValue.
      • The GetMapValue class manually constructs the SQL subselect to
        retrieve the value for the given key.

      Our goal is to alias the subselect in the SELECT portion of the query,
      to keep the subselect unaliased in the WHERE portion, and to use the
      SELECT alias in place of the subselect in the ORDER BY portion.
      Luckily, I believe this can be accomplished easily with a few
      modifcations to the GetMapValue class:

      • Add a "String _alias" member to GetMapValue. This will be a unique
        alias within the select for the subselect we'll produce. I recommend
        generating this value with a monotonically-increasing int in
        JDBCExpressionFactory and passing it to the GetMapValue constructor.
        I.e.:

      class JDBCExpressionFactory {
      private int _getMapValueAlias = 0;
      ...
      Value getMapValue(...)

      { return new GetMapValue(..., "gmv" + _getMapValueAlias++); }

      }

      • In GetMapValue.select(...), append " AS " + the _alias member to the
        SQLBuffer returned by newSQLBuffer(...).
      • In GetMapValue.orderBy(...), just order by the _alias member, not the
        result of newSQLBuffer(...).

      This should work because when we construct the select (see
      org.apache.openjpa.jdbc.exps.SelectConstructor) we automatically call
      select(...) for any ordering value, in addition to orderBy(...). So the
      same GetMapValue instance will have a chance to create both its SELECT
      SQL and its ORDER BY SQL.

      Notes:

      • You might only want to use subselect aliasing at all if the
        DBDictionary in use (accessible through ctx.store.getDBDictionary()) has
        its requiresAliasForSubselect field set to true. Or maybe it would be
        best for all dictionaries. I don't know – it would require a test run
        on all our supported databases to see what each one likes. My hunch
        would be to do it for all dictionaries."

        Activity

        Hide
        Abe White added a comment -

        Applied suggested fix in revision 645589.

        Show
        Abe White added a comment - Applied suggested fix in revision 645589.
        Hide
        Joe Weinstein added a comment -

        Here is the svn diff that solves my example:

        Index: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/kernel/exps/JDBCExpressionFactory.java
        ===================================================================
        — openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/kernel/exps/JDBCExpressionFactory.java
        (revision 633245)
        +++ openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/kernel/exps/JDBCExpressionFactory.java
        (working copy)
        @@ -59,6 +59,8 @@
        private final ClassMapping _type;
        private final SelectConstructor _cons = new SelectConstructor();

        + private int _getMapValueAlias = 0;
        +
        /**

        • Constructor. Supply the type we're querying against.
          */
          @@ -396,6 +398,6 @@
          }

        public Value getMapValue(Value map, Value arg)

        { - return new GetMapValue((Val) map, (Val) arg); + return new GetMapValue((Val) map, (Val) arg, "gmv" + _getMapValueAlias++); }

        }
        Index: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/kernel/exps/GetMapValue.java
        ===================================================================
        — openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/kernel/exps/GetMapValue.java
        (revision 633245)
        +++ openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/kernel/exps/GetMapValue.java
        (working copy)
        @@ -47,13 +47,15 @@
        private final Val _key;
        private ClassMetaData _meta = null;
        private Class _cast = null;
        + private String _alias = null;

        /**

        • Constructor. Provide the map and key to operate on.
          */
        • public GetMapValue(Val map, Val key) {
          + public GetMapValue(Val map, Val key, String alias) { _map = map; _key = key; + _alias = alias; }

        public ClassMetaData getMetaData() {
        @@ -111,7 +113,9 @@

        public void select(Select sel, ExpContext ctx, ExpState state,
        boolean pks)

        { - sel.select(newSQLBuffer(sel, ctx, state), this); + SQLBuffer sqb = newSQLBuffer(sel, ctx, state); + sqb.append(" AS " + _alias ); + sel.select(sqb, this); }

        public void selectColumns(Select sel, ExpContext ctx, ExpState state,
        @@ -127,7 +131,7 @@

        public void orderBy(Select sel, ExpContext ctx, ExpState state,
        boolean asc)

        { - sel.orderBy(newSQLBuffer(sel, ctx, state), asc, false); + sel.orderBy(_alias, asc, false); }

        private SQLBuffer newSQLBuffer(Select sel, ExpContext ctx, ExpState state) {

        Show
        Joe Weinstein added a comment - Here is the svn diff that solves my example: Index: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/kernel/exps/JDBCExpressionFactory.java =================================================================== — openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/kernel/exps/JDBCExpressionFactory.java (revision 633245) +++ openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/kernel/exps/JDBCExpressionFactory.java (working copy) @@ -59,6 +59,8 @@ private final ClassMapping _type; private final SelectConstructor _cons = new SelectConstructor(); + private int _getMapValueAlias = 0; + /** Constructor. Supply the type we're querying against. */ @@ -396,6 +398,6 @@ } public Value getMapValue(Value map, Value arg) { - return new GetMapValue((Val) map, (Val) arg); + return new GetMapValue((Val) map, (Val) arg, "gmv" + _getMapValueAlias++); } } Index: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/kernel/exps/GetMapValue.java =================================================================== — openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/kernel/exps/GetMapValue.java (revision 633245) +++ openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/kernel/exps/GetMapValue.java (working copy) @@ -47,13 +47,15 @@ private final Val _key; private ClassMetaData _meta = null; private Class _cast = null; + private String _alias = null; /** Constructor. Provide the map and key to operate on. */ public GetMapValue(Val map, Val key) { + public GetMapValue(Val map, Val key, String alias) { _map = map; _key = key; + _alias = alias; } public ClassMetaData getMetaData() { @@ -111,7 +113,9 @@ public void select(Select sel, ExpContext ctx, ExpState state, boolean pks) { - sel.select(newSQLBuffer(sel, ctx, state), this); + SQLBuffer sqb = newSQLBuffer(sel, ctx, state); + sqb.append(" AS " + _alias ); + sel.select(sqb, this); } public void selectColumns(Select sel, ExpContext ctx, ExpState state, @@ -127,7 +131,7 @@ public void orderBy(Select sel, ExpContext ctx, ExpState state, boolean asc) { - sel.orderBy(newSQLBuffer(sel, ctx, state), asc, false); + sel.orderBy(_alias, asc, false); } private SQLBuffer newSQLBuffer(Select sel, ExpContext ctx, ExpState state) {

          People

          • Assignee:
            Unassigned
            Reporter:
            Joe Weinstein
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 48h
              48h
              Remaining:
              Remaining Estimate - 48h
              48h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development