You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@phoenix.apache.org by lomoree <gi...@git.apache.org> on 2016/12/03 01:06:05 UTC
[GitHub] phoenix pull request #224: PHOENIX-3477 Support sequence arithmetic in Calci...
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 <el...@bloomberg.net>
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #224: PHOENIX-3477 Support sequence arithmetic in Calci...
Posted by maryannxue <gi...@git.apache.org>.
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, "0000000005", "T5", "0000000005", "S5"},
{8L, "0000000006", "T6", "0000000006", "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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #224: PHOENIX-3477 Support sequence arithmetic in Calci...
Posted by lomoree <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix issue #224: PHOENIX-3477 Support sequence arithmetic in Calcite-Phoe...
Posted by lomoree <gi...@git.apache.org>.
Github user lomoree commented on the issue:
https://github.com/apache/phoenix/pull/224
After discussing options, we kept original design and merged as is. Closing PR.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #224: PHOENIX-3477 Support sequence arithmetic in Calci...
Posted by maryannxue <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #224: PHOENIX-3477 Support sequence arithmetic in Calci...
Posted by lomoree <gi...@git.apache.org>.
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, "0000000005", "T5", "0000000005", "S5"},
{8L, "0000000006", "T6", "0000000006", "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<LogicalProject> NO_SEQUENCE = new Predicate<LogicalProject>() {
@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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #224: PHOENIX-3477 Support sequence arithmetic in Calci...
Posted by maryannxue <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #224: PHOENIX-3477 Support sequence arithmetic in Calci...
Posted by maryannxue <gi...@git.apache.org>.
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, "0000000005", "T5", "0000000005", "S5"},
{8L, "0000000006", "T6", "0000000006", "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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---