[GitHub] phoenix pull request #224: PHOENIX-3477 Support sequence arithmetic in Calci...

[lomoree]

Github user lomoree closed the pull request at:

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


---
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 #224: PHOENIX-3477 Support sequence arithmetic in Calci...

[maryannxue]

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

https://github.com/apache/phoenix/pull/224#discussion_r91163250
  
--- Diff: 
phoenix-core/src/it/java/org/apache/phoenix/calcite/CalciteIT.java ---
@@ -1343,6 +1343,28 @@ public void initTable() throws Exception {
 {7L, "05", "T5", "05", "S5"},
 {8L, "06", "T6", "06", "S6"}})
 .close();
+
--- End diff --

Thank you for the info, @lomoree! Very helpful! I was wondering if there 
had been any check of that kind during conversion. Anyway, would you mind 
removing that NO_SEQUENCE predicate and trying the changes I suggested below?


---
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 #224: PHOENIX-3477 Support sequence arithmetic in Calci...

[lomoree]

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

https://github.com/apache/phoenix/pull/224#discussion_r91162501
  
--- Diff: 
phoenix-core/src/it/java/org/apache/phoenix/calcite/CalciteIT.java ---
@@ -1343,6 +1343,28 @@ public void initTable() throws Exception {
 {7L, "05", "T5", "05", "S5"},
 {8L, "06", "T6", "06", "S6"}})
 .close();
+
--- End diff --

While that was my initial thinking, upon looking deeper I discovered a 
different source for the issue

The reason a ServerProject was being created was because of visitCall(). It 
wasn't recursively checking for sequences. Therefore, when an operator like 
SqlKind.PLUS was present and the sequence was present, the sequence would be 
pushed down to a child operand and not be seen by visitCall()

PhoenixConverterRules is designed so that a ServerProject does not get 
created if a sequence is present

 
 ```
private static Predicate NO_SEQUENCE = new 
Predicate() {
@Override
public boolean apply(LogicalProject input) {
return !CalciteUtils.hasSequenceValueCall(input);
}
};
```


---
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 #224: PHOENIX-3477 Support sequence arithmetic in Calci...

[maryannxue]

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

https://github.com/apache/phoenix/pull/224#discussion_r91152331
  
--- Diff: 
phoenix-core/src/it/java/org/apache/phoenix/calcite/CalciteIT.java ---
@@ -1343,6 +1343,28 @@ public void initTable() throws Exception {
 {7L, "05", "T5", "05", "S5"},
 {8L, "06", "T6", "06", "S6"}})
 .close();
+
--- End diff --

I remember you said you ran into a problem using PhoenixServerProject, but 
it's PhoenixClientProject here again. Does it oscillate between the two? Could 
you please try {{start(false, 1f)}} instead?


---
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 #224: PHOENIX-3477 Support sequence arithmetic in Calci...

[maryannxue]

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

https://github.com/apache/phoenix/pull/224#discussion_r91155610
  
--- Diff: 
phoenix-core/src/main/java/org/apache/phoenix/calcite/rel/PhoenixClientProject.java
 ---
@@ -73,13 +70,10 @@ public QueryPlan implement(PhoenixRelImplementor 
implementor) {
 QueryPlan plan = implementor.visitInput(0, (PhoenixQueryRel) 
getInput());
 implementor.popContext();
 
-
-PhoenixSequence sequence = CalciteUtils.findSequence(this);
-final SequenceManager seqManager = sequence == null ?
-null : new SequenceManager(new 
PhoenixStatement(sequence.pc));
-implementor.setSequenceManager(seqManager);
-TupleProjector tupleProjector = project(implementor);
-if (seqManager != null) {
+
--- End diff --

This entire wrapping logic should live in PhoenixToEnumerableConverter 
instead. Wrapping it with an iterator not a query plan would be better, 
something like:
{code}

return new DelegateQueryPlan((QueryPlan) plan) {
@Override
public ResultIterator iterator() throws SQLException {
ResultIterator iterator = 
iterator(DefaultParallelScanGrouper.getInstance());
if 
(phoenixImplementor.getSequenceManager().getSequenceCount() > 0) {
iterator = new SequenceResultIterator(iterator, 
phoenixImplementor.getSequenceManager());
}
return iterator;
}
@Override
public ExplainPlan getExplainPlan() throws SQLException {
{code}


---
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 #224: PHOENIX-3477 Support sequence arithmetic in Calci...

[maryannxue]

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

https://github.com/apache/phoenix/pull/224#discussion_r91152478
  
--- Diff: 
phoenix-core/src/main/java/org/apache/phoenix/calcite/CalciteUtils.java ---
@@ -1115,6 +1099,11 @@ public Void visitCall(RexCall call) {
 || call.getKind() == SqlKind.NEXT_VALUE)) {
 sequenceValueCall = call;
 }
+if (sequenceValueCall == null){
--- End diff --

Think we are good to remove this class, right?


---
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 #224: PHOENIX-3477 Support sequence arithmetic in Calci...

[lomoree]

GitHub user lomoree opened a pull request:

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

PHOENIX-3477 Support sequence arithmetic in Calcite-Phoenix

PhoenixConverterRules is meant to route any logical project through 
PhoenixClientProject. Before, sequences were not properly being found. This 
patch simplifies the logic and adds that support.
-- Check for nested sequences
-- Use the Sequence Manager now available in StatementContext
-- Add relevant test cases to CalciteIT

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

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

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

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


commit 8eacb9e455f16c97d6f301fdb1c27494905900a0
Author: Eric Lomore 
Date:   2016-12-03T01:00:20Z

PHOENIX-3477 Support sequence arithmetic 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.
---