[jira] [Commented] (PHOENIX-2827) Support OFFSET in Calcite-Phoenix

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/PHOENIX-2827?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15545942#comment-15545942
 ] 

ASF GitHub Bot commented on PHOENIX-2827:
-

Github user lomoree closed the pull request at:

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


> Support OFFSET in Calcite-Phoenix
> -
>
> Key: PHOENIX-2827
> URL: https://issues.apache.org/jira/browse/PHOENIX-2827
> Project: Phoenix
>  Issue Type: Task
>Reporter: Maryann Xue
>Assignee: Eric Lomore
>  Labels: calcite
>




--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (PHOENIX-2827) Support OFFSET in Calcite-Phoenix

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/PHOENIX-2827?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15537344#comment-15537344
 ] 

ASF GitHub Bot commented on PHOENIX-2827:
-

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.


> Support OFFSET in Calcite-Phoenix
> -
>
> Key: PHOENIX-2827
> URL: https://issues.apache.org/jira/browse/PHOENIX-2827
> Project: Phoenix
>  Issue Type: Task
>Reporter: Maryann Xue
>Assignee: Eric Lomore
>  Labels: calcite
>




--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (PHOENIX-2827) Support OFFSET in Calcite-Phoenix

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/PHOENIX-2827?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15537308#comment-15537308
 ] 

ASF GitHub Bot commented on PHOENIX-2827:
-

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.


> Support OFFSET in Calcite-Phoenix
> -
>
> Key: PHOENIX-2827
> URL: https://issues.apache.org/jira/browse/PHOENIX-2827
> Project: Phoenix
>  Issue Type: Task
>Reporter: Maryann Xue
>Assignee: Eric Lomore
>  Labels: calcite
>




--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (PHOENIX-2827) Support OFFSET in Calcite-Phoenix

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/PHOENIX-2827?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15537289#comment-15537289
 ] 

ASF GitHub Bot commented on PHOENIX-2827:
-

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.


> Support OFFSET in Calcite-Phoenix
> -
>
> Key: PHOENIX-2827
> URL: https://issues.apache.org/jira/browse/PHOENIX-2827
> Project: Phoenix
>  Issue Type: Task
>Reporter: Maryann Xue
>Assignee: Eric Lomore
>  Labels: calcite
>




--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (PHOENIX-2827) Support OFFSET in Calcite-Phoenix

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/PHOENIX-2827?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15537155#comment-15537155
 ] 

ASF GitHub Bot commented on PHOENIX-2827:
-

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?


> Support OFFSET in Calcite-Phoenix
> -
>
> Key: PHOENIX-2827
> URL: https://issues.apache.org/jira/browse/PHOENIX-2827
> Project: Phoenix
>  Issue Type: Task
>Reporter: Maryann Xue
>Assignee: Eric Lomore
>  Labels: calcite
>




--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (PHOENIX-2827) Support OFFSET in Calcite-Phoenix

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/PHOENIX-2827?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15537126#comment-15537126
 ] 

ASF GitHub Bot commented on PHOENIX-2827:
-

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).


> Support OFFSET in Calcite-Phoenix
> -
>
> Key: PHOENIX-2827
> URL: https://issues.apache.org/jira/browse/PHOENIX-2827
> Project: Phoenix
>  Issue Type: Task
>Reporter: Maryann Xue
>Assignee: Eric Lomore
>  Labels: calcite
>




--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (PHOENIX-2827) Support OFFSET in Calcite-Phoenix

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/PHOENIX-2827?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15537110#comment-15537110
 ] 

ASF GitHub Bot commented on PHOENIX-2827:
-

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"?


> Support OFFSET in Calcite-Phoenix
> -
>
> Key: PHOENIX-2827
> URL: https://issues.apache.org/jira/browse/PHOENIX-2827
> Project: Phoenix
>  Issue Type: Task
>Reporter: Maryann Xue
>Assignee: Eric Lomore
>  Labels: calcite
>




--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (PHOENIX-2827) Support OFFSET in Calcite-Phoenix

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/PHOENIX-2827?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15537111#comment-15537111
 ] 

ASF GitHub Bot commented on PHOENIX-2827:
-

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

https://github.com/apache/phoenix/pull/213#discussion_r81418723
  
--- 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);
--- End diff --

I'd write:
int offset = this.offset == null ? 0 : RexLiteral.intValue(this.offset);
return Math.min(RexLiteral.intValue(fetch), rows - offset);


> Support OFFSET in Calcite-Phoenix
> -
>
> Key: PHOENIX-2827
> URL: https://issues.apache.org/jira/browse/PHOENIX-2827
> Project: Phoenix
>  Issue Type: Task
>Reporter: Maryann Xue
>Assignee: Eric Lomore
>  Labels: calcite
>




--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (PHOENIX-2827) Support OFFSET in Calcite-Phoenix

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/PHOENIX-2827?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15537113#comment-15537113
 ] 

ASF GitHub Bot commented on PHOENIX-2827:
-

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

https://github.com/apache/phoenix/pull/213#discussion_r81419684
  
--- 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 --

should be "input.fetch != null || input.offset != null"


> Support OFFSET in Calcite-Phoenix
> -
>
> Key: PHOENIX-2827
> URL: https://issues.apache.org/jira/browse/PHOENIX-2827
> Project: Phoenix
>  Issue Type: Task
>Reporter: Maryann Xue
>Assignee: Eric Lomore
>  Labels: calcite
>




--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (PHOENIX-2827) Support OFFSET in Calcite-Phoenix

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/PHOENIX-2827?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15537112#comment-15537112
 ] 

ASF GitHub Bot commented on PHOENIX-2827:
-

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."


> Support OFFSET in Calcite-Phoenix
> -
>
> Key: PHOENIX-2827
> URL: https://issues.apache.org/jira/browse/PHOENIX-2827
> Project: Phoenix
>  Issue Type: Task
>Reporter: Maryann Xue
>Assignee: Eric Lomore
>  Labels: calcite
>




--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (PHOENIX-2827) Support OFFSET in Calcite-Phoenix

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/PHOENIX-2827?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15537114#comment-15537114
 ] 

ASF GitHub Bot commented on PHOENIX-2827:
-

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)"


> Support OFFSET in Calcite-Phoenix
> -
>
> Key: PHOENIX-2827
> URL: https://issues.apache.org/jira/browse/PHOENIX-2827
> Project: Phoenix
>  Issue Type: Task
>Reporter: Maryann Xue
>Assignee: Eric Lomore
>  Labels: calcite
>




--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (PHOENIX-2827) Support OFFSET in Calcite-Phoenix

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/PHOENIX-2827?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15536719#comment-15536719
 ] 

ASF GitHub Bot commented on PHOENIX-2827:
-

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




> Support OFFSET in Calcite-Phoenix
> -
>
> Key: PHOENIX-2827
> URL: https://issues.apache.org/jira/browse/PHOENIX-2827
> Project: Phoenix
>  Issue Type: Task
>Reporter: Maryann Xue
>Assignee: Eric Lomore
>  Labels: calcite
>




--
This message was sent by Atlassian JIRA
(v6.3.4#6332)