METAMODEL-1161 fix maxRows on Oracle closes #158

(cherry picked from commit 7c8628a)
apache · Aug 24, 2017 · 4e820fc · 4e820fc
1 parent d11d9fb
commit 4e820fc
Show file tree

Hide file tree

Showing 14 changed files with 314 additions and 204 deletions.
diff --git a/jdbc/src/main/java/org/apache/metamodel/jdbc/JdbcDataContext.java b/jdbc/src/main/java/org/apache/metamodel/jdbc/JdbcDataContext.java
@@ -391,7 +391,7 @@ private DataSet execute(Connection connection, Query query, Statement statement,
         final Integer firstRow = query.getFirstRow();
         boolean postProcessFirstRow = false;
         if (firstRow != null) {
-            if (_queryRewriter.isFirstRowSupported()) {
+            if (_queryRewriter.isFirstRowSupported(query)) {
                 logger.debug("First row property will be treated by query rewriter");
             } else {
                 postProcessFirstRow = true;

diff --git a/jdbc/src/main/java/org/apache/metamodel/jdbc/dialects/DB2QueryRewriter.java b/jdbc/src/main/java/org/apache/metamodel/jdbc/dialects/DB2QueryRewriter.java
@@ -60,7 +60,7 @@ public boolean isMaxRowsSupported() {
     }
 
     @Override
-    public boolean isFirstRowSupported() {
+    public boolean isFirstRowSupported(final Query query) {
         return true;
     }
 

diff --git a/jdbc/src/main/java/org/apache/metamodel/jdbc/dialects/DefaultQueryRewriter.java b/jdbc/src/main/java/org/apache/metamodel/jdbc/dialects/DefaultQueryRewriter.java
@@ -207,7 +207,7 @@ public boolean isAggregateFunctionSupported(AggregateFunction function) {
     }
 
     @Override
-    public boolean isFirstRowSupported() {
+    public boolean isFirstRowSupported(final Query query) {
         return false;
     }
 

diff --git a/jdbc/src/main/java/org/apache/metamodel/jdbc/dialects/HsqldbQueryRewriter.java b/jdbc/src/main/java/org/apache/metamodel/jdbc/dialects/HsqldbQueryRewriter.java
@@ -49,7 +49,7 @@ public String rewriteColumnType(ColumnType columnType, Integer columnSize) {
     }
 
     @Override
-    public boolean isFirstRowSupported() {
+    public boolean isFirstRowSupported(final Query query) {
         return true;
     }
 

diff --git a/jdbc/src/main/java/org/apache/metamodel/jdbc/dialects/IQueryRewriter.java b/jdbc/src/main/java/org/apache/metamodel/jdbc/dialects/IQueryRewriter.java
@@ -90,8 +90,11 @@ public void setStatementParameter(final PreparedStatement st, final int valueInd
      * 
      * @return whether this query rewriter is able to write the "First row"
      *         query property to the query string.
+     *
+     * @param query For some database engines, the content of the query decides
+     *        the ability to change first row
      */
-    public boolean isFirstRowSupported();
+    public boolean isFirstRowSupported(final Query query);
 
     /**
      * Determines whether a specific scalar function is supported by the

diff --git a/jdbc/src/main/java/org/apache/metamodel/jdbc/dialects/LimitOffsetQueryRewriter.java b/jdbc/src/main/java/org/apache/metamodel/jdbc/dialects/LimitOffsetQueryRewriter.java
@@ -32,7 +32,7 @@ public LimitOffsetQueryRewriter(JdbcDataContext dataContext) {
     }
 
     @Override
-    public final boolean isFirstRowSupported() {
+    public final boolean isFirstRowSupported(final Query query) {
         return true;
     }
 

diff --git a/jdbc/src/main/java/org/apache/metamodel/jdbc/dialects/OffsetFetchQueryRewriter.java b/jdbc/src/main/java/org/apache/metamodel/jdbc/dialects/OffsetFetchQueryRewriter.java
@@ -1,20 +1,14 @@
 /**
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements.  See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership.  The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License.  You may obtain a copy of the License at
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license agreements.  See the NOTICE
+ * file distributed with this work for additional information regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the
+ * License.  You may obtain a copy of the License at
  *
- *   http://www.apache.org/licenses/LICENSE-2.0
+ * http://www.apache.org/licenses/LICENSE-2.0
  *
- * Unless required by applicable law or agreed to in writing,
- * software distributed under the License is distributed on an
- * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
- * KIND, either express or implied.  See the License for the
- * specific language governing permissions and limitations
- * under the License.
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
+ * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations under the License.
  */
 package org.apache.metamodel.jdbc.dialects;
 
@@ -27,43 +21,59 @@
  */
 public abstract class OffsetFetchQueryRewriter extends DefaultQueryRewriter {
 
-    private final String databaseProductName;
-    private final int databaseSupportedVersion;
+    private final String _databaseProductName;
+    private final int _databaseSupportedVersion;
+    private final boolean _fetchNeedsOffsetAndOrderBy;
 
-    public OffsetFetchQueryRewriter(JdbcDataContext dataContext, int minSupportedVersion) {
+    public OffsetFetchQueryRewriter(final JdbcDataContext dataContext, final int minSupportedVersion,
+            final boolean fetchNeedsOrderBy) {
         super(dataContext);
-        databaseProductName = dataContext.getDatabaseProductName();
-        databaseSupportedVersion = minSupportedVersion;
+        _databaseProductName = dataContext.getDatabaseProductName();
+        _databaseSupportedVersion = minSupportedVersion;
+        _fetchNeedsOffsetAndOrderBy = fetchNeedsOrderBy;
     }
 
     @Override
-    public final boolean isFirstRowSupported() {
-        return true;
+    public boolean isFirstRowSupported(final Query query) {
+        return isSupportedVersion(_databaseProductName, _databaseSupportedVersion) && !query.getOrderByClause()
+                .isEmpty();
     }
 
     @Override
-    public final boolean isMaxRowsSupported() {
-        return true;
+    public boolean isMaxRowsSupported() {
+        return isSupportedVersion(_databaseProductName, _databaseSupportedVersion);
     }
 
     /**
      * {@inheritDoc}
-     * 
+     *
      * If the Max rows and First row property of the query is set, then we
      * will use the database's "OFFSET i ROWS FETCH NEXT j ROWS ONLY" construct.
      */
     @Override
-    public String rewriteQuery(Query query) {
+    public String rewriteQuery(final Query query) {
+        final boolean hasOrderBy = !query.getOrderByClause().isEmpty();
         String queryString = super.rewriteQuery(query);
-        if(isSupportedVersion(databaseProductName, databaseSupportedVersion)) {
-            Integer maxRows = query.getMaxRows();
+
+        if (isSupportedVersion(_databaseProductName, _databaseSupportedVersion) && (query.getMaxRows() != null
+                || query.getFirstRow() != null)) {
+            final Integer maxRows = query.getMaxRows();
             Integer firstRow = query.getFirstRow();
-            if (maxRows != null && firstRow != null && queryString.indexOf("ORDER BY") >= 0 ) {
-                queryString = queryString.replaceAll("TOP [0-9]+", "");
-                queryString = queryString + " OFFSET " + (firstRow-1) + " ROWS FETCH NEXT " + maxRows + " ROWS ONLY";
+
+            if (!_fetchNeedsOffsetAndOrderBy || hasOrderBy) {
+                if (firstRow != null) {
+                    queryString = queryString + " OFFSET " + (firstRow - 1) + " ROWS";
+                } else if (_fetchNeedsOffsetAndOrderBy) {
+                    // TOP should do it.
+                    return queryString;
+                }
+
+                if (maxRows != null) {
+                    queryString = queryString.replaceAll(" TOP [0-9]+", "");
+                    queryString += " FETCH NEXT " + maxRows + " ROWS ONLY";
+                }
             }
         }
         return queryString;
     }
-
 }
diff --git a/jdbc/src/main/java/org/apache/metamodel/jdbc/dialects/OracleQueryRewriter.java b/jdbc/src/main/java/org/apache/metamodel/jdbc/dialects/OracleQueryRewriter.java
@@ -18,8 +18,6 @@
  */
 package org.apache.metamodel.jdbc.dialects;
 
-import static org.apache.metamodel.jdbc.JdbcDataContext.DATABASE_PRODUCT_ORACLE;
-
 import org.apache.metamodel.jdbc.JdbcDataContext;
 import org.apache.metamodel.query.FilterItem;
 import org.apache.metamodel.schema.ColumnType;
@@ -32,7 +30,7 @@ public class OracleQueryRewriter extends OffsetFetchQueryRewriter {
     public static final int FIRST_FETCH_SUPPORTING_VERSION = 12;
 
     public OracleQueryRewriter(JdbcDataContext dataContext) {
-        super(dataContext, FIRST_FETCH_SUPPORTING_VERSION);
+        super(dataContext, FIRST_FETCH_SUPPORTING_VERSION, false);
     }
 
     @Override

diff --git a/jdbc/src/main/java/org/apache/metamodel/jdbc/dialects/SQLServerQueryRewriter.java b/jdbc/src/main/java/org/apache/metamodel/jdbc/dialects/SQLServerQueryRewriter.java
@@ -36,7 +36,12 @@ public class SQLServerQueryRewriter extends OffsetFetchQueryRewriter {
     public static final int FIRST_FETCH_SUPPORTING_VERSION = 11;
 
     public SQLServerQueryRewriter(JdbcDataContext dataContext) {
-        super(dataContext, FIRST_FETCH_SUPPORTING_VERSION);
+        super(dataContext, FIRST_FETCH_SUPPORTING_VERSION, true);
+    }
+
+    @Override
+    public boolean isMaxRowsSupported() {
+        return true;
     }
 
     /**

diff --git a/jdbc/src/test/java/org/apache/metamodel/dialects/SQLServerQueryRewriterTest.java b/jdbc/src/test/java/org/apache/metamodel/dialects/SQLServerQueryRewriterTest.java
@@ -20,11 +20,6 @@
 
 import static org.apache.metamodel.jdbc.JdbcDataContext.DATABASE_PRODUCT_SQLSERVER;
 
-import java.sql.Connection;
-import java.sql.DatabaseMetaData;
-
-import junit.framework.TestCase;
-
 import org.apache.metamodel.jdbc.JdbcDataContext;
 import org.apache.metamodel.jdbc.dialects.SQLServerQueryRewriter;
 import org.apache.metamodel.query.FilterItem;
@@ -38,6 +33,9 @@
 import org.apache.metamodel.schema.MutableTable;
 import org.apache.metamodel.util.TimeComparator;
 import org.easymock.EasyMock;
+import org.junit.Assert;
+
+import junit.framework.TestCase;
 
 public class SQLServerQueryRewriterTest extends TestCase {
 
@@ -89,6 +87,31 @@ public void testSelectMaxRowsRewriting() throws Exception {
         assertEquals("SELECT TOP 20 MY_SCHEMA.\"foo\".\"bar\" FROM MY_SCHEMA.\"foo\"", qr.rewriteQuery(q));
     }
 
+    public void testOffsetFetchConstruct() {
+        final int offset = 1000;
+        final int rows = 100;
+
+        final String baseQuery = "SELECT MY_SCHEMA.\"foo\".\"bar\" FROM MY_SCHEMA.\"foo\" ORDER BY id ASC";
+        final String baseQueryWithTop =
+                "SELECT TOP " + rows + " MY_SCHEMA.\"foo\".\"bar\" FROM MY_SCHEMA.\"foo\" ORDER BY id ASC";
+        final String offsetClause = " OFFSET " + (offset - 1) + " ROWS";
+        final String fetchClause = " FETCH NEXT " + rows + " ROWS ONLY";
+
+        Query query = new Query();
+        query.from(table).select(column).orderBy("id");
+        Assert.assertEquals("There shouldn't be OFFSET-FETCH clause.", baseQuery, qr.rewriteQuery(query));
+
+        query.setFirstRow(offset);
+        Assert.assertEquals("Wrong or missing OFFSET clause.", baseQuery + offsetClause, qr.rewriteQuery(query));
+
+        query.setMaxRows(rows);
+        Assert.assertEquals("Wrong or missing OFFSET and FETCH clauses.", baseQuery + offsetClause + fetchClause,
+                qr.rewriteQuery(query));
+
+        query.setFirstRow(null);
+        Assert.assertEquals("Using FETCH clause instead of TOP clause.", baseQueryWithTop, qr.rewriteQuery(query));
+    }
+
     public void testRewriteFilterItem() {
 
         MutableColumn timestampColumn = new MutableColumn("timestamp");

diff --git a/jdbc/src/test/java/org/apache/metamodel/jdbc/H2databaseTest.java b/jdbc/src/test/java/org/apache/metamodel/jdbc/H2databaseTest.java
@@ -221,7 +221,7 @@ public void testQueryRewriter() throws Exception {
         final IQueryRewriter queryRewriter = dc.getQueryRewriter();
         assertEquals("H2QueryRewriter", queryRewriter.getClass().getSimpleName());
 
-        assertTrue(queryRewriter.isFirstRowSupported());
+        assertTrue(queryRewriter.isFirstRowSupported(new Query()));
         assertTrue(queryRewriter.isMaxRowsSupported());
     }