Details

    • Type: Sub-task
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.11.0
    • Labels:
      None

      Description

      Implement required changes to support

      • DECLARE and OPEN a cursor
      • query FETCH NEXT | n ROWS from the cursor
      • CLOSE the cursor

      Based on the feedback in PR #192, implement the changes using ResultSet.

        Issue Links

          Activity

          Hide
          ankit@apache.org Ankit Singhal added a comment -

          I have already reviewed it offline, James Taylor, do you wanna take a look?

          Show
          ankit@apache.org Ankit Singhal added a comment - I have already reviewed it offline, James Taylor , do you wanna take a look?
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user bijugs opened a pull request:

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

          PHOENIX-3572 Support FETCH NEXT|n ROWS query on cursor

          Refer PR #192 for earlier discussion on cursor implementation. As mentioned in the title, this PR implements a sub set of ``cursor`` feature taking into consideration the feedback received in the earlier discussion.

          *Note* Thanks to: Gabriel Jimenez <gjimenez16@bloomberg.com>, Ankit Singhal <ankitsinghal59@gmail.com> who had contributed code for this PR.

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

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

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

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


          commit 5d0be76eb442296edd814acd565f2ad1c1a3bdcf
          Author: Biju Nair <gs.biju@gmail.com>
          Date: 2016-08-16T17:59:52Z

          PHOENIX-3572 Support FETCH NEXT|n ROWS query on cursor
          Authors: Gabriel Jimenez <gjimenez16@bloomberg.com>, Ankit Singhal <ankitsinghal59@gmail.com>


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user bijugs opened a pull request: https://github.com/apache/phoenix/pull/229 PHOENIX-3572 Support FETCH NEXT|n ROWS query on cursor Refer PR #192 for earlier discussion on cursor implementation. As mentioned in the title, this PR implements a sub set of ``cursor`` feature taking into consideration the feedback received in the earlier discussion. * Note * Thanks to: Gabriel Jimenez <gjimenez16@bloomberg.com>, Ankit Singhal <ankitsinghal59@gmail.com> who had contributed code for this PR. You can merge this pull request into a Git repository by running: $ git pull https://github.com/bloomberg/phoenix phoenix_cursor Alternatively you can review and apply these changes as the patch at: https://github.com/apache/phoenix/pull/229.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 #229 commit 5d0be76eb442296edd814acd565f2ad1c1a3bdcf Author: Biju Nair <gs.biju@gmail.com> Date: 2016-08-16T17:59:52Z PHOENIX-3572 Support FETCH NEXT|n ROWS query on cursor Authors: Gabriel Jimenez <gjimenez16@bloomberg.com>, Ankit Singhal <ankitsinghal59@gmail.com>
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/phoenix/pull/229#discussion_r100738646

          — Diff: phoenix-core/src/main/java/org/apache/phoenix/execute/CursorFetchPlan.java —
          @@ -0,0 +1,87 @@
          +package org.apache.phoenix.execute;
          +
          +import java.sql.ParameterMetaData;
          +import java.sql.SQLException;
          +import java.util.List;
          +import java.util.Set;
          +
          +import org.apache.hadoop.hbase.client.Scan;
          +import org.apache.phoenix.compile.ExplainPlan;
          +import org.apache.phoenix.compile.GroupByCompiler.GroupBy;
          +import org.apache.phoenix.compile.OrderByCompiler.OrderBy;
          +import org.apache.phoenix.compile.QueryPlan;
          +import org.apache.phoenix.compile.RowProjector;
          +import org.apache.phoenix.compile.StatementContext;
          +import org.apache.phoenix.iterate.CursorResultIterator;
          +import org.apache.phoenix.iterate.ParallelScanGrouper;
          +import org.apache.phoenix.iterate.ResultIterator;
          +import org.apache.phoenix.jdbc.PhoenixStatement.Operation;
          +import org.apache.phoenix.parse.FilterableStatement;
          +import org.apache.phoenix.query.KeyRange;
          +import org.apache.phoenix.schema.TableRef;
          +
          +public class CursorFetchPlan extends DelegateQueryPlan {
          +
          + //QueryPlan cursorQueryPlan;
          + private CursorResultIterator resultIterator;
          + private int fetchSize;
          +
          + public CursorFetchPlan(QueryPlan cursorQueryPlan)

          { + super(cursorQueryPlan); + }

          +
          +
          + @Override
          + public ResultIterator iterator() throws SQLException {
          + // TODO Auto-generated method stub
          + StatementContext context = delegate.getContext();
          + if (resultIterator != null)

          { + return resultIterator; + } else { + context.getOverallQueryMetrics().startQuery(); + resultIterator = (CursorResultIterator) delegate.iterator(); + return resultIterator; + }
          + }
          +
          + @Override
          + public ResultIterator iterator(ParallelScanGrouper scanGrouper) throws SQLException {
          + // TODO Auto-generated method stub
          + StatementContext context = delegate.getContext();
          + if (resultIterator != null) { + return resultIterator; + }

          else

          { + context.getOverallQueryMetrics().startQuery(); + resultIterator = (CursorResultIterator) delegate.iterator(scanGrouper); + return resultIterator; + }

          + }
          +
          + @Override
          + public ResultIterator iterator(ParallelScanGrouper scanGrouper, Scan scan) throws SQLException {
          + // TODO Auto-generated method stub
          — End diff –

          can you merge these iterators as all are doing the same and base class will be calling iterator(ParallelScanGrouper scanGrouper, Scan scan) internally from other overloaded methods with special parameter value.

          Show
          githubbot ASF GitHub Bot added a comment - Github user ankitsinghal commented on a diff in the pull request: https://github.com/apache/phoenix/pull/229#discussion_r100738646 — Diff: phoenix-core/src/main/java/org/apache/phoenix/execute/CursorFetchPlan.java — @@ -0,0 +1,87 @@ +package org.apache.phoenix.execute; + +import java.sql.ParameterMetaData; +import java.sql.SQLException; +import java.util.List; +import java.util.Set; + +import org.apache.hadoop.hbase.client.Scan; +import org.apache.phoenix.compile.ExplainPlan; +import org.apache.phoenix.compile.GroupByCompiler.GroupBy; +import org.apache.phoenix.compile.OrderByCompiler.OrderBy; +import org.apache.phoenix.compile.QueryPlan; +import org.apache.phoenix.compile.RowProjector; +import org.apache.phoenix.compile.StatementContext; +import org.apache.phoenix.iterate.CursorResultIterator; +import org.apache.phoenix.iterate.ParallelScanGrouper; +import org.apache.phoenix.iterate.ResultIterator; +import org.apache.phoenix.jdbc.PhoenixStatement.Operation; +import org.apache.phoenix.parse.FilterableStatement; +import org.apache.phoenix.query.KeyRange; +import org.apache.phoenix.schema.TableRef; + +public class CursorFetchPlan extends DelegateQueryPlan { + + //QueryPlan cursorQueryPlan; + private CursorResultIterator resultIterator; + private int fetchSize; + + public CursorFetchPlan(QueryPlan cursorQueryPlan) { + super(cursorQueryPlan); + } + + + @Override + public ResultIterator iterator() throws SQLException { + // TODO Auto-generated method stub + StatementContext context = delegate.getContext(); + if (resultIterator != null) { + return resultIterator; + } else { + context.getOverallQueryMetrics().startQuery(); + resultIterator = (CursorResultIterator) delegate.iterator(); + return resultIterator; + } + } + + @Override + public ResultIterator iterator(ParallelScanGrouper scanGrouper) throws SQLException { + // TODO Auto-generated method stub + StatementContext context = delegate.getContext(); + if (resultIterator != null) { + return resultIterator; + } else { + context.getOverallQueryMetrics().startQuery(); + resultIterator = (CursorResultIterator) delegate.iterator(scanGrouper); + return resultIterator; + } + } + + @Override + public ResultIterator iterator(ParallelScanGrouper scanGrouper, Scan scan) throws SQLException { + // TODO Auto-generated method stub — End diff – can you merge these iterators as all are doing the same and base class will be calling iterator(ParallelScanGrouper scanGrouper, Scan scan) internally from other overloaded methods with special parameter value.
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/phoenix/pull/229#discussion_r100738871

          — Diff: phoenix-core/src/main/java/org/apache/phoenix/util/CursorUtil.java —
          @@ -0,0 +1,203 @@
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +package org.apache.phoenix.util;
          +
          +import java.sql.Connection;
          +import java.sql.SQLException;
          +import java.util.HashMap;
          +import java.util.Map;
          +
          +import org.apache.hadoop.hbase.client.Scan;
          +import org.apache.hadoop.hbase.io.ImmutableBytesWritable;
          +import org.apache.phoenix.compile.QueryPlan;
          +import org.apache.phoenix.compile.OrderByCompiler.OrderBy;
          +import org.apache.phoenix.execute.CursorFetchPlan;
          +import org.apache.phoenix.iterate.CursorResultIterator;
          +import org.apache.phoenix.parse.CloseStatement;
          +import org.apache.phoenix.parse.DeclareCursorStatement;
          +import org.apache.phoenix.parse.OpenStatement;
          +import org.apache.phoenix.schema.tuple.Tuple;
          +
          +public final class CursorUtil {
          +
          + private static class CursorWrapper {
          + private final String cursorName;
          + private final String selectSQL;
          + private boolean isOpen = false;
          + QueryPlan queryPlan;
          + ImmutableBytesWritable row;
          + ImmutableBytesWritable previousRow;
          + private Scan scan;
          + private boolean moreValues=true;
          + private boolean isReversed;
          + private boolean islastCallNext;
          + private CursorFetchPlan fetchPlan;
          + private int offset = -1;
          +
          + private CursorWrapper(String cursorName, String selectSQL, QueryPlan queryPlan)

          { + this.cursorName = cursorName; + this.selectSQL = selectSQL; + this.queryPlan = queryPlan; + this.islastCallNext = true; + this.fetchPlan = new CursorFetchPlan(queryPlan); + }

          +
          + private synchronized void openCursor(Connection conn) throws SQLException {
          + if(isOpen)

          { + return; + }

          + this.scan = this.queryPlan.getContext().getScan();
          + isReversed=OrderBy.REV_ROW_KEY_ORDER_BY.equals(this.queryPlan.getOrderBy());
          + isOpen = true;
          + }
          +
          + private void closeCursor() throws SQLException

          { + isOpen = false; + ((CursorResultIterator) fetchPlan.iterator()).closeCursor(); + //TODO: Determine if the cursor should be removed from the HashMap at this point. + //Semantically it makes sense that something which is 'Closed' one should be able to 'Open' again. + mapCursorIDQuery.remove(this.cursorName); + }

          +
          + private QueryPlan getFetchPlan(boolean isNext, int fetchSize) throws SQLException {
          + if (!isOpen)
          + throw new SQLException("Fetch call on closed cursor '" + this.cursorName + "'!");
          + ((CursorResultIterator)fetchPlan.iterator()).setFetchSize(fetchSize);
          + if (!queryPlan.getStatement().isAggregate() || !queryPlan.getStatement().isDistinct()) {
          + if (islastCallNext != isNext) {
          + if (islastCallNext && !isReversed)

          { + ScanUtil.setReversed(scan); + }

          else

          { + ScanUtil.unsetReversed(scan); + }

          — End diff –

          this code seems to be for reverse/prior and belongs to another JIRA. can we remove this if it can affect the functionality?

          Show
          githubbot ASF GitHub Bot added a comment - Github user ankitsinghal commented on a diff in the pull request: https://github.com/apache/phoenix/pull/229#discussion_r100738871 — Diff: phoenix-core/src/main/java/org/apache/phoenix/util/CursorUtil.java — @@ -0,0 +1,203 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.phoenix.util; + +import java.sql.Connection; +import java.sql.SQLException; +import java.util.HashMap; +import java.util.Map; + +import org.apache.hadoop.hbase.client.Scan; +import org.apache.hadoop.hbase.io.ImmutableBytesWritable; +import org.apache.phoenix.compile.QueryPlan; +import org.apache.phoenix.compile.OrderByCompiler.OrderBy; +import org.apache.phoenix.execute.CursorFetchPlan; +import org.apache.phoenix.iterate.CursorResultIterator; +import org.apache.phoenix.parse.CloseStatement; +import org.apache.phoenix.parse.DeclareCursorStatement; +import org.apache.phoenix.parse.OpenStatement; +import org.apache.phoenix.schema.tuple.Tuple; + +public final class CursorUtil { + + private static class CursorWrapper { + private final String cursorName; + private final String selectSQL; + private boolean isOpen = false; + QueryPlan queryPlan; + ImmutableBytesWritable row; + ImmutableBytesWritable previousRow; + private Scan scan; + private boolean moreValues=true; + private boolean isReversed; + private boolean islastCallNext; + private CursorFetchPlan fetchPlan; + private int offset = -1; + + private CursorWrapper(String cursorName, String selectSQL, QueryPlan queryPlan) { + this.cursorName = cursorName; + this.selectSQL = selectSQL; + this.queryPlan = queryPlan; + this.islastCallNext = true; + this.fetchPlan = new CursorFetchPlan(queryPlan); + } + + private synchronized void openCursor(Connection conn) throws SQLException { + if(isOpen) { + return; + } + this.scan = this.queryPlan.getContext().getScan(); + isReversed=OrderBy.REV_ROW_KEY_ORDER_BY.equals(this.queryPlan.getOrderBy()); + isOpen = true; + } + + private void closeCursor() throws SQLException { + isOpen = false; + ((CursorResultIterator) fetchPlan.iterator()).closeCursor(); + //TODO: Determine if the cursor should be removed from the HashMap at this point. + //Semantically it makes sense that something which is 'Closed' one should be able to 'Open' again. + mapCursorIDQuery.remove(this.cursorName); + } + + private QueryPlan getFetchPlan(boolean isNext, int fetchSize) throws SQLException { + if (!isOpen) + throw new SQLException("Fetch call on closed cursor '" + this.cursorName + "'!"); + ((CursorResultIterator)fetchPlan.iterator()).setFetchSize(fetchSize); + if (!queryPlan.getStatement().isAggregate() || !queryPlan.getStatement().isDistinct()) { + if (islastCallNext != isNext) { + if (islastCallNext && !isReversed) { + ScanUtil.setReversed(scan); + } else { + ScanUtil.unsetReversed(scan); + } — End diff – this code seems to be for reverse/prior and belongs to another JIRA. can we remove this if it can affect the functionality?
          Hide
          ankit@apache.org Ankit Singhal added a comment -

          Biju Nair, requires some cleanup so have left some feedback.
          ping James Taylor for review.

          Show
          ankit@apache.org Ankit Singhal added a comment - Biju Nair , requires some cleanup so have left some feedback. ping James Taylor for review.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user bijugs commented on the issue:

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

          @ankitsinghal, Thanks for the review comments. I have made the changes for the comments. Will rebase the code to a single commit once the review process is complete.

          Show
          githubbot ASF GitHub Bot added a comment - Github user bijugs commented on the issue: https://github.com/apache/phoenix/pull/229 @ankitsinghal, Thanks for the review comments. I have made the changes for the comments. Will rebase the code to a single commit once the review process is complete.
          Hide
          ankit@apache.org Ankit Singhal added a comment -

          Thanks Biju Nair, +1, Looks good to me.
          Rajeshbabu Chintaguntla, Do you think any impact on phoenix-calcite with this new construct, i.e how complex it is to support this syntax in calcite branch too?

          Show
          ankit@apache.org Ankit Singhal added a comment - Thanks Biju Nair , +1, Looks good to me. Rajeshbabu Chintaguntla , Do you think any impact on phoenix-calcite with this new construct, i.e how complex it is to support this syntax in calcite branch too?
          Hide
          rajeshbabu Rajeshbabu Chintaguntla added a comment -

          Ankit Singhal As for the discussions in Phoenix Calcite work group any new feature comes after 4.10.x need to be implemented in calcite branch too.

          Show
          rajeshbabu Rajeshbabu Chintaguntla added a comment - Ankit Singhal As for the discussions in Phoenix Calcite work group any new feature comes after 4.10.x need to be implemented in calcite branch too.
          Hide
          jamestaylor James Taylor added a comment -

          Rajeshbabu Chintaguntla - I think we'd need to discuss this first on the dev list before deciding one way or another. IMHO, we'd only start this after the calcite branch becomes the master branch and we'd only do this when the unit tests failure rate drops substantially. Otherwise we'll never have the hope of maintaining any kind of quality/stability as the code continues to evolve.

          Show
          jamestaylor James Taylor added a comment - Rajeshbabu Chintaguntla - I think we'd need to discuss this first on the dev list before deciding one way or another. IMHO, we'd only start this after the calcite branch becomes the master branch and we'd only do this when the unit tests failure rate drops substantially. Otherwise we'll never have the hope of maintaining any kind of quality/stability as the code continues to evolve.
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/phoenix/pull/229#discussion_r109997991

          — Diff: phoenix-core/src/main/java/org/apache/phoenix/iterate/CursorResultIterator.java —
          @@ -0,0 +1,75 @@
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +package org.apache.phoenix.iterate;
          +
          +import org.apache.phoenix.schema.tuple.Tuple;
          +import org.apache.phoenix.util.CursorUtil;
          +
          +import java.sql.SQLException;
          +import java.util.List;
          +
          +public class CursorResultIterator implements ResultIterator {
          + private String cursorName;
          + private PeekingResultIterator delegate;
          + //TODO Configure fetch size from FETCH call
          + private int fetchSize = 0;
          + private int rowsRead = 0;
          + public CursorResultIterator(PeekingResultIterator delegate, String cursorName)

          { + this.delegate = delegate; + this.cursorName = cursorName; + }

          +
          + @Override
          + public Tuple next() throws SQLException {
          + if(!CursorUtil.moreValues(cursorName))

          { + return null; + }

          else if (fetchSize == rowsRead)

          { + return null; + }

          +
          + Tuple next = delegate.next();
          + CursorUtil.updateCursor(cursorName,next, delegate.peek());
          + rowsRead++;
          + return next;
          + }
          +
          + @Override
          + public void explain(List<String> planSteps)

          { + delegate.explain(planSteps); + planSteps.add("CLIENT CURSOR " + cursorName); + }

          +
          + @Override
          + public String toString()

          { + return "CursorResultIterator [cursor=" + cursorName + "]"; + }

          +
          + @Override
          + public void close() throws SQLException {
          + //NOP
          — End diff –

          Does this need to call delegate.close()? If not, what closes the underlying query?

          Show
          githubbot ASF GitHub Bot added a comment - Github user JamesRTaylor commented on a diff in the pull request: https://github.com/apache/phoenix/pull/229#discussion_r109997991 — Diff: phoenix-core/src/main/java/org/apache/phoenix/iterate/CursorResultIterator.java — @@ -0,0 +1,75 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.phoenix.iterate; + +import org.apache.phoenix.schema.tuple.Tuple; +import org.apache.phoenix.util.CursorUtil; + +import java.sql.SQLException; +import java.util.List; + +public class CursorResultIterator implements ResultIterator { + private String cursorName; + private PeekingResultIterator delegate; + //TODO Configure fetch size from FETCH call + private int fetchSize = 0; + private int rowsRead = 0; + public CursorResultIterator(PeekingResultIterator delegate, String cursorName) { + this.delegate = delegate; + this.cursorName = cursorName; + } + + @Override + public Tuple next() throws SQLException { + if(!CursorUtil.moreValues(cursorName)) { + return null; + } else if (fetchSize == rowsRead) { + return null; + } + + Tuple next = delegate.next(); + CursorUtil.updateCursor(cursorName,next, delegate.peek()); + rowsRead++; + return next; + } + + @Override + public void explain(List<String> planSteps) { + delegate.explain(planSteps); + planSteps.add("CLIENT CURSOR " + cursorName); + } + + @Override + public String toString() { + return "CursorResultIterator [cursor=" + cursorName + "]"; + } + + @Override + public void close() throws SQLException { + //NOP — End diff – Does this need to call delegate.close()? If not, what closes the underlying query?
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/phoenix/pull/229#discussion_r109999101

          — Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/QueryPlan.java —
          @@ -59,6 +60,8 @@

          • Returns projector used to formulate resultSet row
            */
            RowProjector getProjector();
            +
            + String getCursorName();
              • End diff –

          Why do we need to have getCursorName in QueryPlan?

          Show
          githubbot ASF GitHub Bot added a comment - Github user JamesRTaylor commented on a diff in the pull request: https://github.com/apache/phoenix/pull/229#discussion_r109999101 — Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/QueryPlan.java — @@ -59,6 +60,8 @@ Returns projector used to formulate resultSet row */ RowProjector getProjector(); + + String getCursorName(); End diff – Why do we need to have getCursorName in QueryPlan?
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/phoenix/pull/229#discussion_r109999394

          — Diff: phoenix-core/src/main/antlr3/PhoenixSQL.g —
          @@ -742,7 +751,26 @@ upsert_column_refs returns [Pair<List<ColumnDef>,List<ColumnName>> ret]
          : d=dyn_column_name_or_def { if (d.getDataType()!=null)

          { $ret.getFirst().add(d); } $ret.getSecond().add(d.getColumnDefName()); }
          (COMMA d=dyn_column_name_or_def { if (d.getDataType()!=null) { $ret.getFirst().add(d); }

          $ret.getSecond().add(d.getColumnDefName()); } )*
          ;

          • +
            +// Parse a full declare cursor expression structure.
            +declare_cursor_node returns [DeclareCursorStatement ret]
            + : DECLARE c=cursor_name CURSOR FOR s=select_node
            +

            {ret = factory.declareCursor(c, s); }

            + ;
            +
            +cursor_open_node returns [OpenStatement ret]
            + : OPEN c=cursor_name

            {ret = factory.open(c);}

            + ;
            +
            +cursor_close_node returns [CloseStatement ret]
            + : CLOSE c=cursor_name

            {ret = factory.close(c);}

            + ;
            +
            +cursor_fetch_node returns [FetchStatement ret]
            + : FETCH NEXT (a=NUMBER)? (ROW|ROWS)? FROM c=cursor_name

            {ret = factory.fetch(c,true, a == null ? 1 : Integer.parseInt( a.getText() )); }

            + | FETCH PRIOR (a=NUMBER)? (ROW|ROWS)? FROM c=cursor_name

            {ret = factory.fetch(c,false, a == null ? 1 : Integer.parseInt( a.getText() )); }
              • End diff –

          How is FETCH PRIOR implemented?

          Show
          githubbot ASF GitHub Bot added a comment - Github user JamesRTaylor commented on a diff in the pull request: https://github.com/apache/phoenix/pull/229#discussion_r109999394 — Diff: phoenix-core/src/main/antlr3/PhoenixSQL.g — @@ -742,7 +751,26 @@ upsert_column_refs returns [Pair<List<ColumnDef>,List<ColumnName>> ret] : d=dyn_column_name_or_def { if (d.getDataType()!=null) { $ret.getFirst().add(d); } $ret.getSecond().add(d.getColumnDefName()); } (COMMA d=dyn_column_name_or_def { if (d.getDataType()!=null) { $ret.getFirst().add(d); } $ret.getSecond().add(d.getColumnDefName()); } )* ; + +// Parse a full declare cursor expression structure. +declare_cursor_node returns [DeclareCursorStatement ret] + : DECLARE c=cursor_name CURSOR FOR s=select_node + {ret = factory.declareCursor(c, s); } + ; + +cursor_open_node returns [OpenStatement ret] + : OPEN c=cursor_name {ret = factory.open(c);} + ; + +cursor_close_node returns [CloseStatement ret] + : CLOSE c=cursor_name {ret = factory.close(c);} + ; + +cursor_fetch_node returns [FetchStatement ret] + : FETCH NEXT (a=NUMBER)? (ROW|ROWS)? FROM c=cursor_name {ret = factory.fetch(c,true, a == null ? 1 : Integer.parseInt( a.getText() )); } + | FETCH PRIOR (a=NUMBER)? (ROW|ROWS)? FROM c=cursor_name {ret = factory.fetch(c,false, a == null ? 1 : Integer.parseInt( a.getText() )); } End diff – How is FETCH PRIOR implemented?
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/phoenix/pull/229#discussion_r109999908

          — Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/CursorWithRowValueConstructorIT.java —
          @@ -0,0 +1,687 @@
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +package org.apache.phoenix.end2end;
          +
          +import org.apache.commons.logging.Log;
          +import org.apache.commons.logging.LogFactory;
          +import org.apache.phoenix.util.*;
          +import org.junit.After;
          +import org.junit.Before;
          +import org.junit.BeforeClass;
          +import org.junit.Test;
          +
          +import java.math.BigDecimal;
          +import java.sql.*;
          +import java.util.Properties;
          +import java.util.Random;
          +
          +import static org.apache.phoenix.util.TestUtil.*;
          +import static org.junit.Assert.*;
          +
          +
          +public class CursorWithRowValueConstructorIT extends BaseClientManagedTimeIT {
          — End diff –

          We're trying to get rid of all tests derived from BaseClientManagedTimeIT. Please derive from ParallelStatsDisabledIT instead, letting HBase manage the timestamps, and generate unique table names for each test (see other tests for examples).

          Show
          githubbot ASF GitHub Bot added a comment - Github user JamesRTaylor commented on a diff in the pull request: https://github.com/apache/phoenix/pull/229#discussion_r109999908 — Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/CursorWithRowValueConstructorIT.java — @@ -0,0 +1,687 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.phoenix.end2end; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.apache.phoenix.util.*; +import org.junit.After; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; + +import java.math.BigDecimal; +import java.sql.*; +import java.util.Properties; +import java.util.Random; + +import static org.apache.phoenix.util.TestUtil.*; +import static org.junit.Assert.*; + + +public class CursorWithRowValueConstructorIT extends BaseClientManagedTimeIT { — End diff – We're trying to get rid of all tests derived from BaseClientManagedTimeIT. Please derive from ParallelStatsDisabledIT instead, letting HBase manage the timestamps, and generate unique table names for each test (see other tests for examples).
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/phoenix/pull/229#discussion_r110000317

          — Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/CursorWithRowValueConstructorIT.java —
          @@ -0,0 +1,687 @@
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +package org.apache.phoenix.end2end;
          +
          +import org.apache.commons.logging.Log;
          +import org.apache.commons.logging.LogFactory;
          +import org.apache.phoenix.util.*;
          +import org.junit.After;
          +import org.junit.Before;
          +import org.junit.BeforeClass;
          +import org.junit.Test;
          +
          +import java.math.BigDecimal;
          +import java.sql.*;
          +import java.util.Properties;
          +import java.util.Random;
          +
          +import static org.apache.phoenix.util.TestUtil.*;
          +import static org.junit.Assert.*;
          +
          +
          +public class CursorWithRowValueConstructorIT extends BaseClientManagedTimeIT {
          + private static final String TABLE_NAME = "CursorRVCTestTable";
          + protected static final Log LOG = LogFactory.getLog(CursorWithRowValueConstructorIT.class);
          +
          + public void createAndInitializeTestTable() throws SQLException {
          + Connection conn = DriverManager.getConnection(getUrl());
          +
          + PreparedStatement stmt = conn.prepareStatement("CREATE TABLE IF NOT EXISTS " + TABLE_NAME +
          + "(a_id INTEGER NOT NULL, " +
          + "a_data INTEGER, " +
          + "CONSTRAINT my_pk PRIMARY KEY (a_id))");
          + stmt.execute();
          + synchronized (conn)

          { + conn.commit(); + }
          +
          + //Upsert test values into the test table
          + Random rand = new Random();
          + stmt = conn.prepareStatement("UPSERT INTO " + TABLE_NAME +
          + "(a_id, a_data) VALUES (?,?)");
          + int rowCount = 0;
          + while(rowCount < 100){ + stmt.setInt(1, rowCount); + stmt.setInt(2, rand.nextInt(501)); + stmt.execute(); + ++rowCount; + }
          + synchronized (conn){ + conn.commit(); + }

          + }
          +
          + public void deleteTestTable() throws SQLException {
          + Connection conn = DriverManager.getConnection(getUrl());
          + PreparedStatement stmt = conn.prepareStatement("DROP TABLE IF EXISTS " + TABLE_NAME);
          + stmt.execute();
          + synchronized (conn)

          { + conn.commit(); + }

          + }
          +
          + @Test
          + public void testCursorsOnTestTablePK() throws SQLException {
          + try{
          + createAndInitializeTestTable();
          + String querySQL = "SELECT a_id FROM " + TABLE_NAME;
          +
          + //Test actual cursor implementation
          + String cursorSQL = "DECLARE testCursor CURSOR FOR " + querySQL;
          + DriverManager.getConnection(getUrl()).prepareStatement(cursorSQL).execute();
          + cursorSQL = "OPEN testCursor";
          + DriverManager.getConnection(getUrl()).prepareStatement(cursorSQL).execute();
          + cursorSQL = "FETCH NEXT FROM testCursor";
          — End diff –

          Is FETCH PRIOR tested at all? If not supported, should we remove it from the grammar for now?

          Show
          githubbot ASF GitHub Bot added a comment - Github user JamesRTaylor commented on a diff in the pull request: https://github.com/apache/phoenix/pull/229#discussion_r110000317 — Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/CursorWithRowValueConstructorIT.java — @@ -0,0 +1,687 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.phoenix.end2end; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.apache.phoenix.util.*; +import org.junit.After; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; + +import java.math.BigDecimal; +import java.sql.*; +import java.util.Properties; +import java.util.Random; + +import static org.apache.phoenix.util.TestUtil.*; +import static org.junit.Assert.*; + + +public class CursorWithRowValueConstructorIT extends BaseClientManagedTimeIT { + private static final String TABLE_NAME = "CursorRVCTestTable"; + protected static final Log LOG = LogFactory.getLog(CursorWithRowValueConstructorIT.class); + + public void createAndInitializeTestTable() throws SQLException { + Connection conn = DriverManager.getConnection(getUrl()); + + PreparedStatement stmt = conn.prepareStatement("CREATE TABLE IF NOT EXISTS " + TABLE_NAME + + "(a_id INTEGER NOT NULL, " + + "a_data INTEGER, " + + "CONSTRAINT my_pk PRIMARY KEY (a_id))"); + stmt.execute(); + synchronized (conn) { + conn.commit(); + } + + //Upsert test values into the test table + Random rand = new Random(); + stmt = conn.prepareStatement("UPSERT INTO " + TABLE_NAME + + "(a_id, a_data) VALUES (?,?)"); + int rowCount = 0; + while(rowCount < 100){ + stmt.setInt(1, rowCount); + stmt.setInt(2, rand.nextInt(501)); + stmt.execute(); + ++rowCount; + } + synchronized (conn){ + conn.commit(); + } + } + + public void deleteTestTable() throws SQLException { + Connection conn = DriverManager.getConnection(getUrl()); + PreparedStatement stmt = conn.prepareStatement("DROP TABLE IF EXISTS " + TABLE_NAME); + stmt.execute(); + synchronized (conn) { + conn.commit(); + } + } + + @Test + public void testCursorsOnTestTablePK() throws SQLException { + try{ + createAndInitializeTestTable(); + String querySQL = "SELECT a_id FROM " + TABLE_NAME; + + //Test actual cursor implementation + String cursorSQL = "DECLARE testCursor CURSOR FOR " + querySQL; + DriverManager.getConnection(getUrl()).prepareStatement(cursorSQL).execute(); + cursorSQL = "OPEN testCursor"; + DriverManager.getConnection(getUrl()).prepareStatement(cursorSQL).execute(); + cursorSQL = "FETCH NEXT FROM testCursor"; — End diff – Is FETCH PRIOR tested at all? If not supported, should we remove it from the grammar for now?
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/phoenix/pull/229#discussion_r110057029

          — Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/QueryPlan.java —
          @@ -59,6 +60,8 @@

          • Returns projector used to formulate resultSet row
            */
            RowProjector getProjector();
            +
            + String getCursorName();
              • End diff –

          I think an alternate implementation would be to have a CursorQueryPlan which wraps another QueryPlan. This would be similar to the way a UnionPlan was implemented. That way you wouldn't need to litter the other plan implementations with logic to wrap them with the CursorResultIterator (which would be brittle if/when new plans are added).

          Show
          githubbot ASF GitHub Bot added a comment - Github user JamesRTaylor commented on a diff in the pull request: https://github.com/apache/phoenix/pull/229#discussion_r110057029 — Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/QueryPlan.java — @@ -59,6 +60,8 @@ Returns projector used to formulate resultSet row */ RowProjector getProjector(); + + String getCursorName(); End diff – I think an alternate implementation would be to have a CursorQueryPlan which wraps another QueryPlan. This would be similar to the way a UnionPlan was implemented. That way you wouldn't need to litter the other plan implementations with logic to wrap them with the CursorResultIterator (which would be brittle if/when new plans are added).
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/phoenix/pull/229#discussion_r113032237

          — Diff: phoenix-core/src/main/java/org/apache/phoenix/iterate/CursorResultIterator.java —
          @@ -0,0 +1,75 @@
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +package org.apache.phoenix.iterate;
          +
          +import org.apache.phoenix.schema.tuple.Tuple;
          +import org.apache.phoenix.util.CursorUtil;
          +
          +import java.sql.SQLException;
          +import java.util.List;
          +
          +public class CursorResultIterator implements ResultIterator {
          + private String cursorName;
          + private PeekingResultIterator delegate;
          + //TODO Configure fetch size from FETCH call
          + private int fetchSize = 0;
          + private int rowsRead = 0;
          + public CursorResultIterator(PeekingResultIterator delegate, String cursorName)

          { + this.delegate = delegate; + this.cursorName = cursorName; + }

          +
          + @Override
          + public Tuple next() throws SQLException {
          + if(!CursorUtil.moreValues(cursorName))

          { + return null; + }

          else if (fetchSize == rowsRead)

          { + return null; + }

          +
          + Tuple next = delegate.next();
          + CursorUtil.updateCursor(cursorName,next, delegate.peek());
          + rowsRead++;
          + return next;
          + }
          +
          + @Override
          + public void explain(List<String> planSteps)

          { + delegate.explain(planSteps); + planSteps.add("CLIENT CURSOR " + cursorName); + }

          +
          + @Override
          + public String toString()

          { + return "CursorResultIterator [cursor=" + cursorName + "]"; + }

          +
          + @Override
          + public void close() throws SQLException {
          + //NOP
          — End diff –

          @JamesRTaylor , closing the cursor will close the underlying query.

          Show
          githubbot ASF GitHub Bot added a comment - Github user bijugs commented on a diff in the pull request: https://github.com/apache/phoenix/pull/229#discussion_r113032237 — Diff: phoenix-core/src/main/java/org/apache/phoenix/iterate/CursorResultIterator.java — @@ -0,0 +1,75 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.phoenix.iterate; + +import org.apache.phoenix.schema.tuple.Tuple; +import org.apache.phoenix.util.CursorUtil; + +import java.sql.SQLException; +import java.util.List; + +public class CursorResultIterator implements ResultIterator { + private String cursorName; + private PeekingResultIterator delegate; + //TODO Configure fetch size from FETCH call + private int fetchSize = 0; + private int rowsRead = 0; + public CursorResultIterator(PeekingResultIterator delegate, String cursorName) { + this.delegate = delegate; + this.cursorName = cursorName; + } + + @Override + public Tuple next() throws SQLException { + if(!CursorUtil.moreValues(cursorName)) { + return null; + } else if (fetchSize == rowsRead) { + return null; + } + + Tuple next = delegate.next(); + CursorUtil.updateCursor(cursorName,next, delegate.peek()); + rowsRead++; + return next; + } + + @Override + public void explain(List<String> planSteps) { + delegate.explain(planSteps); + planSteps.add("CLIENT CURSOR " + cursorName); + } + + @Override + public String toString() { + return "CursorResultIterator [cursor=" + cursorName + "]"; + } + + @Override + public void close() throws SQLException { + //NOP — End diff – @JamesRTaylor , closing the cursor will close the underlying query.
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/phoenix/pull/229#discussion_r113032806

          — Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/QueryPlan.java —
          @@ -59,6 +60,8 @@

          • Returns projector used to formulate resultSet row
            */
            RowProjector getProjector();
            +
            + String getCursorName();
              • End diff –

          @JamesRTaylor , will update the PR with suggested changes.

          Show
          githubbot ASF GitHub Bot added a comment - Github user bijugs commented on a diff in the pull request: https://github.com/apache/phoenix/pull/229#discussion_r113032806 — Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/QueryPlan.java — @@ -59,6 +60,8 @@ Returns projector used to formulate resultSet row */ RowProjector getProjector(); + + String getCursorName(); End diff – @JamesRTaylor , will update the PR with suggested changes.
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/phoenix/pull/229#discussion_r113033881

          — Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/CursorWithRowValueConstructorIT.java —
          @@ -0,0 +1,687 @@
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +package org.apache.phoenix.end2end;
          +
          +import org.apache.commons.logging.Log;
          +import org.apache.commons.logging.LogFactory;
          +import org.apache.phoenix.util.*;
          +import org.junit.After;
          +import org.junit.Before;
          +import org.junit.BeforeClass;
          +import org.junit.Test;
          +
          +import java.math.BigDecimal;
          +import java.sql.*;
          +import java.util.Properties;
          +import java.util.Random;
          +
          +import static org.apache.phoenix.util.TestUtil.*;
          +import static org.junit.Assert.*;
          +
          +
          +public class CursorWithRowValueConstructorIT extends BaseClientManagedTimeIT {
          — End diff –

          Will update the code to use ``ParallelStatsDisabledIT ``.

          Show
          githubbot ASF GitHub Bot added a comment - Github user bijugs commented on a diff in the pull request: https://github.com/apache/phoenix/pull/229#discussion_r113033881 — Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/CursorWithRowValueConstructorIT.java — @@ -0,0 +1,687 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.phoenix.end2end; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.apache.phoenix.util.*; +import org.junit.After; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; + +import java.math.BigDecimal; +import java.sql.*; +import java.util.Properties; +import java.util.Random; + +import static org.apache.phoenix.util.TestUtil.*; +import static org.junit.Assert.*; + + +public class CursorWithRowValueConstructorIT extends BaseClientManagedTimeIT { — End diff – Will update the code to use ``ParallelStatsDisabledIT ``.
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/phoenix/pull/229#discussion_r113034318

          — Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/CursorWithRowValueConstructorIT.java —
          @@ -0,0 +1,687 @@
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +package org.apache.phoenix.end2end;
          +
          +import org.apache.commons.logging.Log;
          +import org.apache.commons.logging.LogFactory;
          +import org.apache.phoenix.util.*;
          +import org.junit.After;
          +import org.junit.Before;
          +import org.junit.BeforeClass;
          +import org.junit.Test;
          +
          +import java.math.BigDecimal;
          +import java.sql.*;
          +import java.util.Properties;
          +import java.util.Random;
          +
          +import static org.apache.phoenix.util.TestUtil.*;
          +import static org.junit.Assert.*;
          +
          +
          +public class CursorWithRowValueConstructorIT extends BaseClientManagedTimeIT {
          + private static final String TABLE_NAME = "CursorRVCTestTable";
          + protected static final Log LOG = LogFactory.getLog(CursorWithRowValueConstructorIT.class);
          +
          + public void createAndInitializeTestTable() throws SQLException {
          + Connection conn = DriverManager.getConnection(getUrl());
          +
          + PreparedStatement stmt = conn.prepareStatement("CREATE TABLE IF NOT EXISTS " + TABLE_NAME +
          + "(a_id INTEGER NOT NULL, " +
          + "a_data INTEGER, " +
          + "CONSTRAINT my_pk PRIMARY KEY (a_id))");
          + stmt.execute();
          + synchronized (conn)

          { + conn.commit(); + }
          +
          + //Upsert test values into the test table
          + Random rand = new Random();
          + stmt = conn.prepareStatement("UPSERT INTO " + TABLE_NAME +
          + "(a_id, a_data) VALUES (?,?)");
          + int rowCount = 0;
          + while(rowCount < 100){ + stmt.setInt(1, rowCount); + stmt.setInt(2, rand.nextInt(501)); + stmt.execute(); + ++rowCount; + }
          + synchronized (conn){ + conn.commit(); + }

          + }
          +
          + public void deleteTestTable() throws SQLException {
          + Connection conn = DriverManager.getConnection(getUrl());
          + PreparedStatement stmt = conn.prepareStatement("DROP TABLE IF EXISTS " + TABLE_NAME);
          + stmt.execute();
          + synchronized (conn)

          { + conn.commit(); + }

          + }
          +
          + @Test
          + public void testCursorsOnTestTablePK() throws SQLException {
          + try{
          + createAndInitializeTestTable();
          + String querySQL = "SELECT a_id FROM " + TABLE_NAME;
          +
          + //Test actual cursor implementation
          + String cursorSQL = "DECLARE testCursor CURSOR FOR " + querySQL;
          + DriverManager.getConnection(getUrl()).prepareStatement(cursorSQL).execute();
          + cursorSQL = "OPEN testCursor";
          + DriverManager.getConnection(getUrl()).prepareStatement(cursorSQL).execute();
          + cursorSQL = "FETCH NEXT FROM testCursor";
          — End diff –

          Since this PR is for ``FETCH NEXT`` will remove the code related to ``FETCH PRIOR``.

          Show
          githubbot ASF GitHub Bot added a comment - Github user bijugs commented on a diff in the pull request: https://github.com/apache/phoenix/pull/229#discussion_r113034318 — Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/CursorWithRowValueConstructorIT.java — @@ -0,0 +1,687 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.phoenix.end2end; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.apache.phoenix.util.*; +import org.junit.After; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; + +import java.math.BigDecimal; +import java.sql.*; +import java.util.Properties; +import java.util.Random; + +import static org.apache.phoenix.util.TestUtil.*; +import static org.junit.Assert.*; + + +public class CursorWithRowValueConstructorIT extends BaseClientManagedTimeIT { + private static final String TABLE_NAME = "CursorRVCTestTable"; + protected static final Log LOG = LogFactory.getLog(CursorWithRowValueConstructorIT.class); + + public void createAndInitializeTestTable() throws SQLException { + Connection conn = DriverManager.getConnection(getUrl()); + + PreparedStatement stmt = conn.prepareStatement("CREATE TABLE IF NOT EXISTS " + TABLE_NAME + + "(a_id INTEGER NOT NULL, " + + "a_data INTEGER, " + + "CONSTRAINT my_pk PRIMARY KEY (a_id))"); + stmt.execute(); + synchronized (conn) { + conn.commit(); + } + + //Upsert test values into the test table + Random rand = new Random(); + stmt = conn.prepareStatement("UPSERT INTO " + TABLE_NAME + + "(a_id, a_data) VALUES (?,?)"); + int rowCount = 0; + while(rowCount < 100){ + stmt.setInt(1, rowCount); + stmt.setInt(2, rand.nextInt(501)); + stmt.execute(); + ++rowCount; + } + synchronized (conn){ + conn.commit(); + } + } + + public void deleteTestTable() throws SQLException { + Connection conn = DriverManager.getConnection(getUrl()); + PreparedStatement stmt = conn.prepareStatement("DROP TABLE IF EXISTS " + TABLE_NAME); + stmt.execute(); + synchronized (conn) { + conn.commit(); + } + } + + @Test + public void testCursorsOnTestTablePK() throws SQLException { + try{ + createAndInitializeTestTable(); + String querySQL = "SELECT a_id FROM " + TABLE_NAME; + + //Test actual cursor implementation + String cursorSQL = "DECLARE testCursor CURSOR FOR " + querySQL; + DriverManager.getConnection(getUrl()).prepareStatement(cursorSQL).execute(); + cursorSQL = "OPEN testCursor"; + DriverManager.getConnection(getUrl()).prepareStatement(cursorSQL).execute(); + cursorSQL = "FETCH NEXT FROM testCursor"; — End diff – Since this PR is for ``FETCH NEXT`` will remove the code related to ``FETCH PRIOR``.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user bijugs commented on the issue:

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

          @ankitsinghal , @JamesRTaylor , updated the PR with the changes for the latest review comments. If there are any other comments please let me know. Once review is complete will submit a ``diff`` to the JIRA.

          Show
          githubbot ASF GitHub Bot added a comment - Github user bijugs commented on the issue: https://github.com/apache/phoenix/pull/229 @ankitsinghal , @JamesRTaylor , updated the PR with the changes for the latest review comments. If there are any other comments please let me know. Once review is complete will submit a ``diff`` to the JIRA.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user JamesRTaylor commented on the issue:

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

          This looks good now, @bijugs - much simpler - nice work. Have you attached a patch in the right format (see https://phoenix.apache.org/contributing.html#Generate_a_patch) to make sure your change doesn't cause any regressions?

          @ankitsinghal - WDYT?

          Show
          githubbot ASF GitHub Bot added a comment - Github user JamesRTaylor commented on the issue: https://github.com/apache/phoenix/pull/229 This looks good now, @bijugs - much simpler - nice work. Have you attached a patch in the right format (see https://phoenix.apache.org/contributing.html#Generate_a_patch ) to make sure your change doesn't cause any regressions? @ankitsinghal - WDYT?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user ankitsinghal commented on the issue:

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

          Looks good to me as well. Thanks @bijugs, for working on this. Let me commit this for you.

          Show
          githubbot ASF GitHub Bot added a comment - Github user ankitsinghal commented on the issue: https://github.com/apache/phoenix/pull/229 Looks good to me as well. Thanks @bijugs, for working on this. Let me commit this for you.
          Hide
          gsbiju Biju Nair added a comment -

          Patch for the changes attached.

          Show
          gsbiju Biju Nair added a comment - Patch for the changes attached.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user bijugs commented on the issue:

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

          Attached patch for the changes to the JIRA ticket.

          Show
          githubbot ASF GitHub Bot added a comment - Github user bijugs commented on the issue: https://github.com/apache/phoenix/pull/229 Attached patch for the changes to the JIRA ticket.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12868385/PHOENIX-3572.patch
          against master branch at commit 1666e932d157be732946e02b474b6c342199bc0f.
          ATTACHMENT ID: 12868385

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 13 new or modified tests.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 javadoc. The javadoc tool appears to have generated 47 warning messages.

          -1 release audit. The applied patch generated 2 release audit warnings (more than the master's current 0 warnings).

          -1 lineLengths. The patch introduces the following lines longer than 100:
          + ResultSet rs = DriverManager.getConnection(getUrl()).prepareStatement(cursorSQL).executeQuery();
          + rs = DriverManager.getConnection(getUrl()).createStatement().executeQuery(cursorSQL);
          + ResultSet cursorRS = DriverManager.getConnection(getUrl()).prepareStatement(cursorSQL).executeQuery();
          + ResultSet rs = DriverManager.getConnection(getUrl()).prepareStatement(querySQL).executeQuery();
          + cursorRS = DriverManager.getConnection(getUrl()).prepareStatement(cursorSQL).executeQuery();
          + ResultSet rs = DriverManager.getConnection(getUrl()).prepareStatement(cursorSQL).executeQuery();
          + rs = DriverManager.getConnection(getUrl()).prepareStatement(cursorSQL).executeQuery();
          + ResultSet rs = DriverManager.getConnection(getUrl()).prepareStatement(cursorSQL).executeQuery();
          + rs = DriverManager.getConnection(getUrl()).prepareStatement(cursorSQL).executeQuery();
          + ResultSet rs = DriverManager.getConnection(getUrl()).prepareStatement(querySQL).executeQuery();

          -1 core tests. The patch failed these unit tests:

          Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/875//testReport/
          Release audit warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/875//artifact/patchprocess/patchReleaseAuditWarnings.txt
          Javadoc warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/875//artifact/patchprocess/patchJavadocWarnings.txt
          Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/875//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12868385/PHOENIX-3572.patch against master branch at commit 1666e932d157be732946e02b474b6c342199bc0f. ATTACHMENT ID: 12868385 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 13 new or modified tests. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 javadoc . The javadoc tool appears to have generated 47 warning messages. -1 release audit . The applied patch generated 2 release audit warnings (more than the master's current 0 warnings). -1 lineLengths . The patch introduces the following lines longer than 100: + ResultSet rs = DriverManager.getConnection(getUrl()).prepareStatement(cursorSQL).executeQuery(); + rs = DriverManager.getConnection(getUrl()).createStatement().executeQuery(cursorSQL); + ResultSet cursorRS = DriverManager.getConnection(getUrl()).prepareStatement(cursorSQL).executeQuery(); + ResultSet rs = DriverManager.getConnection(getUrl()).prepareStatement(querySQL).executeQuery(); + cursorRS = DriverManager.getConnection(getUrl()).prepareStatement(cursorSQL).executeQuery(); + ResultSet rs = DriverManager.getConnection(getUrl()).prepareStatement(cursorSQL).executeQuery(); + rs = DriverManager.getConnection(getUrl()).prepareStatement(cursorSQL).executeQuery(); + ResultSet rs = DriverManager.getConnection(getUrl()).prepareStatement(cursorSQL).executeQuery(); + rs = DriverManager.getConnection(getUrl()).prepareStatement(cursorSQL).executeQuery(); + ResultSet rs = DriverManager.getConnection(getUrl()).prepareStatement(querySQL).executeQuery(); -1 core tests . The patch failed these unit tests: Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/875//testReport/ Release audit warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/875//artifact/patchprocess/patchReleaseAuditWarnings.txt Javadoc warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/875//artifact/patchprocess/patchJavadocWarnings.txt Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/875//console This message is automatically generated.
          Hide
          ankit@apache.org Ankit Singhal added a comment -

          Thanks Biju Nair. Nice Work
          Committed in master and 4.x branches.

          Show
          ankit@apache.org Ankit Singhal added a comment - Thanks Biju Nair . Nice Work Committed in master and 4.x branches.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user bijugs commented on the issue:

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

          Closing the PR since the patch got merged. Thanks @ankitsinghal , @JamesRTaylor .

          Show
          githubbot ASF GitHub Bot added a comment - Github user bijugs commented on the issue: https://github.com/apache/phoenix/pull/229 Closing the PR since the patch got merged. Thanks @ankitsinghal , @JamesRTaylor .
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user bijugs closed the pull request at:

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

          Show
          githubbot ASF GitHub Bot added a comment - Github user bijugs closed the pull request at: https://github.com/apache/phoenix/pull/229
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Jenkins build Phoenix-master #1613 (See https://builds.apache.org/job/Phoenix-master/1613/)
          PHOENIX-3572 Support FETCH NEXT| n ROWS from Cursor (Biju Nair) (ankitsinghal59: rev 2cb617f352048179439d242d1165a9ffb39ad81c)

          • (add) phoenix-core/src/it/java/org/apache/phoenix/end2end/CursorWithRowValueConstructorIT.java
          • (add) phoenix-core/src/test/java/org/apache/phoenix/compile/CursorCompilerTest.java
          • (edit) phoenix-core/src/main/java/org/apache/phoenix/parse/ParseNodeFactory.java
          • (edit) phoenix-core/src/main/antlr3/PhoenixSQL.g
          • (add) phoenix-core/src/main/java/org/apache/phoenix/compile/DeclareCursorCompiler.java
          • (edit) phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixStatement.java
          • (edit) phoenix-core/src/main/java/org/apache/phoenix/parse/SQLParser.java
          • (add) phoenix-core/src/main/java/org/apache/phoenix/util/CursorUtil.java
          • (add) phoenix-core/src/main/java/org/apache/phoenix/parse/OpenStatement.java
          • (add) phoenix-core/src/main/java/org/apache/phoenix/parse/CloseStatement.java
          • (add) phoenix-core/src/main/java/org/apache/phoenix/parse/DeclareCursorStatement.java
          • (add) phoenix-core/src/main/java/org/apache/phoenix/iterate/CursorResultIterator.java
          • (add) phoenix-core/src/main/java/org/apache/phoenix/parse/FetchStatement.java
          • (add) phoenix-core/src/test/java/org/apache/phoenix/parse/CursorParserTest.java
          • (add) phoenix-core/src/main/java/org/apache/phoenix/compile/OpenStatementCompiler.java
          • (add) phoenix-core/src/main/java/org/apache/phoenix/execute/CursorFetchPlan.java
          • (edit) phoenix-core/src/main/java/org/apache/phoenix/schema/MetaDataClient.java
          • (edit) phoenix-core/src/main/java/org/apache/phoenix/util/ScanUtil.java
          • (add) phoenix-core/src/main/java/org/apache/phoenix/parse/CursorName.java
          • (add) phoenix-core/src/main/java/org/apache/phoenix/compile/CloseStatementCompiler.java
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Jenkins build Phoenix-master #1613 (See https://builds.apache.org/job/Phoenix-master/1613/ ) PHOENIX-3572 Support FETCH NEXT| n ROWS from Cursor (Biju Nair) (ankitsinghal59: rev 2cb617f352048179439d242d1165a9ffb39ad81c) (add) phoenix-core/src/it/java/org/apache/phoenix/end2end/CursorWithRowValueConstructorIT.java (add) phoenix-core/src/test/java/org/apache/phoenix/compile/CursorCompilerTest.java (edit) phoenix-core/src/main/java/org/apache/phoenix/parse/ParseNodeFactory.java (edit) phoenix-core/src/main/antlr3/PhoenixSQL.g (add) phoenix-core/src/main/java/org/apache/phoenix/compile/DeclareCursorCompiler.java (edit) phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixStatement.java (edit) phoenix-core/src/main/java/org/apache/phoenix/parse/SQLParser.java (add) phoenix-core/src/main/java/org/apache/phoenix/util/CursorUtil.java (add) phoenix-core/src/main/java/org/apache/phoenix/parse/OpenStatement.java (add) phoenix-core/src/main/java/org/apache/phoenix/parse/CloseStatement.java (add) phoenix-core/src/main/java/org/apache/phoenix/parse/DeclareCursorStatement.java (add) phoenix-core/src/main/java/org/apache/phoenix/iterate/CursorResultIterator.java (add) phoenix-core/src/main/java/org/apache/phoenix/parse/FetchStatement.java (add) phoenix-core/src/test/java/org/apache/phoenix/parse/CursorParserTest.java (add) phoenix-core/src/main/java/org/apache/phoenix/compile/OpenStatementCompiler.java (add) phoenix-core/src/main/java/org/apache/phoenix/execute/CursorFetchPlan.java (edit) phoenix-core/src/main/java/org/apache/phoenix/schema/MetaDataClient.java (edit) phoenix-core/src/main/java/org/apache/phoenix/util/ScanUtil.java (add) phoenix-core/src/main/java/org/apache/phoenix/parse/CursorName.java (add) phoenix-core/src/main/java/org/apache/phoenix/compile/CloseStatementCompiler.java
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Jenkins build Phoenix-master #1619 (See https://builds.apache.org/job/Phoenix-master/1619/)
          PHOENIX-3878 Add license headers missed in PHOENIX-3572 (gjacoby: rev 33d6cdab1e5eed336b7a6f56d3ebaa741893bc43)

          • (edit) phoenix-core/src/main/java/org/apache/phoenix/parse/CursorName.java
          • (edit) phoenix-core/src/main/java/org/apache/phoenix/execute/CursorFetchPlan.java
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Jenkins build Phoenix-master #1619 (See https://builds.apache.org/job/Phoenix-master/1619/ ) PHOENIX-3878 Add license headers missed in PHOENIX-3572 (gjacoby: rev 33d6cdab1e5eed336b7a6f56d3ebaa741893bc43) (edit) phoenix-core/src/main/java/org/apache/phoenix/parse/CursorName.java (edit) phoenix-core/src/main/java/org/apache/phoenix/execute/CursorFetchPlan.java

            People

            • Assignee:
              gsbiju Biju Nair
              Reporter:
              gsbiju Biju Nair
            • Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development