[GitHub] phoenix pull request #213: PHOENIX-2827 Support OFFSET in Calcite-Phoenix

[lomoree]

Github user lomoree closed the pull request at:

https://github.com/apache/phoenix/pull/213


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #213: PHOENIX-2827 Support OFFSET in Calcite-Phoenix

[lomoree]

Github user lomoree commented on a diff in the pull request:

https://github.com/apache/phoenix/pull/213#discussion_r81431821
  
--- Diff: 
phoenix-core/src/main/java/org/apache/phoenix/execute/ClientScanPlan.java ---
@@ -106,12 +106,18 @@ public ExplainPlan getExplainPlan() throws 
SQLException {
 }
 
 @Override
-public QueryPlan limit(Integer limit) {
-if (limit == this.limit || (limit != null && 
limit.equals(this.limit)))
+public QueryPlan limit(Integer limit, Integer offset) {
+if (limit == this.limit || (limit != null && 
limit.equals(this.limit))) {
--- End diff --

Ah, my mistake. I'll get working on a patch that also supports offset 
without limit. This should incorporate all of the changes noted above. Thanks 
for clarifying.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #213: PHOENIX-2827 Support OFFSET in Calcite-Phoenix

[JamesRTaylor]

Github user JamesRTaylor commented on a diff in the pull request:

https://github.com/apache/phoenix/pull/213#discussion_r81430788
  
--- Diff: 
phoenix-core/src/main/java/org/apache/phoenix/execute/ClientScanPlan.java ---
@@ -106,12 +106,18 @@ public ExplainPlan getExplainPlan() throws 
SQLException {
 }
 
 @Override
-public QueryPlan limit(Integer limit) {
-if (limit == this.limit || (limit != null && 
limit.equals(this.limit)))
+public QueryPlan limit(Integer limit, Integer offset) {
+if (limit == this.limit || (limit != null && 
limit.equals(this.limit))) {
--- End diff --

For right or wrong, Phoenix allows an OFFSET to be used without a LIMIT 
today.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #213: PHOENIX-2827 Support OFFSET in Calcite-Phoenix

[maryannxue]

Github user maryannxue commented on a diff in the pull request:

https://github.com/apache/phoenix/pull/213#discussion_r81430086
  
--- Diff: 
phoenix-core/src/main/java/org/apache/phoenix/execute/ClientScanPlan.java ---
@@ -106,12 +106,18 @@ public ExplainPlan getExplainPlan() throws 
SQLException {
 }
 
 @Override
-public QueryPlan limit(Integer limit) {
-if (limit == this.limit || (limit != null && 
limit.equals(this.limit)))
+public QueryPlan limit(Integer limit, Integer offset) {
+if (limit == this.limit || (limit != null && 
limit.equals(this.limit))) {
--- End diff --

Is it a SQL-92 standard that OFFSET can only be used together with LIMIT? I 
know there are some dialects that allow different OFFSET/LIMIT or OFFSET/FETCH 
grammars, so I'm just confused... I tried with Calcite parser and it does allow 
OFFSET without LIMIT.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #213: PHOENIX-2827 Support OFFSET in Calcite-Phoenix

[lomoree]

Github user lomoree commented on a diff in the pull request:

https://github.com/apache/phoenix/pull/213#discussion_r81422631
  
--- Diff: 
phoenix-core/src/main/java/org/apache/phoenix/calcite/rules/PhoenixConverterRules.java
 ---
@@ -232,8 +232,7 @@ public RelNode convert(RelNode rel) {
 private static Predicate HAS_FETCH = new 
Predicate() {
 @Override
 public boolean apply(LogicalSort input) {
-return input.offset == null 
-&& input.fetch != null;
+return input.fetch != null;
--- End diff --

With respect to my comment below, if there is a limit, then we apply the 
rule, otherwise we do not (regardless of offset).
 I may have the wrong idea here, but based on my understanding of offset, 
all of the test cases pass as expected. Are the test cases I have laid out in 
CalciteIT what you would expect?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #213: PHOENIX-2827 Support OFFSET in Calcite-Phoenix

[lomoree]

Github user lomoree commented on a diff in the pull request:

https://github.com/apache/phoenix/pull/213#discussion_r81421343
  
--- Diff: 
phoenix-core/src/main/java/org/apache/phoenix/execute/ClientScanPlan.java ---
@@ -106,12 +106,18 @@ public ExplainPlan getExplainPlan() throws 
SQLException {
 }
 
 @Override
-public QueryPlan limit(Integer limit) {
-if (limit == this.limit || (limit != null && 
limit.equals(this.limit)))
+public QueryPlan limit(Integer limit, Integer offset) {
+if (limit == this.limit || (limit != null && 
limit.equals(this.limit))) {
--- End diff --

This does definitely have ripple effects throughout, but I had done this on 
the basis that SQL only allows an offset if their is an imposed limit (since an 
offset doesn't make any logical sense without a limit).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #213: PHOENIX-2827 Support OFFSET in Calcite-Phoenix

[maryannxue]

Github user maryannxue commented on a diff in the pull request:

https://github.com/apache/phoenix/pull/213#discussion_r81418958
  
--- Diff: 
phoenix-core/src/main/java/org/apache/phoenix/calcite/rel/PhoenixLimit.java ---
@@ -76,20 +76,27 @@ public RelOptCost computeSelfCost(RelOptPlanner 
planner, RelMetadataQuery mq) {
 
 @Override 
 public double estimateRowCount(RelMetadataQuery mq) {
-double rows = super.estimateRowCount(mq);
+double rows = super.estimateRowCount(mq);
+if(offset != null) {
+return Math.max(0, Math.min(RexLiteral.intValue(fetch), rows - 
RexLiteral.intValue(offset)));
+}
 return Math.min(RexLiteral.intValue(fetch), rows);
 }
 
 @Override
 public QueryPlan implement(PhoenixRelImplementor implementor) {
 QueryPlan plan = implementor.visitInput(0, (PhoenixQueryRel) 
getInput());
 int fetchValue = RexLiteral.intValue(fetch);
+int offsetValue = 0;
+if (offset != null){
+offsetValue = RexLiteral.intValue(offset);
+}
 if (plan.getLimit() == null) {
--- End diff --

should be "if (plan.getLimit() == null && plan.getOffset() == null)"


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #213: PHOENIX-2827 Support OFFSET in Calcite-Phoenix

[maryannxue]

Github user maryannxue commented on a diff in the pull request:

https://github.com/apache/phoenix/pull/213#discussion_r81419366
  
--- Diff: 
phoenix-core/src/main/java/org/apache/phoenix/calcite/rel/PhoenixLimit.java ---
@@ -76,20 +76,27 @@ public RelOptCost computeSelfCost(RelOptPlanner 
planner, RelMetadataQuery mq) {
 
 @Override 
 public double estimateRowCount(RelMetadataQuery mq) {
-double rows = super.estimateRowCount(mq);
+double rows = super.estimateRowCount(mq);
+if(offset != null) {
+return Math.max(0, Math.min(RexLiteral.intValue(fetch), rows - 
RexLiteral.intValue(offset)));
+}
 return Math.min(RexLiteral.intValue(fetch), rows);
 }
 
 @Override
 public QueryPlan implement(PhoenixRelImplementor implementor) {
 QueryPlan plan = implementor.visitInput(0, (PhoenixQueryRel) 
getInput());
 int fetchValue = RexLiteral.intValue(fetch);
--- End diff --

I believe now that we've enabled offset, "this.fetch" could be also be 
null, which means it should be handled the same way as this.offset now. Could 
you please also add test cases to cover situations like "offset != null" but 
"limit == null"?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #213: PHOENIX-2827 Support OFFSET in Calcite-Phoenix

[maryannxue]

Github user maryannxue commented on a diff in the pull request:

https://github.com/apache/phoenix/pull/213#discussion_r81420342
  
--- Diff: 
phoenix-core/src/main/java/org/apache/phoenix/execute/ClientScanPlan.java ---
@@ -106,12 +106,18 @@ public ExplainPlan getExplainPlan() throws 
SQLException {
 }
 
 @Override
-public QueryPlan limit(Integer limit) {
-if (limit == this.limit || (limit != null && 
limit.equals(this.limit)))
+public QueryPlan limit(Integer limit, Integer offset) {
+if (limit == this.limit || (limit != null && 
limit.equals(this.limit))) {
--- End diff --

I can see there's a big confusion here. Basically, offset and limit should 
be treated the same way. So here, it should be "if both the old limit and the 
old offset are same as as the new ones specified, we return this; otherwise we 
create a new object."


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #213: PHOENIX-2827 Support OFFSET in Calcite-Phoenix

[maryannxue]

Github user maryannxue commented on a diff in the pull request:

https://github.com/apache/phoenix/pull/213#discussion_r81419580
  
--- Diff: 
phoenix-core/src/main/java/org/apache/phoenix/calcite/rel/PhoenixLimit.java ---
@@ -76,20 +76,27 @@ public RelOptCost computeSelfCost(RelOptPlanner 
planner, RelMetadataQuery mq) {
 
 @Override 
 public double estimateRowCount(RelMetadataQuery mq) {
-double rows = super.estimateRowCount(mq);
+double rows = super.estimateRowCount(mq);
+if(offset != null) {
+return Math.max(0, Math.min(RexLiteral.intValue(fetch), rows - 
RexLiteral.intValue(offset)));
+}
 return Math.min(RexLiteral.intValue(fetch), rows);
 }
 
 @Override
 public QueryPlan implement(PhoenixRelImplementor implementor) {
 QueryPlan plan = implementor.visitInput(0, (PhoenixQueryRel) 
getInput());
 int fetchValue = RexLiteral.intValue(fetch);
+int offsetValue = 0;
+if (offset != null){
+offsetValue = RexLiteral.intValue(offset);
+}
 if (plan.getLimit() == null) {
-return plan.limit(fetchValue);
+return plan.limit(fetchValue, offsetValue);
 }
 
 return new ClientScanPlan(plan.getContext(), plan.getStatement(), 
 implementor.getTableMapping().getTableRef(), 
RowProjector.EMPTY_PROJECTOR, 
-fetchValue, null, null, OrderBy.EMPTY_ORDER_BY, plan);
+fetchValue, offsetValue, null, OrderBy.EMPTY_ORDER_BY, 
plan);
--- End diff --

Since either of them could be null, Boolean objects should be used instead 
of primitive boolean type.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] phoenix pull request #213: PHOENIX-2827 Support OFFSET in Calcite-Phoenix

[lomoree]

GitHub user lomoree opened a pull request:

https://github.com/apache/phoenix/pull/213

PHOENIX-2827 Support OFFSET in Calcite-Phoenix



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/bloomberg/phoenix offset

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/phoenix/pull/213.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #213


commit 488259067d5a610efcc946cb92d48070aa4fbdb0
Author: Eric 
Date:   2016-09-28T21:40:32Z

PHOENIX-2827 Support OFFSET in Calcite-Phoenix




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---