PHOENIX-2722 support mysql limit,offset clauses #154

ankitsinghal · 2016-03-22T08:13:42Z

No description provided.

JamesRTaylor · 2016-03-23T05:58:21Z

Would you mind reviewing this, @maryannxue as you'd be the best person to catch any corner cases missed, in particular around joins?

maryannxue · 2016-03-24T03:01:04Z

phoenix-core/src/main/java/org/apache/phoenix/compile/SubselectRewriter.java

+                }
+            }
+        }
+


Not sure if this would be correct. Suppose we have "select * from (select a from t offset 2 limit 8) offset 3", guess we should return the 5th row instead. If you consider the logic is too complicated to optimize at compiletime, you can simply quit flattening.

Thanks for pointing out. I thought a little and have updated the logic to
if (offsetRewrite != null || (limitRewrite != null & offset != null)) {
return select;
} else {
offsetRewrite = offset;
}
Let me know if any optimization is possible.

maryannxue · 2016-03-24T03:20:56Z

@ankitsinghal Thank you very much for the pull request! I am impressed by how many details you have taken into account in your commit. It took me quite a while to go through all of the changes. That said, it would be great if you could add more test cases covering most (if not all) of the code changes, e.g. the join case (left outer join w/ offset and w/wo limit), the derived table case, the join with subquery case, etc.

Check JoinCompiler.isFlat(SelectStatement), it returns true only when limit == null, think it should be the same for offset. That function is called when a join contains a subquery, e.g. "select * from (select a, b from t1 offset 3) join (select c, d from t2 limit 10)".

ankitsinghal · 2016-03-28T07:31:35Z

@maryannxue Thank you so much for the review! I have incorporated the changes as per your comments and added more test cases related to join, derived table ,group by case and join with subquery as you asked.

maryannxue · 2016-03-29T01:58:00Z

phoenix-core/src/it/java/org/apache/phoenix/end2end/DerivedTableIT.java

@@ -134,7 +134,7 @@ public void testDerivedTableWithWhere() throws Exception {
        Connection conn = DriverManager.getConnection(getUrl(), props);
        try {
            // (where)
-            String query = "SELECT t.eid, t.x + 9 FROM (SELECT entity_id eid, b_string b, a_byte + 1 x FROM aTable WHERE a_byte + 1 < 9) AS t";
+            String query = "SELECT t.eid, t.x + 9 FROM (SELECT entity_id eid, b_string b, a_byte + 1 x FROM aTable WHERE a_byte + 1 < 9 ) AS t";
            PreparedStatement statement = conn.prepareStatement(query);


Unnecessary diff

maryannxue · 2016-03-29T02:25:03Z

@ankitsinghal Thank you very much for making the suggested changes! Added a few more suggestions, most of them being minor.

ankitsinghal · 2016-03-29T09:39:20Z

Thanks @maryannxue for suggestions , I have incorporated the review comments.

maryannxue · 2016-03-31T02:28:18Z

phoenix-core/src/main/java/org/apache/phoenix/execute/LiteralResultIterationPlan.java

@@ -85,7 +85,8 @@ public void close() throws SQLException {

            @Override
            public Tuple next() throws SQLException {
-                while (!this.closed && (offset != null && count++ < offset) && tupleIterator.hasNext()) {
+                while (!this.closed && (offset != null && count < offset) && tupleIterator.hasNext()) {
+                    count++;


What I meant was this "count++" doesn't seem to work with LIMIT(below) together. LIMIT should be number of rows returned starting from the OFFSET row, right?

Ah, I was in a notion that I updated limit to limit+offset already.
Thanks for catching it, I have corrected it now.

maryannxue · 2016-03-31T02:33:55Z

@ankitsinghal Some (maybe final) comments added.

updated LiteralResultIterationPlan updated test in erivedTableIT.java

ankitsinghal · 2016-03-31T19:07:32Z

I have taken care the last review comments in my latest commit.
Let me know if it is good to go now. Thanks you so much @maryannxue for review.

ankitsinghal · 2016-04-04T18:55:43Z

@maryannxue , what do you think .. is it good to go now?

maryannxue · 2016-04-04T19:35:34Z

Hi @ankitsinghal, sorry about the delay. I don't get github notification in mailbox... It looks good to me now. Thank you for all the work! @JamesRTaylor What do you think?

JamesRTaylor · 2016-04-04T23:33:19Z

phoenix-core/src/main/java/org/apache/phoenix/compile/QueryCompiler.java

+                    subSelect = NODE_FACTORY.select(subSelect, select.getOrderBy(), select.getLimit(), null);
+                } else {
+                    subSelect = NODE_FACTORY.select(subSelect, select.getOrderBy(), null, null);
+                }


Rather than this if statement, can we do the following to simplify it?

subSelect = NODE_FACTORY.select(subSelect, select.getOrderBy(), select.getLimit(), select.getOffset())

Actually , In case of union , we need to apply offset at the final result so we can't apply limit or offset in subselect.
but, if offset is not present , limit can be applied

JamesRTaylor · 2016-04-05T00:00:59Z

phoenix-core/src/main/java/org/apache/phoenix/iterate/MergeSortTopNResultIterator.java

        }
+        if (limit >= 0 && count++ >= limit) { return null; }


Should this be just {{count}} or {{count++}} ?

Actually it should count++ only, but there is a bug in offset count .. which I have updated now.

ankitsinghal · 2016-04-05T11:51:54Z

Thanks @JamesRTaylor for review.. I have made the changes are per your review comments.

JamesRTaylor · 2016-04-05T15:00:03Z

phoenix-core/src/main/java/org/apache/phoenix/iterate/OffsetScanGrouper.java

+ * Default implementation that creates a scan group if a plan is row key ordered (which requires a merge sort),
+ * or if a scan crosses a region boundary and the table is salted or a local index.   
+ */
+public class OffsetScanGrouper implements ParallelScanGrouper {


Why is this class needed as it doesn't appear to do anything.

It is needed to avoid scans in parallel even in serialIterators. should I use a anonymous class while creating SerialIterators to do offset on server?

Scans aren't done in parallel for SerialIterator, they're done serially. Have you tried it without this? I can't figure out what purpose it's serving, but perhaps I'm missing something?

@JamesRTaylor , It seems for serialIterators also , we use DefaultParallelScanGrouper for splitting scans in groups by condition (isSalted || table.getIndexType() == IndexType.LOCAL) && ScanUtil.shouldRowsBeInRowKeyOrder(orderBy, context) and run each group in parallel.

Anyways, I have modified the plan to run offset on server by including this condition and re-use the DefaultParallelScanGrouper

JamesRTaylor · 2016-04-05T15:10:01Z

A few minor items, but otherwise it looks good. Nice work, @ankitsinghal! If you could file a JIRA for the corresponding website/doc changes and submit a patch for that when we're closer to the 4.8 release, that'd be much appreciated!

PHOENIX-2722 support mysql limit,offset clauses

5d4e42d

maryannxue reviewed Mar 24, 2016
View reviewed changes

Incorporated 1st iteration of review comments

2b18680

maryannxue reviewed Mar 29, 2016
View reviewed changes

Review iteration 2

311974a

maryannxue reviewed Mar 31, 2016
View reviewed changes

Added literatlResultIterationPlanTest

147e60d

updated LiteralResultIterationPlan updated test in erivedTableIT.java

added assert for limit

6183bbd

JamesRTaylor reviewed Apr 4, 2016
View reviewed changes

JamesRTaylor reviewed Apr 5, 2016
View reviewed changes

review comments by James

611d4a0

JamesRTaylor reviewed Apr 5, 2016
View reviewed changes

handle salted table for doing offset on server

6c93f13

ankitsinghal closed this Apr 12, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHOENIX-2722 support mysql limit,offset clauses #154

PHOENIX-2722 support mysql limit,offset clauses #154

ankitsinghal commented Mar 22, 2016

JamesRTaylor commented Mar 23, 2016

maryannxue Mar 24, 2016

ankitsinghal Mar 28, 2016

maryannxue commented Mar 24, 2016

ankitsinghal commented Mar 28, 2016

maryannxue Mar 29, 2016

maryannxue commented Mar 29, 2016

ankitsinghal commented Mar 29, 2016

maryannxue Mar 31, 2016

ankitsinghal Mar 31, 2016

maryannxue commented Mar 31, 2016

ankitsinghal commented Mar 31, 2016

ankitsinghal commented Apr 4, 2016

maryannxue commented Apr 4, 2016

JamesRTaylor Apr 4, 2016

ankitsinghal Apr 5, 2016

ankitsinghal Apr 5, 2016

JamesRTaylor Apr 5, 2016

ankitsinghal Apr 5, 2016

ankitsinghal commented Apr 5, 2016

JamesRTaylor Apr 5, 2016

ankitsinghal Apr 5, 2016

JamesRTaylor Apr 6, 2016

ankitsinghal Apr 6, 2016

JamesRTaylor commented Apr 5, 2016

PHOENIX-2722 support mysql limit,offset clauses #154

PHOENIX-2722 support mysql limit,offset clauses #154

Conversation

ankitsinghal commented Mar 22, 2016

JamesRTaylor commented Mar 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maryannxue commented Mar 24, 2016

ankitsinghal commented Mar 28, 2016

Choose a reason for hiding this comment

maryannxue commented Mar 29, 2016

ankitsinghal commented Mar 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maryannxue commented Mar 31, 2016

ankitsinghal commented Mar 31, 2016

ankitsinghal commented Apr 4, 2016

maryannxue commented Apr 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ankitsinghal commented Apr 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesRTaylor commented Apr 5, 2016