Repository: kayak/pypika
Pull request #754 (2023-09-25 11:18:17)

feat: Support limiting row count and offsets in Oracle queries
Add support for [Oracle's syntax](https://www.oracletutorial.com/oracle-basics/oracle-fetch/) for limiting the row count returned in a query by overriding the `_limit_sql` method in `OracleQueryBuilder`, as well the offset syntax (`_offset_sql`). 

As these are identical to the `MSSQL` syntax, I created a new subclass of `QueryBuilder` called `FetchNextAndOffsetRowsQueryBuilder` (admittedly not the most clever name) which overrides the two methods, and now have `OracleQueryBuilder` and `MSSQLQueryBuilder` subclass from them. 

I also overrode the `_apply_pagination` method in the `OracleQueryBuilder` to ensure that offset and fetch next are arranged in the correct order for Oracle queries. That override is similar to `MSSQLQueryBuilder`'s implementation, but as Oracle doesn't have the same constraint of a `FETCH NEXT` requiring an `OFFSET _ ROWS`, it's not identical, I opted to keep them separated.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/62871621/badge)](https://coveralls.io/builds/62871621)

coverage: 98.432% (+0.002%) from 98.43% when pulling **da829417694228225cfa431eac9612fc44d92fc2 on danielenricocahall:feature/support-fetch-first-oracle** into **12a8f336e78382817fcddd3c0812da4fc8977bfc on kayak:master**.

danielenricocahall: > Looks good. Thanks for working on this

Absolutely! My pleasure. One observation is that `MSSQLQueryBuilder` has a similar override:

```python
    @builder
    def fetch_next(self, limit: int) -> "MSSQLQueryBuilder":
        # Overridden to provide a more domain-specific API for T-SQL users
        self._limit = limit

...

    def _limit_sql(self) -> str:
        return " FETCH NEXT {limit} ROWS ONLY".format(limit=self._limit)

```

Would we want to use that with `OracleQueryBuilder` for consistency? As in, should I add a `fetch_next` builder method instead of overriding the `limit` method?
AzisK: To be honest, I am not very well familiar with the code and have myself not used this library so far

Having said that, I see a difference of "FETCH FIRST" and "FETCH NEXT" between these. How will that be handled? 

I believe it is not very clear theoretically if `OracleQueryBuilder` inherits `MSSQLQueryBuilder`. If the use the same syntax, we could have an abstract class for this but I am not sure how to call it. Probably best to call it by some standard. Or it could be a mixin, for example, `FetchLimitMixin`
danielenricocahall: > To be honest, I am not very well familiar with the code and have myself not used this library so far
> 
> Having said that, I see a difference of "FETCH FIRST" and "FETCH NEXT" between these. How will that be handled?
> 
> I believe it is not very clear theoretically if `OracleQueryBuilder` inherits `MSSQLQueryBuilder`. If the use the same syntax, we could have an abstract class for this but I am not sure how to call it. Probably best to call it by some standard. Or it could be a mixin, for example, `FetchLimitMixin`

Good question - `FETCH FIRST` and `FETCH NEXT` are functionally equivalent, at least in Oracle it is (_"For the semantic clarity purpose, you can use the keyword ROW instead of ROWS, FIRST instead of  NEXT."_), so that would be a simple change on my end to the query string.

And yes, a mixin is also exactly what I was thinking (composability > inheritance for this IMO unless MSSQL and Oracle have more in common than we know). I considered adding it, but I wasn't sure if that was overkill or violated any design guidelines within the library. I am open to adding it though - something like:

```python
class FetchNextMixin:
    @builder
    def fetch_next(self, limit: int) -> "MSSQLQueryBuilder":
        self._limit = limit
       
    def _limit_sql(self) -> str:
        return " FETCH NEXT {limit} ROWS ONLY".format(limit=self._limit)
```

I think my follow-up question would be if we went that route, how to approach the `limit` method in these two classes, since they are defined in the base `Query` class. We could raise a `NotImplemented` exception but I feel like that's not super developer friendly and violates Liskov Substitution Principle. We could alternatively have the `limit` method call `fetch_next`, but at that point, I'm not sure if the change is worth the additional complexity.

danielenricocahall: Hey @AzisK , just wanted to follow-up on this. I think we have a few options for a path forward:

- Using a mixin, although there are the caveats as detailed in my previous comments
- Just add ` `fetch_next` to `Oracle` for consistency and not utilize a Mixin. The downside is developers can still call `limit` and set the field as well which could be confusing - unless we raise an exception, which circles back to the previous idea and limitations.
- Unify the API by changing the `fetch_next` method in `MSSQL` to `limit` (which can be done in this PR or a separate, subsequent one), similar to what was done in this PR, although that is a breaking change and it does remove the domain-specific API for these special cases.

I think I'm in favor of the third design.

AzisK: I am in favor of 3rd design as well. My arguments that this is beneficial are following. First of all, in this way users can easily switch between data sources and the API will work correctly and accordingly to the source of data or dialect in this sense. Secondly, this library simplifies building SQL queries with the API, so let's keep it simple. Supporting another API keyword for the same functionality but for a different platform makes it more complicated. Thirdly, word "limit" expresses clearly what is being done.
danielenricocahall: Hello @AzisK ! I hope all is going well. I made the changes and wound up making a few more around supporting the offset syntax, as detailed in the updated PR description. Please take a look when you get the chance. :smile: 
danielenricocahall: Hi @AzisK ! Just wanted to follow-up on this.
AzisK: Hi @danielenricocahall I will look into it shortly
danielenricocahall: Hi @AzisK ! I hope all is going well! Any chance we could merge this soon?
danielenricocahall: Hi @AzisK ! I hope all is going well! Any chance we could merge this soon?
AzisK: Hi @danielenricocahall. Happy New Year! 😅

I will ask one more person to take a look into it and he is much more active than I am and then things will hopefully go smooth. Sorry for taking so long

@wd60622 could you take a look?
danielenricocahall: @wd60622 addressed your comments!
danielenricocahall: @wd60622 @AzisK any chance I can get a re-review with the formatting + deprecation warning added? Plus having CI re-run 😅 
wd60622: Seems like the tests that ran were good. Is there an issue with the coveralls version @AzisK ?
AzisK: I could see this error before as a hiccup of Github Actions. For some reason it does not succeed after a re-run
AzisK: Yeah, I believe we can bump the coveralls library and hopefully that will fix it
danielenricocahall: @wd60622 @AzisK wanted to follow-up on this one to see if we can re-run now 
danielenricocahall: @AzisK @wd60622 just following up - was the issue in CI related to coveralls resolved?
danielenricocahall: 
@AzisK @wd60622 just following-up on this inquiry


AzisK: For some reason, I don't see an option to rerun...
danielenricocahall: Hi @AzisK I just merged master into my branch and it's pending re-running CI. Could you approve?
danielenricocahall: @AzisK @wd60622 woohoo it's passing! Can it be merged?
wd60622: > @AzisK @wd60622 woohoo it's passing! Can it be merged?

Looks good. Need @AzisK to do the merge 😅

Patch:
diff --git a/pypika/dialects.py b/pypika/dialects.py
index 8894e56..7653577 100644
--- a/pypika/dialects.py
+++ b/pypika/dialects.py
@@ -1,4 +1,5 @@
 import itertools
+import warnings
 from copy import copy
 from typing import Any, Optional, Union, Tuple as TypedTuple
 
@@ -347,6 +348,19 @@ class VerticaCopyQueryBuilder:
         return self.get_sql()
 
 
+class FetchNextAndOffsetRowsQueryBuilder(QueryBuilder):
+    def _limit_sql(self) -> str:
+        return " FETCH NEXT {limit} ROWS ONLY".format(limit=self._limit)
+
+    def _offset_sql(self) -> str:
+        return " OFFSET {offset} ROWS".format(offset=self._offset or 0)
+
+    @builder
+    def fetch_next(self, limit: int):
+        warnings.warn("`fetch_next` is deprecated - please use the `limit` method", DeprecationWarning)
+        self._limit = limit
+
+
 class OracleQuery(Query):
     """
     Defines a query class for use with Oracle.
@@ -357,7 +371,7 @@ class OracleQuery(Query):
         return OracleQueryBuilder(**kwargs)
 
 
-class OracleQueryBuilder(QueryBuilder):
+class OracleQueryBuilder(FetchNextAndOffsetRowsQueryBuilder):
     QUOTE_CHAR = None
     QUERY_CLS = OracleQuery
 
@@ -370,6 +384,16 @@ class OracleQueryBuilder(QueryBuilder):
         kwargs['groupby_alias'] = False
         return super().get_sql(*args, **kwargs)
 
+    def _apply_pagination(self, querystring: str) -> str:
+        # Note: Overridden as Oracle specifies offset before the fetch next limit
+        if self._offset:
+            querystring += self._offset_sql()
+
+        if self._limit is not None:
+            querystring += self._limit_sql()
+
+        return querystring
+
 
 class PostgreSQLQuery(Query):
     """
@@ -670,7 +694,7 @@ class MSSQLQuery(Query):
         return MSSQLQueryBuilder(**kwargs)
 
 
-class MSSQLQueryBuilder(QueryBuilder):
+class MSSQLQueryBuilder(FetchNextAndOffsetRowsQueryBuilder):
     QUERY_CLS = MSSQLQuery
 
     def __init__(self, **kwargs: Any) -> None:
@@ -695,17 +719,6 @@ class MSSQLQueryBuilder(QueryBuilder):
         self._top_percent: bool = percent
         self._top_with_ties: bool = with_ties
 
-    @builder
-    def fetch_next(self, limit: int) -> "MSSQLQueryBuilder":
-        # Overridden to provide a more domain-specific API for T-SQL users
-        self._limit = limit
-
-    def _offset_sql(self) -> str:
-        return " OFFSET {offset} ROWS".format(offset=self._offset or 0)
-
-    def _limit_sql(self) -> str:
-        return " FETCH NEXT {limit} ROWS ONLY".format(limit=self._limit)
-
     def _apply_pagination(self, querystring: str) -> str:
         # Note: Overridden as MSSQL specifies offset before the fetch next limit
         if self._limit is not None or self._offset:


Test patch:
diff --git a/pypika/tests/dialects/test_mssql.py b/pypika/tests/dialects/test_mssql.py
index f8940b0..7cf53b2 100644
--- a/pypika/tests/dialects/test_mssql.py
+++ b/pypika/tests/dialects/test_mssql.py
@@ -53,18 +53,13 @@ class SelectTests(unittest.TestCase):
 
         self.assertEqual('SELECT "def" FROM "abc" ORDER BY "def" OFFSET 0 ROWS FETCH NEXT 10 ROWS ONLY', str(q))
 
-    def test_fetch_next(self):
-        q = MSSQLQuery.from_("abc").select("def").orderby("def").fetch_next(10)
-
-        self.assertEqual('SELECT "def" FROM "abc" ORDER BY "def" OFFSET 0 ROWS FETCH NEXT 10 ROWS ONLY', str(q))
-
     def test_offset(self):
         q = MSSQLQuery.from_("abc").select("def").orderby("def").offset(10)
 
         self.assertEqual('SELECT "def" FROM "abc" ORDER BY "def" OFFSET 10 ROWS', str(q))
 
-    def test_fetch_next_with_offset(self):
-        q = MSSQLQuery.from_("abc").select("def").orderby("def").fetch_next(10).offset(10)
+    def test_limit_with_offset(self):
+        q = MSSQLQuery.from_("abc").select("def").orderby("def").limit(10).offset(10)
 
         self.assertEqual('SELECT "def" FROM "abc" ORDER BY "def" OFFSET 10 ROWS FETCH NEXT 10 ROWS ONLY', str(q))
 
diff --git a/pypika/tests/dialects/test_oracle.py b/pypika/tests/dialects/test_oracle.py
index c3a757a..21b54f1 100644
--- a/pypika/tests/dialects/test_oracle.py
+++ b/pypika/tests/dialects/test_oracle.py
@@ -19,3 +19,33 @@ class SelectTests(unittest.TestCase):
         q = OracleQuery.from_(subquery).select(col, Count('*')).groupby(col)
 
         self.assertEqual('SELECT sq0.abc a,COUNT(\'*\') FROM (SELECT abc FROM table1) sq0 GROUP BY sq0.abc', str(q))
+
+    def test_limit_query(self):
+        t = Table('table1')
+        limit = 5
+        q = OracleQuery.from_(t).select(t.test).limit(limit)
+
+        self.assertEqual(f'SELECT test FROM table1 FETCH NEXT {limit} ROWS ONLY', str(q))
+
+    def test_offset_query(self):
+        t = Table('table1')
+        offset = 5
+        q = OracleQuery.from_(t).select(t.test).offset(offset)
+
+        self.assertEqual(f'SELECT test FROM table1 OFFSET {offset} ROWS', str(q))
+
+    def test_limit_offset_query(self):
+        t = Table('table1')
+        limit = 5
+        offset = 5
+        q = OracleQuery.from_(t).select(t.test).limit(limit).offset(offset)
+
+        self.assertEqual(f'SELECT test FROM table1 OFFSET {offset} ROWS FETCH NEXT {limit} ROWS ONLY', str(q))
+
+    def test_fetch_next_method_deprecated(self):
+        with self.assertWarns(DeprecationWarning):
+            t = Table('table1')
+            limit = 5
+            q = OracleQuery.from_(t).select(t.test).fetch_next(limit)
+
+            self.assertEqual(f'SELECT test FROM table1 FETCH NEXT {limit} ROWS ONLY', str(q))